[libvirt] [PATCH 0/7] Introduce automatic clearing for virBuffer
First 4 patches clean up existing mess. Note that this series both fixes and then deletes virBufferEscapeN. The split is deliberate, so that if somebody needs virBufferEscapeN in the future they can revert the removal and get a working version. Peter Krempa (7): util: buffer: Remove misleading AUTOPTR func for 'virBuffer' util: buf: Fix memory leak in virBufferEscapeN tests: buf: Fix debug messages in 'testBufEscapeRegex' util: buf: Remove virBufferEscapeN util: alloc: Introduce 'VIR_AUTOCLEAN' macros for clearing stack'd structs util: buffer: Introduce VIR_AUTOCLEAN function for virBuffer qemu: domain: Use VIR_AUTOCLEAN for virBuffer src/libvirt_private.syms | 1 - src/qemu/qemu_domain.c | 27 +++------- src/util/viralloc.h | 32 +++++++++++- src/util/virbuffer.c | 107 --------------------------------------- src/util/virbuffer.h | 7 ++- tests/virbuftest.c | 57 +++++---------------- 6 files changed, 56 insertions(+), 175 deletions(-) -- 2.20.1
'virBufferFreeAndReset' does not free the top level structure itself. Additionally we almost exclusively use stack'd buffers rather than pointers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virbuffer.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/util/virbuffer.h b/src/util/virbuffer.h index 8bf6bee644..1c8e182064 100644 --- a/src/util/virbuffer.h +++ b/src/util/virbuffer.h @@ -119,6 +119,4 @@ int virBufferGetIndent(const virBuffer *buf, bool dynamic); void virBufferTrim(virBufferPtr buf, const char *trim, int len); void virBufferAddStr(virBufferPtr buf, const char *str); -VIR_DEFINE_AUTOPTR_FUNC(virBuffer, virBufferFreeAndReset); - #endif /* LIBVIRT_VIRBUFFER_H */ -- 2.20.1
The conversion to VIR_AUTOFREE of 'escapeList' introduced memory leak of the copied item to be escaped: ==17517== 2 bytes in 1 blocks are definitely lost in loss record 1 of 32 ==17517== at 0x483880B: malloc (vg_replace_malloc.c:309) ==17517== by 0x54D666D: strdup (in /usr/lib64/libc-2.28.so) ==17517== by 0x497663E: virStrdup (virstring.c:956) ==17517== by 0x497663E: virStrdup (virstring.c:945) ==17517== by 0x48F8853: virBufferEscapeN (virbuffer.c:707) ==17517== by 0x403C9D: testBufEscapeN (virbuftest.c:383) ==17517== by 0x405FA8: virTestRun (testutils.c:174) ==17517== by 0x403A70: mymain (virbuftest.c:517) ==17517== by 0x406BC9: virTestMain (testutils.c:1097) ==17517== by 0x5470412: (below main) (in /usr/lib64/libc-2.28.so) [...] (all other have same backtrace as it happens in a loop) Fix it by reverting all the VIR_AUTO nonsense in this function as there is exactly one place where it's handled. This effectively reverts commits: d0a92a037123085398d4123472f59c71b436d485 96fbf6df90335c1f9315fc25162711fd632d4dab d261ed2fb1df95f6c7698b9321a82078a7335112 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virbuffer.c | 24 +++++------------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index 01d71f1b59..7399bdcdfe 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -643,26 +643,11 @@ virBufferEscape(virBufferPtr buf, char escape, const char *toescape, } -typedef struct _virBufferEscapePair virBufferEscapePair; -typedef virBufferEscapePair *virBufferEscapePairPtr; - -struct _virBufferEscapePair { +struct virBufferEscapePair { char escape; char *toescape; }; -static void -virBufferEscapePairFree(virBufferEscapePairPtr pair) -{ - if (!pair) - return; - - VIR_FREE(pair->toescape); - VIR_FREE(pair); -} - -VIR_DEFINE_AUTOPTR_FUNC(virBufferEscapePair, virBufferEscapePairFree); - /** * virBufferEscapeN: @@ -688,8 +673,8 @@ virBufferEscapeN(virBufferPtr buf, VIR_AUTOFREE(char *) escaped = NULL; char *out; const char *cur; - virBufferEscapePair escapeItem; - VIR_AUTOPTR(virBufferEscapePair) escapeList = NULL; + struct virBufferEscapePair escapeItem; + struct virBufferEscapePair *escapeList = NULL; size_t nescapeList = 0; va_list ap; @@ -704,7 +689,7 @@ virBufferEscapeN(virBufferPtr buf, va_start(ap, str); while ((escapeItem.escape = va_arg(ap, int))) { - if (VIR_STRDUP(escapeItem.toescape, va_arg(ap, char *)) < 0) { + if (!(escapeItem.toescape = va_arg(ap, char *))) { virBufferSetError(buf, errno); goto cleanup; } @@ -747,6 +732,7 @@ virBufferEscapeN(virBufferPtr buf, cleanup: va_end(ap); + VIR_FREE(escapeList); } -- 2.20.1
The messages reference testBufEscapeN instead of testBufEscapeRegex. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/virbuftest.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/virbuftest.c b/tests/virbuftest.c index 5bffe94c95..547438c646 100644 --- a/tests/virbuftest.c +++ b/tests/virbuftest.c @@ -412,12 +412,12 @@ testBufEscapeRegex(const void *opaque) virBufferEscapeRegex(&buf, "%s", data->data); if (!(actual = virBufferContentAndReset(&buf))) { - VIR_TEST_DEBUG("testBufEscapeN: buf is empty"); + VIR_TEST_DEBUG("testBufEscapeRegex: buf is empty"); goto cleanup; } if (STRNEQ_NULLABLE(actual, data->expect)) { - VIR_TEST_DEBUG("testBufEscapeN: Strings don't match:\n"); + VIR_TEST_DEBUG("testBufEscapeRegex: Strings don't match:\n"); virTestDifference(stderr, data->expect, actual); goto cleanup; } -- 2.20.1
The function was used only in the tests, remove it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 - src/util/virbuffer.c | 93 ---------------------------------------- src/util/virbuffer.h | 2 - tests/virbuftest.c | 41 ------------------ 4 files changed, 137 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b720acdc93..aa1b2c77be 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1488,7 +1488,6 @@ virBufferContentAndReset; virBufferCurrentContent; virBufferError; virBufferEscape; -virBufferEscapeN; virBufferEscapeRegex; virBufferEscapeSexpr; virBufferEscapeShell; diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index 7399bdcdfe..12bdd13d39 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -643,99 +643,6 @@ virBufferEscape(virBufferPtr buf, char escape, const char *toescape, } -struct virBufferEscapePair { - char escape; - char *toescape; -}; - - -/** - * virBufferEscapeN: - * @buf: the buffer to append to - * @format: a printf like format string but with only one %s parameter - * @str: the string argument which needs to be escaped - * @...: the variable list of escape pairs - * - * The variable list of arguments @... must be composed of - * 'char escape, char *toescape' pairs followed by NULL. - * - * This has the same functionality as virBufferEscape with the extension - * that allows to specify multiple pairs of chars that needs to be escaped. - */ -void -virBufferEscapeN(virBufferPtr buf, - const char *format, - const char *str, - ...) -{ - int len; - size_t i; - VIR_AUTOFREE(char *) escaped = NULL; - char *out; - const char *cur; - struct virBufferEscapePair escapeItem; - struct virBufferEscapePair *escapeList = NULL; - size_t nescapeList = 0; - va_list ap; - - if ((format == NULL) || (buf == NULL) || (str == NULL)) - return; - - if (buf->error) - return; - - len = strlen(str); - - va_start(ap, str); - - while ((escapeItem.escape = va_arg(ap, int))) { - if (!(escapeItem.toescape = va_arg(ap, char *))) { - virBufferSetError(buf, errno); - goto cleanup; - } - - if (strcspn(str, escapeItem.toescape) == len) - continue; - - if (VIR_APPEND_ELEMENT_QUIET(escapeList, nescapeList, escapeItem) < 0) { - virBufferSetError(buf, errno); - goto cleanup; - } - } - - if (nescapeList == 0) { - virBufferAsprintf(buf, format, str); - goto cleanup; - } - - if (xalloc_oversized(2, len) || - VIR_ALLOC_N_QUIET(escaped, 2 * len + 1) < 0) { - virBufferSetError(buf, errno); - goto cleanup; - } - - cur = str; - out = escaped; - while (*cur != 0) { - for (i = 0; i < nescapeList; i++) { - if (strchr(escapeList[i].toescape, *cur)) { - *out++ = escapeList[i].escape; - break; - } - } - *out++ = *cur; - cur++; - } - *out = 0; - - virBufferAsprintf(buf, format, escaped); - - cleanup: - va_end(ap); - VIR_FREE(escapeList); -} - - /** * virBufferURIEncodeString: * @buf: the buffer to append to diff --git a/src/util/virbuffer.h b/src/util/virbuffer.h index 1c8e182064..7e4e7645df 100644 --- a/src/util/virbuffer.h +++ b/src/util/virbuffer.h @@ -84,8 +84,6 @@ void virBufferStrcatVArgs(virBufferPtr buf, va_list ap); void virBufferEscape(virBufferPtr buf, char escape, const char *toescape, const char *format, const char *str); -void virBufferEscapeN(virBufferPtr buf, const char *format, - const char *str, ...); void virBufferEscapeString(virBufferPtr buf, const char *format, const char *str); void virBufferEscapeSexpr(virBufferPtr buf, const char *format, diff --git a/tests/virbuftest.c b/tests/virbuftest.c index 547438c646..bdb0a5e934 100644 --- a/tests/virbuftest.c +++ b/tests/virbuftest.c @@ -372,35 +372,6 @@ testBufEscapeStr(const void *opaque ATTRIBUTE_UNUSED) } -static int -testBufEscapeN(const void *opaque) -{ - const struct testBufAddStrData *data = opaque; - virBuffer buf = VIR_BUFFER_INITIALIZER; - char *actual; - int ret = -1; - - virBufferEscapeN(&buf, "%s", data->data, '\\', "=", ',', ",", NULL); - - if (!(actual = virBufferContentAndReset(&buf))) { - VIR_TEST_DEBUG("testBufEscapeN: buf is empty"); - goto cleanup; - } - - if (STRNEQ_NULLABLE(actual, data->expect)) { - VIR_TEST_DEBUG("testBufEscapeN: Strings don't match:\n"); - virTestDifference(stderr, data->expect, actual); - goto cleanup; - } - - ret = 0; - - cleanup: - VIR_FREE(actual); - return ret; -} - - static int testBufEscapeRegex(const void *opaque) { @@ -506,18 +477,6 @@ mymain(void) DO_TEST_ESCAPE("\x01\x01\x02\x03\x05\x08", "<c>\n <el></el>\n</c>"); -#define DO_TEST_ESCAPEN(data, expect) \ - do { \ - struct testBufAddStrData info = { data, expect }; \ - if (virTestRun("Buf: EscapeN", testBufEscapeN, &info) < 0) \ - ret = -1; \ - } while (0) - - DO_TEST_ESCAPEN("noescape", "noescape"); - DO_TEST_ESCAPEN("comma,escape", "comma,,escape"); - DO_TEST_ESCAPEN("equal=escape", "equal\\=escape"); - DO_TEST_ESCAPEN("comma,equal=escape", "comma,,equal\\=escape"); - #define DO_TEST_ESCAPE_REGEX(data, expect) \ do { \ struct testBufAddStrData info = { data, expect }; \ -- 2.20.1
The new utility macros are useful for variables we put on the stack but require some cleanup. The most prominent of those is virBuffer which is used almost exclusively in that way. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/viralloc.h | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 1d67fff95c..705c2636fa 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -613,7 +613,24 @@ void virAllocTestHook(void (*func)(int, void*), void *data); if (*_ptr) \ (func)(*_ptr); \ *_ptr = NULL; \ - } \ + } + +# define VIR_AUTOCLEAN_FUNC_NAME(type) type##AutoClean + +/** + * VIR_DEFINE_AUTOCLEAN_FUNC: + * @type: type of the variable to be cleared automatically + * @func: cleanup function to be automatically called + * + * This macro defines a function for automatic clearing of + * resources in a stack'd variable of type @type. This newly + * defined function works as a necessary wrapper around @func. + */ +# define VIR_DEFINE_AUTOCLEAN_FUNC(type, func) \ + static inline void VIR_AUTOCLEAN_FUNC_NAME(type)(type *_ptr) \ + { \ + (func)(_ptr); \ + } /** * VIR_AUTOFREE: @@ -637,6 +654,19 @@ void virAllocTestHook(void (*func)(int, void*), void *data); # define VIR_AUTOPTR(type) \ __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) type * +/** + * VIR_AUTOCLEAN: + * @type: type of the variable to be cleared + * + * Macro to automatically calls destructor of @type variable declared directly + * when the variable goes out of scope. + * The destructor is defined by VIR_DEFINE_AUTOCLEAN_FUNC macro for the given + * type. + */ +# define VIR_AUTOCLEAN(type) \ + __attribute__((cleanup(VIR_AUTOCLEAN_FUNC_NAME(type)))) type + + /** * VIR_AUTOUNREF: * @type: type of an virObject subclass to be unref'd automatically -- 2.20.1
On 2/21/19 9:50 AM, Peter Krempa wrote:
The new utility macros are useful for variables we put on the stack but require some cleanup. The most prominent of those is virBuffer which is used almost exclusively in that way.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/viralloc.h | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-)
+/** + * VIR_AUTOCLEAN: + * @type: type of the variable to be cleared
To match what VIR_AUTOUNREF does, should this state: to be clean'd automatically
+ * + * Macro to automatically calls destructor of @type variable declared directly
s/calls/call/ Is it actually a destructor, or just a cleanup function?
+ * when the variable goes out of scope. + * The destructor is defined by VIR_DEFINE_AUTOCLEAN_FUNC macro for the given + * type. + */ +# define VIR_AUTOCLEAN(type) \ + __attribute__((cleanup(VIR_AUTOCLEAN_FUNC_NAME(type)))) type + + /** * VIR_AUTOUNREF: * @type: type of an virObject subclass to be unref'd automatically
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Thu, Feb 21, 2019 at 11:48:56 -0600, Eric Blake wrote:
On 2/21/19 9:50 AM, Peter Krempa wrote:
The new utility macros are useful for variables we put on the stack but require some cleanup. The most prominent of those is virBuffer which is used almost exclusively in that way.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/viralloc.h | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-)
+/** + * VIR_AUTOCLEAN: + * @type: type of the variable to be cleared
To match what VIR_AUTOUNREF does, should this state: to be clean'd automatically
+ * + * Macro to automatically calls destructor of @type variable declared directly
s/calls/call/
I should read the whole sentence when modifying it :)
Is it actually a destructor, or just a cleanup function?
I got carried away slightly. It's just a cleanup function. I'll tweak the comments.
virBuffer is almost always stack-allocated, but requires freeing of the internals on error. Introduce a VIR_AUTOCLEAN function to deal with this. Along with the addition add a test which would leak the buffer contents if it weren't autocleaned. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virbuffer.h | 3 +++ tests/virbuftest.c | 12 ++++++++++++ 2 files changed, 15 insertions(+) diff --git a/src/util/virbuffer.h b/src/util/virbuffer.h index 7e4e7645df..b399c90154 100644 --- a/src/util/virbuffer.h +++ b/src/util/virbuffer.h @@ -59,6 +59,9 @@ int virBufferCheckErrorInternal(const virBuffer *buf, const char *funcname, size_t linenr) ATTRIBUTE_NONNULL(1); + +VIR_DEFINE_AUTOCLEAN_FUNC(virBuffer, virBufferFreeAndReset); + /** * virBufferCheckError * diff --git a/tests/virbuftest.c b/tests/virbuftest.c index bdb0a5e934..34f02b1281 100644 --- a/tests/virbuftest.c +++ b/tests/virbuftest.c @@ -429,6 +429,17 @@ testBufSetIndent(const void *opaque ATTRIBUTE_UNUSED) } +/* Result of this shows up only in valgrind or similar */ +static int +testBufferAutoclean(const void *opaque ATTRIBUTE_UNUSED) +{ + VIR_AUTOCLEAN(virBuffer) buf = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&buf, "test test test\n"); + return 0; +} + + static int mymain(void) { @@ -448,6 +459,7 @@ mymain(void) DO_TEST("Trim", testBufTrim, 0); DO_TEST("AddBuffer", testBufAddBuffer, 0); DO_TEST("set indent", testBufSetIndent, 0); + DO_TEST("autoclean", testBufferAutoclean, 0); #define DO_TEST_ADD_STR(DATA, EXPECT) \ do { \ -- 2.20.1
Replace all uses where virBuffer would need clearing on the cleanup path. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index bbd4a5efe8..8682b27037 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2174,7 +2174,7 @@ static int qemuStorageSourcePrivateDataFormat(virStorageSourcePtr src, virBufferPtr buf) { - virBuffer tmp = VIR_BUFFER_INITIALIZER; + VIR_AUTOCLEAN(virBuffer) tmp = VIR_BUFFER_INITIALIZER; qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); int ret = -1; @@ -2209,7 +2209,6 @@ qemuStorageSourcePrivateDataFormat(virStorageSourcePtr src, ret = 0; cleanup: - virBufferFreeAndReset(&tmp); return ret; } @@ -2345,9 +2344,9 @@ static int qemuDomainObjPrivateXMLFormatNBDMigrationSource(virBufferPtr buf, virStorageSourcePtr src) { - virBuffer attrBuf = VIR_BUFFER_INITIALIZER; - virBuffer childBuf = VIR_BUFFER_INITIALIZER; - virBuffer privateDataBuf = VIR_BUFFER_INITIALIZER; + VIR_AUTOCLEAN(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; + VIR_AUTOCLEAN(virBuffer) childBuf = VIR_BUFFER_INITIALIZER; + VIR_AUTOCLEAN(virBuffer) privateDataBuf = VIR_BUFFER_INITIALIZER; int ret = -1; virBufferSetChildIndent(&childBuf, buf); @@ -2373,10 +2372,6 @@ qemuDomainObjPrivateXMLFormatNBDMigrationSource(virBufferPtr buf, ret = 0; cleanup: - virBufferFreeAndReset(&attrBuf); - virBufferFreeAndReset(&childBuf); - virBufferFreeAndReset(&privateDataBuf); - return ret; } @@ -2385,8 +2380,8 @@ static int qemuDomainObjPrivateXMLFormatNBDMigration(virBufferPtr buf, virDomainObjPtr vm) { - virBuffer attrBuf = VIR_BUFFER_INITIALIZER; - virBuffer childBuf = VIR_BUFFER_INITIALIZER; + VIR_AUTOCLEAN(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; + VIR_AUTOCLEAN(virBuffer) childBuf = VIR_BUFFER_INITIALIZER; size_t i; virDomainDiskDefPtr disk; qemuDomainDiskPrivatePtr diskPriv; @@ -2413,9 +2408,6 @@ qemuDomainObjPrivateXMLFormatNBDMigration(virBufferPtr buf, ret = 0; cleanup: - virBufferFreeAndReset(&attrBuf); - virBufferFreeAndReset(&childBuf); - return ret; } @@ -2425,8 +2417,8 @@ qemuDomainObjPrivateXMLFormatJob(virBufferPtr buf, virDomainObjPtr vm, qemuDomainObjPrivatePtr priv) { - virBuffer attrBuf = VIR_BUFFER_INITIALIZER; - virBuffer childBuf = VIR_BUFFER_INITIALIZER; + VIR_AUTOCLEAN(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; + VIR_AUTOCLEAN(virBuffer) childBuf = VIR_BUFFER_INITIALIZER; qemuDomainJob job = priv->job.active; int ret = -1; @@ -2465,9 +2457,6 @@ qemuDomainObjPrivateXMLFormatJob(virBufferPtr buf, ret = 0; cleanup: - virBufferFreeAndReset(&attrBuf); - virBufferFreeAndReset(&childBuf); - return ret; } -- 2.20.1
On 2/21/19 9:50 AM, Peter Krempa wrote:
First 4 patches clean up existing mess.
Note that this series both fixes and then deletes virBufferEscapeN. The split is deliberate, so that if somebody needs virBufferEscapeN in the future they can revert the removal and get a working version.
Peter Krempa (7): util: buffer: Remove misleading AUTOPTR func for 'virBuffer' util: buf: Fix memory leak in virBufferEscapeN tests: buf: Fix debug messages in 'testBufEscapeRegex' util: buf: Remove virBufferEscapeN util: alloc: Introduce 'VIR_AUTOCLEAN' macros for clearing stack'd structs util: buffer: Introduce VIR_AUTOCLEAN function for virBuffer qemu: domain: Use VIR_AUTOCLEAN for virBuffer
Mail delays prevent me from replying directly to the messages that have not hit my inbox yet. I had some minor comments on 5, but trust you can address those. Series: Reviewed-by: Eric Blake <eblake@redhat.com> (And the longer I take to get my incremental backup stuff in, the more I have to rebase it to start using more of these cool macros...) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa