Re: [Qemu-devel] [v2 Patch 2/9]block: raw-posix image file reopen

[Jeff Cody <jcody@redhat.com>]

On 07/31/2012 01:17 PM, Eric Blake wrote:
> On 07/30/2012 03:34 PM, Supriya Kannery wrote:
>> raw-posix driver changes for bdrv_reopen_xx functions to
>> safely reopen image files. Reopening of image files while 
>> changing hostcache dynamically is handled here.
>>
>> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
>>
>> ---
>> Index: qemu/block/raw.c
>> ===================================================================
> 
>> +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
>> +                              int flags)
>> +{
>> +    BDRVRawReopenState *raw_rs = g_malloc0(sizeof(BDRVRawReopenState));
>> +    BDRVRawState *s = bs->opaque;
>> +    int ret = 0;
>> +
>> +    raw_rs->reopen_state.bs = bs;
>> +
>> +    /* stash state before reopen */
>> +    raw_rs->stash_s = g_malloc0(sizeof(BDRVRawState));
>> +    raw_stash_state(raw_rs->stash_s, s);
>> +    s->fd = dup3(raw_rs->stash_s->fd, s->fd, O_CLOEXEC);
> 
> You called it out in your cover letter, but just pointing out that this
> is one of the locations that needs a conditional fallback to
> dup2/qemu_set_cloexec if dup3 and O_CLOEXEC are missing.
> 
> More importantly, though, you want this to be fcntl(F_DUP_CLOEXEC) and
> NOT dup3, so that you are duplicating to the first available fd rather
> than accidentally overwriting whatever s->fd happened to be on entry.
> Also, where is your error checking that you didn't just assign s->fd to
> -1?  It looks like your goal here is to stash a copy of the fd, so that
> on failure you can then pivot over to your copy.

In addition, as it is written above will always assign -1 to s->fd, and
stash_s->fd every time, with errno "Bad file descriptor".

The function raw_stash_state() does not save s->fd, but instead sets the
stashed copy (raw_rs->stash_s->fd) to be -1.

Then this:
    s->fd = dup3(raw_rs->stash_s->fd, s->fd, O_CLOEXEC);

assigns s->fd to -1.

I don't think dup3() will work at all, because if -1 is used for one of the
file descriptors, that is EBADF, and if stash_s->fd == s->fd, that is
EINVAL.

So I agree, I think we definitely want fcntl(F_DUP_CLOEXEC) here.

> 
>> +
>> +    *prs = &(raw_rs->reopen_state);
>> +
>> +    /* Flags that can be set using fcntl */
>> +    int fcntl_flags = BDRV_O_NOCACHE;
> 
> This variable name is misleading; fcntl_flags in my mind implies O_* not
> BDRV_O_*, as we are not guaranteed that they are the same values.  Maybe
> bdrv_flags is a better name.  Or are you trying to list the subset of
> BDRV_O flags that can be changed via mapping to the appropriate O_ flag
> during fcntl?
> 
>> +
>> +    if ((bs->open_flags & ~fcntl_flags) == (flags & ~fcntl_flags)) {
>> +        if ((flags & BDRV_O_NOCACHE)) {
>> +            s->open_flags |= O_DIRECT;
>> +        } else {
>> +            s->open_flags &= ~O_DIRECT;
>> +        }
>> +        ret = fcntl_setfl(s->fd, s->open_flags);
>> +    } else {
>> +
>> +        /* close and reopen using new flags */
>> +        bs->drv->bdrv_close(bs);
>> +        ret = bs->drv->bdrv_file_open(bs, bs->filename, flags);
>> +    }
> 
> At any rate, in spite of the poor choice of naming for fcntl_flags, the
> logic here looked correct - if the only BDRV_O_ bits that changed
> between existing flags and the requested flags can be handled by fcntl,
> then use fcntl to change them, otherwise open from scratch.
> 
>> +static void raw_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs)
>> +{
>> +    BDRVRawReopenState *raw_rs;
>> +    BDRVRawState *s = bs->opaque;
>> +
>> +    raw_rs = container_of(rs, BDRVRawReopenState, reopen_state);
>> +
>> +    /* revert to stashed state */
>> +    if (s->fd != -1) {
>> +        close(s->fd);
>> +    }
> 
> At first, I worried that you needed to use dup3() here to restore s->fd
> from the cached fd...
> 
>> +    raw_revert_state(s, raw_rs->stash_s);
> 
> then I read the code for raw_revert_state, where you are rewriting the
> state to permanently use the dup'd copy rather than trying to revert
> back to the old fd number.  As long as we are sure that all other points
> in the code base always go through s->fd rather than caching the old
> value, and therefore don't have a stale reference, then swapping the
> struct instead of using dup3 to restore the exact fd value we previously
> had should work.
>