[libvirt] [PATCH v2 0/4] Get rid of VIR_AUTOPTR(virString)

Peter Krempa (4): util: string: Introduce macro for automatic string lists util: string: Use VIR_AUTOSTRINGLIST instead of VIR_AUTOPTR(virString) util: string: Remove the 'virString' type util: alloc: Note that VIR_AUTOPTR/VIR_AUTOCLEAN must not be used with vectors src/libvirt_private.syms | 1 + src/lxc/lxc_process.c | 2 +- src/qemu/qemu_conf.c | 8 ++++---- src/storage/storage_backend_sheepdog.c | 4 ++-- src/storage/storage_backend_zfs.c | 10 +++++----- src/util/viralloc.h | 6 ++++++ src/util/vircommand.c | 2 +- src/util/virfirewall.c | 2 +- src/util/virprocess.c | 2 +- src/util/virstoragefile.c | 10 +++++----- src/util/virstring.c | 10 ++++++++++ src/util/virstring.h | 12 +++++++++--- src/xenconfig/xen_common.c | 14 +++++++------- 13 files changed, 53 insertions(+), 30 deletions(-) -- 2.20.1

Similar to VIR_AUTOPTR, VIR_AUTOSTRINGLIST defines a list of strings which will be freed if the pointer is leaving scope. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstring.c | 10 ++++++++++ src/util/virstring.h | 10 ++++++++++ 3 files changed, 21 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 038a744981..e68e3f3a3b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2958,6 +2958,7 @@ virStringHasControlChars; virStringIsEmpty; virStringIsPrintable; virStringListAdd; +virStringListAutoFree; virStringListFree; virStringListFreeCount; virStringListGetFirstWithPrefix; diff --git a/src/util/virstring.c b/src/util/virstring.c index e890dde546..8a791f96d4 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -318,6 +318,16 @@ void virStringListFree(char **strings) } +void virStringListAutoFree(char ***strings) +{ + if (!*strings) + return; + + virStringListFree(*strings); + *strings = NULL; +} + + /** * virStringListFreeCount: * @strings: array of strings to free diff --git a/src/util/virstring.h b/src/util/virstring.h index aef82471c2..d14b7f4f49 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -53,6 +53,7 @@ int virStringListCopy(char ***dst, const char **src); void virStringListFree(char **strings); +void virStringListAutoFree(char ***strings); void virStringListFreeCount(char **strings, size_t count); @@ -307,6 +308,15 @@ int virStringParsePort(const char *str, unsigned int *port) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +/** + * VIR_AUTOSTRINGLIST: + * + * Declares a NULL-terminated list of strings which will be automatically freed + * when the pointer goes out of scope. + */ +# define VIR_AUTOSTRINGLIST \ + __attribute__((cleanup(virStringListAutoFree))) char ** + VIR_DEFINE_AUTOPTR_FUNC(virString, virStringListFree); #endif /* LIBVIRT_VIRSTRING_H */ -- 2.20.1

On Tue, Feb 26, 2019 at 04:48:23PM +0100, Peter Krempa wrote:
Similar to VIR_AUTOPTR, VIR_AUTOSTRINGLIST defines a list of strings which will be freed if the pointer is leaving scope.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstring.c | 10 ++++++++++ src/util/virstring.h | 10 ++++++++++ 3 files changed, 21 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 038a744981..e68e3f3a3b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2958,6 +2958,7 @@ virStringHasControlChars; virStringIsEmpty; virStringIsPrintable; virStringListAdd; +virStringListAutoFree; virStringListFree; virStringListFreeCount; virStringListGetFirstWithPrefix; diff --git a/src/util/virstring.c b/src/util/virstring.c index e890dde546..8a791f96d4 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -318,6 +318,16 @@ void virStringListFree(char **strings) }
+void virStringListAutoFree(char ***strings) +{ + if (!*strings) + return; + + virStringListFree(*strings); + *strings = NULL; +} + + /** * virStringListFreeCount: * @strings: array of strings to free diff --git a/src/util/virstring.h b/src/util/virstring.h index aef82471c2..d14b7f4f49 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -53,6 +53,7 @@ int virStringListCopy(char ***dst, const char **src);
void virStringListFree(char **strings); +void virStringListAutoFree(char ***strings); void virStringListFreeCount(char **strings, size_t count);
@@ -307,6 +308,15 @@ int virStringParsePort(const char *str, unsigned int *port) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+/** + * VIR_AUTOSTRINGLIST: + * + * Declares a NULL-terminated list of strings which will be automatically freed + * when the pointer goes out of scope. + */ +# define VIR_AUTOSTRINGLIST \ + __attribute__((cleanup(virStringListAutoFree))) char ** +
IIRC at the beginning we said that all the VIR_AUTO macros should be consistent in terms of how we use the macro, IOW: VIR_AUTOFREE(type) VIR_AUTOPTR(type) VIR_AUTOCLEAN(type) although I can't say I personally don't like your version, nevertheless, as I said we should remain consistent and use 'char **' explicitly when using the macro. Reviewed-by: Erik Skultety <eskultet@redhat.com> Unless someone has any objections to this approach but I find it way cleaner than v1 :)

