On Wed, Jul 17, 2013 at 8:54 PM, Eric Blake <ebl...@redhat.com> wrote:
> On 07/17/2013 03:29 AM, Dong Xu Wang wrote:
>> These patches will replace QEMUOptionParameter with QemuOpts. Change logs
>> please go to each patch's commit message.
>>
>> Dong Xu Wang (9):
>> qemu-option: add def_value_str in QemuOptDesc struct and rewrite
>> qemu_opts_print
>> qemu-option: avoid duplication of default value in QemuOpts
>> qemu-option: create four QemuOptsList related functions
>> qemu-option: create some QemuOpts functons
>> block: use QemuOpts support in block layer
>> qapi: query-command-line-options outputs def_value_str
>> qemu-option: remove QEMUOptionParameter related functions and struct
>> qemu-option: make qemu_opts_del accept opts being NULL
>> qemu-option: use qemu_opts_del without judging NULL
>>
>
>>
>> --
>
> Ouch - by putting your additional comments after an 'end-of-message'
> marker, you made it harder for me to reply. (Sane mailers intentionally
> drop text after a "-- " line when replying).
>
>> V17 add qemu_opts_del's support when opts is NULL, and made changes
>> based on Eric's comments. I tried to split PATCH 5 into small pieces,
>> but I found it is very hard, bdrv_* functions are all using
>> QEMUOptionParameter or QemuOpts, if I split them, I have to support two
>> parsers in block.c and qemu-img.c, it will make code very hard to read,
>
> Yes, if you split the patches, then there will be a window of time in
> the series where you have to support BOTH parsers at the same time. But
> since the existing parser is already present, what's so hard about that?
> By having two different callbacks, one with the old signature and one
> with the new, with exactly one of the callbacks being non-NULL, it
> should be relatively easy to decide which of the two callbacks to use.
>
>> so I leave them in one big patch, I am sorry it is realy hard to review.
>
> I still think that the ease of review that would be added by splitting
> the patch is worth making the effort. Yes, it may be harder for you,
Such as bdrv
> but if the end result is that more people are willing to review the
> patch, then the community as a whole spends less collective time, and
> the series will be accepted sooner.
>
Thank you Eric.
If splited into patches and modified each block backend in each patch, code must
look like this:
if (drv->create_options) {
create_options = append_option_parameters(create_options,
drv->create_options);
create_options = append_option_parameters(create_options,
proto_drv->create_options);
print_option_help(create_options);
free_option_parameters(create_options);
}
if (drv->bdrv_create_opts) {
create_opts = qemu_opts_append(drv->bdrv_create_opts,
proto_drv->bdrv_create_opts);
qemu_opts_print_help(create_opts);
qemu_opts_free(create_opts);
}
It will be many "if", I think that will make code hard to review. If
you insist, I can
split into small patches.
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
