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

[Leandro Dorileo]

On Fri, Mar 21, 2014 at 06:09:22PM +0800, Chunyan Liu wrote:
> 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.


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.


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