On Wed, 2016-11-02 at 13:05 -0400, Frediano Ziglio wrote: > > > > > > On Wed, 2016-11-02 at 08:31 -0400, Frediano Ziglio wrote: > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > On Wed, Nov 02, 2016 at 04:34:47AM -0400, Frediano Ziglio > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > From: Jonathon Jongsma <jjongsma@xxxxxxxxxx> > > > > > > > > > > > > Store the channel in the char device object, and unref it > > > > > > in > > > > > > dispose. > > > > > > Previously, we were just creating a channel and allowing it > > > > > > to > > > > > > leak. > > > > > > There may be better long-term solutions, but this works for > > > > > > now. > > > > > > --- > > > > > > server/spicevmc.c | 27 ++++++++++++++++++++------- > > > > > > 1 file changed, 20 insertions(+), 7 deletions(-) > > > > > > > > > > > > Changes: > > > > > > - squashed fixup. > > > > > > > > > > > > diff --git a/server/spicevmc.c b/server/spicevmc.c > > > > > > index 7067bc7..a77e868 100644 > > > > > > --- a/server/spicevmc.c > > > > > > +++ b/server/spicevmc.c > > > > > > @@ -47,6 +47,9 @@ > > > > > > #define BUF_SIZE (64 * 1024 + 32) > > > > > > #define COMPRESS_THRESHOLD 1000 > > > > > > > > > > > > +typedef struct RedVmcChannel RedVmcChannel; > > > > > > +typedef struct RedVmcChannelClass RedVmcChannelClass; > > > > > > + > > > > > > typedef struct RedVmcPipeItem { > > > > > > RedPipeItem base; > > > > > > > > > > > > @@ -75,6 +78,7 @@ typedef struct RedCharDeviceSpiceVmcClass > > > > > > RedCharDeviceSpiceVmcClass; > > > > > > > > > > > > struct RedCharDeviceSpiceVmc { > > > > > > RedCharDevice parent; > > > > > > + RedVmcChannel *channel; > > > > > > }; > > > > > > > > > > > > struct RedCharDeviceSpiceVmcClass > > > > > > @@ -89,9 +93,6 @@ static RedCharDevice > > > > > > *red_char_device_spicevmc_new(SpiceCharDeviceInstance *sin, > > > > > > > > > > > > G_DEFINE_TYPE(RedCharDeviceSpiceVmc, > > > > > > red_char_device_spicevmc, > > > > > > RED_TYPE_CHAR_DEVICE) > > > > > > > > > > > > -#define RED_CHAR_DEVICE_SPICEVMC_PRIVATE(o) \ > > > > > > - (G_TYPE_INSTANCE_GET_PRIVATE ((o), > > > > > > RED_TYPE_CHAR_DEVICE_SPICEVMC, > > > > > > RedCharDeviceSpiceVmcPrivate)) > > > > > > - > > > > > > #define RED_TYPE_VMC_CHANNEL red_vmc_channel_get_type() > > > > > > > > > > > > #define RED_VMC_CHANNEL(obj) \ > > > > > > @@ -103,9 +104,6 @@ G_DEFINE_TYPE(RedCharDeviceSpiceVmc, > > > > > > red_char_device_spicevmc, RED_TYPE_CHAR_DEV > > > > > > #define RED_VMC_CHANNEL_GET_CLASS(obj) \ > > > > > > (G_TYPE_INSTANCE_GET_CLASS((obj), > > > > > > RED_TYPE_VMC_CHANNEL, > > > > > > RedVmcChannelClass)) > > > > > > > > > > > > -typedef struct RedVmcChannel RedVmcChannel; > > > > > > -typedef struct RedVmcChannelClass RedVmcChannelClass; > > > > > > - > > > > > > struct RedVmcChannel > > > > > > { > > > > > > RedChannel parent; > > > > > > @@ -855,9 +853,13 @@ RedCharDevice > > > > > > *spicevmc_device_connect(RedsState > > > > > > *reds, > > > > > > SpiceCharDeviceInst > > > > > > ance > > > > > > *sin, > > > > > > uint8_t > > > > > > channel_type) > > > > > > { > > > > > > + RedCharDeviceSpiceVmc *dev_state; > > > > > > RedVmcChannel *state = red_vmc_channel_new(reds, > > > > > > channel_type, sin); > > > > > > > > > > > > - return state->chardev; > > > > > > + dev_state = RED_CHAR_DEVICE_SPICEVMC(state->chardev); > > > > > > + dev_state->channel = state; > > > > > > + > > > > > > + return RED_CHAR_DEVICE(dev_state); > > > > > > } > > > > > > > > > > > > /* Must be called from RedClient handling thread. */ > > > > > > @@ -904,10 +906,21 @@ SPICE_GNUC_VISIBLE void > > > > > > spice_server_port_event(SpiceCharDeviceInstance *sin, ui > > > > > > } > > > > > > > > > > > > static void > > > > > > +red_char_device_spicevmc_dispose(GObject *object) > > > > > > +{ > > > > > > + RedCharDeviceSpiceVmc *self = > > > > > > RED_CHAR_DEVICE_SPICEVMC(object); > > > > > > + > > > > > > + g_clear_object(&self->channel); > > > > > > > > > > This call produce a double free. > > > > > The channel is already freed in spicevmc_device_disconnect. > > > > > > > > But g_clear_object() verify if channel is null before the unref > > > > and > > > > set > > > > it to NULL afterwards. > > > > > > > > > > Doing a NULL check on a dangling pointer is not much helpful. > > > > Hmm, you're right. If we're going to remove the 'opaque' property > > (in a > > different patch), I think we do need to keep a pointer to the > > channel > > inside of the char device. But perhaps it should be a weak > > reference > > instead. So maybe we could just remove the dispose changes from > > this > > patch and squash it with that patch? > > > > Yes, I think we should kind of merge part of this patch in the other > one. > This could also be fixed setting the pointer to NULL after calling > spicevmc_device_disconnect. > > > > > Also: perhaps the RedsState object should hold a reference to the > > channels added via reds_register_channel() and unref the channel in > > reds_unregister_channel(). It seems that it shouldn't be necessary > > to > > call red_channel_destroy() explicitly. This should just happen in > > the > > RedChannel destructor when the last ref is dropped... > > > > > > There is already a patch in the queue that does this; you wrote it :) > (cfr "RedsState: keep weak reference to channels"). Yeah, I actually wrote a follow-up email saying the exact same thing right after I wrote the one you're quoting ;) Except I forgot to send it. It's been too long since I wrote these patches, I don't even remember what they are anymore :) > This patch does not seem really in line with your proposal for > spice_server_destroy/reds_exit changes. Yeah, perhaps I need to take a step back and evaluate things anew now that we're basically at the end of the refactory merging. > > I was going to reply to it. There are some issues: > - reds_channel_destroyed is called by RedChannel destructor > (dispose/finalize); > As channels is not protected by a lock this means that all calls to > reds_channel_destroyed (so in theory all calls to g_object_unref to > RedChannel hierarchy) has to be done in the "main" thread... or we > need to > add locking! It's easy to check reds_unregister_channel calls, a > bit less > g_object_refs; > - is it the right time to remove the item from the list or is too > late? > Potentially the list is scanned when a new connection arrive to > decide which > RedChannel will handle the connection (and create a new > RedChannelClient). > Not saying that is wrong, surely will avoid dandling pointers but > some race > may occur. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > +} > > > > > > + > > > > > > +static void > > > > > > red_char_device_spicevmc_class_init(RedCharDeviceSpiceVmcC > > > > > > lass > > > > > > *klass) > > > > > > { > > > > > > + GObjectClass *object_class = G_OBJECT_CLASS(klass); > > > > > > RedCharDeviceClass *char_dev_class = > > > > > > RED_CHAR_DEVICE_CLASS(klass); > > > > > > > > > > > > + object_class->dispose = > > > > > > red_char_device_spicevmc_dispose; > > > > > > + > > > > > > char_dev_class->read_one_msg_from_device = > > > > > > spicevmc_chardev_read_msg_from_dev; > > > > > > char_dev_class->send_msg_to_client = > > > > > > spicevmc_chardev_send_msg_to_client; > > > > > > char_dev_class->send_tokens_to_client = > > > > > > spicevmc_char_dev_send_tokens_to_client; > > > > > > > > > > Removing the double free from this patch will basically > > > > > revert > > > > > the > > > > > entire patch except the RED_CHAR_DEVICE_SPICEVMC_PRIVATE > > > > > cleanup. > > > > > > > > > > Frediano _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel