Re: [Qemu-devel] [PATCH 2/4] string-input-visitor: Support alternate types

[Eduardo Habkost]

On Thu, May 04, 2017 at 11:10:56AM -0300, Eduardo Habkost wrote:
> On Thu, May 04, 2017 at 03:42:59PM +0200, Markus Armbruster wrote:
> > Eric Blake <address@hidden> writes:
> > 
> > > On 05/04/2017 03:06 AM, Markus Armbruster wrote:
> > >> Eduardo Habkost <address@hidden> writes:
> > >> 
> > >>> On Wed, May 03, 2017 at 06:07:43PM +0200, Markus Armbruster wrote:
> > >>>> Eduardo Habkost <address@hidden> writes:
> > >>>>
> > >>>>> When parsing alternates from a string, there are some limitations in
> > >>>>> what we can do, but it is a valid use case in some situations. We can
> > >>>>> support booleans, integer types, and enums.
> > >> 
> > >> By the way, the same restrictions apply to the "keyval" variant of the
> > >> QObject input visitor.  It's a known problem stated here:
> > >> 
> > >>     Message-ID: <address@hidden>
> > >>     https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg00046.html
> > >> 
> > >> However, I failed to document it properly in the source.
> > >> 
> > >
> > >>>> Begs the question what happens when you violate these restrictions.
> > >>>
> > >>> Right now, we don't detect those cases and behavior is undefined.
> > >>> I think it will be a good idea to give start_alternate() enough
> > >>> information to detect those cases (by adding a 'const char *const
> > >>> enum_table[]' parameter).
> > >> 
> > >> Alternate types that won't work with the string input visitor can be
> > >> detected at compile time (by qapi.py), but not their actual use.  Pity.
> > >> 
> > >> Do we actually use alternates that violate the restrictions?  If not, we
> > >> could simply restrict alternates so they work with *all* visitors.  If
> > >> we ever run into an actual need for alternates that don't, we'll be no
> > >> worse off than now.
> > >> 
> > >> Let's review existing alternates outside tests:
> > >> 
> > >> * Qcow2OverlapChecks: struct + enum
> > >> * BlockdevRef: struct + str
> > >> * GuestFileWhence: int + enum (all enum members start with a letter)
> > >> 
> > >> Restricting alternates looks practical to me.  Eric, what do you think?
> > >
> > > As in: we forbid the combination of a scalar (whether 'int', 'number',
> > > 'bool', and perhaps 'null') with a plain 'str' (since there's no way to
> > > tell whether '1' should parse as an integer or the string "1"); and
> > > combining a scalar with an 'enum' requires that all enum members be
> > > distinct from what could otherwise be parsed as a scalar?
> > 
> > Exactly.
> > 
> > >                                                            I can live
> > > with such a restriction.
> > 
> > Then let's do it.
> > 
> > Eduardo, are you comfortable implementing this, or would you like me to
> > do it?
> 
> I will give it a try and include it in the next version. Thanks!

So, I made qapi.py detect ambiguous alternates[1]. The bad news
is that lots of the alternates in qapi-schema-test.json already
break those rules. This will require changing the schema and
rewriting tests in test-clone-visitor and qobject-input-visitor.

I think there's a small risk we will want to support some of
those forbidden alternate combinations again in the future. If
that happens, detecting them at runtime in string-input-visitor
will keep us safe.

I plan to submit v2 with code to detect ambiguous alternates at
runtime, only, because it seems simpler than rewriting the test
code. After that, we can still make QAPI reject them at compile
time too, if we really want to.

[1] 
https://github.com/ehabkost/qemu-hacks/commit/cda70a2e1c30c8dadb36fd46095a4f6ee3d89737

-- 
Eduardo