NB: this is a response to a review of the old patch and I didn't get around to
sending it out yesterday. Frediano has since posted a new patch, but I thought
it might still be worth sending my response to the comments.
On Fri, 2016-01-22 at 07:38 -0500, 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 | 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.
Yes, I agree that it's fairly ugly and not type-safe. If I recall, I used void*
here (as well as in the return value for spice_char_device_get_server()) in
order to avoid circular dependencies between RedsState and SpiceCharDeviceState.
Since this was an incremental refactoring, I wanted to keep the changes minimal.
I started investigating what it would take to implement the changes you suggest
here, but it turns out that almost all of the comments from this email appear to
be addressed by additional commits that are still coming on this branch. Trying
to bring parts of those future commits forward into this patch results in
needing to bring additional commits forward, and in the end I'm just not sure
it's worth the effort to re-shuffle the changes.
>
> > };
> >
> > 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.
As mentioned above, at the end of the 'refactory' branch, this function is no
longer called and the RedsState argument is passed to the constructor. (although
it looks like I forgot to remove the unused _set_server() function...).
>
> > +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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel