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

[Kevin Wolf <kwolf@redhat.com>]

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.

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

Kevin