[Spice-devel] [PATCH 07/16] Remove use of global 'reds' var from spice_server_remove_interface()
Pavel Grunt
pgrunt at redhat.com
Wed Feb 3 08:32:48 CET 2016
On Tue, 2016-02-02 at 13:23 -0600, Jonathon Jongsma wrote:
> On Fri, 2016-01-29 at 11:31 +0100, Pavel Grunt wrote:
> > On Wed, 2016-01-27 at 12:48 +0000, Frediano Ziglio wrote:
> > > 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 | 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?
>
> Are you suggesting using an assert here?
g_return should be used for programming errors. If it is expected to
call this function without having a SpiceServer then we should return
without a warning. For now I would use nothing or assert.
Pavel
>
> >
> > > 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 at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list