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

[Christophe Fergeau]

On Wed, Oct 18, 2017 at 06:00:00AM -0400, Frediano Ziglio wrote:
> > 
> > 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?

I would expect someone changing a string -> spice-protocol enum mapping
would *carefully* look at the users to ensure such a change is not going
to break things, yes. I can duplicate the array doing the mapping if
you prefer, keep spice_server_set_channel_security() unchanged, and add
a second copy of that array for the red_channel_name_to_type() method if
you think that's much more safe, but I'd rather not.

> As you said the function should filter smartcard. Did you document this?
> How people editing red_channel_name_to_type should expect to know?

I did not document it because I did really give it a lot of thoughts...
This is not code I expect to be heavily modified in the future, save
for additions, which should be fine for
spice_server_set_channel_security.

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

I don't think we can have NULL values in the range after removing the
#ifdef USE_SMARTCARD. If we can in the future, we can always adjust this
method behaviour. But really, if this is being called with an invalid
type, I'd rather know as soon as possible as this should not happen. So
a big warning in the log is good to me to be informed as early as
possible that something unexpected is happening.


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

I'd prefer that people don't use per-channel security at all to be
honest. Either use plaintext on local systems/development environments,
and TLS for all channels in all other cases.

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/6f04f8f0/attachment-0001.sig>