On 2/27/19 10:52 AM, Erik Skultety wrote:
On Tue, Feb 26, 2019 at 04:48:23PM +0100, Peter Krempa wrote:
Similar to VIR_AUTOPTR, VIR_AUTOSTRINGLIST defines a list of strings which will be freed if the pointer is leaving scope.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstring.c | 10 ++++++++++ src/util/virstring.h | 10 ++++++++++ 3 files changed, 21 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 038a744981..e68e3f3a3b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2958,6 +2958,7 @@ virStringHasControlChars; virStringIsEmpty; virStringIsPrintable; virStringListAdd; +virStringListAutoFree; virStringListFree; virStringListFreeCount; virStringListGetFirstWithPrefix; diff --git a/src/util/virstring.c b/src/util/virstring.c index e890dde546..8a791f96d4 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -318,6 +318,16 @@ void virStringListFree(char **strings) }
+void virStringListAutoFree(char ***strings) +{ + if (!*strings) + return; + + virStringListFree(*strings); + *strings = NULL; +} + + /** * virStringListFreeCount: * @strings: array of strings to free diff --git a/src/util/virstring.h b/src/util/virstring.h index aef82471c2..d14b7f4f49 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -53,6 +53,7 @@ int virStringListCopy(char ***dst, const char **src);
void virStringListFree(char **strings); +void virStringListAutoFree(char ***strings); void virStringListFreeCount(char **strings, size_t count);
@@ -307,6 +308,15 @@ int virStringParsePort(const char *str, unsigned int *port) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+/** + * VIR_AUTOSTRINGLIST: + * + * Declares a NULL-terminated list of strings which will be automatically freed + * when the pointer goes out of scope. + */ +# define VIR_AUTOSTRINGLIST \ + __attribute__((cleanup(virStringListAutoFree))) char ** +
IIRC at the beginning we said that all the VIR_AUTO macros should be consistent in terms of how we use the macro, IOW:
VIR_AUTOFREE(type) VIR_AUTOPTR(type) VIR_AUTOCLEAN(type)
Well, we already have VIR_AUTOCLOSE. I don't mind having a macro that implicitly defines type of a variable. On the other hand, this could be renamed to VIR_AUTOLISTFREE as it is capable of freeing any NULL terminated list of pointers where plain free() ove each member is enough. But it will have to lose the implicit type def in that case. Michal

On Wed, Feb 27, 2019 at 13:32:43 +0100, Michal Privoznik wrote:
On 2/27/19 10:52 AM, Erik Skultety wrote:
On Tue, Feb 26, 2019 at 04:48:23PM +0100, Peter Krempa wrote:
Similar to VIR_AUTOPTR, VIR_AUTOSTRINGLIST defines a list of strings which will be freed if the pointer is leaving scope.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstring.c | 10 ++++++++++ src/util/virstring.h | 10 ++++++++++ 3 files changed, 21 insertions(+)
[...]
+ */ +# define VIR_AUTOSTRINGLIST \ + __attribute__((cleanup(virStringListAutoFree))) char ** +
IIRC at the beginning we said that all the VIR_AUTO macros should be consistent in terms of how we use the macro, IOW:
VIR_AUTOFREE(type) VIR_AUTOPTR(type) VIR_AUTOCLEAN(type)
Well, we already have VIR_AUTOCLOSE. I don't mind having a macro that implicitly defines type of a variable.
It makes sense especially if the freeing function uses a specified type and thus you have to use it anyways. It actually prevents bugs.
On the other hand, this could be renamed to VIR_AUTOLISTFREE as it is capable of freeing any NULL terminated list of pointers where plain free() ove each member is enough. But it will have to lose the implicit type def in that case.
I proposed it in first version with a slightly more complex backed freeing function. It's quite useless with plain free() and I think it would invite to more bugs.

