Re: [Libguestfs] [PATCH libnbd] states: connect_command: Don't set O_NONBLOCK on socket passed to child.

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.