Richard W.M. Jones
2016-Apr-14 13:57 UTC
Re: [Libguestfs] [PATCH v3 libguestfs] launch: Implement a safer getumask.
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. Rich. -- Richard Jones, Virtualization Group, Red Hat people.redhat.com/~rjones Read my programming and virtualization blog: rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. people.redhat.com/~rjones/virt-df
Eric Blake
2016-Apr-14 14:04 UTC
Re: [Libguestfs] [PATCH v3 libguestfs] launch: Implement a safer getumask.
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... + + /* 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 libvirt.org
Richard W.M. Jones
2016-Apr-14 14:11 UTC
Re: [Libguestfs] [PATCH v3 libguestfs] launch: Implement a safer getumask.
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 libvirt.org >-- Richard Jones, Virtualization Group, Red Hat people.redhat.com/~rjones Read my programming and virtualization blog: rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. fedoraproject.org/wiki/MinGW
Apparently Analagous Threads
- Re: [PATCH v3 libguestfs] launch: Implement a safer getumask.
- Re: [PATCH v3 libguestfs] launch: Implement a safer getumask.
- [PATCH v3 libguestfs] launch: Implement a safer getumask.
- Re: [PATCH v3 libguestfs] launch: Implement a safer getumask.
- [PATCH libguestfs] launch: Implement a safer getumask.