On Wed, Feb 27, 2019 at 01:32:43PM +0100, Michal Privoznik wrote:
On 2/27/19 10:52 AM, Erik Skultety wrote:
On Tue, Feb 26, 2019 at 04:48:23PM +0100, Peter Krempa wrote:
Similar to VIR_AUTOPTR, VIR_AUTOSTRINGLIST defines a list of strings which will be freed if the pointer is leaving scope.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstring.c | 10 ++++++++++ src/util/virstring.h | 10 ++++++++++ 3 files changed, 21 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 038a744981..e68e3f3a3b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2958,6 +2958,7 @@ virStringHasControlChars; virStringIsEmpty; virStringIsPrintable; virStringListAdd; +virStringListAutoFree; virStringListFree; virStringListFreeCount; virStringListGetFirstWithPrefix; diff --git a/src/util/virstring.c b/src/util/virstring.c index e890dde546..8a791f96d4 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -318,6 +318,16 @@ void virStringListFree(char **strings) }
+void virStringListAutoFree(char ***strings) +{ + if (!*strings) + return; + + virStringListFree(*strings); + *strings = NULL; +} + + /** * virStringListFreeCount: * @strings: array of strings to free diff --git a/src/util/virstring.h b/src/util/virstring.h index aef82471c2..d14b7f4f49 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -53,6 +53,7 @@ int virStringListCopy(char ***dst, const char **src);
void virStringListFree(char **strings); +void virStringListAutoFree(char ***strings); void virStringListFreeCount(char **strings, size_t count);
@@ -307,6 +308,15 @@ int virStringParsePort(const char *str, unsigned int *port) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+/** + * VIR_AUTOSTRINGLIST: + * + * Declares a NULL-terminated list of strings which will be automatically freed + * when the pointer goes out of scope. + */ +# define VIR_AUTOSTRINGLIST \ + __attribute__((cleanup(virStringListAutoFree))) char ** +
IIRC at the beginning we said that all the VIR_AUTO macros should be consistent in terms of how we use the macro, IOW:
VIR_AUTOFREE(type) VIR_AUTOPTR(type) VIR_AUTOCLEAN(type)
Well, we already have VIR_AUTOCLOSE. I don't mind having a macro that implicitly defines type of a variable.
Oh, I completely forgot about VIR_AUTOCLOSE which supports Peter's argument about the case where the freeing function is tied to the macro regardless of type.

On Wed, Feb 27, 2019 at 10:52:47 +0100, Erik Skultety wrote:
On Tue, Feb 26, 2019 at 04:48:23PM +0100, Peter Krempa wrote:
Similar to VIR_AUTOPTR, VIR_AUTOSTRINGLIST defines a list of strings which will be freed if the pointer is leaving scope.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstring.c | 10 ++++++++++ src/util/virstring.h | 10 ++++++++++ 3 files changed, 21 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 038a744981..e68e3f3a3b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2958,6 +2958,7 @@ virStringHasControlChars; virStringIsEmpty; virStringIsPrintable; virStringListAdd; +virStringListAutoFree; virStringListFree; virStringListFreeCount; virStringListGetFirstWithPrefix; diff --git a/src/util/virstring.c b/src/util/virstring.c index e890dde546..8a791f96d4 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -318,6 +318,16 @@ void virStringListFree(char **strings) }
+void virStringListAutoFree(char ***strings) +{ + if (!*strings) + return; + + virStringListFree(*strings); + *strings = NULL; +} + + /** * virStringListFreeCount: * @strings: array of strings to free diff --git a/src/util/virstring.h b/src/util/virstring.h index aef82471c2..d14b7f4f49 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -53,6 +53,7 @@ int virStringListCopy(char ***dst, const char **src);
void virStringListFree(char **strings); +void virStringListAutoFree(char ***strings); void virStringListFreeCount(char **strings, size_t count);
@@ -307,6 +308,15 @@ int virStringParsePort(const char *str, unsigned int *port) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+/** + * VIR_AUTOSTRINGLIST: + * + * Declares a NULL-terminated list of strings which will be automatically freed + * when the pointer goes out of scope. + */ +# define VIR_AUTOSTRINGLIST \ + __attribute__((cleanup(virStringListAutoFree))) char ** +
IIRC at the beginning we said that all the VIR_AUTO macros should be consistent in terms of how we use the macro, IOW:
VIR_AUTOFREE(type) VIR_AUTOPTR(type) VIR_AUTOCLEAN(type)
although I can't say I personally don't like your version, nevertheless, as I said we should remain consistent and use 'char **' explicitly when using the macro.
I'm not sure I understood. Did you mean that it should be used as: VIR_AUTOSTRINGLIST char **list = NULL; ?

On 2/28/19 8:39 AM, Peter Krempa wrote:
On Wed, Feb 27, 2019 at 10:52:47 +0100, Erik Skultety wrote:
On Tue, Feb 26, 2019 at 04:48:23PM +0100, Peter Krempa wrote:
Similar to VIR_AUTOPTR, VIR_AUTOSTRINGLIST defines a list of strings which will be freed if the pointer is leaving scope.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstring.c | 10 ++++++++++ src/util/virstring.h | 10 ++++++++++ 3 files changed, 21 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 038a744981..e68e3f3a3b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2958,6 +2958,7 @@ virStringHasControlChars; virStringIsEmpty; virStringIsPrintable; virStringListAdd; +virStringListAutoFree; virStringListFree; virStringListFreeCount; virStringListGetFirstWithPrefix; diff --git a/src/util/virstring.c b/src/util/virstring.c index e890dde546..8a791f96d4 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -318,6 +318,16 @@ void virStringListFree(char **strings) }
+void virStringListAutoFree(char ***strings) +{ + if (!*strings) + return; + + virStringListFree(*strings); + *strings = NULL; +} + + /** * virStringListFreeCount: * @strings: array of strings to free diff --git a/src/util/virstring.h b/src/util/virstring.h index aef82471c2..d14b7f4f49 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -53,6 +53,7 @@ int virStringListCopy(char ***dst, const char **src);
void virStringListFree(char **strings); +void virStringListAutoFree(char ***strings); void virStringListFreeCount(char **strings, size_t count);
@@ -307,6 +308,15 @@ int virStringParsePort(const char *str, unsigned int *port) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+/** + * VIR_AUTOSTRINGLIST: + * + * Declares a NULL-terminated list of strings which will be automatically freed + * when the pointer goes out of scope. + */ +# define VIR_AUTOSTRINGLIST \ + __attribute__((cleanup(virStringListAutoFree))) char ** +
IIRC at the beginning we said that all the VIR_AUTO macros should be consistent in terms of how we use the macro, IOW:
VIR_AUTOFREE(type) VIR_AUTOPTR(type) VIR_AUTOCLEAN(type)
although I can't say I personally don't like your version, nevertheless, as I said we should remain consistent and use 'char **' explicitly when using the macro.
I'm not sure I understood. Did you mean that it should be used as:
VIR_AUTOSTRINGLIST char **list = NULL; ?
I have this vague recollection of lengthy discussions over how to handle "char **" when these changes were being made originally. Too lazy to go look that up though. I do think VIR_AUTOSTRINGLIST is ok - perhaps shorter to type and easier to read than VIR_AUTOCHARARRAYLIST. Not sure I like VIR_AUTOLISTFREE because that to me says more like a list of "anything" (not just strings). JMO, John

