device-introspect-test uses HMP, so it should escape the device name
properly.  Because of this, a few devices that had commas in their
names were escaping testing.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/qtest/device-introspect-test.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tests/qtest/device-introspect-test.c b/tests/qtest/device-introspect-test.c
index 9f22340ee5..f471b0e0dd 100644
--- a/tests/qtest/device-introspect-test.c
+++ b/tests/qtest/device-introspect-test.c
@@ -104,7 +104,9 @@ static QList *device_type_list(QTestState *qts, bool abstract)
 static void test_one_device(QTestState *qts, const char *type)
 {
     QDict *resp;
-    char *help;
+    g_autofree char *help;
+    g_autofree GRegex *comma;
+    g_autofree char *escaped;
 
     g_test_message("Testing device '%s'", type);
 
@@ -113,8 +115,9 @@ static void test_one_device(QTestState *qts, const char *type)
                type);
     qobject_unref(resp);
 
-    help = qtest_hmp(qts, "device_add \"%s,help\"", type);
-    g_free(help);
+    comma = g_regex_new(",", 0, 0, NULL);
+    escaped = g_regex_replace_literal(comma, type, -1, 0, ",,", 0, NULL);
+    help = qtest_hmp(qts, "device_add \"%s,help\"", escaped);
 }
 
 static void test_device_intro_list(void)
-- 
2.26.2

On 03/11/2020 16.14, Paolo Bonzini wrote:
> device-introspect-test uses HMP, so it should escape the device name
> properly.  Because of this, a few devices that had commas in their
> names were escaping testing.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  tests/qtest/device-introspect-test.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qtest/device-introspect-test.c b/tests/qtest/device-introspect-test.c
> index 9f22340ee5..f471b0e0dd 100644
> --- a/tests/qtest/device-introspect-test.c
> +++ b/tests/qtest/device-introspect-test.c
> @@ -104,7 +104,9 @@ static QList *device_type_list(QTestState *qts, bool abstract)
>  static void test_one_device(QTestState *qts, const char *type)
>  {
>      QDict *resp;
> -    char *help;
> +    g_autofree char *help;
> +    g_autofree GRegex *comma;
> +    g_autofree char *escaped;
>  
>      g_test_message("Testing device '%s'", type);
>  
> @@ -113,8 +115,9 @@ static void test_one_device(QTestState *qts, const char *type)
>                 type);
>      qobject_unref(resp);
>  
> -    help = qtest_hmp(qts, "device_add \"%s,help\"", type);
> -    g_free(help);
> +    comma = g_regex_new(",", 0, 0, NULL);
> +    escaped = g_regex_replace_literal(comma, type, -1, 0, ",,", 0, NULL);
> +    help = qtest_hmp(qts, "device_add \"%s,help\"", escaped);
>  }

Having "help =" as final statement now, this looks somewhat weird at a first
glance (until you look at the g_autofree at the beginning of the function).
Maybe it's better to drop the help variable completely and just do:
g_free(gtest_hmp(...)) ?

 Thomas

I will just drop autofree usage completely, also because valgrind showed
that GRegex does not support it and apparently is leaked.

Paolo

Il mer 4 nov 2020, 08:44 Thomas Huth <thuth@redhat.com> ha scritto:

> On 03/11/2020 16.14, Paolo Bonzini wrote:
> > device-introspect-test uses HMP, so it should escape the device name
> > properly.  Because of this, a few devices that had commas in their
> > names were escaping testing.
> >
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  tests/qtest/device-introspect-test.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/tests/qtest/device-introspect-test.c
> b/tests/qtest/device-introspect-test.c
> > index 9f22340ee5..f471b0e0dd 100644
> > --- a/tests/qtest/device-introspect-test.c
> > +++ b/tests/qtest/device-introspect-test.c
> > @@ -104,7 +104,9 @@ static QList *device_type_list(QTestState *qts, bool
> abstract)
> >  static void test_one_device(QTestState *qts, const char *type)
> >  {
> >      QDict *resp;
> > -    char *help;
> > +    g_autofree char *help;
> > +    g_autofree GRegex *comma;
> > +    g_autofree char *escaped;
> >
> >      g_test_message("Testing device '%s'", type);
> >
> > @@ -113,8 +115,9 @@ static void test_one_device(QTestState *qts, const
> char *type)
> >                 type);
> >      qobject_unref(resp);
> >
> > -    help = qtest_hmp(qts, "device_add \"%s,help\"", type);
> > -    g_free(help);
> > +    comma = g_regex_new(",", 0, 0, NULL);
> > +    escaped = g_regex_replace_literal(comma, type, -1, 0, ",,", 0,
> NULL);
> > +    help = qtest_hmp(qts, "device_add \"%s,help\"", escaped);
> >  }
>
> Having "help =" as final statement now, this looks somewhat weird at a
> first
> glance (until you look at the g_autofree at the beginning of the function).
> Maybe it's better to drop the help variable completely and just do:
> g_free(gtest_hmp(...)) ?
>
>  Thomas
>
>

Paolo Bonzini <pbonzini@redhat.com> writes:

> device-introspect-test uses HMP, so it should escape the device name
> properly.  Because of this, a few devices that had commas in their
> names were escaping testing.
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

$ git-grep '\.name *= *"[^"]*,' | cat
hw/block/fdc.c:    .name          = "SUNW,fdtwo"

Any others?

On 06/11/20 14:15, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> device-introspect-test uses HMP, so it should escape the device name
>> properly.  Because of this, a few devices that had commas in their
>> names were escaping testing.
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> $ git-grep '\.name *= *"[^"]*,' | cat
> hw/block/fdc.c:    .name          = "SUNW,fdtwo"
> 
> Any others?

Not that I know, but this is a bug anyway. :)

Paolo

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 06/11/20 14:15, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> device-introspect-test uses HMP, so it should escape the device name
>>> properly.  Because of this, a few devices that had commas in their
>>> names were escaping testing.
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>
>> $ git-grep '\.name *= *"[^"]*,' | cat
>> hw/block/fdc.c:    .name          = "SUNW,fdtwo"
>> Any others?
>
> Not that I know, but this is a bug anyway. :)

Yes, but "a few devices" made me curious.