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

[Eric Blake]

On 11/30/2012 02:22 AM, Dietmar Maurer wrote:
> We use a generic BackupDriver struct to encaplulated all archive format

s/encaplulated/encapsulated/

> related function.
> 
> Another option would be to simply dump <devid,cluster_num,cluster_data> to
> the output fh (pipe), and an external binary saves the data. That way we
> could move the whole archive format related code out of qemu.

I'm coming into this review late, so I'm not sure what your series is
adding that cannot already be done with external snapshots and migration
to file.  But any time someone proposes new QMP commands that libvirt
might have to interact with, I try to chime in:

> 
> Signed-off-by: Dietmar Maurer <address@hidden>
> ---
>  backup.h         |   14 +++
>  blockdev.c       |  337 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hmp-commands.hx  |   31 +++++
>  hmp.c            |   63 ++++++++++
>  hmp.h            |    3 +
>  monitor.c        |    7 +
>  qapi-schema.json |   81 +++++++++++++
>  qmp-commands.hx  |   27 +++++
>  8 files changed, 563 insertions(+), 0 deletions(-)
> 

> +++ b/qapi-schema.json
> @@ -358,6 +358,39 @@
>  { 'type': 'EventInfo', 'data': {'name': 'str'} }
>  
>  ##
> +# @BackupStatus:
> +#
> +# Detailed backup status.
> +#
> +# @status: #optional string describing the current backup status.
> +#          Tthis can be 'active', 'done', 'error'. If this field is not

s/Tthis/This/

> +#          returned, no backup process has been initiated
> +#
> +# @errmsg: #optional error message (only returned if status is 'error')
> +#
> +# @total: #optional total amount of bytes involved in the backup process
> +#
> +# @transferred: #optional amount of bytes already backed up.
> +#
> +# @zero-bytes: #optional amount of 'zero' bytes detected.
> +#
> +# @start-time: #optional time (epoch) when backup job started.

Should you account for sub-second resolution here

> +#
> +# @end-time: #optional time (epoch) when backup job finished.

and here?

> +#
> +# @backupfile: #optional backup file name
> +#
> +# @uuid: #optional uuid for this backup job
> +#
> +# Since: 1.4.0
> +##
> +{ 'type': 'BackupStatus',
> +  'data': {'*status': 'str', '*errmsg': 'str', '*total': 'int',
> +           '*transferred': 'int', '*zero-bytes': 'int',
> +           '*start-time': 'int', '*end-time': 'int',
> +           '*backupfile': 'str', '*uuid': 'str' } }
> +
> +##
>  # @query-events:
>  #
>  # Return a list of supported QMP events by this server
> @@ -1764,6 +1797,54 @@
>    'data': { 'path': 'str' },
>    'returns': [ 'ObjectPropertyInfo' ] }
>  
> +
> +##
> +# @backup:
> +#
> +# Starts a VM backup.
> +#
> +# @backupfile: the backup file name

I guess the fdset mechanism is how libvirt would pass in a pipe fd
rather than supplying an actual file name.  Does your implementation
allow for pipes, or must it be seekable?

> +#
> +# @format: format of the backup file

Rather than letting this be a free-form string, it would be nicer as an
enum of actually supported formats.

> +#
> +# @config-filename: #optional name of a configuration file to include into
> +# the backup archive.
> +#
> +# @speed: #optional the maximum speed, in bytes per second
> +#
> +# Returns: the uuid of the backup job

UUID in raw byte format, or in ASCII format?  (Assuming the latter)

> +#
> +# Since: 1.4.0
> +##
> +{ 'command': 'backup', 'data': { 'backupfile': 'str', '*format': 'str',

backupfile sounds like a run-on; is it any better to name it backup-file?

> +                                 '*config-filename': 'str',

especially when compared with config-filename

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

signature.asc
Description: OpenPGP digital signature