[Spice-devel,v2,15/19] replay: Free spice server to detect leaks

Submitted by Frediano Ziglio on Nov. 25, 2016, 2:52 p.m.

Details

Message ID a890bcd8c033e3393b27b9834c6ddbd87fa0cb0a.1480085518.git-series.fziglio@redhat.com
State Superseded
Headers show
Series "Start cleaning objects on destruction" ( rev: 4 3 2 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio Nov. 25, 2016, 2:52 p.m.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 server/tests/replay.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/server/tests/replay.c b/server/tests/replay.c
index f32fa2f..23d4125 100644
--- a/server/tests/replay.c
+++ b/server/tests/replay.c
@@ -221,7 +221,6 @@  static void end_replay(void)
         kill(client_pid, SIGINT);
         waitpid(client_pid, &child_status, 0);
     }
-    exit(0);
 }
 
 static void release_resource(QXLInstance *qin, struct QXLReleaseInfoExt release_info)
@@ -440,6 +439,7 @@  int main(int argc, char **argv)
     if (print_count)
         g_print("Counted %d commands\n", ncommands);
 
+    spice_server_destroy(server);
     end_replay();
     g_async_queue_unref(display_queue);
     g_async_queue_unref(cursor_queue);

Comments

On Fri, 2016-11-25 at 14:52 +0000, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
> ---
>  server/tests/replay.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/server/tests/replay.c b/server/tests/replay.c
> index f32fa2f..23d4125 100644
> --- a/server/tests/replay.c
> +++ b/server/tests/replay.c
> @@ -221,7 +221,6 @@ static void end_replay(void)
>          kill(client_pid, SIGINT);
>          waitpid(client_pid, &child_status, 0);
>      }
> -    exit(0);
>  }
>  
>  static void release_resource(QXLInstance *qin, struct
> QXLReleaseInfoExt release_info)
> @@ -440,6 +439,7 @@ int main(int argc, char **argv)
>      if (print_count)
>          g_print("Counted %d commands\n", ncommands);
>  
> +    spice_server_destroy(server);
>      end_replay();
>      g_async_queue_unref(display_queue);
>      g_async_queue_unref(cursor_queue);
> 
> On Fri, 2016-11-25 at 14:52 +0000, Frediano Ziglio wrote:
> > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> Acked-by: Pavel Grunt <pgrunt@redhat.com>
> > ---
> >  server/tests/replay.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/server/tests/replay.c b/server/tests/replay.c
> > index f32fa2f..23d4125 100644
> > --- a/server/tests/replay.c
> > +++ b/server/tests/replay.c
> > @@ -221,7 +221,6 @@ static void end_replay(void)
> >          kill(client_pid, SIGINT);
> >          waitpid(client_pid, &child_status, 0);
> >      }
> > -    exit(0);
> >  }
> >  
> >  static void release_resource(QXLInstance *qin, struct
> > QXLReleaseInfoExt release_info)
> > @@ -440,6 +439,7 @@ int main(int argc, char **argv)
> >      if (print_count)
> >          g_print("Counted %d commands\n", ncommands);
> >  
> > +    spice_server_destroy(server);
> >      end_replay();
> >      g_async_queue_unref(display_queue);
> >      g_async_queue_unref(cursor_queue);
> 

Are you suggesting I merge these patches (15/19 and 18/19) before
the patches that implements the cleaning?

Frediano
On Tue, 2016-11-29 at 04:03 -0500, Frediano Ziglio wrote:
> > 
> > On Fri, 2016-11-25 at 14:52 +0000, Frediano Ziglio wrote:
> > > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > 
> > Acked-by: Pavel Grunt <pgrunt@redhat.com>
> > > ---
> > >  server/tests/replay.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/server/tests/replay.c b/server/tests/replay.c
> > > index f32fa2f..23d4125 100644
> > > --- a/server/tests/replay.c
> > > +++ b/server/tests/replay.c
> > > @@ -221,7 +221,6 @@ static void end_replay(void)
> > >          kill(client_pid, SIGINT);
> > >          waitpid(client_pid, &child_status, 0);
> > >      }
> > > -    exit(0);
> > >  }
> > >  
> > >  static void release_resource(QXLInstance *qin, struct
> > > QXLReleaseInfoExt release_info)
> > > @@ -440,6 +439,7 @@ int main(int argc, char **argv)
> > >      if (print_count)
> > >          g_print("Counted %d commands\n", ncommands);
> > >  
> > > +    spice_server_destroy(server);
> > >      end_replay();
> > >      g_async_queue_unref(display_queue);
> > >      g_async_queue_unref(cursor_queue);
> 
> Are you suggesting I merge these patches (15/19 and 18/19) before
> the patches that implements the cleaning?

