[Spice-devel] [PATCH 06/15] Remove use of global 'reds' var from spice_server_remove_interface()
Frediano Ziglio
fziglio at redhat.com
Mon Jan 25 07:37:21 PST 2016
Also "spice_server_remove_interface: use local variable" should be merged to this patch
Frediano
>
> >
> > From: Jonathon Jongsma <jjongsma at redhat.com>
> >
> > 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 | 10 ++++++++++
> > server/char-device.h | 2 ++
> > server/inputs-channel.c | 17 ++++++++++++++---
> > server/inputs-channel.h | 7 +++----
> > server/reds.c | 13 +++++++++++--
> > 5 files changed, 40 insertions(+), 9 deletions(-)
> >
> > diff --git a/server/char-device.c b/server/char-device.c
> > index 7c209cd..5fc2eba 100644
> > --- a/server/char-device.c
> > +++ b/server/char-device.c
> > @@ -71,6 +71,7 @@ struct SpiceCharDeviceState {
> >
> > SpiceCharDeviceCallbacks cbs;
> > void *opaque;
> > + void *reds;
>
> Why void*? I'd prefer a proper type.
>
> > };
> >
> > enum {
> > @@ -1034,3 +1035,12 @@ 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, void *server)
> > +{
> > + dev->reds = server;
> > +}
> > +
>
> I think there should be no *_set_server. reds should be passed when the state
> variable is built and stored internally.
> If you think once you create the state object you never change the server.
>
> > +void* spice_char_device_get_server(SpiceCharDeviceState *dev)
> > +{
> > + return dev->reds;
> > +}
>
> Get is fine. I would use a const pointer but is just style.
>
> > diff --git a/server/char-device.h b/server/char-device.h
> > index ca2e96b..7d4f4f1 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, void
> > *server);
> > +void* spice_char_device_get_server(SpiceCharDeviceState *dev);
> >
> > /** Read from device **/
> >
> > diff --git a/server/inputs-channel.c b/server/inputs-channel.c
> > index a5959c1..4222b05 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,31 @@ 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);
> > }
> >
>
> This should accept and save the reds.
>
> > +void spice_tablet_state_set_server(SpiceTabletState *st, void *server)
> > +{
> > + st->reds = server;
> > +}
> > +
>
> Like above.
>
> > +void* spice_tablet_state_get_server(SpiceTabletState *st)
> > +{
> > + return st->reds;
> > +}
> > +
> > typedef struct InputsChannelClient {
> > RedChannelClient base;
> > uint16_t motion_count;
> > diff --git a/server/inputs-channel.h b/server/inputs-channel.h
> > index d26ae43..6ce8c35 100644
> > --- a/server/inputs-channel.h
> > +++ b/server/inputs-channel.h
> > @@ -31,10 +31,6 @@ 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);
> > -
>
> This turn to static should be perhaps in another patch.
>
> > SpiceKbdInstance* inputs_channel_get_keyboard(InputsChannel *inputs);
> > int inputs_channel_set_keyboard(InputsChannel *inputs, SpiceKbdInstance
> > *keyboard);
> > SpiceMouseInstance* inputs_channel_get_mouse(InputsChannel *inputs);
> > @@ -43,4 +39,7 @@ SpiceTabletInstance*
> > inputs_channel_get_tablet(InputsChannel *inputs);
> > int inputs_channel_set_tablet(InputsChannel *inputs, SpiceTabletInstance
> > *tablet);
> > int inputs_channel_has_tablet(InputsChannel *inputs);
> > void inputs_channel_detach_tablet(InputsChannel *inputs,
> > SpiceTabletInstance
> > *tablet);
> > +void spice_tablet_state_set_server(SpiceTabletState *dev, void *server);
> > +void* spice_tablet_state_get_server(SpiceTabletState *dev);
> > +
> > #endif
> > diff --git a/server/reds.c b/server/reds.c
> > index 15d20a5..36354a6 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,15 +3236,17 @@ 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) != 0)
> > {
> > return -1;
> > }
> > + spice_tablet_state_set_server(tablet->st, reds);
> > reds_update_mouse_mode(reds);
> > if (reds->is_client_mouse_allowed) {
> > inputs_channel_set_tablet_logical_size(reds->inputs_channel,
> > reds->monitor_mode.x_res, reds->monitor_mode.y_res);
> > @@ -3299,8 +3302,11 @@ SPICE_GNUC_VISIBLE int
> > spice_server_remove_interface(SpiceBaseInstance *sin)
> > const SpiceBaseInterface *interface = sin->sif;
> >
> > if (strcmp(interface->type, SPICE_INTERFACE_TABLET) == 0) {
> > + SpiceTabletInstance *tablet = SPICE_CONTAINEROF(sin,
> > SpiceTabletInstance, base);
> > + RedsState *reds = spice_tablet_state_get_server(tablet->st);
> > + g_return_val_if_fail(reds != NULL, -1);
> > 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);
> > spice_server_char_device_remove_interface(reds, sin);
> > } else {
> > spice_warning("VD_INTERFACE_REMOVING unsupported");
>
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
More information about the Spice-devel
mailing list