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

[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 - 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
> > > >
> > > >
> >
> >