| Message ID | 1413359710-2799-3-git-send-email-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
* Juan Quintela (quintela@redhat.com) 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? Dave > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > include/sysemu/sysemu.h | 1 + > vl.c | 10 ++++++++++ > 2 files changed, 11 insertions(+) > > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h > index d8539fd..ae217da 100644 > --- a/include/sysemu/sysemu.h > +++ b/include/sysemu/sysemu.h > @@ -28,6 +28,7 @@ bool runstate_check(RunState state); > void runstate_set(RunState new_state); > int runstate_is_running(void); > bool runstate_needs_reset(void); > +int runstate_store(char *str, int size); > typedef struct vm_change_state_entry VMChangeStateEntry; > typedef void VMChangeStateHandler(void *opaque, int running, RunState state); > > diff --git a/vl.c b/vl.c > index 964d634..ce8e28b 100644 > --- a/vl.c > +++ b/vl.c > @@ -677,6 +677,16 @@ bool runstate_check(RunState state) > return current_run_state == state; > } > > +int runstate_store(char *str, int size) > +{ > + const char *state = RunState_lookup[current_run_state]; > + > + if (strlen(state)+1 > size) > + return -1; > + strncpy(str, state, strlen(state)+1); > + return 0; > +} > + > static void runstate_init(void) > { > const RunStateTransition *p; > -- > 2.1.0 > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Juan Quintela (quintela@redhat.com) 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). But it is easier for me if I can sent the numeric value. Libvirt folks? Luiz? What should I do? Later, Juan. > > Dave > >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> include/sysemu/sysemu.h | 1 + >> vl.c | 10 ++++++++++ >> 2 files changed, 11 insertions(+) >> >> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h >> index d8539fd..ae217da 100644 >> --- a/include/sysemu/sysemu.h >> +++ b/include/sysemu/sysemu.h >> @@ -28,6 +28,7 @@ bool runstate_check(RunState state); >> void runstate_set(RunState new_state); >> int runstate_is_running(void); >> bool runstate_needs_reset(void); >> +int runstate_store(char *str, int size); >> typedef struct vm_change_state_entry VMChangeStateEntry; >> typedef void VMChangeStateHandler(void *opaque, int running, RunState state); >> >> diff --git a/vl.c b/vl.c >> index 964d634..ce8e28b 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -677,6 +677,16 @@ bool runstate_check(RunState state) >> return current_run_state == state; >> } >> >> +int runstate_store(char *str, int size) >> +{ >> + const char *state = RunState_lookup[current_run_state]; >> + >> + if (strlen(state)+1 > size) >> + return -1; >> + strncpy(str, state, strlen(state)+1); >> + return 0; >> +} >> + >> static void runstate_init(void) >> { >> const RunStateTransition *p; >> -- >> 2.1.0 >> >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 10/20/2014 04:52 AM, Juan Quintela wrote: > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: >> * Juan Quintela (quintela@redhat.com) 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. > > But it is easier for me if I can sent the numeric value. > > Libvirt folks? As far as I can tell, libvirt is unimpacted by HOW it is represented in the migration stream, only that the destination is able to inform libvirt what state was received as part of migration, with libvirt having an easy way to then get back into that state (of course, libvirt should also still have the option to choose a different state than what just got migrated, as in the case where the user pauses the source in order to avoid convergence problems but wants the destination to start running again).
* Eric Blake (eblake@redhat.com) wrote: > On 10/20/2014 04:52 AM, Juan Quintela wrote: > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > >> * Juan Quintela (quintela@redhat.com) 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? Dave > > > > > But it is easier for me if I can sent the numeric value. > > > > Libvirt folks? > > As far as I can tell, libvirt is unimpacted by HOW it is represented in > the migration stream, only that the destination is able to inform > libvirt what state was received as part of migration, with libvirt > having an easy way to then get back into that state (of course, libvirt > should also still have the option to choose a different state than what > just got migrated, as in the case where the user pauses the source in > order to avoid convergence problems but wants the destination to start > running again). > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > * Eric Blake (eblake@redhat.com) wrote: >> On 10/20/2014 04:52 AM, Juan Quintela wrote: >> > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: >> >> * Juan Quintela (quintela@redhat.com) 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.
On 10/22/2014 05:40 AM, Markus Armbruster wrote: >> 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. And I still need to find time to supply the next revision of that series... > 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). Yes, the generator guarantees that the order that enums are listed in a .json file will be the 0, 1, 2, ... values assigned in the C code. I'll add that in my revision. You cannot rely on the C values being stable unless the .json file takes care to not reorder names for the enum members; if we have enums where the C code is going to rely on a particular ordering, we probably ought to document that in the .json file (since MOST enums are designed for the C code to use them solely by symbolic name and not by specific value). > > 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. Indeed, if you are going to rely on something that the generator happens to give but does not guarantee, then additional build-time checking and documentation is called for.
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index d8539fd..ae217da 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -28,6 +28,7 @@ bool runstate_check(RunState state); void runstate_set(RunState new_state); int runstate_is_running(void); bool runstate_needs_reset(void); +int runstate_store(char *str, int size); typedef struct vm_change_state_entry VMChangeStateEntry; typedef void VMChangeStateHandler(void *opaque, int running, RunState state); diff --git a/vl.c b/vl.c index 964d634..ce8e28b 100644 --- a/vl.c +++ b/vl.c @@ -677,6 +677,16 @@ bool runstate_check(RunState state) return current_run_state == state; } +int runstate_store(char *str, int size) +{ + const char *state = RunState_lookup[current_run_state]; + + if (strlen(state)+1 > size) + return -1; + strncpy(str, state, strlen(state)+1); + return 0; +} + static void runstate_init(void) { const RunStateTransition *p;
This allows us to store the current state to send it through migration. Signed-off-by: Juan Quintela <quintela@redhat.com> --- include/sysemu/sysemu.h | 1 + vl.c | 10 ++++++++++ 2 files changed, 11 insertions(+)