[libvirt] [PATCH 2/5] Introduce VIR_STRDUP to replace strdup
Eric Blake
eblake at redhat.com
Tue Apr 2 17:05:16 UTC 2013
On 04/02/2013 08:22 AM, Michal Privoznik wrote:
What about strndup? Then again, doing strndup in a second patch makes
sense.
> ---
>
> WARNING: THIS PATCH IS NOT COMPLETE !!!
>
> For full patch see the cover letter. I've trimmed the patch and left only
> interesting parts.
>
> HACKING | 5 ++
> cfg.mk | 8 ++
Good - a syntax check is necessary to enforce the new style :)
> src/xenxs/xen_xm.c | 38 ++++----
> 183 files changed, 1133 insertions(+), 1128 deletions(-)
Big, but mostly mechanical fallout to comply to the syntax check.
>
> diff --git a/HACKING b/HACKING
> index c8833c0..6230ffd 100644
> --- a/HACKING
> +++ b/HACKING
> @@ -719,6 +719,11 @@ sizeof(dest) returns something meaningful). Note that this is a macro, so
> arguments could be evaluated more than once. This is equivalent to
> virStrncpy(dest, src, strlen(src), sizeof(dest)).
>
> + VIR_STRDUP(char *c)
> +
> +You should avoid using strdup directly as it does not report OOM error. Use
In HACKING, it might be worth spelling out "out-of-memory" in case the
reader is not yet familiar with the abbreviation.
> +VIR_STRDUP(c) macro instead (@c is type of char *).
> +
>
> Variable length string buffer
> =============================
> diff --git a/cfg.mk b/cfg.mk
> index 7a2c69f..8cc67a5 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -383,6 +383,11 @@ sc_prohibit_asprintf:
> halt='use virAsprintf, not as'printf \
> $(_sc_search_regexp)
>
> +sc_prohibit_strdup:
> + @prohibit='\<strdup\>' \
> + halt='use VIR_STRUP, not strdup' \
s/STRUP/STRDUP/
When you add a later patch to cover strndup, this is easy enough to
generalize to cover both functions in one syntax checker.
If you use @prohibit='\<strn?dup *(', it will cut down on the number of
false positives, so that...
> + $(_sc_search_regexp)
> +
> # Prefer virSetUIDGID.
> sc_prohibit_setuid:
> @prohibit='\<set(re)?[ug]id\> *\(' \
> @@ -814,6 +819,9 @@ exclude_file_name_regexp--sc_prohibit_always_true_header_tests = \
> exclude_file_name_regexp--sc_prohibit_asprintf = \
> ^(bootstrap.conf$$|src/util/virstring\.c$$|examples/domain-events/events-c/event-test\.c$$)
>
> +exclude_file_name_regexp--sc_prohibit_strdup = \
> + ^(bootstrap.conf$$|cfg.mk$$|docs/|examples/|python/|src/storage/parthelper.c$$|src/util/virerror.c$$|src/util/virstring.c$$|tests/|tools/)
...you wouldn't have to list bootstrap.conf here. Also, instead of
exempting lots of entire files, it might be worth exempting just
specific uses of strdup, by using exclude='exempt from syntax-check'
(the way we do things for sc_prohibit_strtol).
> +++ b/daemon/libvirtd-config.c
> @@ -59,7 +59,7 @@ remoteConfigGetStringList(virConfPtr conf, const char *key, char ***list_arg,
> key);
> return -1;
> }
> - list[0] = strdup(p->str);
> + list[0] = VIR_STRDUP(p->str);
Hmm. What happens if VIR_STRDUP fails? We still have to check if
list[0] is NULL. Let's make sure this makes sense compared to what the
macro actually does...
> @@ -90,7 +90,7 @@ remoteConfigGetStringList(virConfPtr conf, const char *key, char ***list_arg,
> VIR_FREE(list);
> return -1;
> }
> - list[i] = strdup(pp->str);
> + list[i] = VIR_STRDUP(pp->str);
> if (list[i] == NULL) {
I'm assuming this is one of the places that you further touch later in
the series to simplify the cleanup paths to realize that the OOM message
has already been reported by VIR_STRDUP, and that this patch is just
using the new macro even if there are redundant error reports.
...oops, you trimmed out the one part I would have found most useful -
the actual definition of VIR_STRDUP.
/me goes and clones from your repo...
> commit ad304d40005e73e9ec60d26a35983b7e59f2562a
> Author: Michal Privoznik <mprivozn at redhat.com>
> Date: Wed Mar 27 18:58:17 2013 +0100
>
> Introduce VIR_STRDUP to replace strdup
>
> diff --git a/src/util/virstring.c b/src/util/virstring.c
> index 5bcaf17..3847811 100644
> --- a/src/util/virstring.c
> +++ b/src/util/virstring.c
> @@ -96,7 +96,7 @@ char **virStringSplit(const char *string,
> if (VIR_RESIZE_N(tokens, maxtokens, ntokens, 1) < 0)
> goto no_memory;
>
> - if (!(tokens[ntokens] = strdup(remainder)))
> + if (!(tokens[ntokens] = VIR_STRDUP(remainder)))
> goto no_memory;
Again, assuming a later patch cleans things up once VIR_STRDUP does the
OOM reporting.
> ntokens++;
> }
> @@ -145,7 +145,7 @@ char *virStringJoin(const char **strings,
> }
> ret = virBufferContentAndReset(&buf);
> if (!ret) {
> - if (!(ret = strdup(""))) {
> + if (!(ret = VIR_STRDUP(""))) {
> virReportOOMError();
> return NULL;
> }
> @@ -501,3 +501,19 @@ virArgvToString(const char *const *argv)
>
> return ret;
> }
> +
> +char *
> +virStrdup(const char *c,
> + bool report,
> + int domcode,
> + const char *filename,
> + const char *funcname,
> + size_t linenr)
Nice - the error reporting is tied to the source, rather than the helper
function.
> +{
> + char *ret = strdup(c);
> +
> + if (!ret && report)
> + virReportOOMErrorFull(domcode, filename, funcname, linenr);
> +
> + return ret;
> +}
> diff --git a/src/util/virstring.h b/src/util/virstring.h
> index e90e689..889951c 100644
> --- a/src/util/virstring.h
> +++ b/src/util/virstring.h
> @@ -85,5 +85,13 @@ void virSkipSpacesBackwards(const char *str, char **endp)
>
> char *virArgvToString(const char *const *argv);
>
> +char * virStrdup(const char *c, bool report, int domcode,
No space after '*'.
> + const char *filename, const char *funcname, size_t linenr)
> + ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4)
> + ATTRIBUTE_NONNULL(5);
> +
> +/* XXX Don't report OOM error for now, unless all code is adapted. */
> +# define VIR_STRDUP(c) virStrdup(c, false, VIR_FROM_THIS, __FILE__, \
> + __FUNCTION__, __LINE__)
This behaves like a drop-in replacement for strdup(), but still risks
that the caller has to deal with a NULL string if they are in an oom
situation. That is, callers still have to write:
if (!(str = VIR_STRDUP(src)))
goto cleanup;
In fact, maybe we should change the paradigm to be more like VIR_ALLOC,
where we have:
int ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
virStrdup(char **dest, const char *src, ...)
{
*dest = strdup(src);
if (!src) {
virReportOOMFull(...);
return -1;
}
return 0;
}
#define VIR_STRDUP(dst, src) virStrdup(&(dst), src, ...)
and where callers then convert:
if (!(str = strdup(src))) {
virReportOOMError();
goto cleanup;
}
into:
if (VIR_STRDUP(str, src) < 0)
goto cleanup;
In other words, while I like the direction we are headed by centralizing
OOM reporting, I'm worried that the interface you have chosen is too
easy to cause a subsequent NULL dereference, whereas reusing the
VIR_ALLOC paradigm would force users to deal with OOM errors. But that
would mean a rewrite of this patch, as VIR_STRDUP would no longer be a
drop-in replacement.
Does anyone else have an opinion on whether drop-in replacement or
VIR_ALLOC semantics are nicer?
--
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/20130402/f39cf1b4/attachment-0001.sig>
More information about the libvir-list
mailing list