if the patches are sent intentionally in series than they should be
merged in the series - author must know :) I will review rest of the
patches today

About these two i think merging them is safe - it may actually show
the difference in the leak

Pavel

> 
> Frediano
> 
> On Tue, 2016-11-29 at 04:03 -0500, Frediano Ziglio wrote:
> > > 
> > > On Fri, 2016-11-25 at 14:52 +0000, Frediano Ziglio wrote:
> > > > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > > 
> > > Acked-by: Pavel Grunt <pgrunt@redhat.com>
> > > > ---
> > > >  server/tests/replay.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/server/tests/replay.c b/server/tests/replay.c
> > > > index f32fa2f..23d4125 100644
> > > > --- a/server/tests/replay.c
> > > > +++ b/server/tests/replay.c
> > > > @@ -221,7 +221,6 @@ static void end_replay(void)
> > > >          kill(client_pid, SIGINT);
> > > >          waitpid(client_pid, &child_status, 0);
> > > >      }
> > > > -    exit(0);
> > > >  }
> > > >  
> > > >  static void release_resource(QXLInstance *qin, struct
> > > > QXLReleaseInfoExt release_info)
> > > > @@ -440,6 +439,7 @@ int main(int argc, char **argv)
> > > >      if (print_count)
> > > >          g_print("Counted %d commands\n", ncommands);
> > > >  
> > > > +    spice_server_destroy(server);
> > > >      end_replay();
> > > >      g_async_queue_unref(display_queue);
> > > >      g_async_queue_unref(cursor_queue);
> > 
> > Are you suggesting I merge these patches (15/19 and 18/19) before
> > the patches that implements the cleaning?
> 
> if the patches are sent intentionally in series than they should be
> merged in the series - author must know :) I will review rest of the
> patches today
> 
> About these two i think merging them is safe - it may actually show
> the difference in the leak
> 
> Pavel
> 

Yes, I was thinking about your previous comment on another patch
about merging a function without being used.

On a first though I was thinking these patch could be merged before
so to see the improvements. But on a second though at least we should
merge before the patches that close the worker thread or you'll get
crashes here and there.

Frediano
On Tue, 2016-11-29 at 04:37 -0500, Frediano Ziglio wrote:
> > 
> > On Tue, 2016-11-29 at 04:03 -0500, Frediano Ziglio wrote:
> > > > 
> > > > On Fri, 2016-11-25 at 14:52 +0000, Frediano Ziglio wrote:
> > > > > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > > > 
> > > > Acked-by: Pavel Grunt <pgrunt@redhat.com>
> > > > > ---
> > > > >  server/tests/replay.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/server/tests/replay.c b/server/tests/replay.c
> > > > > index f32fa2f..23d4125 100644
> > > > > --- a/server/tests/replay.c
> > > > > +++ b/server/tests/replay.c
> > > > > @@ -221,7 +221,6 @@ static void end_replay(void)
> > > > >          kill(client_pid, SIGINT);
> > > > >          waitpid(client_pid, &child_status, 0);
> > > > >      }
> > > > > -    exit(0);
> > > > >  }
> > > > >  
> > > > >  static void release_resource(QXLInstance *qin, struct
> > > > > QXLReleaseInfoExt release_info)
> > > > > @@ -440,6 +439,7 @@ int main(int argc, char **argv)
> > > > >      if (print_count)
> > > > >          g_print("Counted %d commands\n", ncommands);
> > > > >  
> > > > > +    spice_server_destroy(server);
> > > > >      end_replay();
> > > > >      g_async_queue_unref(display_queue);
> > > > >      g_async_queue_unref(cursor_queue);
> > > 
> > > Are you suggesting I merge these patches (15/19 and 18/19)
> > > before
> > > the patches that implements the cleaning?
> > 
> > if the patches are sent intentionally in series than they should
> > be
> > merged in the series - author must know :) I will review rest of
> > the
> > patches today
> > 
> > About these two i think merging them is safe - it may actually
> > show
> > the difference in the leak
> > 
> > Pavel
> > 
> 
> Yes, I was thinking about your previous comment on another patch
> about merging a function without being used.

Here it is used, no ?
> 
> On a first though I was thinking these patch could be merged before
> so to see the improvements. But on a second though at least we
> should
> merge before the patches that close the worker thread or you'll get
> crashes here and there.

If there is a risk of crashing than rather wait with merging to avoid
potential future issues when bisecting 

Pavel

> 
> Frediano