Re: [Qemu-devel] [PATCH 1/2] hmp: add hmp command for incremental backup

[Eric Blake <eblake@redhat.com>]

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

On 01/21/2016 07:04 PM, 张敏 wrote:

>>> -        .args_type  = "reuse:-n,full:-f,device:B,target:s,format:s?",
>>> -        .params     = "[-n] [-f] device target [format]",
>>> +        .args_type  = "reuse:-n,full:-f,device:B,target:s,bitmap:s?,format:s?",
>>> +        .params     = "[-n] [-f] device target [bitmap] [format]",
>> This is HMP, so it may not matter, but this is not backwards compatible.
>>   Scripts targetting the old style of passing a format will now have that
>> format string interpreted as a bitmap name with no format.  Better would
>> be to stick [bitmap] at the end, not the middle.
> 
> But I have a question: If I don't want to input a 'format', only use 'bitmap',
> it will let 'bitmap' as 'format', This problem how to do it.

Several solutions, some nicer than others, some more complicated than
others.  I don't have any strong preference, so much as pointing out the
design space:

1. You don't.  If you want to use 'bitmap', you MUST supply 'format'
(supplying format is good anyways, as implicit formats have led to CVEs
in the past).

1.a. Possible variation: Teach the command to allow empty '' format to
be a synonym for an implicit format, so that it could look like:

drive_backup device target '' /path/to/bitmap

2. You modify the HMP parser to accept optionally-named arguments, so
that you can then supply later optional arguments by name without having
to provide the earlier optional arguments.  Maybe looking something like:

drive_backup device target --bitmap=/path/to/bitmap

3. Instead of trying to overload an existing command, you create a new
command.  Particularly since your overload already declared that '-f'
and 'bitmap' are incompatible.

4. maybe something else?

>> Needs {}.  Run your patch through scripts/checkpatch.pl, to flag this
>> and other style violations.
> 
> I have checked these patches，but I ignored these warnings.

Not a good idea.  And even in the rare case that you plan on ignoring
the warnings, you should at least document in the commit message your
justification for doing so.

-- 
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: 604 bytes --]