2014-03-21 8:07 GMT+08:00 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. >
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 > >