Use of VIR_AUTOPTR and virString is confusing as it's a list and not a single pointer. Replace it by VIR_AUTOSTRINGLIST as string lists are basically the only sane NULL-terminated list we can have. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/lxc/lxc_process.c | 2 +- src/qemu/qemu_conf.c | 8 ++++---- src/storage/storage_backend_sheepdog.c | 4 ++-- src/storage/storage_backend_zfs.c | 10 +++++----- src/util/vircommand.c | 2 +- src/util/virfirewall.c | 2 +- src/util/virprocess.c | 2 +- src/util/virstoragefile.c | 10 +++++----- src/xenconfig/xen_common.c | 6 +++--- 9 files changed, 23 insertions(+), 23 deletions(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index a3481bfa08..e0729a24bf 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1181,7 +1181,7 @@ int virLXCProcessStart(virConnectPtr conn, size_t i; char *logfile = NULL; int logfd = -1; - VIR_AUTOPTR(virString) veths = NULL; + VIR_AUTOSTRINGLIST veths = NULL; int handshakefds[2] = { -1, -1 }; off_t pos = -1; char ebuf[1024]; diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 2f5ef8d0c4..42122dcd97 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -645,7 +645,7 @@ static int virQEMUDriverConfigLoadProcessEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf) { - VIR_AUTOPTR(virString) hugetlbfs = NULL; + VIR_AUTOSTRINGLIST hugetlbfs = NULL; VIR_AUTOFREE(char *) stdioHandler = NULL; VIR_AUTOFREE(char *) corestr = NULL; size_t i; @@ -832,7 +832,7 @@ static int virQEMUDriverConfigLoadNVRAMEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf) { - VIR_AUTOPTR(virString) nvram = NULL; + VIR_AUTOSTRINGLIST nvram = NULL; size_t i; if (virConfGetValueStringList(conf, "nvram", false, &nvram) < 0) @@ -869,8 +869,8 @@ virQEMUDriverConfigLoadSecurityEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf, bool privileged) { - VIR_AUTOPTR(virString) controllers = NULL; - VIR_AUTOPTR(virString) namespaces = NULL; + VIR_AUTOSTRINGLIST controllers = NULL; + VIR_AUTOSTRINGLIST namespaces = NULL; VIR_AUTOFREE(char *) user = NULL; VIR_AUTOFREE(char *) group = NULL; size_t i, j; diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c index 99f3283a1c..6df90937c2 100644 --- a/src/storage/storage_backend_sheepdog.c +++ b/src/storage/storage_backend_sheepdog.c @@ -138,8 +138,8 @@ virStorageBackendSheepdogRefreshAllVol(virStoragePoolObjPtr pool) { size_t i; VIR_AUTOFREE(char *) output = NULL; - VIR_AUTOPTR(virString) lines = NULL; - VIR_AUTOPTR(virString) cells = NULL; + VIR_AUTOSTRINGLIST lines = NULL; + VIR_AUTOSTRINGLIST cells = NULL; VIR_AUTOPTR(virCommand) cmd = NULL; cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "list", "-r", NULL); diff --git a/src/storage/storage_backend_zfs.c b/src/storage/storage_backend_zfs.c index 7ffdff638e..826a95538e 100644 --- a/src/storage/storage_backend_zfs.c +++ b/src/storage/storage_backend_zfs.c @@ -106,8 +106,8 @@ virStorageBackendZFSParseVol(virStoragePoolObjPtr pool, bool is_new_vol = false; virStorageVolDefPtr volume = NULL; virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - VIR_AUTOPTR(virString) tokens = NULL; - VIR_AUTOPTR(virString) name_tokens = NULL; + VIR_AUTOSTRINGLIST tokens = NULL; + VIR_AUTOSTRINGLIST name_tokens = NULL; if (!(tokens = virStringSplitCount(volume_string, "\t", 0, &count))) return -1; @@ -177,7 +177,7 @@ virStorageBackendZFSFindVols(virStoragePoolObjPtr pool, { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); size_t i; - VIR_AUTOPTR(virString) lines = NULL; + VIR_AUTOSTRINGLIST lines = NULL; VIR_AUTOPTR(virCommand) cmd = NULL; VIR_AUTOFREE(char *) volumes_list = NULL; @@ -224,8 +224,8 @@ virStorageBackendZFSRefreshPool(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED) char *zpool_props = NULL; size_t i; VIR_AUTOPTR(virCommand) cmd = NULL; - VIR_AUTOPTR(virString) lines = NULL; - VIR_AUTOPTR(virString) tokens = NULL; + VIR_AUTOSTRINGLIST lines = NULL; + VIR_AUTOSTRINGLIST tokens = NULL; /** * $ zpool get -Hp health,size,free,allocated test diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 3d533c68a6..84a65a2f6d 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -2983,7 +2983,7 @@ virCommandRunRegex(virCommandPtr cmd, int totgroups = 0, ngroup = 0, maxvars = 0; char **groups; VIR_AUTOFREE(char *) outbuf = NULL; - VIR_AUTOPTR(virString) lines = NULL; + VIR_AUTOSTRINGLIST lines = NULL; int ret = -1; /* Compile all regular expressions */ diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index d63ce05ed8..d42c734ea6 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -719,7 +719,7 @@ virFirewallApplyRule(virFirewallPtr firewall, { VIR_AUTOFREE(char *) output = NULL; VIR_AUTOFREE(char *) str = virFirewallRuleToString(rule); - VIR_AUTOPTR(virString) lines = NULL; + VIR_AUTOSTRINGLIST lines = NULL; VIR_INFO("Applying rule '%s'", NULLSTR(str)); if (rule->ignoreErrors) diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 4e69228f34..52b86c549d 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -987,7 +987,7 @@ int virProcessGetStartTime(pid_t pid, int len; VIR_AUTOFREE(char *) filename = NULL; VIR_AUTOFREE(char *) buf = NULL; - VIR_AUTOPTR(virString) tokens = NULL; + VIR_AUTOSTRINGLIST tokens = NULL; if (virAsprintf(&filename, "/proc/%llu/stat", (long long) pid) < 0) return -1; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index b2e308d81d..f361f96203 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1571,7 +1571,7 @@ virStorageFileParseBackingStoreStr(const char *str, size_t nstrings; unsigned int idx = 0; char *suffix; - VIR_AUTOPTR(virString) strings = NULL; + VIR_AUTOSTRINGLIST strings = NULL; *chainIndex = 0; @@ -2661,7 +2661,7 @@ virStorageSourceParseBackingURI(virStorageSourcePtr src, virURIPtr uri = NULL; const char *path = NULL; int ret = -1; - VIR_AUTOPTR(virString) scheme = NULL; + VIR_AUTOSTRINGLIST scheme = NULL; if (!(uri = virURIParse(uristr))) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2765,7 +2765,7 @@ virStorageSourceRBDAddHost(virStorageSourcePtr src, { char *port; size_t skip; - VIR_AUTOPTR(virString) parts = NULL; + VIR_AUTOSTRINGLIST parts = NULL; if (VIR_EXPAND_N(src->hosts, src->nhosts, 1) < 0) return -1; @@ -2921,7 +2921,7 @@ static int virStorageSourceParseNBDColonString(const char *nbdstr, virStorageSourcePtr src) { - VIR_AUTOPTR(virString) backing = NULL; + VIR_AUTOSTRINGLIST backing = NULL; if (!(backing = virStringSplit(nbdstr, ":", 0))) return -1; @@ -4208,7 +4208,7 @@ int virStorageFileCheckCompat(const char *compat) { unsigned int result; - VIR_AUTOPTR(virString) version = NULL; + VIR_AUTOSTRINGLIST version = NULL; if (!compat) return 0; diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index 94e0703cf3..2c8179f19c 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -473,7 +473,7 @@ xenHandleConfGetValueStringListErrors(int ret) static int xenParsePCIList(virConfPtr conf, virDomainDefPtr def) { - VIR_AUTOPTR(virString) pcis = NULL; + VIR_AUTOSTRINGLIST pcis = NULL; virString *entries = NULL; int rc; @@ -666,7 +666,7 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def) } if (!hvm && def->graphics == NULL) { /* New PV guests use this format */ - VIR_AUTOPTR(virString) vfbs = NULL; + VIR_AUTOSTRINGLIST vfbs = NULL; int rc; if ((rc = virConfGetValueStringList(conf, "vfb", false, &vfbs)) == 1) { @@ -764,7 +764,7 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def) static int xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) { - VIR_AUTOPTR(virString) serials = NULL; + VIR_AUTOSTRINGLIST serials = NULL; virDomainChrDefPtr chr = NULL; if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { -- 2.20.1

