Re: [Qemu-devel] [PATCH 2/7] runstate: Add runstate store

[Markus Armbruster]

"Dr. David Alan Gilbert" <address@hidden> writes:

> * Eric Blake (address@hidden) wrote:
>> On 10/20/2014 04:52 AM, Juan Quintela wrote:
>> > "Dr. David Alan Gilbert" <address@hidden> wrote:
>> >> * Juan Quintela (address@hidden) wrote:
>> >>> This allows us to store the current state to send it through migration.
>> >>
>> >> Why store the runstate as a string?  The later code then ends up doing
>> >> string compares and things - why not just use the enum value?
>> > 
>> > How do you know that it has the same values both sides?  As far as I can
>> > see, all interaction with the outside is done with strings (i.e. QMP).
>> 
>> If it's part of the migration stream, then it is not something visible
>> in QMP, and it is your own fault if you ever change the enum values in
>> such a way that the migration stream is incompatible between versions.
>> I think using an enum in the migration stream is just fine, and more
>> efficient.
>
> I think the question here really comes from RunState being an enum defined
> in qapi-schema.json; so we could use that directly in the migration stream
> if we were guaranteed that the encoding of that enum wasn't going to change.
> Does qapi make any guarantees about the enum encoding?

qapi-code-gen.txt in master is silent on the matter:

    === Enumeration types ===

    An enumeration type is a dictionary containing a single key whose value is a
    list of strings.  An example enumeration is:

     { 'enum': 'MyEnum', 'data': [ 'value1', 'value2', 'value3' ] }

Eric's "drop qapi nested structs" series improves the spec a lot.  With
it applied, this reads:

    === Enumeration types ===

    Usage: { 'enum': 'str', 'data': [ 'str' ] }

    An enumeration type is a dictionary containing a single 'data' key
    whose value is a list of strings.  An example enumeration is:

     { 'enum': 'MyEnum', 'data': [ 'value1', 'value2', 'value3' ] }

    Nothing prevents an empty enumeration, although it is probably not
    useful.  The list of strings should be lower case; if an enum name
    represents multiple words, use '-' between words.  The string 'max' is
    not allowed as an enum value, and values should not be repeated.

    The enumeration values are passed as strings over the QMP protocol,
    but are encoded as C enum integral values in generated code.  While
    the C code starts numbering at 0, it is better to use explicit
    comparisons to enum values than implicit comparisons to 0; the C code
    will also include a generated enum member ending in _MAX for tracking
    the size of the enum, useful when using common functions for
    converting between strings and enum values.  Since the wire format
    always passes by name, it is acceptable to reorder or add new
    enumeration members in any location without breaking QMP clients;
    however, removing enum values would break compatibility.  For any
    complex type that has a field that will only contain a finite set of
    string values, using an enum type for that field is better than
    open-coding the field to be type 'str'.

I figure relying on the QAPI code generator assigning values 0, 1, 2 in
order is fair.  If you want to guarantee that more explicitly in the
spec, patch welcome (on top of Eric's, please).

A test or build-time assertion checking the values don't change would be
prudent.  A comment next to the enum definition warning against
incompatible changes wouldn't hurt.