Re: [Qemu-devel] [PATCH v3 3/5] qcow2: Introduce an option for sufficient L2 cache for the entire image

[Leonid Bloch]

On July 26, 2018 11:19:03 PM EEST, Kevin Wolf <address@hidden> wrote:
>Am 26.07.2018 um 21:43 hat Leonid Bloch geschrieben:
>> On 07/26/2018 05:50 PM, Leonid Bloch wrote:
>> > On 07/26/2018 05:42 PM, Kevin Wolf wrote:
>> > > Am 26.07.2018 um 14:24 hat Leonid Bloch geschrieben:
>> > > > > > You mean with QDict? I'll look into that now. But
>> > > > > > already sent v5 before
>> > > > > > reading this email.
>> > > > > 
>> > > > > Yes, with reading it from the QDict. (Or whatever the
>simplest way is
>> > > > > that results in the right external interface, but I suppose
>this is the
>> > > > > one.)
>> > > > 
>> > > > Well, there is a problem with that: I can easily isolate
>> > > > l2-cache-size from QDict, check if it is "full", and if it is -
>> > > > do whatever
>> > > > is needed, and delete this option before parsing. But what if
>it
>> > > > is "foo"?
>> > > > It will not get deleted, and the regular QEMU_OPT_SIZE parsing
>> > > > error will
>> > > > appear, stating that l2-cache-size "expects a non-negative
>> > > > number..." - no
>> > > > word about that it can expect "full" as well. Now, one can try
>to modify
>> > > > local_err->msg for this particular option, but this will
>require
>> > > > substantial
>> > > > additional logic. I think considering this, it would be easier
>> > > > to stick with
>> > > > a dedicated option, l2-cache-full.
>> > > > 
>> > > > Do you think there is a smarter way to parse the l2-cache-size
>> > > > option, so it
>> > > > would accept both size and "full", while handling errors
>> > > > correctly? It seems
>> > > > more elegant to have a single option, but the internal handling
>> > > > will be more
>> > > > elegant and simpler with two mutually exclusive options.
>> > > 
>> > > I think we can live with the suboptimal error message for a
>while. Once
>> > > qcow2 is QAPIfied, it should become easy to improve it. Let's not
>choose
>> > > a worse design (that stays forever) for a temporarily better
>error
>> > > message.
>> > 
>> > OK. I'll add a TODO then.
>> 
>> Another problem without a dedicated option, is that if l2-cache-size
>is
>> processed and deleted, it can not be read again when needed for
>resizing.
>> Without some *extremely* dirty tricks, that is.
>
>Are you sure? The way that the .bdrv_open() callbacks work, _all_
>options that are processed are removed from the QDict. The difference
>is
>just whether qemu_opts_absorb_qdict() removes them or you do that
>manually. In the end, if options are left in the QDict, the block layer
>returns an error because that means that an unknown option was
>specified.
>
>I suppose you use bs->options while resizing. This is a copy of the
>QDict which is not affected by the removal of processed options.

This probably explains why it works on the first resize, but fails on the 
second onward.

>
>> Can QAPI be the solution? I've seen some examples, and it looks like
>the
>> qcow2 driver already uses QAPI to some extent - I mean all the qdict
>stuff
>> is from QAPI, no? Can you please point me to an example of how QAPI
>can
>> solve the issue of an option that can accept both a size and a
>string?
>
>By using QAPI I mean using types like BlockdevOptionsQcow2. These types
>are directly generated from the JSON schema, where it's possible to
>specify an alternate of a string and an integer.
>
>One example for an alternate in the JSON schema is BlockdevRef, where
>you can give a string (to reference a node-name) or an inline
>declaration of a block device. In JSON, it looks like this:
>
>    { 'alternate': 'BlockdevRef',
>      'data': { 'definition': 'BlockdevOptions',
>                'reference': 'str' } }
>
>And the generated C type for it is:
>
>    struct BlockdevRef {
>        QType type;
>        union { /* union tag is @type */
>            BlockdevOptions definition;
>            char *reference;
>        } u;
>    };
>
>In our case, we would get a union of int64_t and char* instead, and
>still the QType type as a discriminator.

Thanks for the explanation!
But qcow2 already has the options in the JSON file, they're just not used for 
some reason?

Leonid.

>
>Kevin

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.