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
