Re: [Qemu-devel] [PATCH 3/7] block: raw-posix image file reopen

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

On 09/07/2012 06:40 AM, Kevin Wolf wrote:
> Am 06.09.2012 17:34, schrieb Corey Bryant:
>>
>>
>> On 09/06/2012 05:23 AM, Kevin Wolf wrote:
>>> Am 05.09.2012 18:43, schrieb Jeff Cody:
>>>>>> +    }
>>>>>> +
>>>>>> +    int fcntl_flags = O_APPEND | O_ASYNC | O_NONBLOCK;
>>>>>> +#ifdef O_NOATIME
>>>>>> +    fcntl_flags |= O_NOATIME;
>>>>>> +#endif
>>>>>> +    if ((raw_s->open_flags & ~fcntl_flags) == (s->open_flags & ~fcntl_flags)) {
>>>>>> +        /* dup the original fd */
>>>>>> +        /* TODO: use qemu fcntl wrapper */
>>>>>> +        raw_s->fd = fcntl(s->fd, F_DUPFD_CLOEXEC, 0);
>>>>>> +        if (raw_s->fd == -1) {
>>>>>> +            ret = -1;
>>>>>> +            goto error;
>>>>>> +        }
>>>>>> +        ret = fcntl_setfl(raw_s->fd, raw_s->open_flags);
>>>>>> +    } else {
>>>>>> +        raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags, 0644);
>>>>>> +        if (raw_s->fd == -1) {
>>>>>> +            ret = -1;
>>>>>> +        }
>>>>>
>>>>> Ignoring this part for now, with qemu_dup_flags() it's going to look a
>>>>> bit different. In particular, I'm hoping that we don't get a second
>>>>> fcntl_flags enumeration here, but can just fall back to qemu_open()
>>>>> whenever qemu_dup_flags() fails.
>>>>
>>>> That will require modification to qemu_dup_flags()... I believe
>>>> qemu_dup_flags() silently filters out fcntl incompatible flags.
>>>>
>>>> Maybe it would be best to create a small helper function in osdep.c, that
>>>> fetches the fcntl_flags.  Then qemu_dup_flags() and this function would
>>>> use the same helper to fetch fcntl_flags.  The results of that would
>>>> determine if we call qemu_dup_flags() or qemu_open().
>>>>
>>>> Although, I do think it makes sense to always try qemu_open() if
>>>> qemu_dup_flags() fails for some reason.
>>
>> I'm curious why you can't always call qemu_open().
>
> I believe the original reason was that qemu_open() is more likely to
> fail, for example if the image file has been renamed/moved/deleted since
> the first open. You could still use fcntl() on an existing file
> descriptor, but reopening would fail.
>
>> Some things to consider so that fd passing doesn't break when a reopen
>> occurs.  Mainly all the concerns revolve around how fd passing keeps
>> track of references to fd sets (note: adding and removing fd set
>> references is all done in qemu_open and qemu_close).
>>
>> * When reopening, qemu_open needs to be called before qemu_close.  This
>> will prevent the reference list for an fdset from becoming empty.  If
>> qemu_close is called before qemu_open, the reference list can become
>> empty, and the fdset could be cleaned up before the qemu_open.  Then
>> qemu_open would fail.
>
> Will automatically be right when we properly implement transactional
> semantics.
>
>> * qemu_open/qemu_close need to be used rather than open/close so that
>> the references for fd passing are properly accounted for.
>
> Congratulations, you've just discovered a bug in Jeff's patches. It was
> a good idea to CC you. ;-)
>
>> * I don't think you want to call qemu_dup_flags directly since it
>> doesn't update the reference list for fd passing.  Only qemu_open and
>> qemu_close update the reference list.
>
> That's a good point, too. So probably a small wrapper that just updates
> the reference list in addition?
>

You could do that.  And yes you'd have to add code to add the new dup fd 
to an fdset's dup_fds list if in fact the fd that you dup'd was a member 
of an fdset's dup_fds list (see how qemu_close() and qemu_open() do this).

But wouldn't it be easier to just go through qemu_close() then 
qemu_open() to perform the reopen?  Then you don't have to add this 
extra code to account for fd passing.

-- 
Regards,
Corey

>>> If we can modify qemu_dup_flags() to fail if it can't provide the right
>>> set of flags, then I think we should do it - and I think we can. Even
>>> for the existing cases with fd passing it shouldn't break anything, but
>>> only add an additional safety check.
>>>
>>> And if touching the function motivates Corey to write some fd passing
>>> test cases so that you can't break it, even better. ;-)
>>
>> :) Sorry, I do plan to do this soon.  I've just been side-tracked with
>> some other things.
>
> No problem, it was just such a great opportunity to remind you. ;-)
>
> Kevin
>