[Spice-devel] [PATCH vdagent-linux 4/4] vdagentd: support fake uinput

[Hans de Goede]

Comments inline.

On 09/02/2013 05:02 PM, Alon Levy wrote:
> This is used with Xspice. Fake means we open a pipe for write only, and
> don't do any ioctls on it. Specifically it means the axis and buttons
> have to be coordinated for now with Xspice (xf86-video-qxl).
>
> Signed-off-by: Alon Levy <alevy at redhat.com>
> ---
>   src/vdagentd-uinput.c | 11 +++++++++--
>   src/vdagentd-uinput.h |  2 +-
>   src/vdagentd.c        | 23 +++++++++++++----------
>   3 files changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/src/vdagentd-uinput.c b/src/vdagentd-uinput.c
> index a415f9c..47e1b45 100644
> --- a/src/vdagentd-uinput.c
> +++ b/src/vdagentd-uinput.c
> @@ -42,12 +42,13 @@ struct vdagentd_uinput {
>       struct vdagentd_guest_xorg_resolution *screen_info;
>       int screen_count;
>       VDAgentMouseState last;
> +    int fake;
>   };
>
>   struct vdagentd_uinput *vdagentd_uinput_create(const char *devname,
>       int width, int height,
>       struct vdagentd_guest_xorg_resolution *screen_info, int screen_count,
> -    int debug)
> +    int debug, int fake)
>   {
>       struct vdagentd_uinput *uinput;
>
> @@ -58,6 +59,7 @@ struct vdagentd_uinput *vdagentd_uinput_create(const char *devname,
>       uinput->devname = devname;
>       uinput->fd      = -1; /* Gets opened by vdagentd_uinput_update_size() */
>       uinput->debug   = debug;
> +    uinput->fake    = fake;
>
>       vdagentd_uinput_update_size(&uinput, width, height,
>                                   screen_info, screen_count);
> @@ -119,13 +121,18 @@ void vdagentd_uinput_update_size(struct vdagentd_uinput **uinputp,
>           return;
>   #endif
>
> -    uinput->fd = open(uinput->devname, O_RDWR);
> +    uinput->fd = open(uinput->devname, uinput->fake ? O_WRONLY : O_RDWR);
>       if (uinput->fd == -1) {
>           syslog(LOG_ERR, "open %s: %m", uinput->devname);
>           vdagentd_uinput_destroy(uinputp);
>           return;
>       }
>
> +    if (uinput->fake) {
> +        /* fake device doesn't understand any ioctls and only writes events */
> +        return;
> +    }
> +
>       rc = write(uinput->fd, &device, sizeof(device));
>       if (rc != sizeof(device)) {
>           syslog(LOG_ERR, "write %s: %m", uinput->devname);
> diff --git a/src/vdagentd-uinput.h b/src/vdagentd-uinput.h
> index 81f0376..b9bd9f1 100644
> --- a/src/vdagentd-uinput.h
> +++ b/src/vdagentd-uinput.h
> @@ -30,7 +30,7 @@ struct vdagentd_uinput;
>   struct vdagentd_uinput *vdagentd_uinput_create(const char *devname,
>       int width, int height,
>       struct vdagentd_guest_xorg_resolution *screen_info, int screen_count,
> -    int debug);
> +    int debug, int fake);
>   void vdagentd_uinput_destroy(struct vdagentd_uinput **uinputp);
>
>   void vdagentd_uinput_do_mouse(struct vdagentd_uinput **uinputp,
> diff --git a/src/vdagentd.c b/src/vdagentd.c
> index 2288671..54765ce 100644
> --- a/src/vdagentd.c
> +++ b/src/vdagentd.c
> @@ -58,6 +58,7 @@ static const char *portdev = "/dev/virtio-ports/com.redhat.spice.0";
>   static const char *vdagentd_socket = VDAGENTD_SOCKET;
>   static const char *uinput_device = "/dev/uinput";
>   static int debug = 0;
> +static int uinput_fake = 0;
>   static struct udscs_server *server = NULL;
>   static struct vdagent_virtio_port *virtio_port = NULL;
>   static GHashTable *active_xfers = NULL;
> @@ -317,11 +318,10 @@ int virtio_port_read_complete(
>                                                   agent_data->height,
>                                                   agent_data->screen_info,
>                                                   agent_data->screen_count,
> -                                                debug > 1);
> +                                                debug > 1,
> +                                                uinput_fake);
>               if (!uinput) {
> -                syslog(LOG_CRIT, "Fatal uinput error");
> -                retval = 1;
> -                quit = 1;
> +                syslog(LOG_CRIT, "uinput error");
>               }
>           }
>           break;

Making this not fatal is a bad idea in the non Xspice case (we keep the
virtio port open then, so all mouse events get send to the agent and then
dropped). Also if we want to make this non fatal (which IMHO we don't),
that should be done in a separate patch.

> @@ -488,7 +488,8 @@ static void check_xorg_resolution(void)
>                                               agent_data->height,
>                                               agent_data->screen_info,
>                                               agent_data->screen_count,
> -                                            debug > 1);
> +                                            debug > 1,
> +                                            uinput_fake);
>           else
>               vdagentd_uinput_update_size(&uinput,
>                                           agent_data->width,
> @@ -496,10 +497,7 @@ static void check_xorg_resolution(void)
>                                           agent_data->screen_info,
>                                           agent_data->screen_count);
>           if (!uinput) {
> -            syslog(LOG_CRIT, "Fatal uinput error");
> -            retval = 1;
> -            quit = 1;
> -            return;
> +            syslog(LOG_INFO, "No uinput available");
>           }
>
>           if (!virtio_port) {

Idem.

> @@ -867,6 +865,11 @@ int main(int argc, char *argv[])
>           }
>       }
>
> +    if (strncmp(uinput_device, "/dev", 4) != 0) {
> +        fprintf(stderr, "%s: using fake uinput\n", __func__);

We don't use fprintf(stderr for logging anywhere in the agent, nor
do we use __func__ anywhere, please stick to using syslog and the existing
log-message style.


> +        uinput_fake = 1;
> +    }
> +
>       memset(&act, 0, sizeof(act));
>       act.sa_flags = SA_RESTART;
>       act.sa_handler = quit_handler;
> @@ -899,7 +902,7 @@ int main(int argc, char *argv[])
>
>   #ifdef WITH_STATIC_UINPUT
>       uinput = vdagentd_uinput_create(uinput_device, 1024, 768, NULL, 0,
> -                                    debug > 1);
> +                                    debug > 1, uinput_fake);
>       if (!uinput) {
>           udscs_destroy_server(server);
>           return 1;
>


Regards,

Hans