Re: [PATCH 07/16] Remove use of global 'reds' var from spice_server_remove_interface()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 2016-01-27 at 12:48 +0000, Frediano Ziglio wrote:
> From: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> 
> Since this is public API, we can't easily change the signature of the
> function to take a RedsState argument, so instead we apply a hack and
> store the reds argument inside the device state struct when the
> interface is added, and retrieve it for use later when it is removed.
> ---
>  server/char-device.c    | 11 +++++++++++
>  server/char-device.h    |  2 ++
>  server/inputs-channel.c | 16 +++++++++++-----
>  server/inputs-channel.h |  8 +++-----
>  server/reds.c           | 13 +++++++++++--
>  5 files changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/server/char-device.c b/server/char-device.c
> index 7c209cd..aa2eafd 100644
> --- a/server/char-device.c
> +++ b/server/char-device.c
> @@ -71,6 +71,7 @@ struct SpiceCharDeviceState {
>  
>      SpiceCharDeviceCallbacks cbs;
>      void *opaque;
> +    SpiceServer *reds;
>  };
>  
>  enum {
> @@ -1034,3 +1035,13 @@ int
> spice_char_device_state_restore(SpiceCharDeviceState *dev,
>      spice_char_device_read_from_device(dev);
>      return TRUE;
>  }
> +
> +void spice_char_device_set_server(SpiceCharDeviceState *dev,
> SpiceServer *server)
> +{
> +    dev->reds = server;
> +}
> +
> +SpiceServer* spice_char_device_get_server(SpiceCharDeviceState *dev)
> +{
> +    return dev->reds;
> +}
> diff --git a/server/char-device.h b/server/char-device.h
> index ca2e96b..a9c666a 100644
> --- a/server/char-device.h
> +++ b/server/char-device.h
> @@ -181,6 +181,8 @@ int
> spice_char_device_client_exists(SpiceCharDeviceState *dev,
>  
>  void spice_char_device_start(SpiceCharDeviceState *dev);
>  void spice_char_device_stop(SpiceCharDeviceState *dev);
> +void spice_char_device_set_server(SpiceCharDeviceState *dev,
> SpiceServer *server);
> +SpiceServer* spice_char_device_get_server(SpiceCharDeviceState
> *dev);
>  
>  /** Read from device **/
>  
> diff --git a/server/inputs-channel.c b/server/inputs-channel.c
> index a5959c1..9f33f0d 100644
> --- a/server/inputs-channel.c
> +++ b/server/inputs-channel.c
> @@ -63,7 +63,7 @@ struct SpiceKbdState {
>      bool key_ext[0x7f];
>  };
>  
> -SpiceKbdState* spice_kbd_state_new(void)
> +static SpiceKbdState* spice_kbd_state_new(void)
>  {
>      return spice_new0(SpiceKbdState, 1);
>  }
> @@ -72,20 +72,25 @@ struct SpiceMouseState {
>      int dummy;
>  };
>  
> -SpiceMouseState* spice_mouse_state_new(void)
> +static SpiceMouseState* spice_mouse_state_new(void)
>  {
>      return spice_new0(SpiceMouseState, 1);
>  }
>  
>  struct SpiceTabletState {
> -    int dummy;
> +    RedsState *reds;
>  };
>  
> -SpiceTabletState* spice_tablet_state_new(void)
> +static SpiceTabletState* spice_tablet_state_new(void)
>  {
>      return spice_new0(SpiceTabletState, 1);
>  }
>  
> +RedsState* spice_tablet_state_get_server(SpiceTabletState *st)
> +{
> +    return st->reds;
> +}
> +
>  typedef struct InputsChannelClient {
>      RedChannelClient base;
>      uint16_t motion_count;
> @@ -685,7 +690,7 @@ SpiceTabletInstance*
> inputs_channel_get_tablet(InputsChannel *inputs)
>      return inputs->tablet;
>  }
>  
> -int inputs_channel_set_tablet(InputsChannel *inputs,
> SpiceTabletInstance *tablet)
> +int inputs_channel_set_tablet(InputsChannel *inputs,
> SpiceTabletInstance *tablet, RedsState *reds)
>  {
>      if (inputs->tablet) {
>          spice_printerr("already have tablet");
> @@ -693,6 +698,7 @@ int inputs_channel_set_tablet(InputsChannel
> *inputs, SpiceTabletInstance *tablet
>      }
>      inputs->tablet = tablet;
>      inputs->tablet->st = spice_tablet_state_new();
> +    inputs->tablet->st->reds = reds;
>      return 0;
>  }
>  
> diff --git a/server/inputs-channel.h b/server/inputs-channel.h
> index d26ae43..31574b5 100644
> --- a/server/inputs-channel.h
> +++ b/server/inputs-channel.h
> @@ -31,16 +31,14 @@ const VDAgentMouseState
> *inputs_channel_get_mouse_state(InputsChannel *inputs);
>  void inputs_channel_on_keyboard_leds_change(InputsChannel *inputs,
> uint8_t leds);
>  void inputs_channel_set_tablet_logical_size(InputsChannel *inputs,
> int x_res, int y_res);
>  
> -SpiceKbdState* spice_kbd_state_new(void);
> -SpiceMouseState* spice_mouse_state_new(void);
> -SpiceTabletState* spice_tablet_state_new(void);
> -
>  SpiceKbdInstance* inputs_channel_get_keyboard(InputsChannel
> *inputs);
>  int inputs_channel_set_keyboard(InputsChannel *inputs,
> SpiceKbdInstance *keyboard);
>  SpiceMouseInstance* inputs_channel_get_mouse(InputsChannel *inputs);
>  int inputs_channel_set_mouse(InputsChannel *inputs,
> SpiceMouseInstance *mouse);
>  SpiceTabletInstance* inputs_channel_get_tablet(InputsChannel
> *inputs);
> -int inputs_channel_set_tablet(InputsChannel *inputs,
> SpiceTabletInstance *tablet);
> +int inputs_channel_set_tablet(InputsChannel *inputs,
> SpiceTabletInstance *tablet, RedsState *reds);
>  int inputs_channel_has_tablet(InputsChannel *inputs);
>  void inputs_channel_detach_tablet(InputsChannel *inputs,
> SpiceTabletInstance *tablet);
> +RedsState* spice_tablet_state_get_server(SpiceTabletState *dev);
> +
>  #endif
> diff --git a/server/reds.c b/server/reds.c
> index 15d20a5..ebb1629 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3160,6 +3160,7 @@ static int
> spice_server_char_device_add_interface(SpiceServer *s,
>          if (reds->vm_running) {
>              spice_char_device_start(char_device->st);
>          }
> +        spice_char_device_set_server(char_device->st, reds);
>          reds_char_device_add_state(reds, char_device->st);
>      } else {
>          spice_warning("failed to create device state for %s",
> char_device->subtype);
> @@ -3235,13 +3236,14 @@ SPICE_GNUC_VISIBLE int
> spice_server_add_interface(SpiceServer *reds,
>          red_dispatcher_init(qxl);
>  
>      } else if (strcmp(interface->type, SPICE_INTERFACE_TABLET) == 0)
> {
> +        SpiceTabletInstance *tablet = SPICE_CONTAINEROF(sin,
> SpiceTabletInstance, base);
>          spice_info("SPICE_INTERFACE_TABLET");
>          if (interface->major_version != SPICE_INTERFACE_TABLET_MAJOR
> ||
>              interface->minor_version > SPICE_INTERFACE_TABLET_MINOR)
> {
>              spice_warning("unsupported tablet interface");
>              return -1;
>          }
> -        if (inputs_channel_set_tablet(reds->inputs_channel,
> SPICE_CONTAINEROF(sin, SpiceTabletInstance, base)) != 0) {
> +        if (inputs_channel_set_tablet(reds->inputs_channel, tablet,
> reds) != 0) {
>              return -1;
>          }
>          reds_update_mouse_mode(reds);
> @@ -3296,11 +3298,15 @@ SPICE_GNUC_VISIBLE int
> spice_server_add_interface(SpiceServer *reds,
>  
>  SPICE_GNUC_VISIBLE int
> spice_server_remove_interface(SpiceBaseInstance *sin)
>  {
> +    RedsState *reds;
>      const SpiceBaseInterface *interface = sin->sif;
>  
>      if (strcmp(interface->type, SPICE_INTERFACE_TABLET) == 0) {
> +        SpiceTabletInstance *tablet = SPICE_CONTAINEROF(sin,
> SpiceTabletInstance, base);
> +        reds = spice_tablet_state_get_server(tablet->st);
> +        g_return_val_if_fail(reds != NULL, -1);

When it can be NULL? Should this public function be used without a
previous spice_server_new() call? I would not use g_return.

It would crash the same way with the global 'reds', no?

>          spice_info("remove SPICE_INTERFACE_TABLET");
> -        inputs_channel_detach_tablet(reds->inputs_channel,
> SPICE_CONTAINEROF(sin, SpiceTabletInstance, base));
> +        inputs_channel_detach_tablet(reds->inputs_channel, tablet);
>          reds_update_mouse_mode(reds);
>      } else if (strcmp(interface->type, SPICE_INTERFACE_PLAYBACK) ==
> 0) {
>          spice_info("remove SPICE_INTERFACE_PLAYBACK");
> @@ -3309,6 +3315,9 @@ SPICE_GNUC_VISIBLE int
> spice_server_remove_interface(SpiceBaseInstance *sin)
>          spice_info("remove SPICE_INTERFACE_RECORD");
>          snd_detach_record(SPICE_CONTAINEROF(sin,
> SpiceRecordInstance, base));
>      } else if (strcmp(interface->type, SPICE_INTERFACE_CHAR_DEVICE)
> == 0) {
> +        SpiceCharDeviceInstance *char_device =
> SPICE_CONTAINEROF(sin, SpiceCharDeviceInstance, base);
> +        reds = spice_char_device_get_server(char_device->st);
> +        g_return_val_if_fail(reds != NULL, -1);
Same here

Pavel
>          spice_server_char_device_remove_interface(reds, sin);
>      } else {
>          spice_warning("VD_INTERFACE_REMOVING unsupported");
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]