[Qemu-devel] [PATCH] machine: Fix replacement of '_' by '-' in machine property names

[Qemu-devel] [PATCH] machine: Fix replacement of '_' by '-' in machine property names

[Markus Armbruster]

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(-)

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

Re: [Qemu-devel] [PATCH] machine: Fix replacement of '_' by '-' in machine property names

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 719 bytes --]

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>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] machine: Fix replacement of '_' by '-' in machine property names

[Eduardo Habkost]

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
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH] machine: Fix replacement of '_' by '-' in machine property names

[Eduardo Habkost]

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

Re: [Qemu-devel] [PATCH] machine: Fix replacement of '_' by '-' in machine property names

[Markus Armbruster]

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?

Re: [Qemu-devel] [PATCH] machine: Fix replacement of '_' by '-' in machine property names

[Eduardo Habkost]

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

Re: [Qemu-devel] [PATCH] machine: Fix replacement of '_' by '-' in machine property names

[Markus Armbruster]

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?

Re: [Qemu-devel] [PATCH] machine: Fix replacement of '_' by '-' in machine property names

[Eduardo Habkost]

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.

-- 
Eduardo