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

[Markus Armbruster]

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

Sounds like this patch is much more than just monitor commands.

> ---
>  backup.h         |   15 ++
>  blockdev.c       |  423 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hmp-commands.hx  |   31 ++++
>  hmp.c            |   63 ++++++++
>  hmp.h            |    3 +
>  monitor.c        |    7 +
>  qapi-schema.json |   95 ++++++++++++
>  qmp-commands.hx  |   27 ++++
>  8 files changed, 664 insertions(+), 0 deletions(-)
>
> diff --git a/backup.h b/backup.h
> index 9b1ea1c..22598a6 100644
> --- a/backup.h
> +++ b/backup.h
> @@ -14,6 +14,9 @@
>  #ifndef QEMU_BACKUP_H
>  #define QEMU_BACKUP_H
>  
> +#include <uuid/uuid.h>
> +#include "block/block.h"
> +

What do you need block.h for?

>  #define BACKUP_CLUSTER_BITS 16
>  #define BACKUP_CLUSTER_SIZE (1<<BACKUP_CLUSTER_BITS)
>  #define BACKUP_BLOCKS_PER_CLUSTER (BACKUP_CLUSTER_SIZE/BDRV_SECTOR_SIZE)
> @@ -27,4 +30,16 @@ int backup_job_create(BlockDriverState *bs, BackupDumpFunc 
> *backup_dump_cb,
>                        BlockDriverCompletionFunc *backup_complete_cb,
>                        void *opaque, int64_t speed);
>  
> +typedef struct BackupDriver {
> +    const char *format;
> +    void *(*open)(const char *filename, uuid_t uuid, Error **errp);
> +    int (*close)(void *opaque, Error **errp);
> +    int (*register_config)(void *opaque, const char *name, gpointer data,

void * rather than gpointer, please!

> +                              size_t data_len);
> +    int (*register_stream)(void *opaque, const char *devname, size_t size);
> +    int (*dump)(void *opaque, uint8_t dev_id, int64_t cluster_num,
> +                   unsigned char *buf, size_t *zero_bytes);
> +    int (*complete)(void *opaque, uint8_t dev_id, int ret);
> +} BackupDriver;
> +
>  #endif /* QEMU_BACKUP_H */
[...]
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 64008a9..0f178d8 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -83,6 +83,35 @@ STEXI
>  Copy data from a backing file into a block device.
>  ETEXI
>  
> +   {
> +        .name       = "backup",
> +        .args_type  = "backupfile:s,speed:o?,devlist:s?",
> +        .params     = "backupfile [speed [devlist]]",
> +        .help       = "create a VM Backup.",
> +        .mhandler.cmd = hmp_backup,
> +    },
> +
> +STEXI
> address@hidden backup
> address@hidden backup
> +Create a VM backup.

-v please.

