[Libguestfs] [PATCH libnbd] states: connect_command: Don't set O_NONBLOCK on socket passed to child.
Richard W.M. Jones
rjones at redhat.com
Sat May 25 21:32:34 UTC 2019
On Sat, May 25, 2019 at 03:30:18PM -0500, Eric Blake wrote:
> On 5/25/19 3:22 PM, Eric Blake wrote:
> > On 5/25/19 1:33 PM, Richard W.M. Jones wrote:
> >> I also made the code a bit more robust about closing the socket along
> >> error paths.
> >> ---
> >> generator/states-connect.c | 21 ++++++++++++++++++---
> >> 1 file changed, 18 insertions(+), 3 deletions(-)
> >>
> >
> >> assert (!h->sock);
> >> assert (h->argv);
> >> assert (h->argv[0]);
> >> - if (socketpair (AF_UNIX, SOCK_STREAM|SOCK_NONBLOCK|SOCK_CLOEXEC, 0,
> >> - sv) == -1) {
> >> + if (socketpair (AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0, sv) == -1) {
> >
> > Is it any easier to keep SOCK_NONBLOCK here and then clear O_NONBLOCK in
> > the child process? It may matter if we try to port to a system that
> > lacks SOCK_CLOEXEC (SOCK_NONBLOCK and SOCK_CLOEXEC were added at the
> > same time). But we can deal with portability when someone reports a
> > problem. For now the patch looks fine.
>
> Hmm. Your use of SOCK_CLOEXEC here made me look for other fds that we
> might inadvertently destroy or leak in a multi-threaded process that
> does fork/exec (or even if the program linking against libnbd does
> connect_command() in two separate threads on two different nbd objects).
> I found:
> lib/crypto.c: fp = fopen (pskfile, "r");
>
> We need to use either fopen(pskfile, "re") (if libc is new-enough to
> support "e" for O_CLOEXEC) or raw open(O_CLOEXEC) + fdopen() instead.
Based on this I pushed:
https://github.com/libguestfs/libnbd/commit/516113b955be557f234534ab28b641a118bfe6c4
I don't know if this will actually fail on old / non-supporting libc
or if it will just get ignored?
Also I pushed some APIs for URI support, based on the discussion of
NBD URIs here:
https://lists.debian.org/nbd/2019/05/msg00034.html
Of course we can change / revert them if the final proposal is
different.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine. Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/
More information about the Libguestfs
mailing list