On Fri, Oct 13, 2017 at 14:14:37 -0400, John Ferlan wrote:
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@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;
Yeah, that's what returning a pointer would require, but when the function returns int, it's just if (virStringListCopy(©, blockers) < 0) goto error; If blockers are supposed to be non-NULL, the caller would need a separate check for it (possibly returning an error) in both cases.
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.
No, there's no need to validate @dst at all. The caller may validate @src if it requires it to be non-NULL.
Furthermore, since @src is a (const char **) input value, no sense in telling the caller they must free it...
OK
Finally, I think there should be a "if (dst) *dst = NULL", prior to "if (!src)" - at least that avoids one more ambiguity.
Yeah, this part is obviously missing there. Jirka