[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH 1/2] hmp: add hmp command for incremental backup
Date: Fri, 22 Jan 2016 09:39:38 -0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0

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: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]