[Libguestfs] [PATCH v3 libguestfs] launch: Implement a safer getumask.

[Richard W.M. Jones]

On Thu, Apr 14, 2016 at 08:04:39AM -0600, Eric Blake wrote:
> On 04/14/2016 07:57 AM, Richard W.M. Jones wrote:
> > On Thu, Apr 14, 2016 at 07:38:23AM -0600, Eric Blake wrote:
> >>> +  /* Read the umask. */
> >>> +  if (read (fd[0], &mask, sizeof mask) != sizeof mask) {
> >>> +    perrorf (g, "read");
> >>> +    close (fd[0]);
> >>> +    return -1;
> >>
> >> Oops - this strands a child process.  You have to reap the child, even
> >> if the read() failed.
> > 
> > Bleah that was stupid.  Try attached version - the only difference is
> > I added a waitpid call to the error path above.
> 
> But without looping on EINTR...

Thanks - I pushed it with your suggested loop added.

BTW we don't loop in waitpid/EINTR anywhere else in libguestfs, but I
guess this is something we should be doing.  Can you tell us why it's
necessary?

Rich.

> 
> +
> +  /* Read the umask. */
> +  if (read (fd[0], &mask, sizeof mask) != sizeof mask) {
> +    perrorf (g, "read");
> +    close (fd[0]);
> +    waitpid (pid, NULL, 0);
> 
> I think it's enough to make this line:
> 
>     while (waitpid (pid, &status, 0) == -1 && errno == EINTR);
> 
> +    return -1;
> 
> as you've already reported the initial error, and are merely focused on
> cleanup of the child.
> 
> +  }
> +  close (fd[0]);
> +
> + again:
> +  if (waitpid (pid, &status, 0) == -1) {
> +    if (errno == EINTR) goto again;
> +    perrorf (g, "waitpid");
> +    return -1;
> 
> Anything else, like trying to reuse the again: loop, seems like it be
> more complicated control flow.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW