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

[Dietmar Maurer]

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

My thought was that the BackupDriver is the simplest way to allow different 
backend.
For example you can easily create a BackupDriver to write to a NBD server 
(sure, you still need
to find a way to store configuration).

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

Ok, will remove that.

> 
> > +    /* 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.

We can allow to pass fd for that later (if someone really uses it that way).

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

The solution is to not pass a path which is on NFS. But that is up to the 
management tool.
 
> 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 includes configuration, so you need a way to save that.
But I do not really get your suggestion - creating a backup without 
configuration
does not make much sense?

In future, we can allow to pass multiple config files - the vma archive format 
can
already handle that.

> > +##
> > +# @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 # #
> > address@hidden: #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'].

I want to be able to use that command on the human monitor.
That is no longer possible if I use ['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.

I do not want multiple backup jobs. You can easily run 2 jobs in sequence to 
solve above use case.