4:48 p.m.
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
4:48 p.m.
New subject: [libvirt] [PATCH v2 1/4] util: string: Introduce macro for automatic string lists
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(a)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
10:52 a.m.
New subject: [libvirt] [PATCH v2 1/4] util: string: Introduce macro for automatic string lists
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(a)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(a)redhat.com>
Unless someone has any objections to this approach but I find it way cleaner
than v1 :)
1:32 p.m.
New subject: [libvirt] [PATCH v2 1/4] util: string: Introduce macro for automatic string lists
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(a)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
2:43 p.m.
New subject: [libvirt] [PATCH v2 1/4] util: string: Introduce macro for automatic string lists
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(a)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.
3:38 p.m.
New subject: [libvirt] [PATCH v2 1/4] util: string: Introduce macro for automatic string lists
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(a)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.
2:39 p.m.
New subject: [libvirt] [PATCH v2 1/4] util: string: Introduce macro for automatic string lists
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(a)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; ?
2:55 p.m.
New subject: [libvirt] [PATCH v2 1/4] util: string: Introduce macro for automatic string lists
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(a)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
4:48 p.m.
New subject: [libvirt] [PATCH v2 2/4] util: string: Use VIR_AUTOSTRINGLIST instead of VIR_AUTOPTR(virString)
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(a)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
12:40 p.m.
New subject: [libvirt] [PATCH v2 2/4] util: string: Use VIR_AUTOSTRINGLIST instead of VIR_AUTOPTR(virString)
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(a)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(a)redhat.com>
4:48 p.m.
New subject: [libvirt] [PATCH v2 3/4] util: string: Remove the 'virString' type
We don't need it as there's a separate macro for auto-freeing of string
lists.
Signed-off-by: Peter Krempa <pkrempa(a)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
12:44 p.m.
New subject: [libvirt] [PATCH v2 3/4] util: string: Remove the 'virString' type
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(a)redhat.com>
---
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
4:48 p.m.
New subject: [libvirt] [PATCH v2 4/4] util: alloc: Note that VIR_AUTOPTR/VIR_AUTOCLEAN must not be used with vectors
We'd free only the first element of the vector leaking the rest.
Signed-off-by: Peter Krempa <pkrempa(a)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
3:05 p.m.
New subject: [libvirt] [PATCH v2 4/4] util: alloc: Note that VIR_AUTOPTR/VIR_AUTOCLEAN must not be used with vectors
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(a)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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
2:56 p.m.
New subject: [libvirt] [PATCH v2 4/4] util: alloc: Note that VIR_AUTOPTR/VIR_AUTOCLEAN must not be used with vectors
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(a)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.
3:44 p.m.
New subject: [libvirt] [PATCH v2 4/4] util: alloc: Note that VIR_AUTOPTR/VIR_AUTOCLEAN must not be used with vectors
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(a)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
2030
days inactive
2032
days old
15 comments
4 participants
participants (4)
-
Erik Skultety -
John Ferlan -
Michal Privoznik -
Peter Krempa