[Spice-devel] [PATCH spice-gtk 1/6] channel: return "unknown" in spice_channel_type_to_string()

[Frediano Ziglio]

> 
> ----- Original Message -----
> > Hi
> > 
> > 
> > On 08/11/2017 06:37 PM, Marc-André Lureau wrote:
> > > Hi
> > >
> > > ----- Original Message -----
> > >>> From: Marc-André Lureau <marcandre.lureau at redhat.com>
> > >>>
> > >>> This fixes the following gcc warning:
> > >>>
> > >>> spice-channel.c: In function ‘spice_channel_constructed’:
> > >>> spice-channel.c:144:41: error: ‘%s’ directive output may be truncated
> > >>> writing
> > >>> likely 20 or more bytes into a region of size 16
> > >>> [-Werror=format-truncation=]
> > >>>       snprintf(c->name, sizeof(c->name), "%s-%d:%d",
> > >>>                                           ^~
> > >>> spice-channel.c:144:40: note: assuming directive output of 20 bytes
> > >>>       snprintf(c->name, sizeof(c->name), "%s-%d:%d",
> > >>>
> > >>> Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> > >>> ---
> > >>>   src/spice-channel.c | 4 ++--
> > >>>   1 file changed, 2 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/src/spice-channel.c b/src/spice-channel.c
> > >>> index 4c3db9d..7cd6251 100644
> > >>> --- a/src/spice-channel.c
> > >>> +++ b/src/spice-channel.c
> > >>> @@ -2130,13 +2130,13 @@ static const char *to_string[] = {
> > >>>    **/
> > >>>   const gchar* spice_channel_type_to_string(gint type)
> > >>>   {
> > >>> -    const char *str = NULL;
> > >>> +    const char *str = "unknown";
> > >>>   
> > >>>       if (type >= 0 && type < G_N_ELEMENTS(to_string)) {
> > >>>           str = to_string[type];
> > >>>       }
> > >>>   
> > >>> -    return str ? str : "unknown channel type";
> > >>> +    return str;
> > >>>   }
> > >>>   
> > >>>   /**
> > >> Note that some array entries contains NULL so changing the code
> > >> that way you are returning NULL in some cases while before
> > >> was never NULL. Simply modifying the string solve the gcc issue
> > > oops, my bad.
> > >
> > > At first I thought the function should return NULL instead, but it is API
> > > since 0.20. It is likely that is only used to print a user friendly
> > > message. I think we made the wrong choice. Although it should be fine to
> > > change to return NULL, and updating the doc. Then in various places,
> > > either have gprintf %s/null behaviour, or we have to_string() ?:
> > > "unknown"..
> > >
> > >> without this regression. Snir was working in the same issue but
> > >> you arrived first with a patch.
> > >> Potentially this change the reply to a public API. Maybe to avoid
> > >> potentially having an "unknown" channel we could use "UNKNOWN"
> > >> or "Unknown" ?
> > > I'll just modify the patch to return "unknown" for now.
> > 
> > Why doesn't gcc complain also on the integers? ,"%s-%d:%d"
> > may still be more than 16 bytes if integers are large enough
> > (not here though, but it doesn't seem gcc check for possible integer
> > values...)
> > Anyway, "unknown" seems good enough to me..
> 
> Yeah you are right, gcc should probably also complain there. Since we don't
> parse this back it's not such a big deal.
> 
> But it's good enough for now, otherwise, we can also switch the snprintf() to
> g_strdup_printf(). That would be another patch. Feel free to make one.
> 
> thanks
> 

I won't get crazy for some debug print.
snprintf and truncation was more than good IMO.

Frediano