Re: [PATCH] remove dandling pointer for RedCharDeviceVDIPort

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> Hey,
> 
> On Tue, May 03, 2016 at 06:53:49AM -0400, Frediano Ziglio wrote:
> > 
> > Honestly more I look at the patch and this fix and more I think it's all
> > a big bug...
> > What I know for sure is that this patch fix a dandling pointer.
> 
> NB: 'dangling' not 'dandling' (there is this typo in the short log,
> would be nice to fix it ;)
> 

Ok, should I post another version.

> > 
> > Coming to your question. agent_attached. Should be a well defined things,
> > there is an agent in the guest attached to spice-server. Now... can we have
> > more agents or this "agent_attached" is referring to the agent daemon
> > instead
> > of the an agent? All code seems to not to handle multiple agents. I don't
> > understand if this is a bug/missing feature or agent here refers to the
> > daemon
> > which handle multiple agents.
> > Handling the agent_attached. Previous code delete the RedCharDevice before
> > setting agent_attached to FALSE. This does not make much sense to me! The
> > RedCharDevice represents a device created by Qemu (in the normal usage of
> > spice-server) so there is no sense in deleting it when an agent is
> > detached.
> > Also looking at the if:
> 
> For what it's worth, 'agent_attached' was added in
> https://cgit.freedesktop.org/spice/spice/commit/?id=1cec1c5118b65
> "reds: Make VDIPortState a GObject".
> Before that, we had
>  struct RedCharDeviceVDIPortPrivate {
>      RedCharDevice *base;
>      /* ... */
> }
> and 'base' and RedCharDeviceVDIPortPrivate had different lifecycles
> ('base' could be NULL with a non-NULL RedChanDeviceVDIPortPrivate)
> agent_attached was an attempt at handling this differently, which
> apparently is not good enough :(

Yes, the commit id is in the commit message too.
I'm not sure if the current code is worst or better than the previous.
As I said in my comments the RedCharDevice lifespan should be related to
the device as the current code. For the other stuff I don't have any
clear opinion, I don't know the code that much and looks quite
complicated.

> 
> Christophe
> 

Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]