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

[Daniel P. Berrange]

On Wed, Jul 22, 2009 at 02:30:55PM -0400, Laine Stump wrote:
> 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).

Hmm, everyone gets -Wformat -Wformat-security added by default if
they run  autogen.sh without the 'compile-warnings' flag.

I'd say its more likely that you are building without gettext
support, hence _() turns into a no-op, and thus lets the compiler
identify these format problems more reliably.

> >
> >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?).

This is actually good, because it protects against a translator 
introduceing a '%' sequence in the non-english text either delibrately
or accidentally.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|