[Top][All Lists]

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

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

From: Dong Xu Wang
Subject: Re: [Qemu-devel] [PATCH V17 0/9] replace QEMUOptionParameter with QemuOpts parser
Date: Wed, 17 Jul 2013 22:11:51 +0800

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,
    create_options = append_option_parameters(create_options,

if (drv->bdrv_create_opts) {
    create_opts = qemu_opts_append(drv->bdrv_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

reply via email to

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