From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50994) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VTo4R-0003Ly-Vk for qemu-devel@nongnu.org; Wed, 09 Oct 2013 03:20:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VTo4M-0000cE-E3 for qemu-devel@nongnu.org; Wed, 09 Oct 2013 03:20:35 -0400 Received: from mproxygzc3.163.com ([218.107.63.211]:52188) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VTo4L-0000bi-0R for qemu-devel@nongnu.org; Wed, 09 Oct 2013 03:20:30 -0400 Date: Wed, 9 Oct 2013 14:49:32 +0800 (CST) From: junqing.wang@cs2c.com.cn Sender: lancelotds@163.com In-Reply-To: <5249F842.8050405@redhat.com> References: <1380485683-4626-1-git-send-email-junqing.wang@cs2c.com.cn> <1380485683-4626-3-git-send-email-junqing.wang@cs2c.com.cn> <5249F842.8050405@redhat.com> Content-Type: multipart/alternative; boundary="----=_Part_140597_741811640.1381301372609" MIME-Version: 1.0 Message-ID: <2c17177e.92cb.1419bfa46c2.Coremail.junqing.wang@cs2c.com.cn> Subject: Re: [Qemu-devel] [PATCH v2 2/4] Curling: cmdline interface. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: pbonzini@redhat.com, quintela@redhat.com, qemu-devel@nongnu.org, owasserm@redhat.com ------=_Part_140597_741811640.1381301372609 Content-Type: text/plain; charset=GBK Content-Transfer-Encoding: 7bit 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 > ------=_Part_140597_741811640.1381301372609 Content-Type: text/html; charset=GBK Content-Transfer-Encoding: 7bit
At 2013-10-01 06:16:34,"Eric Blake" <eblake@redhat.com> 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 <junqing.wang@cs2c.com.cn> >> --- >>          .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
>


------=_Part_140597_741811640.1381301372609--