Re: [Qemu-devel] [PATCH v2 2/4] Curling: cmdline interface.
On Wed, 2013-10-09 at 06:02 -0600, Eric Blake wrote:
> [your emailer munged the reply, making it a bit hard to read. Are you
> set for plain-text-only mail to the list?]
Thanks VERY much for remind me that, I'm using another client now.
> On 10/09/2013 12:49 AM, junqing.w...@cs2c.com.cn wrote:
>
> > >> +++ b/hmp.c
> >>> @@ -1213,10 +1213,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
> >>> int detach = qdict_get_try_bool(qdict, "detach", 0);
> >>> int blk = qdict_get_try_bool(qdict, "blk", 0);
> >>> int inc = qdict_get_try_bool(qdict, "inc", 0);
> >>> + int ft = qdict_get_try_bool(qdict, "ft", 0);
> >>
> >> Why two spaces?
> >
> > To align the '=', I will remove them if you like.
>
> It's not a problem with me either way, other than we have a lot of code
> that doesn't care about alignment and consistently uses one space, and a
> fair amount of code where everything in a block of code is consistently
> aligned. But your patch was neither, in the context of the block it
> lives within - if you're going to align, then line up everything with
> the longest line 'int detach' (including blk and inc).
>
oh, I got it, you are right, I missed the longest line 'int detach ...'
I'm not going to align them.
> >
> > >
> >>> +++ b/qapi-schema.json
> >>> @@ -2420,7 +2420,8 @@
> >>> # Since: 0.14.0
> >>> ##
> >>> { 'command': 'migrate',
> >>> - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach':
> >>> 'bool' } }
> >>> + 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach':
> >>> 'bool',
> >>> + '*ft': 'bool' } }
> >>
> >> Missing documentation, including mention that the new option was only
> >> made available in 1.7. We still don't have introspection; is there some
> >> other means by which libvirt and other management apps can tell whether
> >> this feature is available?
> >
> > I'm not clear about how to do that, could you pls give me some hints, where
> > to
> > add code and documentation.
>
> As for the documentation, qapi-schema.json has plenty of examples (look
> for a field with "(since 1.7)" as a hint for how to document an optional
> field added in a later release than the main struct).
I see. Thanks.
>
> As for the introspection, Amos Kong was most recently working on trying
> to add that (but missed the 1.6 deadline, and I haven't seen work on it
> since). Introspection is not a hard requirement, but it makes it harder
> for libvirt to know if it can use 'ft':true if there is no other
> 'query-*' command that it can call first that would give it a hint that
> this is a new enough qemu to support 'ft' during migration. Maybe even
> having something listed under query-migrate-capabilities would be
> sufficient (ie. modify the 'MigrationCapability' enum to advertise a new
> capability).
Adding a new migration capability is a work-around method. we turn on ft
by using the -f option instead of setting fault-tolerant-capability to
true. I hesitate to add it.
What about adding a query for the options of migration similar to
@query-command-line-options?
Re: [Qemu-devel] [PATCH v2 2/4] Curling: cmdline interface.
[your emailer munged the reply, making it a bit hard to read. Are you
set for plain-text-only mail to the list?]
On 10/09/2013 12:49 AM, junqing.w...@cs2c.com.cn wrote:
> >> +++ b/hmp.c
>>> @@ -1213,10 +1213,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
>>> int detach = qdict_get_try_bool(qdict, "detach", 0);
>>> int blk = qdict_get_try_bool(qdict, "blk", 0);
>>> int inc = qdict_get_try_bool(qdict, "inc", 0);
>>> + int ft = qdict_get_try_bool(qdict, "ft", 0);
>>
>> Why two spaces?
>
> To align the '=', I will remove them if you like.
It's not a problem with me either way, other than we have a lot of code
that doesn't care about alignment and consistently uses one space, and a
fair amount of code where everything in a block of code is consistently
aligned. But your patch was neither, in the context of the block it
lives within - if you're going to align, then line up everything with
the longest line 'int detach' (including blk and inc).
>
> >
>>> +++ b/qapi-schema.json
>>> @@ -2420,7 +2420,8 @@
>>> # Since: 0.14.0
>>> ##
>>> { 'command': 'migrate',
>>> - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool'
>>> } }
>>> + 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool',
>>> + '*ft': 'bool' } }
>>
>> Missing documentation, including mention that the new option was only
>> made available in 1.7. We still don't have introspection; is there some
>> other means by which libvirt and other management apps can tell whether
>> this feature is available?
>
> I'm not clear about how to do that, could you pls give me some hints, where
> to
> add code and documentation.
As for the documentation, qapi-schema.json has plenty of examples (look
for a field with "(since 1.7)" as a hint for how to document an optional
field added in a later release than the main struct).
As for the introspection, Amos Kong was most recently working on trying
to add that (but missed the 1.6 deadline, and I haven't seen work on it
since). Introspection is not a hard requirement, but it makes it harder
for libvirt to know if it can use 'ft':true if there is no other
'query-*' command that it can call first that would give it a hint that
this is a new enough qemu to support 'ft' during migration. Maybe even
having something listed under query-migrate-capabilities would be
sufficient (ie. modify the 'MigrationCapability' enum to advertise a new
capability).
--
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 v2 2/4] Curling: cmdline interface.
At 2013-10-01 06:16:34,"Eric Blake" wrote: >On 09/29/2013
02:14 PM, Jules Wang wrote: >> Add an option '-f' to migration cmdline. >>
Indicating whether to enable fault tolerant or not. >> >> Signed-off-by: Jules
Wang >> --- >> .help = "migrate to
URI (using -d to not wait for completion)" >> "\n\t\t\t -b for
migration without shared storage with" >> " full copy of disk\n\t\t\t
-i for migration without " >> "shared storage with incremental copy of
disk " >> - "(base image shared between src and destination)", >> +
"(base image shared between src and destination)" >> + "\n\t\t\t -f for
fault tolerant, this is another " >> + "feature rather than migrate", >
>That sounds awkward, and overly long. Maybe go with just: > >-f for fault
tolerance mode > >and let the user then read the full documentation for what it
entails.
Agree.
>> -@item migrate [-d] [-b] [-i] @var{uri}
>> +@item migrate [-d] [-b] [-i] [-f] @var{uri}
>> @findex migrate
>> Migrate to @var{uri} (using -d to not wait for completion).
>> -b for migration with full copy of disk
>> -i for migration with incremental copy of disk (base image is shared)
>> +-f for fault tolerant
>
>Can -d and -f be used at the same time, or are they exclusive?
AFAK, The migration is always detached(In the code, the -d option is always
false), -d and -f can be used at the same time with no doubt.
qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, !!ft, ft, &err);
By the way, neither -b nor -i could be used at the same time with -f, fault
tolerant needs shared storage.
>> +++ b/hmp.c
>> @@ -1213,10 +1213,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
>> int detach = qdict_get_try_bool(qdict, "detach", 0);
>> int blk = qdict_get_try_bool(qdict, "blk", 0);
>> int inc = qdict_get_try_bool(qdict, "inc", 0);
>> + int ft = qdict_get_try_bool(qdict, "ft", 0);
>
>Why two spaces?
To align the '=', I will remove them if you like.
>
>> +++ b/qapi-schema.json
>> @@ -2420,7 +2420,8 @@
>> # Since: 0.14.0
>> ##
>> { 'command': 'migrate',
>> - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool'
>> } }
>> + 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool',
>> + '*ft': 'bool' } }
>
>Missing documentation, including mention that the new option was only
>made available in 1.7. We still don't have introspection; is there some
>other means by which libvirt and other management apps can tell whether
>this feature is available?
I'm not clear about how to do that, could you pls give me some hints, where to
add code and documentation.
>Furthermore, 'ft' is an awfully short name;
>for QMP, we prefer to use full words where possible, such as
>'fault-tolerant'.
Agree.
>--
>Eric Blake eblake redhat com+1-919-301-3266
>Libvirt virtualization library http://libvirt.org
>
Re: [Qemu-devel] [PATCH v2 2/4] Curling: cmdline interface.
On 09/29/2013 02:14 PM, Jules Wang wrote:
> Add an option '-f' to migration cmdline.
> Indicating whether to enable fault tolerant or not.
>
> Signed-off-by: Jules Wang
> ---
> .help = "migrate to URI (using -d to not wait for completion)"
> "\n\t\t\t -b for migration without shared storage with"
> " full copy of disk\n\t\t\t -i for migration without "
> "shared storage with incremental copy of disk "
> - "(base image shared between src and destination)",
> + "(base image shared between src and destination)"
> + "\n\t\t\t -f for fault tolerant, this is another "
> + "feature rather than migrate",
That sounds awkward, and overly long. Maybe go with just:
-f for fault tolerance mode
and let the user then read the full documentation for what it entails.
> -@item migrate [-d] [-b] [-i] @var{uri}
> +@item migrate [-d] [-b] [-i] [-f] @var{uri}
> @findex migrate
> Migrate to @var{uri} (using -d to not wait for completion).
> -b for migration with full copy of disk
> -i for migration with incremental copy of disk (base image is shared)
> + -f for fault tolerant
Can -d and -f be used at the same time, or are they exclusive?
> +++ b/hmp.c
> @@ -1213,10 +1213,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
> int detach = qdict_get_try_bool(qdict, "detach", 0);
> int blk = qdict_get_try_bool(qdict, "blk", 0);
> int inc = qdict_get_try_bool(qdict, "inc", 0);
> +int ft = qdict_get_try_bool(qdict, "ft", 0);
Why two spaces?
> +++ b/qapi-schema.json
> @@ -2420,7 +2420,8 @@
> # Since: 0.14.0
> ##
> { 'command': 'migrate',
> - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' }
> }
> + 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool',
> + '*ft': 'bool' } }
Missing documentation, including mention that the new option was only
made available in 1.7. We still don't have introspection; is there some
other means by which libvirt and other management apps can tell whether
this feature is available? Furthermore, 'ft' is an awfully short name;
for QMP, we prefer to use full words where possible, such as
'fault-tolerant'.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH v2 2/4] Curling: cmdline interface.
Add an option '-f' to migration cmdline.
Indicating whether to enable fault tolerant or not.
Signed-off-by: Jules Wang
---
hmp-commands.hx | 11 +++
hmp.c | 3 ++-
include/migration/migration.h | 1 +
migration.c | 3 ++-
qapi-schema.json | 3 ++-
qmp-commands.hx | 3 ++-
6 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 65b7f60..8418d37 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -877,23 +877,26 @@ ETEXI
{
.name = "migrate",
-.args_type = "detach:-d,blk:-b,inc:-i,uri:s",
-.params = "[-d] [-b] [-i] uri",
+.args_type = "detach:-d,blk:-b,inc:-i,ft:-f,uri:s",
+.params = "[-d] [-b] [-i] [-f] uri",
.help = "migrate to URI (using -d to not wait for completion)"
"\n\t\t\t -b for migration without shared storage with"
" full copy of disk\n\t\t\t -i for migration without "
"shared storage with incremental copy of disk "
- "(base image shared between src and destination)",
+ "(base image shared between src and destination)"
+ "\n\t\t\t -f for fault tolerant, this is another "
+ "feature rather than migrate",
.mhandler.cmd = hmp_migrate,
},
STEXI
-@item migrate [-d] [-b] [-i] @var{uri}
+@item migrate [-d] [-b] [-i] [-f] @var{uri}
@findex migrate
Migrate to @var{uri} (using -d to not wait for completion).
-b for migration with full copy of disk
-i for migration with incremental copy of disk (base image is shared)
+ -f for fault tolerant
ETEXI
{
diff --git a/hmp.c b/hmp.c
index fcca6ae..91beae9 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1213,10 +1213,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
int detach = qdict_get_try_bool(qdict, "detach", 0);
int blk = qdict_get_try_bool(qdict, "blk", 0);
int inc = qdict_get_try_bool(qdict, "inc", 0);
+int ft = qdict_get_try_bool(qdict, "ft", 0);
const char *uri = qdict_get_str(qdict, "uri");
Error *err = NULL;
-qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, &err);
+qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, !!ft, ft, &err);
if (err) {
monitor_printf(mon, "migrate: %s\n", error_get_pretty(err));
error_free(err);
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 140e6b4..fc2b066 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -25,6 +25,7 @@
struct MigrationParams {
bool blk;
+bool ft;
bool shared;
};
diff --git a/migration.c b/migration.c
index 200d404..8989a51 100644
--- a/migration.c
+++ b/migration.c
@@ -394,7 +394,7 @@ void migrate_del_blocker(Error *reason)
void qmp_migrate(const char *uri, bool has_blk, bool blk,
bool has_inc, bool inc, bool has_detach, bool detach,
- Error **errp)
+ bool has_ft, bool ft, Error **errp)
{
Error *local_err = NULL;
MigrationState *s = migrate_get_current();
@@ -403,6 +403,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
params.blk = has_blk && blk;
params.shared = has_inc && inc;
+params.ft = has_ft && ft;
if (s->state == MIG_STATE_ACTIVE || s->state == MIG_STATE_SETUP) {
error_set(errp, QERR_MIGRATION_ACTIVE);
diff --git a/qapi-schema.json b/qapi-schema.json
index a51f7d2..a8187cf 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2420,7 +2420,8 @@
# Since: 0.14.0
##
{ 'command': 'migrate',
- 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } }
+ 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool',
+ '*ft': 'bool' } }
# @xen-save-devices-state:
#
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 8a8f342..1fa0e60 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -611,7 +611,7 @@ EQMP
{
.name = "migrate",
-.args_type = "detach:-d,blk:-b,inc:-i,uri:s",
+.args_type = "detach:-d,blk:-b,inc:-i,ft:-f,uri:s",
.mhandler.cmd_new = qmp_marshal_input_migrate,
},
@@ -625,6 +625,7 @@ Arguments:
- "blk": block migration, full disk copy (json-bool, optional)
- "inc": incremental disk copy (json-bool, optional)
+- "ft" : fault tolerant (json-bool, optional)
- "uri": Destination URI (json-string)
Example:
--
1.8.0.1
