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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel