[PATCH 18/20] keyval: Use GString to accumulate value strings

Markus Armbruster posted 20 patches 5 years, 1 month ago
Maintainers: Jiaxun Yang <jiaxun.yang@flygoat.com>, Christian Borntraeger <borntraeger@de.ibm.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Laurent Vivier <lvivier@redhat.com>, Yuval Shaia <yuval.shaia.ml@gmail.com>, Laszlo Ersek <lersek@redhat.com>, Gerd Hoffmann <kraxel@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Halil Pasic <pasic@linux.ibm.com>, Michael Roth <mdroth@linux.vnet.ibm.com>, Kevin Wolf <kwolf@redhat.com>, Stafford Horne <shorne@gmail.com>, Juan Quintela <quintela@redhat.com>, Markus Armbruster <armbru@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Artyom Tarasenko <atar4qemu@gmail.com>, Peter Maydell <peter.maydell@linaro.org>, Michael Rolnik <mrolnik@gmail.com>, Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>, Richard Henderson <richard.henderson@linaro.org>, Fam Zheng <fam@euphon.net>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, "Philippe Mathieu-Daudé" <philmd@redhat.com>, Cornelia Huck <cohuck@redhat.com>, Aurelien Jarno <aurelien@aurel32.net>, Jason Dillaman <dillaman@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, David Gibson <david@gibson.dropbear.id.au>, Max Reitz <mreitz@redhat.com>, Andrzej Zaborowski <balrogg@gmail.com>, Sarah Harris <S.E.Harris@kent.ac.uk>, Thomas Huth <thuth@redhat.com>
[PATCH 18/20] keyval: Use GString to accumulate value strings
Posted by Markus Armbruster 5 years, 1 month ago
QString supports modifying its string, but it's quite limited: you can
only append.  The remaining callers use it for building an initial
string, never for modifying it later.

Change keyval_parse_one() to do build the initial string with GString.
This is another step towards making QString immutable.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 util/keyval.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/util/keyval.c b/util/keyval.c
index 7f625ad33c..be34928813 100644
--- a/util/keyval.c
+++ b/util/keyval.c
@@ -189,7 +189,7 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
     QDict *cur;
     int ret;
     QObject *next;
-    QString *val;
+    GString *val;
 
     key = params;
     val_end = NULL;
@@ -263,7 +263,7 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
 
     if (key == implied_key) {
         assert(!*s);
-        val = qstring_from_substr(params, 0, val_end - params);
+        val = g_string_new_len(params, val_end - params);
         s = val_end;
         if (*s == ',') {
             s++;
@@ -276,7 +276,7 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
         }
         s++;
 
-        val = qstring_new();
+        val = g_string_new(NULL);
         for (;;) {
             if (!*s) {
                 break;
@@ -286,11 +286,12 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
                     break;
                 }
             }
-            qstring_append_chr(val, *s++);
+            g_string_append_c(val, *s++);
         }
     }
 
-    if (!keyval_parse_put(cur, key_in_cur, val, key, key_end, errp)) {
+    if (!keyval_parse_put(cur, key_in_cur, qstring_from_gstring(val),
+                          key, key_end, errp)) {
         return NULL;
     }
     return s;
-- 
2.26.2


Re: [PATCH 18/20] keyval: Use GString to accumulate value strings
Posted by Paolo Bonzini 5 years, 1 month ago
On 11/12/20 18:11, Markus Armbruster wrote:
> QString supports modifying its string, but it's quite limited: you can
> only append.  The remaining callers use it for building an initial
> string, never for modifying it later.
> 
> Change keyval_parse_one() to do build the initial string with GString.
> This is another step towards making QString immutable.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

It's a bit unfortunate that the infamous "keyval: accept escaped commas 
in implied option" patch was already getting rid of mutable QString.

It used a completely different mechanism, namely unescaping the string 
in place.  This means that my patch was doing n+1 allocations, versus a 
best case of n and a generic case of O(n) for this patch.  The 
difference does not really matter, though I still like my code better.

Paolo

> ---
>   util/keyval.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/util/keyval.c b/util/keyval.c
> index 7f625ad33c..be34928813 100644
> --- a/util/keyval.c
> +++ b/util/keyval.c
> @@ -189,7 +189,7 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
>       QDict *cur;
>       int ret;
>       QObject *next;
> -    QString *val;
> +    GString *val;
>   
>       key = params;
>       val_end = NULL;
> @@ -263,7 +263,7 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
>   
>       if (key == implied_key) {
>           assert(!*s);
> -        val = qstring_from_substr(params, 0, val_end - params);
> +        val = g_string_new_len(params, val_end - params);
>           s = val_end;
>           if (*s == ',') {
>               s++;
> @@ -276,7 +276,7 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
>           }
>           s++;
>   
> -        val = qstring_new();
> +        val = g_string_new(NULL);
>           for (;;) {
>               if (!*s) {
>                   break;
> @@ -286,11 +286,12 @@ static const char *keyval_parse_one(QDict *qdict, const char *params,
>                       break;
>                   }
>               }
> -            qstring_append_chr(val, *s++);
> +            g_string_append_c(val, *s++);
>           }
>       }
>   
> -    if (!keyval_parse_put(cur, key_in_cur, val, key, key_end, errp)) {
> +    if (!keyval_parse_put(cur, key_in_cur, qstring_from_gstring(val),
> +                          key, key_end, errp)) {
>           return NULL;
>       }
>       return s;
> 


Re: [PATCH 18/20] keyval: Use GString to accumulate value strings
Posted by Markus Armbruster 5 years ago
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 11/12/20 18:11, Markus Armbruster wrote:
>> QString supports modifying its string, but it's quite limited: you can
>> only append.  The remaining callers use it for building an initial
>> string, never for modifying it later.
>> Change keyval_parse_one() to do build the initial string with
>> GString.
>> This is another step towards making QString immutable.
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> It's a bit unfortunate that the infamous "keyval: accept escaped
> commas in implied option" patch was already getting rid of mutable
> QString.
>
> It used a completely different mechanism, namely unescaping the string
> in place.  This means that my patch was doing n+1 allocations, versus
> a best case of n and a generic case of O(n) for this patch.  The 
> difference does not really matter, though I still like my code better.

My patch is not intended as a replacement of yours.  Mine does much
less.

I had to choose between creating a conflict and holding back my series
while we figure out what to do with your patch.  The dilemma is my own
doing; your patch is waiting just for me.  I picked the conflict.

I can look into rebasing your patch on top of mine.


Re: [PATCH 18/20] keyval: Use GString to accumulate value strings
Posted by Paolo Bonzini 5 years ago
On 11/01/21 14:05, Markus Armbruster wrote:
> I had to choose between creating a conflict and holding back my series
> while we figure out what to do with your patch.  The dilemma is my own
> doing; your patch is waiting just for me.  I picked the conflict.
> 
> I can look into rebasing your patch on top of mine.

No need to.  My patch also removes the use of GString, so the code after 
my patch can be exactly the same.  Or in other words, squashing a revert 
in front of my patch just works.

Paolo