On Tue, Feb 26, 2019 at 04:48:24PM +0100, Peter Krempa wrote:
Use of VIR_AUTOPTR and virString is confusing as it's a list and not a single pointer. Replace it by VIR_AUTOSTRINGLIST as string lists are basically the only sane NULL-terminated list we can have.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/lxc/lxc_process.c | 2 +- src/qemu/qemu_conf.c | 8 ++++---- src/storage/storage_backend_sheepdog.c | 4 ++-- src/storage/storage_backend_zfs.c | 10 +++++----- src/util/vircommand.c | 2 +- src/util/virfirewall.c | 2 +- src/util/virprocess.c | 2 +- src/util/virstoragefile.c | 10 +++++----- src/xenconfig/xen_common.c | 6 +++--- 9 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index a3481bfa08..e0729a24bf 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1181,7 +1181,7 @@ int virLXCProcessStart(virConnectPtr conn, size_t i; char *logfile = NULL; int logfd = -1; - VIR_AUTOPTR(virString) veths = NULL; + VIR_AUTOSTRINGLIST veths = NULL;
Since I'm responsible (as a reviewer) for breaking the VIR_AUTO consistency in terms of where to declare the variables (not that it would matter in any way), I'm not going to argue about that anymore. With the adjustments coming from the previous patch: Reviewed-by: Erik Skultety <eskultet@redhat.com>

We don't need it as there's a separate macro for auto-freeing of string lists. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstring.h | 4 ---- src/xenconfig/xen_common.c | 8 ++++---- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/util/virstring.h b/src/util/virstring.h index d14b7f4f49..f2e72936c8 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -24,8 +24,6 @@ # include "internal.h" # include "viralloc.h" -typedef char *virString; - char **virStringSplitCount(const char *string, const char *delim, size_t max_tokens, @@ -317,6 +315,4 @@ int virStringParsePort(const char *str, # define VIR_AUTOSTRINGLIST \ __attribute__((cleanup(virStringListAutoFree))) char ** -VIR_DEFINE_AUTOPTR_FUNC(virString, virStringListFree); - #endif /* LIBVIRT_VIRSTRING_H */ diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index 2c8179f19c..21c56edd58 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -474,14 +474,14 @@ static int xenParsePCIList(virConfPtr conf, virDomainDefPtr def) { VIR_AUTOSTRINGLIST pcis = NULL; - virString *entries = NULL; + char **entries = NULL; int rc; if ((rc = virConfGetValueStringList(conf, "pci", false, &pcis)) <= 0) return xenHandleConfGetValueStringListErrors(rc); for (entries = pcis; *entries; entries++) { - virString entry = *entries; + char *entry = *entries; virDomainHostdevDefPtr hostdev; if (!(hostdev = xenParsePCI(entry))) @@ -789,7 +789,7 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) /* Try to get the list of values to support multiple serial ports */ if ((rc = virConfGetValueStringList(conf, "serial", false, &serials)) == 1) { - virString *entries; + char **entries; int portnum = -1; if (STREQ(nativeFormat, XEN_CONFIG_FORMAT_XM)) { @@ -799,7 +799,7 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) } for (entries = serials; *entries; entries++) { - virString port = *entries; + char *port = *entries; portnum++; if (STREQ(port, "none")) -- 2.20.1

On Tue, Feb 26, 2019 at 04:48:25PM +0100, Peter Krempa wrote:
We don't need it as there's a separate macro for auto-freeing of string lists.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>

We'd free only the first element of the vector leaking the rest. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/viralloc.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 15451d4673..572b7d1c1c 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -650,6 +650,9 @@ void virAllocTestHook(void (*func)(int, void*), void *data); * the variable declared with it by calling the function * defined by VIR_DEFINE_AUTOPTR_FUNC when the variable * goes out of scope. + * + * Note that this macro must NOT be used with vectors! The cleaning function + * will not free any elements beyond the first. */ # define VIR_AUTOPTR(type) \ __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) type * @@ -662,6 +665,9 @@ void virAllocTestHook(void (*func)(int, void*), void *data); * when the variable goes out of scope. * The cleanup function is registered by VIR_DEFINE_AUTOCLEAN_FUNC macro for * the given type. + * + * Note that this macro must NOT be used with vectors! The cleaning function + * will not free any elements beyond the first. */ # define VIR_AUTOCLEAN(type) \ __attribute__((cleanup(VIR_AUTOCLEAN_FUNC_NAME(type)))) type -- 2.20.1

