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

[Dietmar Maurer]

> > +++ 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'.

@MigrationInfo also use a string for @status. I try to use the same
style as existing code.

> > +#
> > +# @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?

IMHO second is good enough.

> > +#
> > +# @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?

no, you can't change the speed later (currently).

> Also, shouldn't the current speed be displayed as part of BackupStatus?

You can compute the actual speed from those value. 

> > +
> > +##
> >  # @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'.

will change that.
> 
> > +#
> > +# @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.

will fix that.