|
From: | Philippe Mathieu-Daudé |
Subject: | Re: [PATCH] qcow2: Use a GString in report_unsupported_feature() |
Date: | Wed, 15 Jan 2020 15:23:23 +0100 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 |
On 1/14/20 10:35 PM, Alex Bennée wrote:
Philippe Mathieu-Daudé <address@hidden> writes:On 1/14/20 7:08 PM, Alex Bennée wrote:Alberto Garcia <address@hidden> 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é <address@hidden> Signed-off-by: Alberto Garcia <address@hidden> --- 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/address@hidden/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/BiteSizedTasksAnyway not mandatory for this patch so with the autoptr fix: Reviewed-by: Alex Bennée <address@hidden>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); } /*
[Prev in Thread] | Current Thread | [Next in Thread] |