[PATCH v2] qcow2: Use a GString in report_unsupported_feature()

[PATCH v2] qcow2: Use a GString in report_unsupported_feature()

[Alberto Garcia]

This is a bit more efficient than having to allocate and free memory
for each item.

The default size (60) is enough for all the existing incompatible
features or the "Unknown incompatible feature" message.

Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
 block/qcow2.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

v2: Use g_autoptr and update commit message

diff --git a/block/qcow2.c b/block/qcow2.c
index cef9d72b3a..e29fc07068 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -453,16 +453,15 @@ static void cleanup_unknown_header_ext(BlockDriverState *bs)
 static void report_unsupported_feature(Error **errp, Qcow2Feature *table,
                                        uint64_t mask)
 {
-    char *features = g_strdup("");
-    char *old;
+    g_autoptr(GString) features = g_string_sized_new(60);
 
     while (table && table->name[0] != '\0') {
         if (table->type == QCOW2_FEAT_TYPE_INCOMPATIBLE) {
             if (mask & (1ULL << table->bit)) {
-                old = features;
-                features = g_strdup_printf("%s%s%.46s", old, *old ? ", " : "",
-                                           table->name);
-                g_free(old);
+                if (features->len > 0) {
+                    g_string_append(features, ", ");
+                }
+                g_string_append_printf(features, "%.46s", table->name);
                 mask &= ~(1ULL << table->bit);
             }
         }
@@ -470,14 +469,14 @@ static void report_unsupported_feature(Error **errp, Qcow2Feature *table,
     }
 
     if (mask) {
-        old = features;
-        features = g_strdup_printf("%s%sUnknown incompatible feature: %" PRIx64,
-                                   old, *old ? ", " : "", mask);
-        g_free(old);
+        if (features->len > 0) {
+            g_string_append(features, ", ");
+        }
+        g_string_append_printf(features,
+                               "Unknown incompatible feature: %" PRIx64, mask);
     }
 
-    error_setg(errp, "Unsupported qcow2 feature(s): %s", features);
-    g_free(features);
+    error_setg(errp, "Unsupported qcow2 feature(s): %s", features->str);
 }
 
 /*
-- 
2.20.1

Re: [PATCH v2] qcow2: Use a GString in report_unsupported_feature()

[Philippe Mathieu-Daudé]

On 1/15/20 2:56 PM, Alberto Garcia wrote:
> This is a bit more efficient than having to allocate and free memory
> for each item.
> 
> The default size (60) is enough for all the existing incompatible
> features or the "Unknown incompatible feature" message.
> 
> Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   block/qcow2.c | 23 +++++++++++------------
>   1 file changed, 11 insertions(+), 12 deletions(-)
> 
> v2: Use g_autoptr and update commit message
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index cef9d72b3a..e29fc07068 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -453,16 +453,15 @@ static void cleanup_unknown_header_ext(BlockDriverState *bs)
>   static void report_unsupported_feature(Error **errp, Qcow2Feature *table,
>                                          uint64_t mask)
>   {
> -    char *features = g_strdup("");
> -    char *old;
> +    g_autoptr(GString) features = g_string_sized_new(60);
>   
>       while (table && table->name[0] != '\0') {
>           if (table->type == QCOW2_FEAT_TYPE_INCOMPATIBLE) {
>               if (mask & (1ULL << table->bit)) {
> -                old = features;
> -                features = g_strdup_printf("%s%s%.46s", old, *old ? ", " : "",
> -                                           table->name);
> -                g_free(old);
> +                if (features->len > 0) {
> +                    g_string_append(features, ", ");
> +                }
> +                g_string_append_printf(features, "%.46s", table->name);
>                   mask &= ~(1ULL << table->bit);
>               }
>           }
> @@ -470,14 +469,14 @@ static void report_unsupported_feature(Error **errp, Qcow2Feature *table,
>       }
>   
>       if (mask) {
> -        old = features;
> -        features = g_strdup_printf("%s%sUnknown incompatible feature: %" PRIx64,
> -                                   old, *old ? ", " : "", mask);
> -        g_free(old);
> +        if (features->len > 0) {
> +            g_string_append(features, ", ");
> +        }
> +        g_string_append_printf(features,
> +                               "Unknown incompatible feature: %" PRIx64, mask);
>       }
>   
> -    error_setg(errp, "Unsupported qcow2 feature(s): %s", features);
> -    g_free(features);
> +    error_setg(errp, "Unsupported qcow2 feature(s): %s", features->str);
>   }
>   
>   /*
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v2] qcow2: Use a GString in report_unsupported_feature()

[Stefano Garzarella]

On Wed, Jan 15, 2020 at 02:56:26PM +0100, Alberto Garcia wrote:
> This is a bit more efficient than having to allocate and free memory
> for each item.
> 
> The default size (60) is enough for all the existing incompatible
> features or the "Unknown incompatible feature" message.
> 
> Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  block/qcow2.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

> 
> v2: Use g_autoptr and update commit message
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index cef9d72b3a..e29fc07068 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -453,16 +453,15 @@ static void cleanup_unknown_header_ext(BlockDriverState *bs)
>  static void report_unsupported_feature(Error **errp, Qcow2Feature *table,
>                                         uint64_t mask)
>  {
> -    char *features = g_strdup("");
> -    char *old;
> +    g_autoptr(GString) features = g_string_sized_new(60);
>  
>      while (table && table->name[0] != '\0') {
>          if (table->type == QCOW2_FEAT_TYPE_INCOMPATIBLE) {
>              if (mask & (1ULL << table->bit)) {
> -                old = features;
> -                features = g_strdup_printf("%s%s%.46s", old, *old ? ", " : "",
> -                                           table->name);
> -                g_free(old);
> +                if (features->len > 0) {
> +                    g_string_append(features, ", ");
> +                }
> +                g_string_append_printf(features, "%.46s", table->name);
>                  mask &= ~(1ULL << table->bit);
>              }
>          }
> @@ -470,14 +469,14 @@ static void report_unsupported_feature(Error **errp, Qcow2Feature *table,
>      }
>  
>      if (mask) {
> -        old = features;
> -        features = g_strdup_printf("%s%sUnknown incompatible feature: %" PRIx64,
> -                                   old, *old ? ", " : "", mask);
> -        g_free(old);
> +        if (features->len > 0) {
> +            g_string_append(features, ", ");
> +        }
> +        g_string_append_printf(features,
> +                               "Unknown incompatible feature: %" PRIx64, mask);
>      }
>  
> -    error_setg(errp, "Unsupported qcow2 feature(s): %s", features);
> -    g_free(features);
> +    error_setg(errp, "Unsupported qcow2 feature(s): %s", features->str);
>  }
>  
>  /*
> -- 
> 2.20.1
> 
> 

Re: [PATCH v2] qcow2: Use a GString in report_unsupported_feature()

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2457 bytes --]

On 15.01.20 14:56, Alberto Garcia wrote:
> This is a bit more efficient than having to allocate and free memory
> for each item.
> 
> The default size (60) is enough for all the existing incompatible
> features or the "Unknown incompatible feature" message.

That doesn’t make sense to me.  The existing incompatible features are
known to qemu, and as such will never be printed here.

But just because it doesn’t make sense doesn’t make it wrong.  I suppose
we can assume that if new features are added, they will have a similar
length like the ones we have now.

(Well, it does make “60” a weirdly specific number, but whatever.)

((I’d just have created an empty string.  Considering this is part of
the explanation for a fatal error, nobody cares whether this function
takes a couple of microseconds more.  Or maybe a length of 47, so it can
definitely hold exactly one incompatible feature name.))

> Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  block/qcow2.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)

Thanks, applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

> diff --git a/block/qcow2.c b/block/qcow2.c
> index cef9d72b3a..e29fc07068 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c

[...]

> @@ -470,14 +469,14 @@ static void report_unsupported_feature(Error **errp, Qcow2Feature *table,
>      }
>  
>      if (mask) {
> -        old = features;
> -        features = g_strdup_printf("%s%sUnknown incompatible feature: %" PRIx64,
> -                                   old, *old ? ", " : "", mask);
> -        g_free(old);
> +        if (features->len > 0) {
> +            g_string_append(features, ", ");
> +        }
> +        g_string_append_printf(features,
> +                               "Unknown incompatible feature: %" PRIx64, mask);

Existing problem: This can lead to stuff like

“Unsupported qcow2 feature(s): feat1, feat2, Unknown incompatible
feature: 0xec0”

(i.e., singular “feature” when there are multiple unknown features, and
capitalization of “Unknown”)

O:-)

But whatever.  It’s unlikely there’s going to be more than one
incompatible feature, and it’s extremely unlikely it won’t have a string
description.

Max


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

Re: [PATCH v2] qcow2: Use a GString in report_unsupported_feature()

[Alberto Garcia]

On Thu 16 Jan 2020 01:19:34 PM CET, Max Reitz wrote:
> On 15.01.20 14:56, Alberto Garcia wrote:
>> This is a bit more efficient than having to allocate and free memory
>> for each item.
>> 
>> The default size (60) is enough for all the existing incompatible
>> features or the "Unknown incompatible feature" message.
>
> That doesn’t make sense to me.  The existing incompatible features are
> known to qemu, and as such will never be printed here.

I know, it's sort of an arbitrary number, I just chose a default size
that is big enough for the strings we have.

> ((I’d just have created an empty string.  Considering this is part of
> the explanation for a fatal error, nobody cares whether this function
> takes a couple of microseconds more.  Or maybe a length of 47, so it
> can definitely hold exactly one incompatible feature name.))

That would be fine with me as well, I don't think it matters so much
either way. Feel free to change the default size or remove the
explanation from the commit message if you want.

Berto