Re: [Qemu-devel] [PATCH v3 09/19] block: raw-posix image file reopen

[Eric Blake]

On 09/18/2012 12:53 PM, Jeff Cody wrote:
> This is derived from the Supriya Kannery's reopen patches.
> 
> This contains the raw-posix driver changes for the bdrv_reopen_*
> functions.  All changes are staged into a temporary scratch buffer
> during the prepare() stage, and copied over to the live structure
> during commit().  Upon abort(), all changes are abandoned, and the
> live structures are unmodified.
> 
> The _prepare() will create an extra fd - either by means of a dup,
> if possible, or opening a new fd if not (for instance, access
> control changes).  Upon _commit(), the original fd is closed and
> the new fd is used.  Upon _abort(), the duplicate/new fd is closed.
> 
> Signed-off-by: Jeff Cody <address@hidden>
> ---
>  block/raw-posix.c | 99 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 99 insertions(+)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 6bf5480..edd1eca 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -138,6 +138,15 @@ typedef struct BDRVRawState {
>  #endif
>  } BDRVRawState;
>  
> +typedef struct BDRVRawReopenState {
> +    int fd;
> +    int open_flags;
> +#ifdef CONFIG_LINUX_AIO
> +    int use_aio;
> +    void *aio_ctx;
> +#endif

These members are conditional...

> +static int raw_reopen_prepare(BDRVReopenState *state,
> +                              BlockReopenQueue *queue, Error **errp)
> +{
> +    BDRVRawState *s;
> +    BDRVRawReopenState *raw_s;
> +    int ret = 0;
> +
> +    assert(state != NULL);
> +    assert(state->bs != NULL);
> +
> +    s = state->bs->opaque;
> +
> +    state->opaque = g_malloc0(sizeof(BDRVRawReopenState));
> +    raw_s = state->opaque;
> +    raw_s->use_aio = s->use_aio;
> +    raw_s->aio_ctx = s->aio_ctx;

...but you are unconditionally assigning into them.  This will introduce
compile failures on other platforms.

> +    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);

F_DUPFD_CLOEXEC is not defined everywhere yet; you still need to address
your TODO.

> +
> +    /* If we cannot use fctnl, or fcntl failed, fall back to qemu_open() */

s/fctnl/fcntl/

> +static void raw_reopen_commit(BDRVReopenState *state)
> +{
> +    BDRVRawReopenState *raw_s = state->opaque;
> +    BDRVRawState *s = state->bs->opaque;
> +
> +    s->open_flags = raw_s->open_flags;
> +
> +    qemu_close(s->fd);
> +    s->fd = raw_s->fd;
> +    s->use_aio = raw_s->use_aio;
> +    s->aio_ctx = raw_s->aio_ctx;

Again, more unconditional use of conditional members.

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

signature.asc
Description: OpenPGP digital signature