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

[Frediano Ziglio]

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

Just a comment like:

/* These names are used to parse command line options, don't change them */

the enum values are already part of a protocol ABI.

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

0 for instance. And any possible future change adding holes in
spice.proto file. Why not be prepared for future changes?

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

Nevertheless this not not part of function generalization but an
hidden change to the code behaviour.

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

Agree is weird to have some channel encrypted and others not but
somebody implemented it, probably they have their reason.
Maybe just to save some bandwidth for some information they don't
care much to be sneaked.
Not a good reason to potentially break VM configurations.

Frediano