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

[Eric Blake]

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 <address@hidden>
> 
> ---
> 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.

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

-- 
Eric Blake   address@hidden    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature