Re: [Qemu-devel] [PATCH v5 3/6] add backup related monitor commands

[Eric Blake]

On 02/27/2013 03:31 AM, Markus Armbruster wrote:
> First pass, concentrating on interfaces, implementation mostly ignored.
> 
> Dietmar Maurer <address@hidden> writes:
> 
>> We use a generic BackupDriver struct to encapsulate all archive format
>> related function.
>>

>> +# @backup:
>> +#
>> +# Starts a VM backup.
>> +#
>> +# @backup-file: the backup file name
> 
> We'll need support for backup up to a fd passed with SCM rights.

This already works with /dev/fdset/nnn for fds passed with 'add-fd', if
you used qemu_open() on the passed file name.

>> +# @devlist: #optional list of block device names (separated by ',', ';'
>> +# or ':'). By default the backup includes all writable block devices.
> 
> Make this a proper list, please.

That is, make it a JSON array: '*devlist' : [ 'str' ]
Any time that you pass a string through JSON that then requires further
ad-hoc parsing (such as splitting on ',' or ':'), it is a sign that your
JSON representation was incorrect.

> 
> You bake the "only one backup at a time" restriction right into the API.
> That's fine if and only if multiple backups cannot possibly make sense.
> 
> Unless we agree that's the case, the API needs to be designed so it can
> grow: backup needs to return an ID (it already does), backup-cancel
> needs to take it as argument (it doesn't), and query-backup either
> returns a list, or takes an ID argument.

Agreed.  In the case of backup-cancel, if you want to make the argument
optional, and have it do the right thing when only one job is active
(and only fail if multiple jobs are running), that would also work.

> 
> The implementation may still restrict to a single backup, of course.

The initial implementation, obviously.  The point is that the API should
allow for growth, even if the initial implementation doesn't support
everything that the API allows.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature