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

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

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().

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.

* qemu_open/qemu_close need to be used rather than open/close so that 
the references for fd passing are properly accounted for.

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

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

-- 
Regards,
Corey