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

[Stefan Hajnoczi]

On Wed, Feb 20, 2013 at 10:32:00AM +0100, 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.

That sounds like the NBD option - write the backup to an NBD disk image.
The NBD server process can take the WRITE requests and do whatever it
wants.

The disadvantage of using NBD is that it strictly transports block data.
It doesn't really have the concept of VM configuration or device state.

> diff --git a/backup.h b/backup.h
> index d9395bc..c8ba153 100644
> --- a/backup.h
> +++ b/backup.h
> @@ -29,4 +29,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_cb)(const char *filename, uuid_t uuid, Error **errp);
> +    int (*close_cb)(void *opaque, Error **errp);
> +    int (*register_config_cb)(void *opaque, const char *name, gpointer data,
> +                              size_t data_len);
> +    int (*register_stream_cb)(void *opaque, const char *devname, size_t 
> size);
> +    int (*dump_cb)(void *opaque, uint8_t dev_id, int64_t cluster_num,
> +                   unsigned char *buf, size_t *zero_bytes);
> +    int (*complete_cb)(void *opaque, uint8_t dev_id, int ret);

No need to suffix the functions with _cb.

> +    /* add configuration file to archive */
> +    if (has_config_file) {
> +        char *cdata = NULL;
> +        gsize clen = 0;
> +        GError *err = NULL;
> +        if (!g_file_get_contents(config_file, &cdata, &clen, &err)) {
> +            error_setg(errp, "unable to read file '%s'", config_file);
> +            goto err;
> +        }
> +
> +        const char *basename = g_path_get_basename(config_file);
> +        if (driver->register_config_cb(writer, basename, cdata, clen) < 0) {
> +            error_setg(errp, "register_config failed");
> +            g_free(cdata);
> +            goto err;
> +        }
> +        g_free(cdata);
> +    }

This is doing too much inside QEMU.

First off, when QEMU is sandboxed or run with SELinux, we cannot expect
to be able to access arbitrary files.  This is why new QMP commands of
this sort usually support file descriptor passing - then a more
privileged component can give QEMU access.

g_file_get_contents() hangs the VM if the file is over NFS while the
server is down.  This is bad for reliability.

Management tools (proxmox, libvirt, or something else) handle the VM
configuration.  It may not be a single file.  Saving external VM
configuration is out of scope for QEMU.

> +##
> +# @backup:
> +#
> +# Starts a VM backup.
> +#
> +# @backup-file: the backup file name
> +#
> +# @format: format of the backup file
> +#
> +# @config-filename: #optional name of a configuration file to include into
> +# the backup archive.
> +#
> +# @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.
> +#
> +# 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' },

devlist should be ['String'].

> 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,
> +    },

We might want to more than one backup if the guest has multiple disks.
For example, the database disk is backed up independently of the main OS
disk.

This API only allows one backup to run at a time.