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

[Kevin Wolf]

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?

>> 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