Philippe Mathieu-Daudé <phi...@redhat.com> writes:
> On 1/14/20 7:08 PM, Alex Bennée wrote: >> Alberto Garcia <be...@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é <phi...@redhat.com> >>> Signed-off-by: Alberto Garcia <be...@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.ben...@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); >>> } >>> /* >> -- Alex Bennée