On 04/29/2013 07:50 AM, Michal Privoznik wrote:
s/STRUP/STRDUP/ in the subject line.
> 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 _QUIET variant of a macro.
s/use/use the/
Thinking about backportability - it looks like Fedora 19 will probably
be based on libvirt 1.0.5 + patches. I'm hoping that if we backport
this patch, but not the syntax check or the general cleanups of using
VIR_STRDUP everywhere, then further backports of any new code that uses
VIR_STRDUP can be done by using this interface, and any merge conflicts
of code that used to use strdup should be fairly easy to resolve during
the backport process (of course, the IDEAL situation is that 1.0.5
doesn't have any bugs that require backporting across this refactoring,
right? :)
> +++ 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 the does not report
s/the does/they do/
> + out-of-memory error. Use VIR_STRDUP() or VIR_STRNDUP macros instead.
> + Note, that these two behave in similar way to VIR_ALLOC: on success zero
s/in similar way/similar/
> + is returned, otherwise caller is left with negative one. In very specific
s/caller is left with negative one/the result is -1 and dst is
guaranteed to be NULL/
> + case, when you don't want to report the out-of-memory error, you can use
s/case/cases/
> + VIR_STRDUP_QUIET or VIR_STRNDUP_QUIET, but such usage is very rare and
> + usually considered a flaw.
> + </p>
> +
> <h2><a name="strbuf">Variable length string buffer</a></h2>
>
> +++ b/src/util/virstring.c
> @@ -515,3 +515,77 @@ virArgvToString(const char *const *argv)
>
> return ret;
> }
> +
> +/**
> + * virStrdup:
> + * @dest: where to store duplicated string
> + * @src: the source string to duplicate
> + * @report: whether to report OOM error, if there's a one
s/there's a/there is/
> + * @domcode: error domain code
> + * @filename: caller's filename
> + * @funcname: caller's funcname
> + * @linenr: caller's line number
> + *
> + * Wrapper over strdup, which reports OOM error if told so,
> + * in which case callers wants to pass @domcode, @filename,
> + * @funcname and @linenr which should represent location in
> + * caller's body where virStrdup is called from. Consider
> + * using VIR_STRDUP which sets these automatically.
> + *
> + * Returns: 0 on success, -1 othervise.
s/othervise/otherwise/
> + */
> +int
> +virStrdup(char **dest,
> + const char *src,
> + bool report,
> + int domcode,
> + const char *filename,
> + const char *funcname,
> + size_t linenr)
> +{
> + if (!(*dest = strdup(src))) {
> + if (report)
> + virReportOOMErrorFull(domcode, filename, funcname, linenr);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * virStrndup:
> + * @dest: where to store duplicated string
> + * @src: the source string to duplicate
> + * @n: how many bytes to copy
> + * @report: whether to report OOM error, if there's a one
s/there's a/there is/
> + * @domcode: error domain code
> + * @filename: caller's filename
> + * @funcname: caller's funcname
> + * @linenr: caller's line number
> + *
> + * Wrapper over strndup, which reports OOM error if told so,
> + * in which case callers wants to pass @domcode, @filename,
> + * @funcname and @linenr which should represent location in
> + * caller's body where virStrdup is called from. Consider
s/virStrdup/virStrndup/
> + * using VIR_STRNDUP which sets these automatically.
> + *
> + * Returns: 0 on success, -1 othervise.
s/othervise/otherwise/
> + */
> +int
> +virStrndup(char **dest,
> + const char *src,
> + size_t n,
> + bool report,
> + int domcode,
> + const char *filename,
> + const char *funcname,
> + size_t linenr)
> +{
> + if (!(*dest = strndup(src, n))) {
> + if (report)
> + virReportOOMErrorFull(domcode, filename, funcname, linenr);
> + return -1;
> + }
> +
> + return 0;
> +}
Implementation of the helper functions looks correct.
> diff --git a/src/util/virstring.h b/src/util/virstring.h
> index 457caa2..620efba 100644
> --- a/src/util/virstring.h
> +++ b/src/util/virstring.h
> @@ -87,4 +87,24 @@ 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))
> +
> +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);
> +
> +/* XXX Don't report OOM error for now, unless all code is adapted. */
I disagree. Since the calling convention changed, anywhere that we use
VIR_STRDUP should already be dealing with the new semantics of expecting
VIR_STRDUP to report the error. Passing false only made sense if you
were trying to make VIR_STRDUP a drop-in replacement for strdup (where
you could save work by doing a global search-and-replace for the
drop-in, then revisit code later for the new semantics). But since you
already have to revisit semantics for this macro, you might as well
assume OOM reporting from the beginning, by...
> +# define VIR_STRDUP(dst, src) virStrdup(&(dst), src, false, VIR_FROM_THIS, \
> + __FILE__, __FUNCTION__, __LINE__)
...doing s/false/true/
> +# define VIR_STRDUP_QUIET(dst, src) virStrdup(&(dst), src, false, 0, NULL, NULL, 0)
> +int virStrndup(char **dest, const char *src, size_t n, bool report, int domcode,
> + const char *filename, const char *funcname, size_t linenr)
> + ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
It is hard to read function and macro declarations intermixed like this.
I would much rather see this done like viralloc.h, by separating the
function declarations up front (and with a BIG disclaimer that most code
should not use it directly), then the macros down below, and with full
documentation of the macros (since their arguments are slightly
different than the arguments of the functions).
> +
> +/* XXX Don't report OOM error for now, unless all code is adapted. */
> +# define VIR_STRNDUP(dst, src, n) virStrndup(&(dst), src, n, false, \
> + VIR_FROM_THIS, __FILE__, \
> + __FUNCTION__, __LINE__)
Again, s/false/true/ since you are already changing calling conventions
on anywhere converted to use this macro.
> +
> +# define VIR_STRNDUP_QUIET(dst, src, n) virStrndup(&(dst), src, n, false, \
> + 0, NULL, NULL, 0)
> #endif /* __VIR_STRING_H__ */
>
Getting closer. Looking forward to v3.
--
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