[libvirt] [PATCH] fdstream: drop delete argument

[Eric Blake]

On 08/02/2011 01:29 PM, Eric Blake wrote:
> On 08/02/2011 01:18 PM, Laine Stump wrote:
>> On 08/02/2011 01:31 PM, Eric Blake wrote:
>>> Revert 6a1f5f568f8. Now that libvirt_iohelper no longer has a
>>> race where it can open() a file after the parent process has
>>> unlink()d the same file, it makes more sense to make the callers
>>> both create and unlink, rather than the caller create and the
>>> stream unlink.
>>
>>
>> I wasn't paying attention to the messages/patches related to the race
>> condition you reference,
>
> Commit 1eb66479 plugged the race; commit 6a1f5f5 introduced the race in
> the first place.
>
> The problem was that if we use libvirt_iohelper, and the child process
> calls open(), but the parent process calls unlink() before the child
> process gets to run very far, then the child process will fail to
> open(). But by changing fdstream to pass the fd to libvirt-iohelper by
> fd inheritance instead of by name-wise open() calls, there is no longer
> an open() race, so we can once again unlink() in the parent.
>
>  > but this (caller creates and unlinks)
>> definitely seems cleaner than the other way. Beyond that, the patch
>> seems to be correct. ACK.
>
> Should this go in for 0.9.4, or am I correct in deferring it until after
> the release?

Laine and I chatted some more on IRC:

<eblake>	laine: should I push both patches now (or even squash 
together), or delay the second patch till after the release?
<laine>	eblake: I don't see a problem with the second patch, I think I 
like it better. Not knowing the history of the race you mentioned and 
whether or not this code still touches it, I wouldn't single-handledly 
say to squash the two together, but if you're comfortable with it, then 
it looks good to me.
<eblake>	I'll update the commit message of the second; the race was 
first solved in 6a1f5f5 by making the parent not unlink() if 
libvirt_iohelper will also be open()ing the file
<eblake>	but solving it in that manner required passing delete=true all 
the way through the fdstream code
<eblake>	later, I pushed commit 1eb66479, which taught libvirt_iostream 
to operate on an inherited fd instead of calling open()
<eblake>	and in so doing, the child no longer needs to unlink()
<eblake>	so we no longer have the unlink() in parent prior to 
open()/unlink() in the child, and can get rid of the extra parameter

Given that, I went ahead and updated the commit comment, then pushed 
both patches separately, to make it into 0.9.4.

     Revert 6a1f5f568f8.  Now that libvirt_iohelper takes fds by
     inheritance rather than by open() (commit 1eb66479), there is
     no longer a race where the parent can unlink() a file prior to
     the iohelper open()ing the same file.  From there, it makes
     more sense to have the callers both create and unlink, rather
     than the caller create and the stream unlink, since the latter
     was only needed when iohelper had to do the unlink.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org