[Spice-devel] cc47d9eba84d33012da660d7dcee3a6e3eb8d99f caused regression

[Frediano Ziglio]

> 
> On Thu, 2016-06-02 at 05:16 -0400, Frediano Ziglio wrote:
> > Hi,
> >   I think this was the start of the regression that basically caused
> > the regression introduced by d232e92794c74a326d1e48355d257f24a6d497c2
> > ("Use GQueue for RedCharDevice::send_queue").
> > 
> > Basically on cc47d9eba84d33012da660d7dcee3a6e3eb8d99f
> > ("reds: Derive VDIPortReadBuf from PipeItem") VDIPortReadBuf was derived
> > from PipeItem (now RedPipeItem) mostly to reuse reference counting
> > (and possibly free). However this was propagated to RedCharDevice
> > where read_one_msg_from_device and send_msg_to_client use RedPipeItems
> > but leaded to imagine that RedPipeItems could be inserted in multiple
> > client pipes which is basically would cause a spice_assert.
> > As said in
> > https://lists.freedesktop.org/archives/spice-devel/2016-May/029204.
> > html
> > this does not cause the crash as multiple clients is not supported
> > however I think this as a bad design. Simply we should have a class
> > to represent a data buffer with:
> > - reference counting;
> > - embedded destroy.
> > This would simplify CharDevice and agent code and avoid the wrong
> > assumption that caused d232e92794c74a326d1e48355d257f24a6d497c2.
> > This will probably rollback some code and introduce back a RedPipeItem
> > however I think all this was and will be the right way. "Was" as all these
> > changes allowed some callbacks and many code to be removed or simplified.
> > "Will" as it's the shortest path to remove regression (which IMHO should
> > be short as possible).
> > 
> > Frediano
> > 
> 
> 
> Can you explain what regression you're talking about here?
> 
> Jonathon
> 

If multiple clients connect server will crash as the same
RedPipeItem will be attempted to be inserted in multiple
pipes.
Before different RedPipeItems were created for each clients.
Actually this is theory as all RedCharDevice are far to
support multiple clients.

Note the comment in char-device.h:

/*
 * Note about multiple-clients:
 * Multiclients are currently not supported in any of the character devices:
 * spicevmc does not allow more than one client (and at least for usb, it should stay this way).
 * smartcard code is not compatible with more than one reader.
 * The server and guest agent code doesn't distinguish messages from different clients.
 * In addition, its current flow control code (e.g., tokens handling) is wrong and doesn't
 * take into account the different clients.
 *
...

Not really clear how multi client should work for RedCharDevices...

Frediano