Re: [Qemu-devel] [PATCH V17 0/9] replace QEMUOptionParameter with QemuOpts parser

[Dong Xu Wang]

On Wed, Jul 17, 2013 at 8:54 PM, Eric Blake <address@hidden> 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
>