Hi Chunyan, On Mon, Mar 24, 2014 at 11:02:14AM +0800, Chunyan Liu wrote: > 2014-03-21 20:31 GMT+08:00 Leandro Dorileo <l...@dorileo.org>: > > > On Fri, Mar 21, 2014 at 06:09:22PM +0800, Chunyan Liu wrote: > > > 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. > > > > > > The thing is the balance between complexity and the change size. Do we > > really > > want to avoid a small patch - doing all the change - and increase the whole > > thing complexity? I don't see a great benefit on that :) > > > > > > > > > > > > > > > > > > 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. > > > > > > Not true, the only place you need to allocate QemuOpts or QemuOptsList is > > on qemu-img.c and block.c, if they're doing so they should free it, not > > the lower lavels. The block drivers should just use it, unless they do > > allocate anything themselves. > > > > > The reason qemu_opt_get_*_del functions should be used in backend drivers, > is to > keep same behavior as how previous QEMUOptionParameter handles. At least, > in one > case: create a qcow2 img. "size" option is handled by qcow2 driver, then > delete; in 2nd raw > driver, there is no "size" option any more, it will create a 0 size file. > If qemu_opt_get but > not delete, then all options will be passed to 2nd raw driver, it will > create a full sized file. > That is not expected.
I couldn't find the described use case - I still think you don't need to unset it, but if that's the case what about using for example: uint64_t sectos = qemu_opt_get_size(options, BLOCK_OPT_SIZE) / 512; ret = qemu_opt_unset(options, BLOCK_OPT_SIZE); You don't need to introduce a new API for this. Regards.. -- Leandro Dorileo > > > > > > > > > > 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. > > > > > > > Ok. > > > > -- > > Leandro Dorileo > > > > > > > > > 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 > > > > > > > > > > > >
