[spice-gtk,v1,2/3] main: migration: check return value to count channels

Submitted by Victor Toso on Sept. 4, 2019, 12:20 p.m.

Details

Message ID 20190904122041.23408-2-victortoso@redhat.com
State Accepted
Commit d77b2db80c246b57beec15c257511bb03e4faf41
Headers show
Series "Series without cover letter" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Sept. 4, 2019, 12:20 p.m.
From: Victor Toso <me@victortoso.com>

Although spice_channel_connect() works in idle, if it returns false
it'll not emit any signal further and we would be counting a
'connected' channel that wouldn't be emitting anything.

As other callbacks take this in consideration, we should only
increment the counter if we reached spice-channel::connect_delayed()
callback.

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 src/channel-main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/channel-main.c b/src/channel-main.c
index a1e5498..3d1b1b5 100644
--- a/src/channel-main.c
+++ b/src/channel-main.c
@@ -2174,8 +2174,9 @@  migrate_channel_connect(spice_migrate *mig, int type, int id)
     SPICE_DEBUG("migrate_channel_connect %d:%d", type, id);
 
     SpiceChannel *newc = spice_channel_new(mig->session, type, id);
-    spice_channel_connect(newc);
-    mig->nchannels++;
+    if (newc != NULL && spice_channel_connect(newc)) {
+        mig->nchannels++;
+    }
 }
 
 /* coroutine context */

Comments

> 
> From: Victor Toso <me@victortoso.com>
> 
> Although spice_channel_connect() works in idle, if it returns false
> it'll not emit any signal further and we would be counting a
> 'connected' channel that wouldn't be emitting anything.
> 
> As other callbacks take this in consideration, we should only
> increment the counter if we reached spice-channel::connect_delayed()
> callback.
> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  src/channel-main.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/channel-main.c b/src/channel-main.c
> index a1e5498..3d1b1b5 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -2174,8 +2174,9 @@ migrate_channel_connect(spice_migrate *mig, int type,
> int id)
>      SPICE_DEBUG("migrate_channel_connect %d:%d", type, id);
>  
>      SpiceChannel *newc = spice_channel_new(mig->session, type, id);
> -    spice_channel_connect(newc);
> -    mig->nchannels++;
> +    if (newc != NULL && spice_channel_connect(newc)) {
> +        mig->nchannels++;
> +    }
>  }
>  
>  /* coroutine context */

Acked-by: Frediano Ziglio <fziglio@redhat.com>

It makes sense but if spice_channel_connect for any reason fails the channel
will go to unconnected state and we will "forget" the pointer so basically
looks like a leak.
At least I would expect that if I attempt to disconnect it the object
is freed.

Frediano