Re: [libvirt] [PATCH v2 1/5] introduce virSnprintf helper
On 09/12/2014 12:47 PM, John Ferlan wrote:
On 09/10/2014 08:08 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> ---
> src/libvirt_private.syms | 1 +
> src/util/virstring.c | 25 +++++++++++++++++++++++++
> src/util/virstring.h | 20 ++++++++++++++++++++
> 3 files changed, 46 insertions(+)
>
Not sure why this is necessary - I do see calls to 'snprintf' from
within existing libvirt code. The usage you have in patch 5 could
easily use snprintf. There is no need for variable argument list.
I'm not against it, but if it's created shouldn't other snprintf's use
it?
I agree that if we introduce this, we should add a syntax check that
forbids raw use of snprintf, and fix all existing callers to start using
it. It does have the nice aspect of doing automatic error reporting if
the buffer is too small, so I'm not opposed to adding it, but it would
need another version. I'm also okay if you just directly use snprintf
(seeing as how we already have existing code that does that) without
trying to add this wrapper.
> +/**
> + * virSnprintf:
> + * @strp: variable to hold result (char*)
> + * @fmt: printf format
> + *
> + * Like glibc's_snprintf but report error if the src string is longer
> + * then dst string.
Wouldn't that really be "report error if the resulting output is too
long for the dst string"?
> + *
> + * Returns -1 on failure, number of bytes printed on success.
> + */
> +
> +# define virSnprintf(strp, ...) \
> + virSnprintfInternal(VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__, \
> + strp, ARRAY_CARDINALITY(strp), __VA_ARGS__)
Hmm, this doesn't match your documentation. ARRAY_CARDINALITY only
works on char[], not on char* (that is, the compiler has to know how
long the array is, not just that it is a pointer).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 539 bytes)