[Spice-devel] [PATCH spice-common v3 2/2] log: Force format in log macro to be a string

[Christophe Fergeau]

On Wed, Jul 05, 2017 at 04:07:08AM -0400, Frediano Ziglio wrote:
> > 
> > On Tue, Jul 04, 2017 at 10:18:52AM +0100, Frediano Ziglio wrote:
> > > Make sure format is a string and not a pointer.
> > > This prevents usages like:
> > > 
> > >   spice_debug(NULL);
> > > 
> > > This also prevents computed format strings too but on the other
> > > hand allows the compiler to check argument types.
> > 
> > This is vague and misleading, the compiler should already be able to
> > check that we have a format string with the right arguments. This patch
> > just adds a little bit on top of that, which is ensuring the format
> > string is not NULL.
> > 
> > Christophe
> > 
> 
> How can the compiler know if 
> 
>   spice_debug(my_string, 1, 2, 3);
> 
> is correct or not? The compiler must know the value of "my_string".
> In the case of computed format strings the compiler cannot check types.
> Currently we don't use computed strings but just constants or NULL,
> with this patch code only allows constant strings.

Ah ok, I read "on the other hand allows the compiler to check argument
types" as "on the other hand, *this patch* allows ...", while it really
goes together with the beginning of the sentence (more logical to read
this way, but I was too prompt handling this /o\ )


Maybe just expands on this?
"This forbids computed format strings, but on the other hand, forcing
the use of string constants allows the compiler to always check argument
types against the format string. Moreover, there is no occurrences of
computed format strings in the current codebase"

Acked-by: Christophe Fergeau <cfergeau at redhat.com>

Christophe

> 
> > > 
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > > Changes since v1:
> > > - extent commit message rationale.
> > > ---
> > >  common/log.h | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/common/log.h b/common/log.h
> > > index 9f5fcbb..06d48d2 100644
> > > --- a/common/log.h
> > > +++ b/common/log.h
> > > @@ -62,23 +62,23 @@ void spice_log(GLogLevelFlags log_level,
> > >  } G_STMT_END
> > >  
> > >  #define spice_info(format, ...) G_STMT_START {                         \
> > > -    spice_log(G_LOG_LEVEL_INFO, SPICE_STRLOC, __FUNCTION__, format, ##
> > > __VA_ARGS__); \
> > > +    spice_log(G_LOG_LEVEL_INFO, SPICE_STRLOC, __FUNCTION__, "" format, ##
> > > __VA_ARGS__); \
> > >  } G_STMT_END
> > >  
> > >  #define spice_debug(format, ...) G_STMT_START {                         \
> > > -    spice_log(G_LOG_LEVEL_DEBUG, SPICE_STRLOC, __FUNCTION__, format, ##
> > > __VA_ARGS__); \
> > > +    spice_log(G_LOG_LEVEL_DEBUG, SPICE_STRLOC, __FUNCTION__, "" format, ##
> > > __VA_ARGS__); \
> > >  } G_STMT_END
> > >  
> > >  #define spice_warning(format, ...) G_STMT_START {                       \
> > > -    spice_log(G_LOG_LEVEL_WARNING, SPICE_STRLOC, __FUNCTION__, format, ##
> > > __VA_ARGS__); \
> > > +    spice_log(G_LOG_LEVEL_WARNING, SPICE_STRLOC, __FUNCTION__, "" format,
> > > ## __VA_ARGS__); \
> > >  } G_STMT_END
> > >  
> > >  #define spice_critical(format, ...) G_STMT_START {
> > >  \
> > > -    spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, __FUNCTION__, format, ##
> > > __VA_ARGS__); \
> > > +    spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, __FUNCTION__, "" format,
> > > ## __VA_ARGS__); \
> > >  } G_STMT_END
> > >  
> > >  #define spice_error(format, ...) G_STMT_START {                         \
> > > -    spice_log(G_LOG_LEVEL_ERROR, SPICE_STRLOC, __FUNCTION__, format, ##
> > > __VA_ARGS__); \
> > > +    spice_log(G_LOG_LEVEL_ERROR, SPICE_STRLOC, __FUNCTION__, "" format, ##
> > > __VA_ARGS__); \
> > >  } G_STMT_END
> > >  
> > >  #define spice_warn_if_fail(x) G_STMT_START {            \
> 
> Frediano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170705/a841dba9/attachment.sig>