On 09/25/2012 03:42 PM, Eric Blake wrote:
> On 09/25/2012 10:29 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 one below the active image) - see note below
>
> This says that for 'base' <- 'mid' <- 'active', omitting 'top' is the
> same as specifying 'top':'mid'.
>
That is the intended default.
>> speed: maximum speed, in bytes/sec
>>
>> 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 an error
>> will be returned.
>
> This says that for 'base' <- 'mid' <- 'active', omitting 'top' is an
> error (because it would be the same as an explicit 'top:'active').
This is how I originally had it, and then I changed it to default to
top->backing_hd. I apparently did not remember to catch the note,
however.
>
> Let's check the code...
>
>> + if (top && has_top) {
>> + /* if we want to allow the active layer,
>> + * use 'bdrv_find_image()' here */
>> + top_bs = bdrv_find_backing_image(bs, top);
>> + } else {
>> + /* default is one below the active layer, unless that is
>> + * the base */
>> + top_bs = bs->backing_hd;
>
> Aha - the former is correct as coded (you default to 'top':'mid' in my
> example), so the 'note' in your commit message needs editing.
>
> On the other hand, when we ever DO get around to adding live commit,
> which is the saner default? That is, am I more likely to want to do
> live commit from 'active', or more likely to do commit of the layer
> below 'active'? If live commit is the more desirable case, then that
> argues that THIS commit should always error out if !has_top, and that
> the future patch will default top_bs = bs, and the rest of your commit
> message and documentation would need tweaking accordingly.
>
> I don't have a preference either way (libvirt can arrange to always pass
> the top argument, and thus avoid the confusion when top is omitted), but
> if anyone else cares, now is the time to get the API right before we are
> locked in to it.
>
I guess I don't have a strong preference either - I originally had it
the other way, but then that meant the default in the current
implementation was actually an error.
Also, I assumed (danger!) that the most common use of commit would be a
snapshot, followed by a commit of active->backing_hd. With that
assumption, it seemed like a sane default.
>> +++ b/qapi-schema.json
>> @@ -1468,6 +1468,41 @@
>> 'returns': 'str' }
>>
>> ##
>> +# @block-commit
>> +#
>> +# Live commit of data from overlay image nodes into backing nodes - i.e.,
>> +# writes data between 'top' and 'base' into 'base'.
>> +#
>> +# @device: the name of the device
>> +#
>> +# @base: #optional The file name of the backing image to write data into.
>> +# If not specified, this is the deepest backing image
>> +#
>> +# @top: #optional The file name of the backing image within the image
>> chain,
>> +# which contains the topmost data to be committed down.
>> +# If not specified, this is one layer below the active
>> +# layer (i.e. active->backing_hd).
>> +#
>> +# If top == base, that is an error.
>> +#
>
> This documentation of @top matches the first paragraph of your commit
> message.
>
>> +#
>> +# @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, a generic error is returned
>> +# If @top does not exist, a generic error is returned
>
> These two lines could be merged, or even made more generic (for example,
> based on the rest of this thread, you will also be erroring out if base
> and top exist, but appear as swapped arguments):
>
> If @base or @top is invalid, a generic error is returned
>
OK
