2014-03-25 15:15 GMT+08:00 Chunyan Liu <cy...@suse.com>: > > > > 2014-03-24 23:00 GMT+08:00 Leandro Dorileo <l...@dorileo.org>: > > 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 - > > > In block drivers that calls bdrv_create_file in its bdrv_create (like: > qcow.c qcow2.c vhdx.c vpc.c, etc), current code, will see, it get options > from QEMUOptionParameter one by one with options++, after that passes > 'options' to bdrv_create_file (at this time, 'options' actually points to > left options > that's not be handled yet). > >
Hi, Leandro, I found some comment very long before: http://lists.gnu.org/archive/html/qemu-devel/2013-01/msg02446.html I think maybe keep qemu_opt_get_del is better than optionally doing qemu_opt_unset. Using qemu_opt_get_del will keep same logic with previous options++, we don't need to take care which option should be unset, which need effort to know who will use it later and easy to cause problem. So I'll still keep qemu_opt_get_del in next version. Will post soon. Regards, Chunyan > 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. >> > > OK. I checked and it seems only those call bdrv_create_file or > bdrv_create() in > their .bdrv_create functions should take care. Currently, only need to > take care of > options that's parsed in raw-posix.c: raw_create(). That is, currently, > only > BLOCK_OPT_SIZE should be taken care. Base on this, we can unset after > parsing > BLOCK_OPT_SIZE as you point in related drivers. > > I'll adjust the patches for this, after feedback of other patches and > after there is a > consensus about in which way doing patches: > a. keep changes simple in 'natural way' (allowing break between patch) > or > b. keep current way (make each patch build and work successfully) > > Regards, > Chunyan > > >> 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 >> > > > > >> > > > > >> > > >> > > >> >> >
