qemu-devel
[Top][All Lists]
Advanced

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

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


From: Chunyan Liu
Subject: Re: [Qemu-devel] [PATCH v22 00/25] replace QEMUOptionParameter with QemuOpts
Date: Fri, 21 Mar 2014 18:09:22 +0800




2014-03-21 8:07 GMT+08:00 Leandro Dorileo <address@hidden>:
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.

Well, if breaking the build could be allowed between each patch, then it could be
much cleaner. Indeed there are lots of code just for build and function unbroken
between each patch. I'm inclined to listen to more voice. If all agree to this method,
it's OK to me.
 

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.

qemu_opt_*_del functions are needed. Each driver handles options they expected then
delete, left options passed to 2nd driver and let it handle. Like qcow2 create, first, qcow2
driver handle, then raw driver handle.

But as you point, some changes are not required for this job, I've omitted in my new patch
series, like: qemu_opt_set, NULL check in qemu_opt_get and qemu_opt_find, assert()
update in qemu_opt_get.
 
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.

No matter. I'm OK to follow a more acceptable way :)
 

Regards....

--
Leandro Dorileo



reply via email to

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