[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 3/6] add backup related monitor commands
From: |
Dietmar Maurer |
Subject: |
Re: [Qemu-devel] [PATCH v4 3/6] add backup related monitor commands |
Date: |
Thu, 21 Feb 2013 06:21:32 +0000 |
> > 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.
- Re: [Qemu-devel] [PATCH v4 2/6] add basic backup support to block driver, (continued)
[Qemu-devel] [PATCH v4 6/6] add vm state to backups, Dietmar Maurer, 2013/02/20
[Qemu-devel] [PATCH v4 1/6] add documenation for new backup framework, Dietmar Maurer, 2013/02/20
[Qemu-devel] [PATCH v4 3/6] add backup related monitor commands, Dietmar Maurer, 2013/02/20
- Re: [Qemu-devel] [PATCH v4 3/6] add backup related monitor commands, Stefan Hajnoczi, 2013/02/20
- Re: [Qemu-devel] [PATCH v4 3/6] add backup related monitor commands,
Dietmar Maurer <=
- Re: [Qemu-devel] [PATCH v4 3/6] add backup related monitor commands, Stefan Hajnoczi, 2013/02/21
- Re: [Qemu-devel] [PATCH v4 3/6] add backup related monitor commands, Dietmar Maurer, 2013/02/21
- Re: [Qemu-devel] [PATCH v4 3/6] add backup related monitor commands, Stefan Hajnoczi, 2013/02/22
- Re: [Qemu-devel] [PATCH v4 3/6] add backup related monitor commands, Dietmar Maurer, 2013/02/22
- Re: [Qemu-devel] [PATCH v4 3/6] add backup related monitor commands, Stefan Hajnoczi, 2013/02/25
- Re: [Qemu-devel] [PATCH v4 3/6] add backup related monitor commands, Dietmar Maurer, 2013/02/27
- Re: [Qemu-devel] [PATCH v4 3/6] add backup related monitor commands, Stefan Hajnoczi, 2013/02/28
- Re: [Qemu-devel] [PATCH v4 3/6] add backup related monitor commands, Dietmar Maurer, 2013/02/28
- Re: [Qemu-devel] [PATCH v4 3/6] add backup related monitor commands, Dietmar Maurer, 2013/02/28
Re: [Qemu-devel] [PATCH v4 3/6] add backup related monitor commands, Stefan Hajnoczi, 2013/02/21