Re: [libvirt] [PATCH 2/4] virNetSocket: Be more safe with fork() around virNetSocketDupFD()

On 9/21/18 5:29 AM, Michal Privoznik wrote: blank line I don't see this last sentence as happening in this patch... I'm with Marc on this - documenting virNetSocketDupFD is a good thing. Although it could be considered out of the scope of what you're doing, since you are changing the semantics describing the nuance that fork brings about w/r/t fd duplication in the code rather than the commit message doesn't make someone have to go back and look at the commit message to gain more insight. It also should provide the reader with the knowledge of what virSetInherit would provide to remove if so desired. Of course while you're at it, documenting virNetClientDupFD similarly would be quite beneficial. Caveat: my sock knowledge of setting or duplication of the socket fd isn't that deep, especially as it relates to fork "mechanics". So prior to this change, the call to virNetSocketDupFD(fd, false) would: fd = dup(sock->fd); Or essentially create a new socket fd from the existing one copying everything it already has... With this change you have (discounting some new error paths) fd = fcntl(sock->fd, F_DUPFD_CLOEXEC, 0); fcntl(fd, F_SETFD, ~FD_CLOEXEC) which seems to be a nop and/or unnecessary to me. Beyond that if the sock->fd didn't have CLOEXEC already, then what impact does briefly changing CLOEXEC have on something that arrives while set? And of course if sock->fd had CLOEXEC and then you clear it on the 'shared' socket, that doesn't seem right either. Seems to me the dup() does the trick and then lets the caller decide what CLOEXEC semantics they want with the duplicated fd without perhaps changing the sock->fd semantics. So removing this "instance" of fork and client usage - is there no belief that some day it may be a desirable option. I think the end result you have in patch3 do the SetInherit call is probably still proper since at that point you know by calling w/ false all you'd be getting is a straight duplicate of what exists and the decision at that point is to "alter" the view of the socket you have received to change the CLOEXEC value John hopefully I'm not too far off into the weeds on this, but I'm not convinced this patch is right even though I do think documenting the semantics would be a really good thing to happen because I cannot imagine the nuance(s) is(are) all that well understood.