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

[Eric Blake]

On 02/19/2013 04:31 AM, Dietmar Maurer wrote:
> We use a generic BackupDriver struct to encapsulate all archive format
> 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.
> 
> Signed-off-by: Dietmar Maurer <address@hidden>
> ---

Focusing my review on just the QMP interface:


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

This should be an enum type, not an open-coded 'str'.

> +#
> +# @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.
> +#
> +# @end-time: #optional time (epoch) when backup job finished.

Is 1-second resolution good enough, or should we be accounting for
sub-second information?

> +#
> +# @backupfile: #optional backup file name
> +#
> +# @uuid: #optional uuid for this backup job
> +#
> +# Since: 1.5.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' } }

You can optional set the speed when starting a backup job, but can you
later change the job speed on the fly, and if so, with what command?
Also, shouldn't the current speed be displayed as part of BackupStatus?

> +
> +##
>  # @query-events:
>  #
>  # Return a list of supported QMP events by this server
> @@ -1824,6 +1857,64 @@
>    'data': { 'path': 'str' },
>    'returns': [ 'ObjectPropertyInfo' ] }
>  
> +
> +##
> +# @BackupFormat
> +#
> +# An enumeration of supported backup formats.
> +#
> +# @vma: Proxmox vma backup format
> +##
> +{ 'enum': 'BackupFormat',
> +  'data': [ 'vma' ] }
> +
> +##
> +# @backup:
> +#
> +# Starts a VM backup.
> +#
> +# @backupfile: the backup file name
> +#
> +# @format: format of the backup file
> +#
> +# @config-filename: #optional name of a configuration file to include into
> +# the backup archive.

'backupfile' vs. 'config-filename' feels inconsistent; better might be
'backup-file' and 'config-file'.

> +#
> +# @speed: #optional the maximum speed, in bytes per second
> +#

@devlist is missing.

> +# Returns: the uuid of the backup job
> +#
> +# Since: 1.5.0
> +##
> +{ 'command': 'backup', 'data': { 'backupfile': 'str', '*format': 
> 'BackupFormat',
> +                                 '*config-filename': 'str',
> +                                 '*devlist': 'str', '*speed': 'int' },
> +  'returns': 'str' }
> +
> +##
> +# @query-backup
> +#
> +# Returns information about current/last backup task.
> +#
> +# Returns: @BackupStatus
> +#
> +# Since: 1.5.0
> +##
> +{ 'command': 'query-backup', 'returns': 'BackupStatus' }
> +
> +##
> +# @backup-cancel
> +#
> +# Cancel the current executing backup process.
> +#
> +# Returns: nothing on success
> +#
> +# Notes: This command succeeds even if there is no backup process running.
> +#
> +# Since: 1.5.0
> +##
> +{ 'command': 'backup-cancel' }
> +

> +++ b/qmp-commands.hx
> @@ -889,6 +889,18 @@ EQMP
>      },
>  
>      {
> +        .name       = "backup",
> +        .args_type  = 
> "backupfile:s,format:s?,config-filename:F?,speed:o?,devlist:s?",
> +        .mhandler.cmd_new = qmp_marshal_input_backup,
> +    },
> +
> +    {
> +        .name       = "backup_cancel",

This doesn't match the spelling in the .json file.

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

signature.asc
Description: OpenPGP digital signature