Message ID | 20200114145437.28382-1-berto@igalia.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | qcow2: Use a GString in report_unsupported_feature() | expand |
Alberto Garcia <berto@igalia.com> writes: > 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. > > Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block/qcow2.c | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/block/qcow2.c b/block/qcow2.c > index cef9d72b3a..ecf6827420 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; > + GString *features = g_string_sized_new(60); g_autoptr(GString) features = g_string_sized_new(60); will save you the clean-up later. > > 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); We have a number of cases of this sort of idiom in the code base. I wonder if it calls for a utility function: qemu_append_with_sep(features, ", ", "%.46s", table->name) Anyway not mandatory for this patch so with the autoptr fix: Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > mask &= ~(1ULL << table->bit); > } > } > @@ -470,14 +469,15 @@ 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); > + g_string_free(features, TRUE); > } > > /*
On 1/14/20 7:08 PM, Alex Bennée wrote: > Alberto Garcia <berto@igalia.com> writes: > >> 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. >> >> Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> Signed-off-by: Alberto Garcia <berto@igalia.com> >> --- >> block/qcow2.c | 24 ++++++++++++------------ >> 1 file changed, 12 insertions(+), 12 deletions(-) >> >> diff --git a/block/qcow2.c b/block/qcow2.c >> index cef9d72b3a..ecf6827420 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; >> + GString *features = g_string_sized_new(60); > > g_autoptr(GString) features = g_string_sized_new(60); > > will save you the clean-up later. Does this work with g_string_free() too? >> >> 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); > > We have a number of cases of this sort of idiom in the code base. I > wonder if it calls for a utility function: > > qemu_append_with_sep(features, ", ", "%.46s", table->name) Good idea for https://wiki.qemu.org/Contribute/BiteSizedTasks > Anyway not mandatory for this patch so with the autoptr fix: > > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > >> mask &= ~(1ULL << table->bit); >> } >> } >> @@ -470,14 +469,15 @@ 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); >> + g_string_free(features, TRUE); >> } >> >> /* > >
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > On 1/14/20 7:08 PM, Alex Bennée wrote: >> Alberto Garcia <berto@igalia.com> writes: >> >>> 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. >>> >>> Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> Signed-off-by: Alberto Garcia <berto@igalia.com> >>> --- >>> block/qcow2.c | 24 ++++++++++++------------ >>> 1 file changed, 12 insertions(+), 12 deletions(-) >>> >>> diff --git a/block/qcow2.c b/block/qcow2.c >>> index cef9d72b3a..ecf6827420 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; >>> + GString *features = g_string_sized_new(60); >> g_autoptr(GString) features = g_string_sized_new(60); >> will save you the clean-up later. > > Does this work with g_string_free() too? It does: static inline void g_autoptr_cleanup_gstring_free (GString *string) { if (string) g_string_free (string, TRUE); } If you want to keep the allocated string but drop the GString you are still best doing that yourself. > >>> 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); >> We have a number of cases of this sort of idiom in the code base. I >> wonder if it calls for a utility function: >> qemu_append_with_sep(features, ", ", "%.46s", table->name) > > Good idea for https://wiki.qemu.org/Contribute/BiteSizedTasks > >> Anyway not mandatory for this patch so with the autoptr fix: >> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> >> >>> mask &= ~(1ULL << table->bit); >>> } >>> } >>> @@ -470,14 +469,15 @@ 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); >>> + g_string_free(features, TRUE); >>> } >>> /* >>
On Tue 14 Jan 2020 07:08:04 PM CET, Alex Bennée wrote: > g_autoptr(GString) features = g_string_sized_new(60); > > will save you the clean-up later. Ok, will send v2 now. >> + if (features->len > 0) { >> + g_string_append(features, ", "); >> + } >> + g_string_append_printf(features, "%.46s", >> table->name); > > We have a number of cases of this sort of idiom in the code base. I > wonder if it calls for a utility function: > > qemu_append_with_sep(features, ", ", "%.46s", table->name) Maybe it's worth checking. I can have a look once this is applied (there was another similar one in the queue already). Berto
On 1/14/20 10:35 PM, Alex Bennée wrote: > Philippe Mathieu-Daudé <philmd@redhat.com> writes: > >> On 1/14/20 7:08 PM, Alex Bennée wrote: >>> Alberto Garcia <berto@igalia.com> writes: >>> >>>> 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. >>>> >>>> Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>> Signed-off-by: Alberto Garcia <berto@igalia.com> >>>> --- >>>> block/qcow2.c | 24 ++++++++++++------------ >>>> 1 file changed, 12 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/block/qcow2.c b/block/qcow2.c >>>> index cef9d72b3a..ecf6827420 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; >>>> + GString *features = g_string_sized_new(60); >>> g_autoptr(GString) features = g_string_sized_new(60); >>> will save you the clean-up later. >> >> Does this work with g_string_free() too? > > It does: > > static inline void g_autoptr_cleanup_gstring_free (GString *string) > { > if (string) > g_string_free (string, TRUE); > } The implicit use of free_segment=TRUE was not obvious to me. Thanks for checking it in glib source. > If you want to keep the allocated string but drop the GString you are > still best doing that yourself. I agree. I asked because I had the other patch from Alberto in mind: https://www.mail-archive.com/qemu-devel@nongnu.org/msg669862.html In this case we can not use the g_autoptr feature. >>>> 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); >>> We have a number of cases of this sort of idiom in the code base. I >>> wonder if it calls for a utility function: >>> qemu_append_with_sep(features, ", ", "%.46s", table->name) >> >> Good idea for https://wiki.qemu.org/Contribute/BiteSizedTasks >> >>> Anyway not mandatory for this patch so with the autoptr fix: >>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> >>> >>>> mask &= ~(1ULL << table->bit); >>>> } >>>> } >>>> @@ -470,14 +469,15 @@ 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); >>>> + g_string_free(features, TRUE); >>>> } >>>> /* >>>
diff --git a/block/qcow2.c b/block/qcow2.c index cef9d72b3a..ecf6827420 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; + 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,15 @@ 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); + g_string_free(features, TRUE); } /*
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. Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com> Signed-off-by: Alberto Garcia <berto@igalia.com> --- block/qcow2.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)