Re: [Qemu-devel] [PATCH v5 3/6] add backup related monitor commands
Dietmar Maurer diet...@proxmox.com writes:
+# @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.
That is, make it a JSON array: '*devlist' : [ 'str' ] Any time that
you pass a string
through JSON that then requires further ad-hoc parsing (such as
splitting on ',' or
':'), it is a sign that your JSON representation was incorrect.
I want to use that on HMP, so I need to parse ad-hoc anyways?
HMP syntax should be designed for humans, QMP syntax for machines.
Example: HMP size and time parameters accept numbers with unit suffixes.
QMP accept only plain numbers.
Example: HMP's netdev_add takes a single argument of the form
KEY=VALUE,... QMP takes a JSON object { KEY: VALUE, ... }.
Your QMP syntax should make use of JSON as much as possible. In
particular, use JSON numbers for numeric quantities, JSON arrays for
lists, JSON objects for dictionaries.
You may have to do some ad hoc parsing to go from HMP syntax to
QAPI/QMP. In your particular case: you want a list of device names.
Device names are of the form /[a-z][-._a-z0-9]/i (see id_wellformed()).
Thus, having them in a single argument separated by /[,;:]/ is
syntactically unambiguous.
Three alternative separators are too baroque for my taste. Even if you
drop all but one, it's still ad hoc syntax. We've been there, done
that, it's bound to degenerate into inconsistent mess.
Therefore, I'd very much prefer to use generic syntax. Options I can
see:
1. Define generic syntax for lists of identifiers, in monitor.c.
2. Why new syntax when existing syntax works? Instead of
backup foo.vma dev1:dev2:dev3
have
backup foo.vma dev1 dev2 dev3
Requires support for arg_type B* in monitor.c.
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.
Agreed. In the case of backup-cancel, if you want to make the argument
optional, and have it do the right thing when only one job is active
(and only fail
if multiple jobs are running), that would also work.
The implementation may still restrict to a single backup, of course.
The initial implementation, obviously. The point is that the API
should allow for
growth, even if the initial implementation doesn't support everything that
the
API allows.
You can easily extend the API to allow multiple backups (In a fully backwards
compatible way). So there is no need to change that now.
I disagree.
You propose something like:
1. - { execute: backup,
arguments: { backup-file: foo.vma } }
- { return: { deb8e5da-5b69-46d1-86c6-1b715a22809f } }
This *deletes* any prior BackupStatus. Clearly inappropriate with
multiple backups. How exactly do you propose to extend it in a
backwards compatible way?
2. - { execute: query-backup }
This is specified to return exactly one BackupStatus.
How exactly do you propose to extend it for multiple backups?
3. - { execute: backup-cancel }
This is specified to cancel the current execute backup process.
This becomes *ill-defined* when multiple backup processes are
executing.
With multiple backup support, we still need to keep it working
unchanged when no or just one backup is executing. When more are
executing, we have to make it fail, unless an optional argument is
given.
I don't like the resulting complexity at all.
I'd rather do:
1. - { execute: backup,
arguments: { id: client-chosen-id,
backup-file: foo.vma } }
- { return: { } }
Restriction: fails if a backup already exists, regardless of its
state. This is intended to be the only bit that needs to be changed
when we go to multiple backups.
Note that I have the client pass an id, for consistency with other
QMP commands.
I could be persuaded to return an UUID, if you can demonstrate a
need for it.
2. - { execute: query-backup }
Returns status of all backups, as a list of BackupStatus. Backups
stay around until the client gets rid of them (see 4.).
If you like, you can add suitable optional arguments to get only
selected status.
3. - { execute: backup-cancel,
arguments: { id: client-chosen-id } }
If backup is in state active, make it transition to state error,
and succeed. Else fail.
4. - { execute: backup-forget,
arguments: { id: client-chosen-id } }
If backup is in state done or error, free remaining resources
associated with it (id becomes invalid) and succeed. Else fail.
Re: [Qemu-devel] [PATCH v5 3/6] add backup related monitor commands
You can easily extend the API to allow multiple backups (In a fully
backwards compatible way). So there is no need to change that now.
I disagree.
You propose something like:
1. - { execute: backup,
arguments: { backup-file: foo.vma } }
- { return: { deb8e5da-5b69-46d1-86c6-1b715a22809f } }
This *deletes* any prior BackupStatus. Clearly inappropriate with
multiple backups. How exactly do you propose to extend it in a
backwards compatible way?
It currently deletes the BackupStatus. But this is related to the current
implementation, not the API definition.
2. - { execute: query-backup }
This is specified to return exactly one BackupStatus.
How exactly do you propose to extend it for multiple backups?
Add an optional 'uuid' parameter. We can auto-select the job if there is only
one job running
3. - { execute: backup-cancel }
This is specified to cancel the current execute backup process.
This becomes *ill-defined* when multiple backup processes are
executing.
Again, we can add an optional 'uuid' parameter.
Re: [Qemu-devel] [PATCH v5 3/6] add backup related monitor commands
Dietmar Maurer diet...@proxmox.com writes:
You can easily extend the API to allow multiple backups (In a fully
backwards compatible way). So there is no need to change that now.
I disagree.
You propose something like:
1. - { execute: backup,
arguments: { backup-file: foo.vma } }
- { return: { deb8e5da-5b69-46d1-86c6-1b715a22809f } }
This *deletes* any prior BackupStatus. Clearly inappropriate with
multiple backups. How exactly do you propose to extend it in a
backwards compatible way?
It currently deletes the BackupStatus. But this is related to the
current implementation, not the API definition.
What exactly makes BackupStatus get deleted is as API as it gets :)
2. - { execute: query-backup }
This is specified to return exactly one BackupStatus.
How exactly do you propose to extend it for multiple backups?
Add an optional 'uuid' parameter. We can auto-select the job if there
is only one job running
Adds a bit of complexity for no discernable gain.
3. - { execute: backup-cancel }
This is specified to cancel the current execute backup process.
This becomes *ill-defined* when multiple backup processes are
executing.
Again, we can add an optional 'uuid' parameter.
Yes, but:
With multiple backup support, we still need to keep it working
unchanged when no or just one backup is executing. When more are
executing, we have to make it fail, unless an optional argument is
given.
Again, unnecessary complexity.
Moreover, omitting the optional argument will break hard unless you
control all monitors. Because if you don't, you never know whether
there's another backup in flight.
Evolution of APIs occasionally gets us optional parameters that are
really mandatory. Tolerable. But when a little foresight lets us avoid
such warts, we should.
Re: [Qemu-devel] [PATCH v5 3/6] add backup related monitor commands
With multiple backup support, we still need to keep it working unchanged when no or just one backup is executing. When more are executing, we have to make it fail, unless an optional argument is given. Again, unnecessary complexity. Moreover, omitting the optional argument will break hard unless you control all monitors. Because if you don't, you never know whether there's another backup in flight. Evolution of APIs occasionally gets us optional parameters that are really mandatory. Tolerable. But when a little foresight lets us avoid such warts, we should. OK, will try to fix that.
Re: [Qemu-devel] [PATCH v5 3/6] add backup related monitor commands
First pass, concentrating on interfaces, implementation mostly ignored.
Dietmar Maurer diet...@proxmox.com 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 diet...@proxmox.com
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 (1BACKUP_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
+@item backup
+@findex 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
+@item backup_cancel
+@findex 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
+@item 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
Re: [Qemu-devel] [PATCH v5 3/6] add backup related monitor commands
On 02/27/2013 03:31 AM, Markus Armbruster wrote: First pass, concentrating on interfaces, implementation mostly ignored. Dietmar Maurer diet...@proxmox.com writes: We use a generic BackupDriver struct to encapsulate all archive format related function. +# @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. This already works with /dev/fdset/nnn for fds passed with 'add-fd', if you used qemu_open() on the passed file name. +# @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. That is, make it a JSON array: '*devlist' : [ 'str' ] Any time that you pass a string through JSON that then requires further ad-hoc parsing (such as splitting on ',' or ':'), it is a sign that your JSON representation was incorrect. 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. Agreed. In the case of backup-cancel, if you want to make the argument optional, and have it do the right thing when only one job is active (and only fail if multiple jobs are running), that would also work. The implementation may still restrict to a single backup, of course. The initial implementation, obviously. The point is that the API should allow for growth, even if the initial implementation doesn't support everything that the API allows. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v5 3/6] add backup related monitor commands
+# @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. That is, make it a JSON array: '*devlist' : [ 'str' ] Any time that you pass a string through JSON that then requires further ad-hoc parsing (such as splitting on ',' or ':'), it is a sign that your JSON representation was incorrect. I want to use that on HMP, so I need to parse ad-hoc anyways? 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. Agreed. In the case of backup-cancel, if you want to make the argument optional, and have it do the right thing when only one job is active (and only fail if multiple jobs are running), that would also work. The implementation may still restrict to a single backup, of course. The initial implementation, obviously. The point is that the API should allow for growth, even if the initial implementation doesn't support everything that the API allows. You can easily extend the API to allow multiple backups (In a fully backwards compatible way). So there is no need to change that now.
