Re: [Qemu-devel] [PATCH] qapi: Fix crash with enum dealloc when kind is invalid

[Fam Zheng <famz@redhat.com>]

On Wed, 09/10 22:17, Eric Blake wrote:
> On 09/10/2014 06:53 PM, Fam Zheng wrote:
> > On Wed, 09/10 17:32, Paolo Bonzini wrote:
> >> Il 10/09/2014 17:02, Fam Zheng ha scritto:
> >>>> A bit hackish, but I don't have any better idea.
> >>>>
> >>>> Hmm... what about adding a new member to the visitors for "invalid enum"
> >>>> value?  The dealloc visitor could override it to do nothing, while the
> >>>> default could abort or set an error.  Would that work?
> >>>
> >>> The invalid state of enum still needs to be saved in the data.  It is detected
> >>> by the input visitor, but should be checked by other visitors (output, dealloc)
> >>> later.
> >>
> >> Yes, that's fine.  The only part where I'm not sure is the special
> >> casing of the _MAX enum.
> >>
> > 
> > Yes, it is abusing. Let's add an _INVALID = 0 enum which is much clearer.
> 
> If I understand correctly, you mean that for:
> 
> { 'enum': 'Foo', 'data': [ 'one', 'two' ] }
> 
> FOO_ONE would now be 1 instead of its current value of 0?
> 
> We just barely saw a case where Hu Tao's code was relying on the
> implicit value 0 assigned to the first enum in the json file [1]
> although I strongly argued that it should be nuked (and so it was fixed
> in [2]).  So I could live with reserving 0 for internal use for flagging
> parse errors (such as attempting to pass the string 'three' where a Foo
> value is expected).
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2014-09/msg01691.html
> [2] https://lists.gnu.org/archive/html/qemu-devel/2014-09/msg01938.html
> 

A closer looking shows me a huge risk with _lookup table. We have for loops
starting with index 0 all over the place to peek info _lookup arrays, but in
this case it is _INVALID and shouldn't be looked at.

Maybe we have to put _INVALID at the end after _MAX.

Fam