[libvirt] [PATCH 2/4] Update modified mac address in place in virGetInterface

[Laine Stump]

On 07/22/2009 11:36 AM, Daniel P. Berrange wrote:
> On Wed, Jul 22, 2009 at 11:25:38AM -0400, Laine Stump wrote:
>   
>> On 07/22/2009 10:20 AM, Daniel Veillard wrote:
>>     
>>> Agreed, patch applied, I only had to add _() to get the message localized,
>>>       
>> I've idly wondered about that macro, as it causes scores of compile 
>> warnings, like this:
>>
>> datatypes.c:291: warning: format not a string literal and no format 
>> arguments
>>
>> What's it for? And what's the best way to change things to eliminate the 
>> warnings?
>>     
>
> It is a potential security hole, if the user finds a way to send a string
> with an embeded format specifier. Thus if you're not doing any subsitutions,
> and the string isn't constant, then you should always at least do
>
>   char *therealstring = ...from somewhere untrusted...
>   printf("%s", therealstring)
>   

Yeah, I understand the perils of using a non-literal string as the 
format to *printf(). I'm wondering what the purpose of _() is, and why 
it causes the compiler to believe the string isn't a literal. (I 
received this warning when others don't because I use "-Wformat 
-Wformat-security" in my CFLAGS, at Jim's suggestion).
>
> NB, anyone sending patches should always set
>
>    --enable-compile-warnings=error
>
> when running 'autogen.sh' and make sure all warnings & errors are fixed 
> before submitting a patch.
>   

It's actually because I like doing this that I'd like to know the 
preferred method of eliminating the warnings I mentioned. There are a 
bunch of them pre-existing in the code that I want to get rid of so I 
can turn on warnings=error (without turning off these warnings in 
CFLAGS), and I want to do it the "accepted" way. For example, from 
domain_conf.c:2137:

virDomainReportError(conn, VIR_ERR_XML_ERROR,
_("invalid security type"));

spits out the warning. We all know that it really *is* literal, but the 
macro is changing the class so the compile thinks it isn't. It would be 
simple to just change it to:

virDomainReportError(conn, VIR_ERR_XML_ERROR,
"%s", _("invalid security type"));

(and there are plenty of those too), but that's inefficient, and doesn't 
do the _() around the "%s" (is that correct or not?).

If someone wants to tell me the preferred way of doing these, I'll 
handle the grunt work of making the changes.