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

[Christophe Fergeau]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20171018/ff37a618/attachment.sig>