> +ETEXI
> +
> +    {
> +        .name       = "backup_cancel",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "cancel the current VM backup",
> +        .mhandler.cmd = hmp_backup_cancel,
> +    },
> +
> +STEXI
> address@hidden backup_cancel
> address@hidden backup_cancel
> +Cancel the current VM backup.
> +
> +ETEXI
> +
>      {
>          .name       = "block_job_set_speed",
>          .args_type  = "device:B,speed:o",

Recommend to do HMP in a separate patch, for easier review.

> @@ -1630,6 +1659,8 @@ show CPU statistics
>  show user network stack connection states
>  @item info migrate
>  show migration status
> address@hidden info backup
> +show backup status
>  @item info migrate_capabilities
>  show current migration capabilities
>  @item info migrate_cache_size
> diff --git a/hmp.c b/hmp.c
> index 2f47a8a..b2c1f23 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -131,6 +131,38 @@ void hmp_info_mice(Monitor *mon, const QDict *qdict)
>      qapi_free_MouseInfoList(mice_list);
>  }
>  
> +void hmp_info_backup(Monitor *mon, const QDict *qdict)
> +{
> +    BackupStatus *info;
> +
> +    info = qmp_query_backup(NULL);
> +    if (info->has_status) {
> +        if (info->has_errmsg) {
> +            monitor_printf(mon, "Backup status: %s - %s\n",
> +                           info->status, info->errmsg);
> +        } else {
> +            monitor_printf(mon, "Backup status: %s\n", info->status);
> +        }
> +    }
> +    if (info->has_backup_file) {
> +        int per = (info->has_total && info->total &&
> +            info->has_transferred && info->transferred) ?
> +            (info->transferred * 100)/info->total : 0;
> +        int zero_per = (info->has_total && info->total &&
> +                        info->has_zero_bytes && info->zero_bytes) ?
> +            (info->zero_bytes * 100)/info->total : 0;
> +        monitor_printf(mon, "Backup file: %s\n", info->backup_file);
> +        monitor_printf(mon, "Backup uuid: %s\n", info->uuid);
> +        monitor_printf(mon, "Total size: %zd\n", info->total);
> +        monitor_printf(mon, "Transferred bytes: %zd (%d%%)\n",
> +                       info->transferred, per);
> +        monitor_printf(mon, "Zero bytes: %zd (%d%%)\n",
> +                       info->zero_bytes, zero_per);
> +    }
> +
> +    qapi_free_BackupStatus(info);
> +}
> +
>  void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>  {
>      MigrationInfo *info;
> @@ -998,6 +1030,37 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, &error);
>  }
>  
> +void hmp_backup_cancel(Monitor *mon, const QDict *qdict)
> +{
> +    Error *errp = NULL;
> +
> +    qmp_backup_cancel(&errp);
> +
> +    if (error_is_set(&errp)) {
> +        monitor_printf(mon, "%s\n", error_get_pretty(errp));
> +        error_free(errp);
> +        return;
> +    }
> +}
> +
> +void hmp_backup(Monitor *mon, const QDict *qdict)
> +{
> +    const char *backup_file = qdict_get_str(qdict, "backup-file");
> +    const char *devlist = qdict_get_try_str(qdict, "devlist");
> +    int64_t speed = qdict_get_try_int(qdict, "speed", 0);
> +
> +    Error *errp = NULL;
> +
> +    qmp_backup(backup_file, true, BACKUP_FORMAT_VMA, false, NULL, !!devlist,
> +               devlist, qdict_haskey(qdict, "speed"), speed, &errp);
> +
> +    if (error_is_set(&errp)) {
> +        monitor_printf(mon, "%s\n", error_get_pretty(errp));
> +        error_free(errp);
> +        return;
> +    }
> +}
> +
>  void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict)
>  {
>      Error *error = NULL;
> diff --git a/hmp.h b/hmp.h
> index 30b3c20..ad4cf80 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -28,6 +28,7 @@ void hmp_info_mice(Monitor *mon, const QDict *qdict);
>  void hmp_info_migrate(Monitor *mon, const QDict *qdict);
>  void hmp_info_migrate_capabilities(Monitor *mon, const QDict *qdict);
>  void hmp_info_migrate_cache_size(Monitor *mon, const QDict *qdict);
> +void hmp_info_backup(Monitor *mon, const QDict *qdict);
>  void hmp_info_cpus(Monitor *mon, const QDict *qdict);
>  void hmp_info_block(Monitor *mon, const QDict *qdict);
>  void hmp_info_blockstats(Monitor *mon, const QDict *qdict);
> @@ -65,6 +66,8 @@ void hmp_eject(Monitor *mon, const QDict *qdict);
>  void hmp_change(Monitor *mon, const QDict *qdict);
>  void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict);
>  void hmp_block_stream(Monitor *mon, const QDict *qdict);
> +void hmp_backup(Monitor *mon, const QDict *qdict);
> +void hmp_backup_cancel(Monitor *mon, const QDict *qdict);
>  void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
>  void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
>  void hmp_block_job_pause(Monitor *mon, const QDict *qdict);
> diff --git a/monitor.c b/monitor.c
> index 6a0f257..e4a810c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2666,6 +2666,13 @@ static mon_cmd_t info_cmds[] = {
>      },
>  #endif
>      {
> +        .name       = "backup",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "show backup status",
> +        .mhandler.cmd = hmp_info_backup,
> +    },
> +    {
>          .name       = "migrate",
>          .args_type  = "",
>          .params     = "",
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 7275b5d..09ca8ef 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -425,6 +425,39 @@
>  { 'type': 'EventInfo', 'data': {'name': 'str'} }
>  
>  ##
> +# @BackupStatus:
> +#
> +# Detailed backup status.

Is there any non-detailed 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

Enum rather than string, please.

> +#
> +# @errmsg: #optional error message (only returned if status is 'error')
> +#
> +# @total: #optional total amount of bytes involved in the backup process

You mean total size of data to be backed up?

> +#
> +# @transferred: #optional amount of bytes already backed up.
> +#
> +# @zero-bytes: #optional amount of 'zero' bytes detected.

These are backed up more efficiently, right?

Why does the user need to know?

> +#
> +# @start-time: #optional time (epoch) when backup job started.
> +#
> +# @end-time: #optional time (epoch) when backup job finished.
> +#
> +# @backupfile: #optional backup file name
> +#
> +# @uuid: #optional uuid for this backup job

For all optional members: how is their presence related to @status?
Already answered for @errmsg, want to know for the others.

> +#
> +# Since: 1.5.0
> +##
> +{ 'type': 'BackupStatus',
> +  'data': {'*status': 'str', '*errmsg': 'str', '*total': 'int',
> +           '*transferred': 'int', '*zero-bytes': 'int',
> +           '*start-time': 'int', '*end-time': 'int',
> +           '*backup-file': 'str', '*uuid': 'str' } }
> +
> +##
>  # @query-events:
>  #
>  # Return a list of supported QMP events by this server
> @@ -1824,6 +1857,68 @@
>    '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.
> +#
> +# @backup-file: the backup file name

We'll need support for backup up to a fd passed with SCM rights.

> +#
> +# @format: format of the backup file
> +#
> +# @config-filename: #optional name of a configuration file to include into
> +# the backup archive.

This lets you include a single host file in the backup.  Can be
anything, not just configuration.

Like previous reviewers, I think backing up some other piece of
software's configuration file is none of QEMU's business.  If you want
to include it in the backup, better do it at the appropriate level of
the stack.  If you think that's not practical, please explain why.

> +#
> +# @speed: #optional the maximum speed, in bytes per second
> +#
> +# @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.

> +#
> +# Returns: the uuid of the backup job
> +#
> +# Since: 1.5.0
> +##
> +{ 'command': 'backup', 'data': { 'backup-file': 'str',
> +                                 '*format': 'BackupFormat',
> +                                 '*config-file': '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' }
> +
>  ##
>  # @qom-get:
>  #

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.

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

> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 799adea..17e381b 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -889,6 +889,18 @@ EQMP
>      },
>  
>      {
> +        .name       = "backup",
> +        .args_type  = 
> "backup-file:s,format:s?,config-file:F?,speed:o?,devlist:s?",
> +        .mhandler.cmd_new = qmp_marshal_input_backup,
> +    },
> +
> +    {
> +        .name       = "backup-cancel",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_input_backup_cancel,
> +    },

Documentation missing.

> +
> +    {
>          .name       = "block-job-set-speed",
>          .args_type  = "device:B,speed:o",
>          .mhandler.cmd_new = qmp_marshal_input_block_job_set_speed,
> @@ -2566,6 +2578,21 @@ EQMP
>      },
>  
>  SQMP
> +
> +query-backup
> +-------------
> +
> +Backup status.
> +
> +EQMP
> +
> +    {
> +        .name       = "query-backup",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_input_query_backup,
> +    },
> +
> +SQMP
>  migrate-set-capabilities
>  -------