Re: [PATCH v2 1/5] introduce virSnprintf helper

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
>> ---
>>  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

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]