Re: [Qemu-devel] [PATCH v2 2/4] Curling: cmdline interface.

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2627 bytes --]

[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.wang@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]