[libvirt] [PATCH v2 1/4] util: string: Introduce macro for automatic string lists

[John Ferlan]

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 at 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