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)
+{
+    char **copy = NULL;
+    size_t i;
+
+    if (!src)
+        return 0;
+
+    if (VIR_ALLOC_N(copy, virStringListLength(src) + 1) < 0)
+        goto error;
+
+    for (i = 0; src[i]; i++) {
+        if (VIR_STRDUP(copy[i], src[i]) < 0)
+            goto error;
+    }
+
+    *dst = copy;
+    return 0;
+
+ error:
+    virStringListFree(copy);
+    return -1;
+}
+
+
 /**
  * virStringListFree:
  * @str_array: a NULL-terminated array of strings to free
diff --git a/src/util/virstring.h b/src/util/virstring.h
index 1290fcce15..cfd91be314 100644
--- a/src/util/virstring.h
+++ b/src/util/virstring.h
@@ -46,6 +46,9 @@ char **virStringListAdd(const char **strings,
 void virStringListRemove(char ***strings,
                          const char *item);
 
+int virStringListCopy(char ***dst,
+                      const char **src);
+
 void virStringListFree(char **strings);
 void virStringListFreeCount(char **strings,
                             size_t count);
-- 
2.14.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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)...

w/ the return of char** similar to other vir*Copy's and even
virStringListAdd...

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

Ironically virStringListAdd does essentially the same thing if you pass
@item==NULL since VIR_STRDUP(x, NULL) just sets x to NULL...  Of course
the other difference is that @dst in the *Add case could be an array of
NULL string entries.

> +    char **copy = NULL;
> +    size_t i;
> +
> +    if (!src)
> +        return 0;
> +
> +    if (VIR_ALLOC_N(copy, virStringListLength(src) + 1) < 0)> +        goto error;
> +
> +    for (i = 0; src[i]; i++) {
> +        if (VIR_STRDUP(copy[i], src[i]) < 0)
> +            goto error;
> +    }
> +
> +    *dst = copy;
> +    return 0;
> +
> + error:
> +    virStringListFree(copy);
> +    return -1;
> +}
> +
> +
>  /**
>   * virStringListFree:
>   * @str_array: a NULL-terminated array of strings to free
> diff --git a/src/util/virstring.h b/src/util/virstring.h
> index 1290fcce15..cfd91be314 100644
> --- a/src/util/virstring.h
> +++ b/src/util/virstring.h
> @@ -46,6 +46,9 @@ char **virStringListAdd(const char **strings,
>  void virStringListRemove(char ***strings,
>                           const char *item);
>  
> +int virStringListCopy(char ***dst,
> +                      const char **src);
> +
>  void virStringListFree(char **strings);
>  void virStringListFreeCount(char **strings,
>                              size_t count);
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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;

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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(&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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list