| Message ID | 1476377054-14572-1-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 10/13/2016 11:44 AM, Markus Armbruster wrote: > machine_set_property() replaces '_' by '-' in the property name. > Except it fails to replace an initial '_'. Screwed up in commit > b0ddb8b. Reproducer: "-M pc,__foo_bar=true" produces "Property > '._-foo-bar' not found". > > Error messages using a mangled name rather than the name the user > actually wrote is user-hostile, but that's a different topic. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > vl.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com>
On Thu, Oct 13, 2016 at 06:44:14PM +0200, Markus Armbruster wrote: > machine_set_property() replaces '_' by '-' in the property name. > Except it fails to replace an initial '_'. Screwed up in commit > b0ddb8b. Reproducer: "-M pc,__foo_bar=true" produces "Property > '._-foo-bar' not found". > > Error messages using a mangled name rather than the name the user > actually wrote is user-hostile, but that's a different topic. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> I suggest we follow the same approach we used in the x86 CPU code: instead of requiring a special parser that magically translate strings, just add property aliases for the old names that contained "_". It would fix the user-hostile error messages as well. > --- > vl.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/vl.c b/vl.c > index c657acd..1c0b0ba 100644 > --- a/vl.c > +++ b/vl.c > @@ -2804,17 +2804,16 @@ static int machine_set_property(void *opaque, > { > Object *obj = OBJECT(opaque); > Error *local_err = NULL; > - char *c, *qom_name; > + char *p, *qom_name; > > if (strcmp(name, "type") == 0) { > return 0; > } > > qom_name = g_strdup(name); > - c = qom_name; > - while (*c++) { > - if (*c == '_') { > - *c = '-'; > + for (p = qom_name; *p; p++) { > + if (*p == '_') { > + *p = '-'; > } > } > > -- > 2.5.5 >
On Thu, Oct 13, 2016 at 06:44:14PM +0200, Markus Armbruster wrote: > machine_set_property() replaces '_' by '-' in the property name. > Except it fails to replace an initial '_'. Screwed up in commit > b0ddb8b. Reproducer: "-M pc,__foo_bar=true" produces "Property > '._-foo-bar' not found". > > Error messages using a mangled name rather than the name the user > actually wrote is user-hostile, but that's a different topic. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> Applied to machine-next. Thanks.
Eduardo Habkost <ehabkost@redhat.com> writes: > On Thu, Oct 13, 2016 at 06:44:14PM +0200, Markus Armbruster wrote: >> machine_set_property() replaces '_' by '-' in the property name. >> Except it fails to replace an initial '_'. Screwed up in commit >> b0ddb8b. Reproducer: "-M pc,__foo_bar=true" produces "Property >> '._-foo-bar' not found". >> >> Error messages using a mangled name rather than the name the user >> actually wrote is user-hostile, but that's a different topic. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > I suggest we follow the same approach we used in the x86 CPU > code: instead of requiring a special parser that magically > translate strings, just add property aliases for the old names > that contained "_". It would fix the user-hostile error messages > as well. Adding the aliases is slightly annoying, but it's probably the easiest way to get decent error messages. How can we ensure no new alias-requiring names get added?
On Mon, Oct 17, 2016 at 01:00:29PM +0200, Markus Armbruster wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > On Thu, Oct 13, 2016 at 06:44:14PM +0200, Markus Armbruster wrote: > >> machine_set_property() replaces '_' by '-' in the property name. > >> Except it fails to replace an initial '_'. Screwed up in commit > >> b0ddb8b. Reproducer: "-M pc,__foo_bar=true" produces "Property > >> '._-foo-bar' not found". > >> > >> Error messages using a mangled name rather than the name the user > >> actually wrote is user-hostile, but that's a different topic. > >> > >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > > > I suggest we follow the same approach we used in the x86 CPU > > code: instead of requiring a special parser that magically > > translate strings, just add property aliases for the old names > > that contained "_". It would fix the user-hostile error messages > > as well. > > Adding the aliases is slightly annoying, but it's probably the easiest > way to get decent error messages. I started working on it, but then I noticed I had do check for "_" in all TYPE_MACHINE subclasses and it would take more time than I expected. Then I put it at the end of my queue and went work on something else. > How can we ensure no new alias-requiring names get added? Being a style issue, maybe checkpatch.pl could detect some cases. It's not different from the other QOM classes.
Eduardo Habkost <ehabkost@redhat.com> writes: > On Mon, Oct 17, 2016 at 01:00:29PM +0200, Markus Armbruster wrote: >> Eduardo Habkost <ehabkost@redhat.com> writes: >> >> > On Thu, Oct 13, 2016 at 06:44:14PM +0200, Markus Armbruster wrote: >> >> machine_set_property() replaces '_' by '-' in the property name. >> >> Except it fails to replace an initial '_'. Screwed up in commit >> >> b0ddb8b. Reproducer: "-M pc,__foo_bar=true" produces "Property >> >> '._-foo-bar' not found". >> >> >> >> Error messages using a mangled name rather than the name the user >> >> actually wrote is user-hostile, but that's a different topic. >> >> >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> > >> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> >> > >> > I suggest we follow the same approach we used in the x86 CPU >> > code: instead of requiring a special parser that magically >> > translate strings, just add property aliases for the old names >> > that contained "_". It would fix the user-hostile error messages >> > as well. >> >> Adding the aliases is slightly annoying, but it's probably the easiest >> way to get decent error messages. > > I started working on it, but then I noticed I had do check for > "_" in all TYPE_MACHINE subclasses and it would take more time > than I expected. Then I put it at the end of my queue and went > work on something else. > >> How can we ensure no new alias-requiring names get added? > > Being a style issue, maybe checkpatch.pl could detect some cases. > It's not different from the other QOM classes. Can we adopt the rule for all of QOM, and enforce it right when properties get added? Or at some other suitable time?
On Mon, Oct 17, 2016 at 07:10:01PM +0200, Markus Armbruster wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > On Mon, Oct 17, 2016 at 01:00:29PM +0200, Markus Armbruster wrote: > >> Eduardo Habkost <ehabkost@redhat.com> writes: > >> > >> > On Thu, Oct 13, 2016 at 06:44:14PM +0200, Markus Armbruster wrote: > >> >> machine_set_property() replaces '_' by '-' in the property name. > >> >> Except it fails to replace an initial '_'. Screwed up in commit > >> >> b0ddb8b. Reproducer: "-M pc,__foo_bar=true" produces "Property > >> >> '._-foo-bar' not found". > >> >> > >> >> Error messages using a mangled name rather than the name the user > >> >> actually wrote is user-hostile, but that's a different topic. > >> >> > >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > >> > > >> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > >> > > >> > I suggest we follow the same approach we used in the x86 CPU > >> > code: instead of requiring a special parser that magically > >> > translate strings, just add property aliases for the old names > >> > that contained "_". It would fix the user-hostile error messages > >> > as well. > >> > >> Adding the aliases is slightly annoying, but it's probably the easiest > >> way to get decent error messages. > > > > I started working on it, but then I noticed I had do check for > > "_" in all TYPE_MACHINE subclasses and it would take more time > > than I expected. Then I put it at the end of my queue and went > > work on something else. > > > >> How can we ensure no new alias-requiring names get added? > > > > Being a style issue, maybe checkpatch.pl could detect some cases. > > It's not different from the other QOM classes. > > Can we adopt the rule for all of QOM, and enforce it right when > properties get added? Or at some other suitable time? You mean, at runtime? We could, but if there are existing properties that would fail the test we need to fix them first.
diff --git a/vl.c b/vl.c index c657acd..1c0b0ba 100644 --- a/vl.c +++ b/vl.c @@ -2804,17 +2804,16 @@ static int machine_set_property(void *opaque, { Object *obj = OBJECT(opaque); Error *local_err = NULL; - char *c, *qom_name; + char *p, *qom_name; if (strcmp(name, "type") == 0) { return 0; } qom_name = g_strdup(name); - c = qom_name; - while (*c++) { - if (*c == '_') { - *c = '-'; + for (p = qom_name; *p; p++) { + if (*p == '_') { + *p = '-'; } }
machine_set_property() replaces '_' by '-' in the property name. Except it fails to replace an initial '_'. Screwed up in commit b0ddb8b. Reproducer: "-M pc,__foo_bar=true" produces "Property '._-foo-bar' not found". Error messages using a mangled name rather than the name the user actually wrote is user-hostile, but that's a different topic. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- vl.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)