[RFC] vdsm/libvirtvm.py: spice channels: send correct names

[Alon Levy]

Old code used to remove the first char unconditionally. This requires a
hack in the vdsm user (i.e. ovirt engine) to keep sending the leading
's', even though upstream spice has stopped using the first char to
signify secure channel, and passes instead a specific message for that
purpose. Change the logic to only remove 's' from the specific legacy
channel names. This will not break old clients that only know about the
six channels, [main, inputs, display, cursor, playback, record].

Signed-off-by: Alon Levy <alevy(a)redhat.com&gt;
---
 vdsm/libvirtvm.py |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/vdsm/libvirtvm.py b/vdsm/libvirtvm.py
index 5a4d1bc..df06393 100644
--- a/vdsm/libvirtvm.py
+++ b/vdsm/libvirtvm.py
@@ -778,6 +778,12 @@ class _DomXML:
            <target type='virtio' name='com.redhat.spice.0'/>
          </channel>
         """
+        legacy_s_prefixed_channels = ['smain', 'sinputs',
'sdisplay', 'scursor',
+                                      'splayback', 'srecord']
+        def plainChannelName(channel):
+            if channel in legacy_s_prefixed_channels:
+                return channel[1:]
+            return channel
         graphics = self.doc.createElement('graphics')
         if self.conf['display'] == 'vnc':
             graphics.setAttribute('type', 'vnc')
@@ -791,7 +797,7 @@ class _DomXML:
             if self.conf.get('spiceSecureChannels'):
                 for channel in
self.conf['spiceSecureChannels'].split(','):
                     m = self.doc.createElement('channel')
-                    m.setAttribute('name', channel[1:])
+                    m.setAttribute('name', plainChannelName(channel))
                     m.setAttribute('mode', 'secure')
                     graphics.appendChild(m)
 
-- 
1.7.10.1

[Alon Levy]

On Wed, May 09, 2012 at 10:55:06AM +0300, Dan Kenigsberg wrote:
>  On Wed, May 09, 2012 at 09:32:38AM +0300, Alon Levy wrote:
>  > Old code used to remove the first char unconditionally. This requires a
>  > hack in the vdsm user (i.e. ovirt engine) to keep sending the leading
>  > 's', even though upstream spice has stopped using the first char to
>  > signify secure channel, and passes instead a specific message for that
>  > purpose. Change the logic to only remove 's' from the specific legacy
>  > channel names. This will not break old clients that only know about the
>  > six channels, [main, inputs, display, cursor, playback, record].
>  > 
>  > Signed-off-by: Alon Levy <alevy(a)redhat.com&gt;
>  
>  Ok... Few minor style comments.
>  (You do know that we prefer posts to gerrit.ovirt.org?) 
My bad - I read the main vdsm page, didn't look into
http://www.ovirt.org/wiki/Vdsm_Developers

I'll send the next version via gerrit.

>  
>  > ---
>  >  vdsm/libvirtvm.py |    8 +++++++-
>  >  1 file changed, 7 insertions(+), 1 deletion(-)
>  > 
>  > diff --git a/vdsm/libvirtvm.py b/vdsm/libvirtvm.py
>  > index 5a4d1bc..df06393 100644
>  > --- a/vdsm/libvirtvm.py
>  > +++ b/vdsm/libvirtvm.py
>  > @@ -778,6 +778,12 @@ class _DomXML:
>  >             <target type='virtio' name='com.redhat.spice.0'/>
>  >           </channel>
>  >          """
>  > +        legacy_s_prefixed_channels = ['smain', 'sinputs',
> 'sdisplay', 'scursor',
>  > +                                      'splayback', 'srecord']
>  
>  CONSTANTS are all caps, and preferably immutable tuple, not list. In
>  this case, set() would be even better.
>  
>  > +        def plainChannelName(channel):
>  > +            if channel in legacy_s_prefixed_channels:
>  > +                return channel[1:]
>  > +            return channel
>  >          graphics = self.doc.createElement('graphics')
>  >          if self.conf['display'] == 'vnc':
>  >              graphics.setAttribute('type', 'vnc')
>  > @@ -791,7 +797,7 @@ class _DomXML:
>  >              if self.conf.get('spiceSecureChannels'):
>  >                  for channel in
> self.conf['spiceSecureChannels'].split(','):
>  >                      m = self.doc.createElement('channel')
>  > -                    m.setAttribute('name', channel[1:])
>  > +                    m.setAttribute('name', plainChannelName(channel))
>  >                      m.setAttribute('mode', 'secure')
>  >                      graphics.appendChild(m)
>  >  
>  > -- 
>  > 1.7.10.1
>  >

[Dan Kenigsberg]

On Wed, May 09, 2012 at 09:32:38AM +0300, Alon Levy wrote:
>  Old code used to remove the first char unconditionally. This requires
> a
>  hack in the vdsm user (i.e. ovirt engine) to keep sending the leading
>  's', even though upstream spice has stopped using the first char to
>  signify secure channel, and passes instead a specific message for that
>  purpose. Change the logic to only remove 's' from the specific legacy
>  channel names. This will not break old clients that only know about the
>  six channels, [main, inputs, display, cursor, playback, record].
>  
>  Signed-off-by: Alon Levy <alevy(a)redhat.com&gt; 
Ok... Few minor style comments.
(You do know that we prefer posts to gerrit.ovirt.org?)

>  ---
>   vdsm/libvirtvm.py |    8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>  
>  diff --git a/vdsm/libvirtvm.py b/vdsm/libvirtvm.py
>  index 5a4d1bc..df06393 100644
>  --- a/vdsm/libvirtvm.py
>  +++ b/vdsm/libvirtvm.py
>  @@ -778,6 +778,12 @@ class _DomXML:
>              <target type='virtio' name='com.redhat.spice.0'/>
>            </channel>
>           """
>  +        legacy_s_prefixed_channels = ['smain', 'sinputs',
> 'sdisplay', 'scursor',
>  +                                      'splayback', 'srecord'] 
CONSTANTS are all caps, and preferably immutable tuple, not list. In
this case, set() would be even better.

>  +        def plainChannelName(channel):
>  +            if channel in legacy_s_prefixed_channels:
>  +                return channel[1:]
>  +            return channel
>           graphics = self.doc.createElement('graphics')
>           if self.conf['display'] == 'vnc':
>               graphics.setAttribute('type', 'vnc')
>  @@ -791,7 +797,7 @@ class _DomXML:
>               if self.conf.get('spiceSecureChannels'):
>                   for channel in
> self.conf['spiceSecureChannels'].split(','):
>                       m = self.doc.createElement('channel')
>  -                    m.setAttribute('name', channel[1:])
>  +                    m.setAttribute('name', plainChannelName(channel))
>                       m.setAttribute('mode', 'secure')
>                       graphics.appendChild(m)
>   
>  -- 
>  1.7.10.1
>