On Wed, Jul 11, 2018 at 05:30:25PM +0100, Frediano Ziglio wrote:
> Leak detectors did not manage to find leaks, possibly as double list
> have all elements likely with a pointer to them.
> The reference from the agent is necessary for inserting it into
> the list.
>
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
> server/reds.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/server/reds.c b/server/reds.c
> index f1e34529a..85043a88d 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3132,6 +3132,7 @@ static int spice_server_char_device_add_interface(SpiceServer *reds,
> return -1;
> }
> dev_state = attach_to_red_agent(reds, char_device);
> + g_object_ref(dev_state);
This g_object_ref() looks a bit out of place to be honest. It's there
so that attach_to_red_agent() behaves similarly to the
xxx_device_connect() calls in that we want to own a reference to
dev_state. Imo it would make more sense to move the g_object_ref()
inside attach_to_red_agent() as it's an implementation detail of that
method that we return the same object rather than creating a new one.
Actually, that whole thing reds->char_devices which somehow owns a ref,
but that ref is released in
spicevmc_device_disconnect/smartcard_device_disconnect even if their
implementations never took/owned that ref is quite convoluted. The
attache patch cleans up things imo, but I've only compile-tested it, so
it's likely broken..
> }
> #ifdef USE_SMARTCARD
> else if (strcmp(char_device->subtype, SUBTYPE_SMARTCARD) == 0) {
> @@ -3682,6 +3683,19 @@ SPICE_GNUC_VISIBLE void spice_server_destroy(SpiceServer *reds)
> }
> reds_cleanup_net(reds);
> g_clear_object(&reds->agent_dev);
> +
> + // NOTE: don't replace with g_list_free_full as this function that passed callback
> + // don't change the list while g_object_unref in this case will change it.
> + RedCharDevice *dev;
> + GLIST_FOREACH(reds->char_devices, RedCharDevice, dev) {
> + g_object_unref(dev);
> + }
> + g_list_free(reds->char_devices);
> + reds->char_devices = NULL;
> +
> + g_list_free(reds->channels);
> + reds->channels = NULL;
If reds->channels is not empty, this means we did not call
reds_unregister_channel, and so we'll actually be leaking what is in the
list. At the moment, it's odd that SpiceServer owns a ref to the input
and main channels, but not to the other channels in reds->channels. It
seems it should be possible to add reds_{register,unregister}_channel()
to take ownership of the channels so that reds->channels also owns a
ref.
Christophe
diff --git a/server/char-device.c b/server/char-device.c
index ae538fab1..94bbe15c0 100644
--- a/server/char-device.c
+++ b/server/char-device.c
@@ -694,12 +694,6 @@ void red_char_device_reset_dev_instance(RedCharDevice *dev,
g_object_notify(G_OBJECT(dev), "sin");
}
-void red_char_device_destroy(RedCharDevice *char_dev)
-{
- g_return_if_fail(RED_IS_CHAR_DEVICE(char_dev));
- g_object_unref(char_dev);
-}
-
static RedCharDeviceClient *red_char_device_client_new(RedClient *client,
int do_flow_control,
uint32_t max_send_queue_size,
diff --git a/server/reds.c b/server/reds.c
index 85043a88d..e44fac264 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -3025,7 +3025,7 @@ static RedCharDevice *attach_to_red_agent(RedsState *reds, SpiceCharDeviceInstan
}
if (!reds_main_channel_connected(reds)) {
- return RED_CHAR_DEVICE(dev);
+ return g_object_ref(RED_CHAR_DEVICE(dev));
}
dev->priv->read_filter.discard_all = FALSE;
@@ -3073,7 +3073,7 @@ static RedCharDevice *attach_to_red_agent(RedsState *reds, SpiceCharDeviceInstan
main_channel_push_agent_connected(reds->main_channel);
}
- return RED_CHAR_DEVICE(dev);
+ return g_object_ref(RED_CHAR_DEVICE(dev));
}
SPICE_GNUC_VISIBLE void spice_server_char_device_wakeup(SpiceCharDeviceInstance* sin)
@@ -3106,16 +3106,16 @@ SPICE_GNUC_VISIBLE const char** spice_server_char_device_recognized_subtypes(voi
static void reds_add_char_device(RedsState *reds, RedCharDevice *dev)
{
- reds->char_devices = g_list_append(reds->char_devices, dev);
+ reds->char_devices = g_list_append(reds->char_devices, g_object_ref(dev));
}
-static void reds_on_char_device_destroy(RedsState *reds,
- RedCharDevice *dev)
+static void reds_remove_char_device(RedsState *reds, RedCharDevice *dev)
{
g_return_if_fail(reds != NULL);
g_warn_if_fail(g_list_find(reds->char_devices, dev) != NULL);
reds->char_devices = g_list_remove(reds->char_devices, dev);
+ g_object_unref(dev);
}
static int spice_server_char_device_add_interface(SpiceServer *reds,
@@ -3132,7 +3132,6 @@ static int spice_server_char_device_add_interface(SpiceServer *reds,
return -1;
}
dev_state = attach_to_red_agent(reds, char_device);
- g_object_ref(dev_state);
}
#ifdef USE_SMARTCARD
else if (strcmp(char_device->subtype, SUBTYPE_SMARTCARD) == 0) {
@@ -3160,15 +3159,13 @@ static int spice_server_char_device_add_interface(SpiceServer *reds,
* just a sanity check to ensure that assumption is correct */
spice_assert(dev_state == char_device->st);
- g_object_weak_ref(G_OBJECT(dev_state),
- (GWeakNotify)reds_on_char_device_destroy,
- reds);
/* setting the char_device state to "started" for backward compatibily with
* qemu releases that don't call spice api for start/stop (not implemented yet) */
if (reds->vm_running) {
red_char_device_start(dev_state);
}
reds_add_char_device(reds, dev_state);
+ g_object_unref(dev_state);
} else {
spice_warning("failed to create device state for %s", char_device->subtype);
return -1;
@@ -3188,19 +3185,14 @@ static int spice_server_char_device_remove_interface(RedsState *reds, SpiceBaseI
reds_agent_remove(reds);
red_char_device_reset_dev_instance(RED_CHAR_DEVICE(reds->agent_dev), NULL);
}
- }
+ } else if (strcmp(char_device->subtype, SUBTYPE_USBREDIR) != 0 &&
#ifdef USE_SMARTCARD
- else if (strcmp(char_device->subtype, SUBTYPE_SMARTCARD) == 0) {
- smartcard_device_disconnect(char_device);
- }
+ strcmp(char_device->subtype, SUBTYPE_SMARTCARD) != 0 &&
#endif
- else if (strcmp(char_device->subtype, SUBTYPE_USBREDIR) == 0 ||
- strcmp(char_device->subtype, SUBTYPE_PORT) == 0) {
- spicevmc_device_disconnect(reds, char_device);
- } else {
- spice_warning("failed to remove char device %s", char_device->subtype);
+ strcmp(char_device->subtype, SUBTYPE_PORT) != 0) {
+ g_warning("failed to remove char device %s", char_device->subtype);
}
-
+ reds_remove_char_device(reds, RED_CHAR_DEVICE(char_device->st));
char_device->st = NULL;
return 0;
}
@@ -3684,13 +3676,7 @@ SPICE_GNUC_VISIBLE void spice_server_destroy(SpiceServer *reds)
reds_cleanup_net(reds);
g_clear_object(&reds->agent_dev);
- // NOTE: don't replace with g_list_free_full as this function that passed callback
- // don't change the list while g_object_unref in this case will change it.
- RedCharDevice *dev;
- GLIST_FOREACH(reds->char_devices, RedCharDevice, dev) {
- g_object_unref(dev);
- }
- g_list_free(reds->char_devices);
+ g_list_free_full(reds->char_devices, g_object_unref);
reds->char_devices = NULL;
g_list_free(reds->channels);
diff --git a/server/smartcard.c b/server/smartcard.c
index 2cb68e066..b7e111ecf 100644
--- a/server/smartcard.c
+++ b/server/smartcard.c
@@ -284,13 +284,6 @@ static RedCharDeviceSmartcard *smartcard_device_new(RedsState *reds, SpiceCharDe
return RED_CHAR_DEVICE_SMARTCARD(char_dev);
}
-void smartcard_device_disconnect(SpiceCharDeviceInstance *char_device)
-{
- g_return_if_fail(RED_IS_CHAR_DEVICE_SMARTCARD(char_device->st));
-
- g_object_unref(char_device->st);
-}
-
RedCharDevice *smartcard_device_connect(RedsState *reds, SpiceCharDeviceInstance *char_device)
{
RedCharDeviceSmartcard *dev;
diff --git a/server/smartcard.h b/server/smartcard.h
index f0b4fa440..7ee09fa5d 100644
--- a/server/smartcard.h
+++ b/server/smartcard.h
@@ -45,7 +45,6 @@ struct RedCharDeviceSmartcardClass
* connect to smartcard interface, used by smartcard channel
*/
RedCharDevice *smartcard_device_connect(RedsState *reds, SpiceCharDeviceInstance *char_device);
-void smartcard_device_disconnect(SpiceCharDeviceInstance *char_device);
void smartcard_channel_write_to_reader(RedCharDeviceWriteBuffer *write_buf);
SpiceCharDeviceInstance* smartcard_readers_get(uint32_t reader_id);
SpiceCharDeviceInstance *smartcard_readers_get_unattached(void);
diff --git a/server/spicevmc.c b/server/spicevmc.c
index c2de5037f..abd5d91b6 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -824,13 +824,6 @@ RedCharDevice *spicevmc_device_connect(RedsState *reds,
return dev;
}
-/* Must be called from RedClient handling thread. */
-void spicevmc_device_disconnect(RedsState *reds, SpiceCharDeviceInstance *sin)
-{
- g_object_unref(RED_CHAR_DEVICE(sin->st));
- sin->st = NULL;
-}
-
static void spicevmc_port_event(RedCharDevice *char_dev, uint8_t event)
{
RedVmcChannel *channel;
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel