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

[Jeff Cody <jcody@redhat.com>]

On 09/18/2012 05:20 PM, Eric Blake wrote:
> 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 <jcody@redhat.com>
>> ---
>>  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.

Argh.  Thanks.

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