> Store the channel in the char device object, and unref it in dispose. > Previously, we were just creating a channel and potentially allowing it > to leak. There may be better long-term solutions, but this works for > now. > --- > Changes from last version: > - set channel to NULL in spicevmc_device_disconnect() to avoid the > double-free > mentioned by Frediano > I was considering 1/3 and 3/3 patches. Would make sense to have just the spicevmc_device_disconnect (and similar smartcard) to just unref the device however g_clear_object on chardev is not enough: - device destruction should call reds_unregister_channel to prevent possible future RedChannelClient to connect to the channel; - destruction should call red_channel_destroy and not just g_object_unref (called by g_clear_object) to close current clients (no reasons to have client to handle closed devices). Note that the above cannot/shouldn't be moved to the channel destructor as there will be external (like from RedChannelClient) reference to the channel that will prevent these stuff to be executed. Possibly there could be a red_char_device_clear_channel(RedChannel **p_channel) utility that check NULL pointer and do all these stuff to be called by Smartcard/SpiceVmc instead of g_clear_pointer. > server/spicevmc.c | 46 ++++++++++++++++++++++++++++++---------------- > 1 file changed, 30 insertions(+), 16 deletions(-) > > diff --git a/server/spicevmc.c b/server/spicevmc.c > index 7067bc7..8e23248 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,28 +853,33 @@ RedCharDevice *spicevmc_device_connect(RedsState *reds, > SpiceCharDeviceInstance *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. */ > void spicevmc_device_disconnect(RedsState *reds, SpiceCharDeviceInstance > *sin) > { > - RedVmcChannel *state; > + RedVmcChannel *channel; > + RedCharDeviceSpiceVmc *vmc = RED_CHAR_DEVICE_SPICEVMC(sin->st); > > - /* FIXME */ > - state = (RedVmcChannel > *)red_char_device_opaque_get((RedCharDevice*)sin->st); > + channel = vmc->channel; > + vmc->channel = NULL; > > - red_char_device_write_buffer_release(state->chardev, > &state->recv_from_client_buf); > + red_char_device_write_buffer_release(channel->chardev, > &channel->recv_from_client_buf); > /* FIXME */ > - red_char_device_destroy((RedCharDevice*)sin->st); > - state->chardev = NULL; > + red_char_device_destroy(RED_CHAR_DEVICE(vmc)); > + channel->chardev = NULL; > sin->st = NULL; > > - reds_unregister_channel(reds, RED_CHANNEL(state)); > - free(state->pipe_item); > - red_channel_destroy(RED_CHANNEL(state)); > + reds_unregister_channel(reds, RED_CHANNEL(channel)); > + free(channel->pipe_item); > + red_channel_destroy(RED_CHANNEL(channel)); > } > > SPICE_GNUC_VISIBLE void spice_server_port_event(SpiceCharDeviceInstance > *sin, uint8_t event) > @@ -904,10 +907,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); > +} > + > +static void > red_char_device_spicevmc_class_init(RedCharDeviceSpiceVmcClass *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; Otherwise the patch is good. My proposal can apply to this patch as a change or future change either. Frediano _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel