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

[Dave Martin <Dave.Martin@arm.com>]

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:
> > On Thu, Jul 29, 2021 at 04:15:17PM +0100, Mark Brown wrote:
> 
> > > +	child = fork();
> > > +	if (child == -1) {
> > > +		ksft_print_msg("fork() failed: %d (%s)\n",
> > > +			       errno, strerror(errno));
> > > +		close(pipefd[0]);
> > > +		close(pipefd[1]);
> > > +		return -1;
> 
> > 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.

> 
> > > +		if (!WIFEXITED(ret)) {
> > > +			ksft_print_msg("child exited abnormally\n");
> > > +			close(pipefd[0]);
> > > +			return -1;
> > > +		}
> 
> > 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