[Spice-devel] Purpose of interface version checks in server/reds.c

[Uri Lublin]

On 07/05/2011 11:57 AM, Christophe Fergeau wrote:
> On Mon, Jul 04, 2011 at 02:37:09PM +0200, Christophe Fergeau wrote:
>> On Mon, Jul 04, 2011 at 01:27:16PM +0200, Gerd Hoffmann wrote:
>>> On 07/01/11 15:58, Christophe Fergeau wrote:
>>>> if (strcmp(interface->type, SPICE_INTERFACE_RECORD) == 0) {
>>>>      red_printf("SPICE_INTERFACE_RECORD");
>>>>      if (interface->major_version != SPICE_INTERFACE_RECORD_MAJOR ||
>>>>          interface->minor_version<   SPICE_INTERFACE_RECORD_MINOR) {
>>>>          red_printf("unsupported record interface");
>>>>          return -1;
>>>>      }
>>>>      snd_attach_record(SPICE_CONTAINEROF(sin, SpiceRecordInstance, base));
>>>> }
>>>
>>> That looks bogous.
>>
>> Given your explanation below, I agree with you. I'm wondering if what was
>> originally intended was the opposite check, ie fail to create the channel
>> if QEMU supports a *newer* version of the interface than the installed
>> spice-server. Otherwise I'll make a patch to
>> - fail the channel creation if interface->major_version !=
>>    SPICE_INTERFACE_RECORD_MAJOR
>> - only log a message if interface->minor_version<
>>    SPICE_INTERFACE_RECORD_MINOR) (but create the channel anyway)
>
> Does anyone have any thoughts on what was intended in this code?

In previous/early versions, mostly spice-server was calling qemu-kvm's 
functions. Nowadays mostly qemu-kvm calls spice-server's functions. Just a 
thought that may explain it. Or maybe a typo in the inequality sign.

In Gerd's example there is also a version check in the code (which makes it 
safe), as the function that was added is a qemu-kvm function called by spice-server.

I think we do want the opposite check, as you mentioned.

Probably worth fixing other channels too.