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

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

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