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

[Marc-André Lureau]

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