Re: [PATCH v3 3/4] kselftest/arm64: Add tests for SVE vector configuration

On Thu, Jul 29, 2021 at 06:34:11PM +0100, Mark Brown wrote:

On Thu, Jul 29, 2021 at 05:06:42PM +0100, Dave Martin wrote:

quoted

On Thu, Jul 29, 2021 at 04:15:17PM +0100, Mark Brown wrote:

quoted

quoted

+	child = fork();
+	if (child == -1) {
+		ksft_print_msg("fork() failed: %d (%s)\n",
+			       errno, strerror(errno));
+		close(pipefd[0]);
+		close(pipefd[1]);
+		return -1;

quoted

Since nothing reopens pipefd[0] or pipefd[1], you could also follow the
"goto out" convention and just (re)close both fds at the end, rather
than having to repeat the close() multiple times.  But it works as-is.

I find that when fork() gets involved that starts to get confusing since
you have multiple contexts/control flows around and working out which
cleanup path goes where is more of the issue.

Ack, the other option would be to factor out the child stuff into a
separate function, but this doesn't quite seem worth it here.

Although the code seemed a bit repetitive, it is at least clear in its
current form, so I don't have a strong view.

quoted

quoted

+		if (!WIFEXITED(ret)) {
+			ksft_print_msg("child exited abnormally\n");
+			close(pipefd[0]);
+			return -1;
+		}

quoted

The WEXITSTATUS() check could go outside the loop.

OTOH I find that logically it's part of working out what happened with
the child which is what the loop body is doing.  Anyway I changed to the
do/while you suggested.

That's a reasonable position, but thinking about it a bit more, there's
not really any loop at all here.

There definitely is an unwaited-for child and we don't pass WHONANG to
wait(), so it will either return the child pid, or fail.

Without WUNTRACED or similar, the child must terminate to wake up the
wait().

So is this just a matter of

	pid = wait(&ret);
	if (pid == -1) {
		/* barf */
	}
	assert(pid == child);

	if (!WIFEXITED(ret)) {
		/* barf */
	}

	if (WEXITSTATUS(ret) != 0) {
		/* barf */
	}

	/* parse child's stdout etc. */

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

Hmmm, usually I at least try to do that, but I did seem to leave rather
a lot of trailing junk that time.

(Working out which context is relevant is not always an exact science,
but in this case, it looks like I just forgot.)

Cheers
---Dave 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel