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

Reply via email to