[Spice-devel] [spice-server 1/5] utils: Introduce helpers to map channel types to names

[Frediano Ziglio]

> 
> On Wed, Oct 18, 2017 at 04:08:50AM -0400, Frediano Ziglio wrote:
> > > 
> > > On Tue, Oct 17, 2017 at 05:15:22PM -0400, Frediano Ziglio wrote:
> > > > > +static const char *const channel_names[] = {
> > > > > +    [ SPICE_CHANNEL_MAIN     ] = "main",
> > > > > +    [ SPICE_CHANNEL_DISPLAY  ] = "display",
> > > > > +    [ SPICE_CHANNEL_INPUTS   ] = "inputs",
> > > > > +    [ SPICE_CHANNEL_CURSOR   ] = "cursor",
> > > > > +    [ SPICE_CHANNEL_PLAYBACK ] = "playback",
> > > > > +    [ SPICE_CHANNEL_RECORD   ] = "record",
> > > > > +#ifdef USE_SMARTCARD
> > > > > +    [ SPICE_CHANNEL_SMARTCARD] = "smartcard",
> > > > > +#endif
> > > > 
> > > > Not a regression. Is it worth having the #ifdef ?
> > > > SPICE_CHANNEL_SMARTCARD is always defined.
> > > 
> > > Hmm, red_channel_name_to_type() uses that to return -1 when we were
> > > built without smartcard support. I think it's better if we keep that
> > > behaviour.
> > > 
> > 
> > So you should document that this function is designed to be
> > used just by spice_server_set_channel_security.
> 
> If you prefer that the #ifdef USE_SMARTCARD filtering is done in
> spice_server_set_channel_security(), just say so..
> 
> > > In my opinion, red_channel_type_to_str() should not be called with an
> > > invalid type, the caller should make sure this does not happen. At the
> > > moment, the only caller is red_channel_get_name() which is added in this
> > > commit as well, so this can't be a regression.
> > > 
> > 
> > If this is the design of the function this should be documented (this
> > function is designed to be called only by red_channel_get_name).
> 
> You are stretching what I said quite a bit ;) All I'm saying is that it
> should not be called with an invalid channel type, or a warning is going
> to be shown. This is no different than having a method starting with
> g_return_if_fail(RED_IS_CHANNEL_CLIENT(client));
> I can add a comment saying that you should not pass random values to
> that function if you want to keep stderr clean if you want.
> 
> Christophe
> 

Your comment says "We can make its code more generic" that is you are
writing function to be reused for different purposes. But you just extracted
code and constants from spice_server_set_channel_security into new
channel_names/red_channel_name_to_type/red_channel_type_to_str.
Now after these patches everything work. But is the code robust and
maintainable? What happens if you change some names? Previously
you known we would change spice_server_set_channel_security but now is
less obvious. Having programmers to check every call or history is not
a good idea. In this case the constants in spice_server_set_channel_security
are used to parse some options from Qemu. That is changing these constant
will change Qemu command line! Did you document this on the new code?
As you said the function should filter smartcard. Did you document this?
How people editing red_channel_name_to_type should expect to know?

If i write

const char *red_channel_type_to_str(int type)
{
    if (type < 0 || type >= G_N_ELEMENTS(channel_names)) {
        return NULL;
    }

    return channel_names[type];
}

the function is more consistent. If either I call
red_channel_type_to_str(-1) or red_channel_type_to_str(300) or
a value inside [0, G_N_ELEMENTS(channel_names)) which is NULL
I get a NULL and no log while with your code you get a log if
the value is out of range but not if is NULL but in the range.
About the #ifdef in spice_server_set_channel_security I think
that not having it would just make Qemu command line accepts
smartcard option in any case. Not strictly related to these
patches but I think won't hurt to have it, as smartcard channel
won't be available this at the end won't change anything.
On the other hand the Qemu machine can be configured with
a secure smartcard channel so if the machine in the future
will run on a Qemu with smartcard compiled in will be secured.

Frediano