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 <diet...@proxmox.com> > ---
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
