[libvirt] [PATCH v3 01/34] virstring: Introduce VIR_STRDUP and VIR_STRNDUP

[Eric Blake]

On 05/03/2013 08:53 AM, Michal Privoznik wrote:
> The code adaptation is not done right now, but in subsequent patches.
> Hence I am not implementing syntax-check rule as it would break
> compilation. Developers are strongly advised to use these new macros.
> They are similar to VIR_ALLOC() logic: VIR_STRDUP(dst, src) returns zero
> on success, -1 otherwise. In case you don't want to report OOM error,
> use the _QUIET variant of a macro.
> ---
>  HACKING                  | 11 +++++++
>  docs/hacking.html.in     | 14 +++++++++
>  src/libvirt_private.syms |  2 ++
>  src/util/virstring.c     | 74 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/util/virstring.h     | 62 ++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 163 insertions(+)
> 

> +++ b/docs/hacking.html.in
> @@ -853,6 +853,20 @@
>        virStrncpy(dest, src, strlen(src), sizeof(dest)).
>      </p>
>  
> +<pre>
> +  VIR_STRDUP(char *dst, const char *src);
> +  VIR_STRNDUP(char *dst, const char *src, size_t n);
> +</pre>
> +    <p>
> +      You should avoid using strdup or strndup directly as they do not report
> +      out-of-memory error.  Use VIR_STRDUP() or VIR_STRNDUP macros instead.

Hmm, you weren't consistent on whether to use () after the macro name...

> +      Note, that these two behave similar to VIR_ALLOC: on success zero is
> +      returned, otherwise the result is -1 and dst is guaranteed to be NULL. In
> +      very specific cases, when you don't want to report the out-of-memory
> +      error, you can use VIR_STRDUP_QUIET or VIR_STRNDUP_QUIET, but such usage
> +      is very rare and usually considered a flaw.

Since most of the references didn't use it, I'd drop the one () you did use.

> +++ b/src/util/virstring.h
> @@ -87,4 +87,66 @@ char *virStrncpy(char *dest, const char *src, size_t n, size_t destbytes)
>  char *virStrcpy(char *dest, const char *src, size_t destbytes)
>      ATTRIBUTE_RETURN_CHECK;
>  # define virStrcpyStatic(dest, src) virStrcpy((dest), (src), sizeof(dest))
> +
> +
> +/* Don't call these directly - use the macros below */
> +int virStrdup(char **dest, const char *src, bool report, int domcode,
> +              const char *filename, const char *funcname, size_t linenr)
> +    ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

You know, we COULD play games where we allow a NULL src (and in that
case silently set dest to NULL, without an OOM message even when report
is true), so that it is easier to blindly use src even if it stems from
a failure earlier in the function.  That is, right now, we have to do:

void foo(char *source) {
  char *copy = NULL;
  if (source && VIR_STRDUP(copy, source) < 0)
    goto cleanup;
  do stuff with copy
cleanup:
  VIR_FREE(copy);
}

whereas my proposal to drop ATTRIBUTE_NONNULL(2) would let us do the
slightly shorter:

void foo(char *source) {
  char *copy;
  if (VIR_STRDUP(copy, source) < 0)
    goto cleanup;
  do stuff with copy
cleanup:
  VIR_FREE(copy);
}

That is, just because strdup(NULL) crashes with SEGV doesn't mean our
function has to; and we could argue that duplicating NULL into NULL
without OOM error is useful behavior.  Maybe even return -1 for OOM, 0
for duplicating NULL, and 1 for actual duplication (so callers can use
<0 vs. <=0 depending on what they care about).

But I'm not sure if we need that yet (and it can be done as a separate
patch, if we do decide that it simplifies things without the risk of
problems).

> +/**
> + * VIR_STRDUP:
> + * @dst: pointer to copied string

Maybe make it more obvious that you don't pass &src, by having this (and
the other three macros) read:
 @dst: variable to hold result (char*, not char**)

> +/**
> + * VIR_STRNDUP:
> + * @dst: pointer to copied string
> + * @src: string to duplicate
> + * @n: the maximum number of bytes to copy
> + *
> + * Duplicate @src string and store it ino @dst. If @src is longer than @n,

s/ino/into/ (here, and below)

My findings (other than the suggestion of accepting NULL source, which
would be a separate patch anyways) are minor, so I'm okay giving ACK; it
might be worth pushing this in now so we can start using it in new code,
even while still waiting for the rest of the series to be reviewed.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130503/38fc7577/attachment-0001.sig>