[Spice-devel] [qxl] xspice: Adjust to X.org 1.19 changes

[Uri Lublin]

Hi Christophe,

Please see some comments below

On 12/14/2016 12:51 PM, Christophe Fergeau wrote:
> In newer X.org versions, it's no longer supported to modify the set of
> FDs passed to a BlockHandler method to get notified when the FD has data
> to be read. This was limited anyway as we could only get read events
> this way, and had to do our own polling to get notified about socket
> writeability.
>
> Starting from xserver 1.19, the supported way of doing this is to use
> the SetNotifyFd/RemoveNotifyFd API, which is actually a much better way
> as it matches very well the 'watch' API spice-server expects Xspice to
> implement.
>
> This commit switches to that new API, which removes the need for
> RegisterBlockAndWakeupHandlers().
> ---
> I've lightly (xeyes/rxvt) tested this on f25, and tested this still builds on
> f24.
> This should fix the issues Hans mentioned in
> https://lists.freedesktop.org/archives/spice-devel/2016-October/032501.html
>
>
>  configure.ac             |  5 ---
>  src/spiceqxl_main_loop.c | 90 ++++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 84 insertions(+), 11 deletions(-)
[skip]
> diff --git a/src/spiceqxl_main_loop.c b/src/spiceqxl_main_loop.c
> index 49b715a..81bc418 100644
> --- a/src/spiceqxl_main_loop.c
> +++ b/src/spiceqxl_main_loop.c
[skip]
> @@ -276,7 +272,7 @@ static void xspice_block_handler(pointer data, OSTimePtr timeout, pointer readma
>  /*
>   * xserver only calls wakeup_handler with the read fd_set, so we
>   * must either patch it or do a polling select ourselves, this is the
> - * later approach. Since we are already doing a polling select, we
> + * latter approach. Since we are already doing a polling select, we
>   * already select on all (we could avoid selecting on the read since
>   * that *is* actually taken care of by the wakeup handler).
>   */
> @@ -338,9 +334,88 @@ static void xspice_wakeup_handler(pointer data, int nfds, pointer readmask)
>      select_and_check_watches();
>  }
>
> +#else /* GET_ABI_MAJOR(ABI_VIDEODRV_VERSION) < 23 */
> +
> +struct SpiceWatch {
> +    int fd;
> +    int event_mask;
> +    SpiceWatchFunc func;
> +    void *opaque;
> +};
> +
> +static void watch_fd_notified(int fd, int xevents, void *data)
> +{
> +    SpiceWatch *watch = (SpiceWatch *)data;
> +
> +    if ((watch->event_mask & SPICE_WATCH_EVENT_READ) && (xevents & X_NOTIFY_READ)) {
> +        watch->func(watch->fd, SPICE_WATCH_EVENT_READ, watch->opaque);
> +    }
> +
> +    if ((watch->event_mask & SPICE_WATCH_EVENT_WRITE) && (xevents & X_NOTIFY_WRITE)) {
> +        watch->func(watch->fd, SPICE_WATCH_EVENT_WRITE, watch->opaque);
> +    }
> +}
> +
> +static int watch_update_mask2(SpiceWatch *watch, int event_mask)
> +{
> +    SetNotifyFd(watch->fd, NULL, X_NOTIFY_NONE, NULL);
> +    watch->event_mask = 0;
> +
> +    if (event_mask & SPICE_WATCH_EVENT_READ) {
> +        SetNotifyFd(watch->fd, watch_fd_notified, X_NOTIFY_READ, watch);
> +    } else if (event_mask & SPICE_WATCH_EVENT_READ) {

1. This should be (event_mask & SPICE_WATCH_EVENT_WRITE)
2. The "else if" fails to support event_mask which is READ | WRITE.
    Can it not watch both events ?

Regards,
     Uri.

> +        SetNotifyFd(watch->fd, watch_fd_notified, X_NOTIFY_WRITE, watch);
> +    } else {
> +        DPRINTF(0, "Unexpected watch event_mask: %i", event_mask);
> +        return -1;
> +    }
> +    watch->event_mask = event_mask;
> +
> +    return 0;
> +}
> +
> +static SpiceWatch *watch_add(int fd, int event_mask, SpiceWatchFunc func, void *opaque)
> +{
> +    SpiceWatch *watch = malloc(sizeof(SpiceWatch));
> +
> +    DPRINTF(0, "adding %p, fd=%d", watch, fd);
> +
> +    watch->fd = fd;
> +    watch->func = func;
> +    watch->opaque = opaque;
> +    if (watch_update_mask2(watch, event_mask) != 0) {
> +        free(watch);
> +        return NULL;
> +    }
> +
> +    return watch;
> +}
> +
> +static void watch_update_mask(SpiceWatch *watch, int event_mask)
> +{
> +    DPRINTF(0, "fd %d to %d", watch->fd, event_mask);
> +    watch_update_mask2(watch, event_mask);
> +}
> +
> +static void watch_remove(SpiceWatch *watch)
> +{
> +    DPRINTF(0, "remove %p (fd %d)", watch, watch->fd);
> +    RemoveNotifyFd(watch->fd);
> +    free(watch);
> +}
> +#endif
> +
> +static void channel_event(int event, SpiceChannelEventInfo *info)
> +{
> +    NOT_IMPLEMENTED
> +}
> +
> +
>  SpiceCoreInterface *basic_event_loop_init(void)
>  {
> +#if GET_ABI_MAJOR(ABI_VIDEODRV_VERSION) < 23
>      ring_init(&watches);
> +#endif
>      bzero(&core, sizeof(core));
>      core.base.major_version = SPICE_INTERFACE_CORE_MAJOR;
>      core.base.minor_version = SPICE_INTERFACE_CORE_MINOR; // anything less then 3 and channel_event isn't called
> @@ -355,7 +430,10 @@ SpiceCoreInterface *basic_event_loop_init(void)
>      return &core;
>  }
>
> +
>  void xspice_register_handlers(void)
>  {
> +#if GET_ABI_MAJOR(ABI_VIDEODRV_VERSION) < 23
>      RegisterBlockAndWakeupHandlers(xspice_block_handler, xspice_wakeup_handler, 0);
> +#endif
>  }
>