Re: [Qemu-devel] [PATCH 6/8] QAPI: add command for live block commit, 'block-commit'

[Jeff Cody <jcody@redhat.com>]

On 09/14/2012 09:05 PM, Eric Blake wrote:
> On 09/14/2012 07:41 AM, Jeff Cody wrote:
>> The command for live block commit is added, which has the following
>> arguments:
>>
>> device: the block device to perform the commit on (mandatory)
>> base:   the base image to commit into; optional (if not specified,
>>         it is the underlying original image)
>> top:    the top image of the commit - all data from inside top down
>>         to base will be committed into base. optional (if not specified,
>>         it is the active image) - see note below
>> speed:  maximum speed, in bytes/sec
>> on_error: action to take on error (optional - default is report)
> 
> Shouldn't this be on-error, with a dash?  Also, this doesn't match the
> actual commit, since you pulled it while waiting to rebase on Paolo's
> patches.
> 

Yes, thanks, I need to update the commit message.

>>
>> note: eventually this will support merging down the active layer,
>>       but that code is not yet complete.  If the active layer is passed
>>       in currently as top, or top is left to the default, then the error
>>       QERR_TOP_NOT_FOUND will be returned.
> 
> I don't think we need a new error category for this particular failure
> (and in 4/8, you labeled the error as generic); so it is sufficient to
> state that 'an error will be returned'.
> 

Yes, QERR_TOP_NOT_FOUND will be a generic error, so I will just note that
an error will be returned, as you suggest.

>>
>> The is done as a block job, so upon completion a BLOCK_JOB_COMPLETED will
>> be emitted.
>>
> 
>> +++ b/blockdev.c
>> @@ -825,7 +825,6 @@ exit:
>>      return;
>>  }
>>  
>> -
>>  static void eject_device(BlockDriverState *bs, int force, Error **errp)
> 
> Spurious whitespace change.
> 

Thanks

> 
>> +void qmp_block_commit(const char *device,
>> +                      bool has_base, const char *base,
>> +                      bool has_top, const char *top,
>> +                      bool has_speed, int64_t speed,
>> +                      Error **errp)
>> +{
>> +    BlockDriverState *bs;
>> +    BlockDriverState *base_bs, *top_bs;
>> +    Error *local_err = NULL;
>> +    /* This will be part of the QMP command, if/when the
>> +     * BlockdevOnError change for blkmirror makes it in
>> +     */
>> +    BlockErrorAction on_error = BLOCK_ERR_REPORT;
>> +
>> +    /* drain all i/o before commits */
>> +    bdrv_drain_all();
> 
> Is this technically necessary for now?  Since you are forbidding actions
> on the active image for now, and changing the active image (via snapshot
> or pull) already drains all I/O, there should be nothing remaining to
> drain for any of the backing files.  Obviously, it will be necessary in
> the future when you do add support for committing the active layer.
> 

Technically, it is not needed for the current iteration, but since this
is the command handler that will be the same as when we add the active
layer, and I know we will eventually support it, I went ahead and added
it now. It shouldn't hurt anything.


>> +++ b/qapi-schema.json
>> @@ -1404,6 +1404,41 @@
>>    'returns': 'str' }
>>  
>>  ##
>> +# @block-commit
>> +#
>> +# Live commit of data from child image nodes into parent nodes - i.e.,
>> +# writes data between 'top' and 'base' into 'base'.
>> +#
>> +# @device:  the name of the device
>> +#
>> +# @base:   #optional The file name of the parent image of the device to write
>> +#                    data into.  If not specified, this is the original parent
>> +#                    image.
> 
> I though we wanted to fix the terminology to avoid 'parent'; how about:
> 
> The file name of the backing image to write data into.  If not
> specified, this is the deepest backing image.
> 

OK, sounds good.


>> +#
>> +# @top:    #optional The file name of the child image, above which data will
>> +#                    not be committed down.  If not specified, this is one
>> +#                    layer below the active layer (i.e. active->backing_hd).
> 
> Again, how about:
> 
> The file name of the backing image within the chain which contains the
> data to be committed down.  If not specified...
> 

Hmm, how about a slight tweak:

The file name of the backing image within the image chain, which
contains the topmost data to be committed down.  If not specified...

Since it doesn't necessarily have _the_ data to commit down, but
indicates the upper bounds of the data to commit down.

>> +#
>> +#                    If top == base, that is an error.
>> +#
>> +#
>> +# @speed:  #optional the maximum speed, in bytes per second
>> +#
>> +# Returns: Nothing on success
>> +#          If commit or stream is already active on this device, DeviceInUse
>> +#          If @device does not exist, DeviceNotFound
>> +#          If image commit is not supported by this device, NotSupported
>> +#          If @base does not exist, BaseNotFound
>> +#          If @top does not exist, TopNotFound
> 
> BaseNotFound is a generic error, and I'm arguing that TopNotFound should
> be likewise.
> 

Agree - both TopNotFound and BaseNotFound are of type ERROR_CLASS_GENERIC_ERROR