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