On Tue, Feb 26, 2019 at 04:48:26PM +0100, Peter Krempa wrote:
We'd free only the first element of the vector leaking the rest.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/viralloc.h | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 15451d4673..572b7d1c1c 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -650,6 +650,9 @@ void virAllocTestHook(void (*func)(int, void*), void *data); * the variable declared with it by calling the function * defined by VIR_DEFINE_AUTOPTR_FUNC when the variable * goes out of scope. + * + * Note that this macro must NOT be used with vectors! The cleaning function + * will not free any elements beyond the first.
s/cleaning/freeing/ I understand, but if you have happen to have a dedicated list type, then you'd have a dedicated destructor, so both of these would be okay with vectors. On the other hand I'm not sure whether we have such a thing at the moment, so I guess it's meaningful to document in the meantime. Reviewed-by: Erik Skultety <eskultet@redhat.com>
*/ # define VIR_AUTOPTR(type) \ __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) type * @@ -662,6 +665,9 @@ void virAllocTestHook(void (*func)(int, void*), void *data); * when the variable goes out of scope. * The cleanup function is registered by VIR_DEFINE_AUTOCLEAN_FUNC macro for * the given type. + * + * Note that this macro must NOT be used with vectors! The cleaning function + * will not free any elements beyond the first. */ # define VIR_AUTOCLEAN(type) \ __attribute__((cleanup(VIR_AUTOCLEAN_FUNC_NAME(type)))) type -- 2.20.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, Feb 27, 2019 at 15:05:18 +0100, Erik Skultety wrote:
On Tue, Feb 26, 2019 at 04:48:26PM +0100, Peter Krempa wrote:
We'd free only the first element of the vector leaking the rest.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/viralloc.h | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 15451d4673..572b7d1c1c 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -650,6 +650,9 @@ void virAllocTestHook(void (*func)(int, void*), void *data); * the variable declared with it by calling the function * defined by VIR_DEFINE_AUTOPTR_FUNC when the variable * goes out of scope. + * + * Note that this macro must NOT be used with vectors! The cleaning function + * will not free any elements beyond the first.
s/cleaning/freeing/
I understand, but if you have happen to have a dedicated list type, then you'd have a dedicated destructor, so both of these would be okay with vectors. On
Note that the function registered via __attribute(cleanup ... gets only the pointer to the stack'd variable as an argument. This means that you can do only 'value-terminated' (NULL, -1, ... ) lists. Anything requiring count of elements will need to be encapsulated in a struct which makes it a container. Thus the comment does not apply.
the other hand I'm not sure whether we have such a thing at the moment, so I guess it's meaningful to document in the meantime.
I don't think we'll get much value terminated lists because the usage is quite cumbersome and error-prone (e.g. if you forget your terminator). In such case we can always do a rather simple macro for them.

On Thu, Feb 28, 2019 at 02:56:36PM +0100, Peter Krempa wrote:
On Wed, Feb 27, 2019 at 15:05:18 +0100, Erik Skultety wrote:
On Tue, Feb 26, 2019 at 04:48:26PM +0100, Peter Krempa wrote:
We'd free only the first element of the vector leaking the rest.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/viralloc.h | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 15451d4673..572b7d1c1c 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -650,6 +650,9 @@ void virAllocTestHook(void (*func)(int, void*), void *data); * the variable declared with it by calling the function * defined by VIR_DEFINE_AUTOPTR_FUNC when the variable * goes out of scope. + * + * Note that this macro must NOT be used with vectors! The cleaning function + * will not free any elements beyond the first.
s/cleaning/freeing/
I understand, but if you have happen to have a dedicated list type, then you'd have a dedicated destructor, so both of these would be okay with vectors. On
Note that the function registered via __attribute(cleanup ... gets only the pointer to the stack'd variable as an argument. This means that you can do only 'value-terminated' (NULL, -1, ... ) lists.
Anything requiring count of elements will need to be encapsulated in a struct which makes it a container. Thus the comment does not apply.
Yeah, true. Erik
participants (4)
-
Erik Skultety
-
John Ferlan
-
Michal Privoznik
-
Peter Krempa