[libvirt] [PATCH 01/23] util: Introduce virStringListCopy

[Jiri Denemark]

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 at 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(&copy, 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