Re: [libvirt] [PATCH 01/23] util: Introduce virStringListCopy
On 10/13/2017 09:27 AM, Jiri Denemark wrote:
On Thu, Oct 12, 2017 at 07:22:51 -0400, John Ferlan wrote:
>
>
> On 10/04/2017 10:58 AM, Jiri Denemark wrote:
>> The API makes a deep copy of a NULL-terminated string list.
>>
>> Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
>> ---
>> src/util/virstring.c | 37 +++++++++++++++++++++++++++++++++++++
>> src/util/virstring.h | 3 +++
>> 2 files changed, 40 insertions(+)
>>
>> diff --git a/src/util/virstring.c b/src/util/virstring.c
>> index 0288d1e677..820b282ac5 100644
>> --- a/src/util/virstring.c
>> +++ b/src/util/virstring.c
>> @@ -239,6 +239,43 @@ virStringListRemove(char ***strings,
>> }
>>
>>
>> +/**
>> + * virStringListCopy:
>> + * @dst: where to store the copy of @strings
>> + * @src: a NULL-terminated array of strings
>> + *
>> + * Makes a deep copy of the @src string list and stores it in @dst. Callers
>> + * are responsible for freeing both @dst and @src.
>> + *
>> + * Returns 0 on success, -1 on error.
>> + */
>> +int
>> +virStringListCopy(char ***dst,
>> + const char **src)
>> +{
>
> I think it would make more sense to have this return @copy (or call it
> @dst, doesn't matter) rather than 0, -1 which only means @dst wasn't
> populated. There's only 1 consumer (in patch 2)...
Returning the pointer rather than int makes it impossible to allow NULL
input since returning NULL would mean something failed. This is similar
to VIR_STRDUP and several others.
Jirka
However, if !src, then you're returning 0 and @dst is not changed and
the caller *still* needs to check it. While this works for what you have
there's also other examples where callers will do:
if (blockers && !blockersCopy = virStringListCopy(blockers))
goto error;
What you have works for your implementation mostly because
virDomainCapsCPUModelsAddSteal allows/checks for a NULL 4th parameter
and you initialize blockersCopy = NULL. But future eventual other
callers would have to check the returned value for 0 and the returned
@dst parameter anyway before using it. Because your check is hidden
deeper in another call someone mimicing your code sequence may not
realize that a NULL input @src would return an indeterminate @dst result.
Obviously my preference is for return @dst, but I'm OK with what you've
done as long you modify the comments to indicate it's up to the caller
to validate @dst. Furthermore, since @src is a (const char **) input
value, no sense in telling the caller they must free it... Finally, I
think there should be a "if (dst) *dst = NULL", prior to "if (!src)"
-
at least that avoids one more ambiguity.
IDC - either way, I trust that you can handle either option
appropriately for my R-B.
John