Re: [Qemu-devel] [PATCH v22 00/25] replace QEMUOptionParameter with QemuOpts

[Leandro Dorileo <l@dorileo.org>]

Hi Chunyan,

On Mon, Mar 10, 2014 at 03:31:36PM +0800, Chunyan Liu wrote:
> This patch series is to replace QEMUOptionParameter with QemuOpts, so that only
> one Qemu Option structure is kept in QEMU code.
> 


Last night I took some time do take a deeper look at you series and the required
effort to do the QemuOptionParameter -> QemuOpts migration.

I think you've over complicated the things, I understand you tried to keep your
serie's bisectability (?), but the final result was something really hard to
review and to integrate as well. The overall approach wasn't even resolving the
bisectability problem since it breaks the tree until the last commit. Moreover,
in the path of getting things ready you created new problems and their respective
fixes, what we really don't need to.

In this regards you could have kept things as simple as possible and submitted
the patches in a "natural way", even if they were breaking the build between each
patch, you could get all the required maintainer's Reviewed-by + Tested-by +
Signed-off-by and so on for each individual patch and when it was time to
integrate get squashed the needed patches.

I mean, add N patches introducing new required QemuOpts API's, 1 patch migrating
the block upper layer (block.c, block.h, etc), one patch for each block driver
(i.e ssh.c, qcow.c, qcow2.c, etc), one patch for qemu-img.c and finally a last
patch removing the QEMUOptionParamer itself. When time comes to integrate your
series the patches changing the block layer + patches changing the block drivers +
patches changing qemu-img.c could be squashed adding all the collected Reviewed-by
to this single squashed patch.

As I said, last night I took a deeper look at the problem and, understood most
of changes weren't required to do the job. We don't need an adaptation layer between
QemuOptionParameter and QemuOpts, we don't need to add new opts accessors (like
those qemu_opt_*_del() functions), all we need is 1) that qemu_opts_append() function
so we can merge the protocol and drivers options in a single QemuOptList and
2) the default value support. All we need is already present in the QemuOpts APIs.

With that simpler approach in mind I ended up putting my hands in the source code
trying to see how feasible it is, and turns out I came up with a full solution. I'm
sending the job's resulting series to the mailing list so I can show you what
I mean and have some more room for discussion. It doesn't mean I want to overlap
you work, I just needed to have a little more input on that.

Regards....

-- 
Leandro Dorileo