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

[Jeff Cody <jcody@redhat.com>]

On 09/20/2012 10:10 AM, Kevin Wolf wrote:
> Am 18.09.2012 20:53, schrieb Jeff Cody:
>> 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>
> 
>> +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;
> 
> You can immediately set s->aio_ctx instead of going through
> BDRVRawReopenState with it. It seems to be valid to have it present
> while use_aio = 0. If it wasn't valid, you'd have to free the context
> when reopening without Linux AIO.
>

Good catch, thanks.

>> +
>> +    raw_parse_flags(state->flags, &raw_s->open_flags);
>> +    raw_set_aio(&raw_s->aio_ctx, &raw_s->use_aio, state->flags);
> 
> At least you're consistently omitting the error check. :-)
> 

Thanks, fixed.

I guess I am an optimist at heart :)

>> +
>> +    raw_s->fd = -1;
>> +
>> +    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 */
> 
> Hm, still not addressed?
> 

No. I mentioned this in the cover letter for v2.  I'd rather see the
qemu fcntl wrapper happen as a separate series, and then come back and
update this, if that is OK.  I'm afraid changes to qemu_open or
qemu_dup_flags would delay getting this series in.

Although Eric is right, I do need to ifdef the F_DUPFD_CLOEXEC.


> Kevin
>