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@xxxxxxxxxx> > 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list