[Spice-devel,v2,12/19] Support QXL remove on spice_server_remove_interface

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

Details

Message ID ad8aa029092f6909906c11e56e14754ee54cebc3.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/reds.c | 7 +++++++
 1 file changed, 7 insertions(+)

Patch hide | download patch | download mbox

diff --git a/server/reds.c b/server/reds.c
index bc0cc01..05afb7c 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -3321,6 +3321,13 @@  SPICE_GNUC_VISIBLE int spice_server_remove_interface(SpiceBaseInstance *sin)
         SpiceCharDeviceInstance *char_device = SPICE_CONTAINEROF(sin, SpiceCharDeviceInstance, base);
         reds = red_char_device_get_server(char_device->st);
         spice_server_char_device_remove_interface(reds, sin);
+    } else if (strcmp(interface->type, SPICE_INTERFACE_QXL) == 0) {
+        QXLInstance *qxl;
+
+        qxl = SPICE_CONTAINEROF(sin, QXLInstance, base);
+        reds = red_qxl_get_server(qxl->st);
+        reds->qxl_instances = g_list_remove(reds->qxl_instances, qxl);
+        red_qxl_destroy(qxl);
     } else {
         spice_warning("VD_INTERFACE_REMOVING unsupported");
         return -1;

Comments

Could be called in replay ? Or is it called indirectly ?

Pavel

On Fri, 2016-11-25 at 14:52 +0000, Frediano Ziglio wrote:
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  server/reds.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/server/reds.c b/server/reds.c
> index bc0cc01..05afb7c 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3321,6 +3321,13 @@ SPICE_GNUC_VISIBLE int
> spice_server_remove_interface(SpiceBaseInstance *sin)
>          SpiceCharDeviceInstance *char_device =
> SPICE_CONTAINEROF(sin, SpiceCharDeviceInstance, base);
>          reds = red_char_device_get_server(char_device->st);
>          spice_server_char_device_remove_interface(reds, sin);
> +    } else if (strcmp(interface->type, SPICE_INTERFACE_QXL) == 0) {
> +        QXLInstance *qxl;
> +
> +        qxl = SPICE_CONTAINEROF(sin, QXLInstance, base);
> +        reds = red_qxl_get_server(qxl->st);
> +        reds->qxl_instances = g_list_remove(reds->qxl_instances,
> qxl);
> +        red_qxl_destroy(qxl);
>      } else {
>          spice_warning("VD_INTERFACE_REMOVING unsupported");
>          return -1;
This most likely deserves a longer commit log explaining why we want
that (and maybe why it's not going to cause unexpected side effects in
existing code).

Christophe

On Fri, Nov 25, 2016 at 02:52:42PM +0000, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  server/reds.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/server/reds.c b/server/reds.c
> index bc0cc01..05afb7c 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3321,6 +3321,13 @@ SPICE_GNUC_VISIBLE int spice_server_remove_interface(SpiceBaseInstance *sin)
>          SpiceCharDeviceInstance *char_device = SPICE_CONTAINEROF(sin, SpiceCharDeviceInstance, base);
>          reds = red_char_device_get_server(char_device->st);
>          spice_server_char_device_remove_interface(reds, sin);
> +    } else if (strcmp(interface->type, SPICE_INTERFACE_QXL) == 0) {
> +        QXLInstance *qxl;
> +
> +        qxl = SPICE_CONTAINEROF(sin, QXLInstance, base);
> +        reds = red_qxl_get_server(qxl->st);
> +        reds->qxl_instances = g_list_remove(reds->qxl_instances, qxl);
> +        red_qxl_destroy(qxl);
>      } else {
>          spice_warning("VD_INTERFACE_REMOVING unsupported");
>          return -1;
> -- 
> git-series 0.9.1
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> Could be called in replay ? Or is it called indirectly ?
> 
> Pavel
> 

Yes and not. While I was writing this series spice-server-replay
was using it. However now this is done automatically by
"Free QXL instances in spice_server_destroy" so replay utility
has no reason to free the interface.

Frediano

> On Fri, 2016-11-25 at 14:52 +0000, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > ---
> >  server/reds.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/server/reds.c b/server/reds.c
> > index bc0cc01..05afb7c 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -3321,6 +3321,13 @@ SPICE_GNUC_VISIBLE int
> > spice_server_remove_interface(SpiceBaseInstance *sin)
> >          SpiceCharDeviceInstance *char_device =
> > SPICE_CONTAINEROF(sin, SpiceCharDeviceInstance, base);
> >          reds = red_char_device_get_server(char_device->st);
> >          spice_server_char_device_remove_interface(reds, sin);
> > +    } else if (strcmp(interface->type, SPICE_INTERFACE_QXL) == 0) {
> > +        QXLInstance *qxl;
> > +
> > +        qxl = SPICE_CONTAINEROF(sin, QXLInstance, base);
> > +        reds = red_qxl_get_server(qxl->st);
> > +        reds->qxl_instances = g_list_remove(reds->qxl_instances,
> > qxl);
> > +        red_qxl_destroy(qxl);
> >      } else {
> >          spice_warning("VD_INTERFACE_REMOVING unsupported");
> >          return -1;
>
Taking into account that in a following patch the object is destroyed
automatically with spice_server_destroy this patch get less important.

Currently QXL interfaces are added when physical card are added and
QXL cards don't support (in Qemu) hot swap so there is no reason to
have this feature. Unless we want to support hot swap. I'll add some
comments and move the commit.
This can't cause unexpected side effects as is currently dead code.

Frediano

> 
> This most likely deserves a longer commit log explaining why we want
> that (and maybe why it's not going to cause unexpected side effects in
> existing code).
> 
> Christophe
> 
> On Fri, Nov 25, 2016 at 02:52:42PM +0000, Frediano Ziglio wrote:
> > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > ---
> >  server/reds.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/server/reds.c b/server/reds.c
> > index bc0cc01..05afb7c 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -3321,6 +3321,13 @@ SPICE_GNUC_VISIBLE int
> > spice_server_remove_interface(SpiceBaseInstance *sin)
> >          SpiceCharDeviceInstance *char_device = SPICE_CONTAINEROF(sin,
> >          SpiceCharDeviceInstance, base);
> >          reds = red_char_device_get_server(char_device->st);
> >          spice_server_char_device_remove_interface(reds, sin);
> > +    } else if (strcmp(interface->type, SPICE_INTERFACE_QXL) == 0) {
> > +        QXLInstance *qxl;
> > +
> > +        qxl = SPICE_CONTAINEROF(sin, QXLInstance, base);
> > +        reds = red_qxl_get_server(qxl->st);
> > +        reds->qxl_instances = g_list_remove(reds->qxl_instances, qxl);
> > +        red_qxl_destroy(qxl);
> >      } else {
> >          spice_warning("VD_INTERFACE_REMOVING unsupported");
> >          return -1;
> > --
> > git-series 0.9.1
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>