Xen devel - Sep 2006 - [PATCH] Paravirt framebuffer backend tools [2/5]

Initially from Anthony Liguori and then modified for
* Integration with xenstore (armrbu)
* Adding option parsing (katzj)
* Integration with Xen makefiles 
* Error handling (armbru)
* Future-proof layout of shared page (armbru)
* Memory barriers (armbru)
* Support for vncunused and vnclisten as support in qemu-dm vnc (katzj)

Signed-off-by: Jeremy Katz <katzj@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Anthony Liguori <aliguori@us.ibm.com>




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> --- a/tools/Makefile	Sat Sep 02 15:11:17 2006 -0400
> +++ b/tools/Makefile	Sat Sep 02 15:19:25 2006 -0400
> @@ -18,6 +18,7 @@ SUBDIRS-y += xenstat
>  SUBDIRS-y += xenstat
>  SUBDIRS-y += libaio
>  SUBDIRS-y += blktap
> +SUBDIRS-y += xenfb
>  
>  # These don''t cross-compile
>  ifeq ($(XEN_COMPILE_ARCH),$(XEN_TARGET_ARCH))
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/tools/xenfb/Makefile	Sat Sep 02 15:19:25 2006 -0400
> @@ -0,0 +1,36 @@
> +XEN_ROOT=../..
> +include $(XEN_ROOT)/tools/Rules.mk
> +
> +CFLAGS += -g -Wall
You shouldn''t need to add -g here; Rules.mk handles it for you if
debug is set.
> +CFLAGS += -I$(XEN_LIBXC) -I$(XEN_XENSTORE)
-I$(XEN_ROOT)/linux-2.6-xen-sparse/include
> +LDFLAGS += -L$(XEN_LIBXC) -L$(XEN_XENSTORE)
> +
> +INSTALL         = install
> +INSTALL_PROG    = $(INSTALL) -m0755
> +INSTALL_DIR     = $(INSTALL) -d -m0755
> +
> +.PHONY: all
> +all: build
> +
> +.PHONY: build
> +build: mk-symlinks
> +	$(MAKE) vncfb sdlfb
> +
> +install: all
> +	$(INSTALL_DIR) -p $(DESTDIR)/usr/$(LIBDIR)/xen/bin
> +	$(INSTALL_PROG) vncfb $(DESTDIR)/usr/$(LIBDIR)/xen/bin/xen-vncfb
> +	$(INSTALL_PROG) sdlfb $(DESTDIR)/usr/$(LIBDIR)/xen/bin/xen-sdlfb
> +
> +sdlfb: sdlfb.o xenfb.o
> +
> +sdlfb.o: CFLAGS += $(shell sdl-config --cflags)
> +sdlfb: LDLIBS += $(shell sdl-config --libs) -lxenctrl -lxenstore
> +
> +clean:
> +	$(RM) *.o *~ vncfb sdlfb
> +
> +keymapping.o: CFLAGS += $(shell pkg-config --cflags gtk+-2.0)
> +
> +vncfb: vncfb.o xenfb.o keymapping.o
> +vncfb.o: CFLAGS += $(shell libvncserver-config --cflags)
> +vncfb: LDLIBS += $(shell libvncserver-config --libs) -lxenctrl -lxenstore
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/tools/xenfb/keymapping.c	Sat Sep 02 15:19:25 2006 -0400
> @@ -0,0 +1,141 @@
> +#include <stdint.h>
> +#include <gdk/gdkkeysyms.h>
> +#include <linux/input.h>
> +
> +uint32_t gdk_linux_mapping[0x10000] = {
> +	[GDK_a] = KEY_A,
This is kind of ugly.  Is there any chance it could be autogenerated?
Also, where did 0x10000 come from?

Also, depending on GTK just for the keymap table is a real pain.  Or
is it already required for libvncserver?

<snip>
> +	[GDK_plus] = KEY_EQUAL,
> +};
> +
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/tools/xenfb/sdlfb.c	Sat Sep 02 15:19:25 2006 -0400
> @@ -0,0 +1,191 @@
> +#include <SDL.h>
> +#include <sys/types.h>
> +#include <sys/select.h>
> +#include <stdlib.h>
> +#include <linux/input.h>
> +#include <getopt.h>
> +#include <string.h>
> +#include "xenfb.h"
> +
> +struct data
That''s a really wonderful name.
> +{
> +	SDL_Surface *dst;
> +	SDL_Surface *src;
> +};
> +
> +void sdl_update(struct xenfb *xenfb, int x, int y, int width, int height)
> +{
> +	struct data *data = xenfb->user_data;
> +	SDL_Rect r = { x, y, width, height };
> +	SDL_BlitSurface(data->src, &r, data->dst, &r);
> +	SDL_UpdateRect(data->dst, x, y, width, height);
> +}
> +
> +int sdl2linux[1024] = {
> +	[SDLK_a] = KEY_A,
Another really ugly mapping table, although not quite as bad as the
GTK one.

Where''d the magic 1024 come from?
> +	[SDLK_RALT] = KEY_RIGHTALT,
> +};
> +
> +static struct option options[] = {
> +    { "domid", 1, NULL, ''d'' },
> +    { "title", 1, NULL, ''t'' },
> +};
> +
> +int main(int argc, char **argv)
> +{
> +	struct xenfb *xenfb;
> +	int fd;
> +	int domid = -1;
> +        char * title = NULL;
> +	fd_set readfds;
> +	struct data data;
> +	SDL_Rect r;
> +	struct timeval tv = { 0, 500 };
> +	int do_quit = 0;
> +        int opt;
Slightly strange whitespace, but nevermind.
> +
> +	while ((opt = getopt_long(argc, argv, "d:t:", options,
> +				  NULL)) != -1) {
> +		switch (opt) {
> +                case ''d'':
> +                    domid = strtol(optarg, NULL, 10);
It''d be nice to check for a malformed argument here.
> +                    break;
> +                case ''t'':
> +                    title = strdup(optarg);
This can fail.
> +                    break;
> +                }
> +        }
> +        if (optind != argc) {
> +            fprintf(stderr, "Invalid options!\n");
> +            exit(1);
errx() maybe?
> +        }
> +        if (domid == -1) {
> +            fprintf(stderr, "Domain ID must be specified!\n");
> +            exit(1);
> +        }
> +
> +	xenfb = xenfb_new();
> +	if (xenfb == NULL)
> +		return 1;
Why have you used exit(1) in some places and return 1 in others?

Also, an error message here would be a good idea.
> +
> +	if (!xenfb_attach_dom(xenfb, domid))
> +		return 1;
An error mesasge would be good.
> +
> +	SDL_Init(SDL_INIT_VIDEO);
> +
> +	fd = xenfb_get_fileno(xenfb);
> +
> +	data.dst = SDL_SetVideoMode(xenfb->width, xenfb->height,
xenfb->depth,
> +				    SDL_SWSURFACE);
> +
> +	data.src = SDL_CreateRGBSurfaceFrom(xenfb->pixels,
> +					    xenfb->width, xenfb->height,
> +					    xenfb->depth, xenfb->row_stride,
> +					    0xFF0000, 0xFF00, 0xFF, 0);
> +
> +        if (title == NULL)
> +            title = strdup("xen-sdlfb");
This can fail.
> +        SDL_WM_SetCaption(title, title);
> +
> +	r.x = r.y = 0;
> +	r.w = xenfb->width;
> +	r.h = xenfb->height;
> +	SDL_BlitSurface(data.src, &r, data.dst, &r);
> +	SDL_UpdateRect(data.dst, 0, 0, xenfb->width, xenfb->height);
> +
> +	xenfb->update = sdl_update;
> +	xenfb->user_data = &data;
> +
> +	FD_ZERO(&readfds);
> +	FD_SET(fd, &readfds);
> +
> +	SDL_ShowCursor(0);
> +
> +	while (!do_quit && select(fd + 1, &readfds, NULL, NULL,
&tv) != -1) {
Select can say -1 because of EINTR (e.g. when strace attaches).  It''s
not clear to me whether you want to exit or retry in that case.

Also, if you quit because select returns -1, you need an error
message.
> +		SDL_Event event;
> +
> +		while (SDL_PollEvent(&event)) {
> +			switch (event.type) {
> +			case SDL_KEYDOWN:
> +			case SDL_KEYUP:
> +				xenfb_send_key(xenfb,
> +					       event.type == SDL_KEYDOWN,
> +					       sdl2linux[event.key.keysym.sym]);
> +				break;
> +			case SDL_MOUSEMOTION: {
> +				int x, y;
> +				Uint8 button;
> +
> +				button = SDL_GetRelativeMouseState(&x, &y);
> +				xenfb_send_motion(xenfb, x, y);
> +			}	break;
> +			case SDL_MOUSEBUTTONDOWN:
> +			case SDL_MOUSEBUTTONUP:
> +				xenfb_send_button(xenfb,
> +						  event.type==SDL_MOUSEBUTTONDOWN,
> +						  3 - event.button.button);
> +				break;
> +			case SDL_QUIT:
> +				do_quit = 1;
> +				break;
> +			}
> +		}
> +		if (FD_ISSET(fd, &readfds))
> +			xenfb_on_incoming(xenfb);
> +
> +		FD_ZERO(&readfds);
> +		FD_SET(fd, &readfds);
> +
> +		tv = (struct timeval){0, 500};
I think 500us is a little short here.  About ten milliseconds sounds
more plausible.  This is a bit of a bikeshed.

It''s a pity SDL doesn''t allow you to wait for either an SDL
event or
an fd to become readable.  Could you do something like spawn a thread
which does the selects in a loop, and then SDL_PushEvent()s a user
event when the fd becomes readable?

Admittedly, SDL_WaitEvent also contains this kind of loop-with-sleep,
but it''d reduce the number of magic tunables a bit.
> +	}
> +
> +	xenfb_delete(xenfb);
> +
> +	SDL_Quit();
> +
> +	return 0;
> +}
> --- b/tools/xenfb/vncfb.c	Sat Sep 02 15:19:25 2006 -0400
> +++ b/tools/xenfb/vncfb.c	Sat Sep 02 15:22:19 2006 -0400
Minor nit: generally, putting a vnc_ prefix on these functions
confused me, since it looks like they should be in libvncserver.  This
may just be because I''m not paying enough attention.
> @@ -0,0 +1,245 @@
> +#define _GNU_SOURCE
> +#include <errno.h>
> +#include <getopt.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <malloc.h>
> +#include <rfb/rfb.h>
> +#include <xs.h>
> +#include "xenfb.h"
> +
> +static void on_kbd_event(rfbBool down, rfbKeySym keycode, rfbClientPtr cl)
> +{
> +	extern uint32_t gdk_linux_mapping[0x10000];
Is there any chance of moving this into a header file somewhere?
> +	rfbScreenInfoPtr server = cl->screen;
> +	struct xenfb *xenfb = server->screenData;
> +	xenfb_send_key(xenfb, down, gdk_linux_mapping[keycode & 0xFFFF]);
> +}
> +
> +static void on_ptr_event(int buttonMask, int x, int y, rfbClientPtr cl)
> +{
> +	static int last_x = -1, last_y = -1;
> +	static int last_button = -1;
> +	rfbScreenInfoPtr server = cl->screen;
> +	struct xenfb *xenfb = server->screenData;
> +
> +	if (last_button != -1) {
> +		int i;
> +
> +		for (i = 0; i < 8; i++) {
> +			if ((last_button & (1 << i)) !> +			    (buttonMask &
(1 << i))) {
> +				printf("%d %d\n", buttonMask & (1 << i), i);
Umm?
> +				xenfb_send_button(xenfb, buttonMask & (1 << i),
> +						  2 - i);
> +			}
> +		}
> +	}
> +
> +	if (last_x != -1)
> +		xenfb_send_motion(xenfb, x - last_x, y - last_y);
> +
> +	last_button = buttonMask;
> +
> +	last_x = x;
> +	last_y = y;
> +}
> +
> +static void xenstore_write_vncport(int port, int domid)
> +{
> +    char *buf = NULL, *path;
> +    char *portstr = NULL;
> +    struct xs_handle *xsh = NULL;
> +
> +    xsh = xs_daemon_open();
> +    if (xsh == NULL)
> +	return;
> +
> +    path = xs_get_domain_path(xsh, domid);
> +    if (path == NULL) {
> +        fprintf(stderr, "xs_get_domain_path() error\n");
> +        goto out;
> +    }
> +
> +    buf = malloc(256);
Could fail.  Also, consider using asprintf.
> +    if (snprintf(buf, 256, "%s/console/vnc-port", path) == -1)
> +	goto out;
> +
> +    portstr = malloc(10);
Why is this on the heap rather than the stack?
> +    if (snprintf(portstr, 10, "%d", port) == -1)
> +	goto out;
> +
> +    if (xs_write(xsh, XBT_NULL, buf, portstr, strlen(portstr)) == 0)
> +        fprintf(stderr, "xs_write() vncport failed\n");
> +
> + out:
> +    free(portstr);
> +    free(buf);
> +}
> +
> +
> +static void vnc_update(struct xenfb *xenfb, int x, int y, int w, int h)
> +{
> +	rfbScreenInfoPtr server = xenfb->user_data;
> +	rfbMarkRectAsModified(server, x, y, x + w, y + h);
> +}
> +
> +static int vnc_start_viewer(int port) 
I''m not convinced the backend server process is the best place to
start the viewer from.  Perhaps xend would be a better choice?  Not
sure about this.
> +{
> +    int pid;
> +    char s[16];
> +
> +    snprintf(s, 16, ":%d", port);
> +    switch (pid = fork()) {
> +    case -1:
> +	fprintf(stderr, "vncviewer failed fork\n");
> +	exit(1);
err()?
> +
> +    case 0:	/* child */
> +	execlp("vncviewer", "vncviewer", s, 0);
> +	fprintf(stderr, "vncviewer execlp failed\n");
> +	exit(1);
err()?
> +
> +    default:
> +	return pid;
This is ignored.  Also, the parent process makes no attempt to check
whether the child was exec()ed successfully or anything along those
lines.  This is enough of a pain to fix that I''d probably just ignore
it, though.
> +    }
> +}
> +
> +static struct option options[] = {
> +    { "domid", 1, NULL, ''d'' },
> +    { "vncport", 1, NULL, ''p'' },
> +    { "title", 1, NULL, ''t'' },
> +    { "unused", 0, NULL, ''u'' },
What does this do?
> +    { "listen", 1, NULL, ''l'' },
> +    { "vncviewer", 0, NULL, ''v'' },
> +};
> +
> +int main(int argc, char **argv)
> +{
> +	rfbScreenInfoPtr server;
> +	char *fake_argv[7] = { "vncfb", "-rfbport",
"5901",
> +                               "-desktop",
"xen-vncfb",
> +                               "-listen", "0.0.0.0" };
> +	int fake_argc = sizeof(fake_argv) / sizeof(fake_argv[0]);
> +	int domid = -1, port = -1;
> +        char * title = NULL;
> +        char * listen = NULL;
Whitespace is a bit funny again.
> +	struct xenfb *xenfb;
> +	fd_set readfds;
> +	int fd;
> +	char buffer[1024];
Could do with a better name, and is larger than it needs to be.
> +        int opt;
> +        bool unused = FALSE;
You''re inconsistent about the capitalisation of bools.
> +        bool viewer = FALSE;
> +
> +	while ((opt = getopt_long(argc, argv, "d:p:t:u", options,
> +				  NULL)) != -1) {
> +		switch (opt) {
> +                case ''d'':
> +                    domid = strtol(optarg, NULL, 10);
It would be nice to sanity check the argument here.
> +                    break;
> +                case ''p'':
> +                    port = strtol(optarg, NULL, 10);
Again.
> +                    break;
> +                case ''t'':
> +                    title = strdup(optarg);
Can fail.
> +                    break;
> +                case ''u'':
> +                    unused = TRUE;
> +                    break;
> +                case ''l'':
> +                    listen = strdup(optarg);
Can fail.
> +                    break;
> +                case ''v'':
> +                    viewer = TRUE;
> +                    break;
> +                case ''l'':
> +                    listen = strdup(optarg);
Can fail.
> +                    break;
> +                }
> +        }
> +        if (optind != argc) {
> +            fprintf(stderr, "Invalid options!\n");
> +            exit(1);
> +        }
> +        if (domid == -1) {
> +            fprintf(stderr, "Domain ID must be specified!\n");
> +            exit(1);
> +        }
> +            
> +        if (port == -1)
> +            port = 5900 + domid;
> +	snprintf(buffer, sizeof(buffer), "%d", port);
> +	fake_argv[2] = buffer;
> +
> +        if (title != NULL)
> +            fake_argv[4] = title;
> +
> +        if (listen != NULL)
> +            fake_argv[6] = listen;
> +
> +        if (listen != NULL)
> +            fake_argv[6] = listen;
Umm... What''s going on here?
> +
> +	xenfb = xenfb_new();
> +	if (xenfb == NULL) {
> +		fprintf(stderr, "Could not create framebuffer (%s)\n",
> +			strerror(errno));
> +		exit(1);
> +	}
> +
> +	if (!xenfb_attach_dom(xenfb, domid)) {
> +		fprintf(stderr, "Could not connect to domain (%s)\n",
> +			strerror(errno));
> +		exit(1);
> +	}
> +
> +	server = rfbGetScreen(&fake_argc, fake_argv, 
> +			      xenfb->width, xenfb->height,
> +			      8, 3, xenfb->depth / 8);
> +	if (server == NULL) {
> +		fprintf(stderr, "Could not create VNC server\n");
> +		exit(1);
> +	}
> +
> +	xenfb->user_data = server;
> +	xenfb->update = vnc_update;
> +
> +        if (unused)
> +            server->autoPort = TRUE;
> +
> +	server->serverFormat.redShift = 16;
> +	server->serverFormat.greenShift = 8;
> +	server->serverFormat.blueShift = 0;
> +	server->kbdAddEvent = on_kbd_event;
> +	server->ptrAddEvent = on_ptr_event;
> +	server->frameBuffer = (char *)xenfb->pixels;
> +	server->screenData = xenfb;
> +	rfbInitServer(server);
> +
> +	rfbRunEventLoop(server, -1, TRUE);
> +
> +	fd = xenfb_get_fileno(xenfb);
> +
> +	FD_ZERO(&readfds);
> +	FD_SET(fd, &readfds);
> +
> +        xenstore_write_vncport(server->port, domid);
> +
> +        if (viewer)
> +            vnc_start_viewer(server->port);
> +
> +	while (select(fd + 1, &readfds, NULL, NULL, NULL) != -1) {
> +		if (FD_ISSET(fd, &readfds)) {
> +			xenfb_on_incoming(xenfb);
> +		}
> +		
> +		FD_ZERO(&readfds);
> +		FD_SET(fd, &readfds);
> +	}
> +
> +	rfbScreenCleanup(server);
> +	xenfb_delete(xenfb);
> +
> +	return 0;
> +}
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/tools/xenfb/xenfb.c	Sat Sep 02 15:19:25 2006 -0400
> @@ -0,0 +1,434 @@
> +#include <malloc.h>
> +#include <stdlib.h>
> +#include <sys/types.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <xenctrl.h>
> +#include <linux/xenfb.h>
> +#include <linux/xenkbd.h>
> +#include <sys/select.h>
> +#include <stdbool.h>
> +#include <xen/linux/evtchn.h>
> +#include <xen/event_channel.h>
> +#include <sys/mman.h>
> +#include <errno.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <time.h>
> +#include <xs.h>
> +
> +#include "xenfb.h"
> +
> +// FIXME defend against malicous backend?
Well, this is the backend, so defending against a malicious frontend
would be a better choice.
> +
> +struct xenfb_private
> +{
> +	struct xenfb pub;
> +	int domid;
> +	unsigned long fbdev_mfn, kbd_mfn;
> +	int fbdev_evtchn, kbd_evtchn;
> +	evtchn_port_t fbdev_port, kbd_port;
How do {fbdev,kbd}_port differ from {fbdev,kbd}_evtchn?

The _evtchn fields are only ever accessed from xenfb_attach_dom.  Could
they be locals to that function?
> +	int evt_xch;
> +	int xc;
> +	unsigned char *fb;
> +	struct xenfb_page *fb_info;
> +	struct xenkbd_info *kbd_info;
> +	unsigned long *fbmfns;
> +	int n_fbmfns, n_fbdirs;
> +};
> +
> +struct xenfb *xenfb_new(void)
> +{
> +	struct xenfb_private *xenfb = malloc(sizeof(*xenfb));
> +
> +	if (xenfb == NULL)
> +		return NULL;
> +
> +	memset(xenfb, 0, sizeof(*xenfb));
Use calloc instead of malloc, perhaps?
> +
> +	xenfb->domid = -1;
> +
> +	xenfb->evt_xch = xc_evtchn_open();
> +	if (xenfb->evt_xch == -1) {
> +		int serrno = errno;
> +		free(xenfb);
> +		errno = serrno;
> +		return NULL;
> +	}
> +
> +	xenfb->xc = xc_interface_open();
> +	if (xenfb->xc == -1) {
> +		int serrno = errno;
> +		xc_evtchn_close(xenfb->evt_xch);
> +		free(xenfb);
> +		errno = serrno;
> +		return NULL;
It''s a pity we don''t have a macro which hides this ugliness. 
Perhaps

#define PRESERVING_ERRNO(x) do {
	int tmp = errno;
	x;
	errno = tmp;
} while (0)

You could then do something like

if (xenfb_evt_sch == -1) {
	PRESERVING_ERRNO(xc_evtchn_close(xenfb->evt_xch);free(xenfb));
	return NULL;
}

Not sure whether that''s more or less ugly, to be honest.
> +	}
> +
> +	return &xenfb->pub;
> +}
> +
> +int xenfb_get_fileno(struct xenfb *xenfb_pub)
> +{
> +	struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
> +
> +	return xc_evtchn_fd(xenfb->evt_xch);
> +}
> +
> +static void xenfb_detach_dom(struct xenfb_private *xenfb)
> +{
> +	xenfb->domid = -1;
> +	munmap(xenfb->fb, xenfb->fb_info->mem_length);
> +	munmap(xenfb->fb_info, XC_PAGE_SIZE);
> +	munmap(xenfb->kbd_info, XC_PAGE_SIZE);
> +	xc_evtchn_unbind(xenfb->evt_xch, xenfb->fbdev_port);
> +	xc_evtchn_unbind(xenfb->evt_xch, xenfb->kbd_port);
> +}
> +
> +void xenfb_delete(struct xenfb *xenfb_pub)
> +{
> +	struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
> +	if (xenfb->domid != -1)
> +		xenfb_detach_dom(xenfb);
> +	free(xenfb);
> +}
> +
> +static int xenfb_fb_event(struct xenfb_private *xenfb, union
xenfb_in_event *event)
> +{
> +	uint32_t prod;
> +	struct xenfb_page *info = xenfb->fb_info;
> +
> +	prod = info->in_prod;
> +	if (prod - info->in_cons == XENFB_IN_RING_LEN) {
> +	    errno = EAGAIN;
> +	    return -1;
> +	}
> +
> +	mb();			/* ensure ring space available */
> +	XENFB_IN_RING_REF(info, prod) = *event;
> +	wmb();			/* ensure ring contents visible */
> +	info->in_prod = prod + 1;
> +	return xc_evtchn_notify(xenfb->evt_xch, xenfb->fbdev_port);
> +}
> +
> +static int xenfb_kbd_event(struct xenfb_private *xenfb, union
xenkbd_in_event *event)
> +{
> +	uint32_t prod;
> +	struct xenkbd_info *info = xenfb->kbd_info;
> +
> +	prod = info->in_prod;
> +	if (prod - info->in_cons == XENKBD_IN_RING_LEN) {
> +	    errno = EAGAIN;
> +	    return -1;
> +	}
> +
> +	mb();			/* ensure ring space available */
> +	XENKBD_IN_RING_REF(info, prod) = *event;
> +	wmb();			/* ensure ring contents visible */
> +	info->in_prod = prod + 1;
> +	return xc_evtchn_notify(xenfb->evt_xch, xenfb->kbd_port);
> +}
> +
> +static char *xenfb_path_in_dom(struct xs_handle *h,
> +			 unsigned domid, const char *path,
> +			 char *buffer, size_t size)
> +{
> +	char *domp = xs_get_domain_path(h, domid);
Can fail.
> +	int n = snprintf(buffer, size, "%s/%s", domp, path);
> +	free(domp);
> +	if (n >= size)
> +		return NULL;
> +	return buffer;
> +}
> +
> +static int xenfb_xs_scanf1(struct xs_handle *xsh, unsigned domid,
> +			   const char *path, const char *fmt,
> +			   void *dest)
> +{
> +	char buffer[1024];
> +	char *p;
> +	int ret;
> +
> +	p = xenfb_path_in_dom(xsh, domid, path, buffer, sizeof(buffer));
What happens if this fails?
> +	p = xs_read(xsh, XBT_NULL, p, NULL);
> +	if (!p)
> +		return -ENOENT;
> +	ret = sscanf(p, fmt, dest);
> +	free(p);
> +	if (ret != 1)
> +		return -EDOM;
> +	return 0;
> +}
You''re somewhat inconsistent about returning error numbers as negative
return values or through errno.  I''d prefer the latter in userspace
code, but it doesn''t matter too much, privided you pick one.
> +
> +bool xenfb_attach_dom(struct xenfb *xenfb_pub, int domid)
> +{
> +	struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
> +	char buffer[1024];
> +	struct xs_handle *xsh;
> +	unsigned dummy;
> +	int ret;
> +	char *p, **vec;
> +	union xenfb_in_event event;
> +
> +	if (xenfb->domid != -1) {
> +		xenfb_detach_dom(xenfb);
> +		if (domid == -1)
> +			return true;
> +	}
> +
> +	xsh = xs_daemon_open_readonly();
> +	if (!xsh)
> +	    goto error;
> +
> +	p = xenfb_path_in_dom(xsh, domid, "vfb", buffer,
sizeof(buffer));
> +	if (!xs_watch(xsh, p, ""))
> +		goto error;
> +	p = xenfb_path_in_dom(xsh, domid, "vkbd", buffer,
sizeof(buffer));
> +	if (!xs_watch(xsh, p, ""))
> +		goto error;
> +
> +	for (;;) {
> +		ret = xenfb_xs_scanf1(xsh, domid, "vfb/page-ref",
"%lu",
> +				      &xenfb->fbdev_mfn);
> +		if (ret == -ENOENT || ret == -EAGAIN)
xenfb_xs_scanf can''t return -EAGAIN.  What are you trying to achieve
here?
> +			goto wait;
> +		if (ret < 0)
> +			goto error;
> +		ret = xenfb_xs_scanf1(xsh, domid, "vfb/event-channel",
"%u",
> +				      &xenfb->fbdev_evtchn);
> +		if (ret == -ENOENT || ret == -EAGAIN)
> +			goto wait;
> +		if (ret < 0)
> +			goto error;
> +		ret = xenfb_xs_scanf1(xsh, domid, "vkbd/page-ref",
"%lu",
> +				      &xenfb->kbd_mfn);
> +		if (ret == -ENOENT || ret == -EAGAIN)
> +			goto wait;
> +		if (ret < 0)
> +			goto error;
> +		ret = xenfb_xs_scanf1(xsh, domid, "vkbd/event-channel",
"%u",
> +				      &xenfb->kbd_evtchn);
> +		if (ret == -ENOENT || ret == -EAGAIN)
> +			goto wait;
> +		if (ret < 0)
> +			goto error;
> +		break;
> +
> +	wait:
> +		printf("Waiting...\n");
Where does this message go?
> +		vec = xs_read_watch(xsh, &dummy);
> +		if (!vec)
> +			goto error;
> +		free(vec);
> +	}
> +	xs_daemon_close(xsh);
> +	xsh = NULL;
> +
> +	xenfb->fbdev_port = xc_evtchn_bind_interdomain(xenfb->evt_xch,
domid,
> +						       xenfb->fbdev_evtchn);
> +	if (xenfb->fbdev_port == -1)
> +		goto error;
> +
> +	xenfb->kbd_port = xc_evtchn_bind_interdomain(xenfb->evt_xch, domid,
> +						     xenfb->kbd_evtchn);
> +	if (xenfb->kbd_port == -1) 
> +		goto error_fbdev;
> +
> +	xenfb->fb_info = xc_map_foreign_range(xenfb->xc, domid,
XC_PAGE_SIZE,
> +					      PROT_READ | PROT_WRITE,
> +					      xenfb->fbdev_mfn);
> +	if (xenfb->fb_info == NULL)
> +		goto error_kbd;
> +
> +	xenfb->kbd_info = xc_map_foreign_range(xenfb->xc, domid,
XC_PAGE_SIZE,
> +					       PROT_READ | PROT_WRITE,
> +					       xenfb->kbd_mfn);
> +	if (xenfb->kbd_info == NULL)
> +		goto error_fbinfo;
> +
> +	xenfb->n_fbmfns = (xenfb->fb_info->mem_length + (XC_PAGE_SIZE -
1)) / XC_PAGE_SIZE;
> +	xenfb->n_fbdirs = xenfb->n_fbmfns * sizeof(unsigned long);
> +	xenfb->n_fbdirs = (xenfb->n_fbdirs + (XC_PAGE_SIZE - 1)) /
XC_PAGE_SIZE;
> +
> +	xenfb->fbmfns = xc_map_foreign_batch(xenfb->xc, domid, PROT_READ,
xenfb->fb_info->pd, xenfb->n_fbdirs);
> +	if (xenfb->fbmfns == NULL)
> +		goto error_kbdinfo;
> +
> +	xenfb->fb = xc_map_foreign_batch(xenfb->xc, domid, PROT_READ |
PROT_WRITE, xenfb->fbmfns, xenfb->n_fbmfns);
> +	if (xenfb->fb == NULL)
> +		goto error_fbmfns;
> +
> +	event.type = XENFB_TYPE_SET_EVENTS;
> +	event.set_events.flags = XENFB_FLAG_UPDATE;
> +	if (xenfb_fb_event(xenfb, &event))
> +		goto error_fb;
> +
> +	munmap(xenfb->fbmfns, xenfb->n_fbdirs * XC_PAGE_SIZE);
Please make fbmfns a local rather than putting it in the info
structure.
> +
> +	xenfb->domid = domid;
> +
> +	xenfb->pub.pixels = xenfb->fb;
> +
> +	xenfb->pub.row_stride = xenfb->fb_info->line_length;
> +	xenfb->pub.depth = xenfb->fb_info->depth;
> +	xenfb->pub.width = xenfb->fb_info->width;
> +	xenfb->pub.height = xenfb->fb_info->height;
> +
> +	return true;
> +
> + error_fb:
The error path here is utterly revolting.  Perhaps something like this:

error:
	serrno = errno;
	if (xenfb->fb)
		munmap(xenfb->fb, xenfb->fb_info->mem_length);
	if (fbmfns)
		munmap(fbmfns, xenfb->fb_info->mem_length);
	...
	errno = serrno;

	return false;

Or would that be too easy?
> +	printf("%d\n", __LINE__);
> +	{
> +		int serrno = errno;
> +		munmap(xenfb->fb, xenfb->fb_info->mem_length);
> +		errno = serrno;
> +	}
> + error_fbmfns:
> +	printf("%d\n", __LINE__);
> +	{
> +		int serrno = errno;
> +		munmap(xenfb->fbmfns, xenfb->n_fbdirs * XC_PAGE_SIZE);
> +		errno = serrno;
> +	}
> + error_kbdinfo:
> +	printf("%d\n", __LINE__);
> +	{
> +		int serrno = errno;
> +		munmap(xenfb->kbd_info, XC_PAGE_SIZE);
> +		errno = serrno;
> +	}
> + error_fbinfo:
> +	printf("%d\n", __LINE__);
> +	{
> +		int serrno = errno;
> +		munmap(xenfb->fb_info, XC_PAGE_SIZE);
> +		errno = serrno;
> +	}
> + error_kbd:
> +	printf("%d\n", __LINE__);
> +	{
> +		int serrno = errno;
> +		xc_evtchn_unbind(xenfb->evt_xch, xenfb->kbd_port);
> +		errno = serrno;
> +	}
> + error_fbdev:
> +	printf("%d\n", __LINE__);
> +	{
> +		int serrno = errno;
> +		xc_evtchn_unbind(xenfb->evt_xch, xenfb->fbdev_port);
> +		errno = serrno;
> +	}
> + error:
> +	printf("%d\n", __LINE__);
> +	if (xsh) {
> +		int serrno = errno;
> +		xs_daemon_close(xsh);
I think you may end up closing the connection to the daemon twice here.
> +		errno = serrno;
> +	}
> +
> +	return false;
> +}
> +
> +static void xenfb_update(struct xenfb_private *xenfb, int x, int y, int
width, int height)
> +{
> +	if (xenfb->pub.update)
> +		xenfb->pub.update(&xenfb->pub, x, y, width, height);
> +}
I''m not convinced this wrapper is actually needed, given that
it''s
utterly trivial and only called from one place.
> +
> +static void xenfb_on_fb_event(struct xenfb_private *xenfb)
> +{
> +	uint32_t prod, cons;
> +	struct xenfb_page *info = xenfb->fb_info;
> +
> +	prod = info->out_prod;
> +	rmb();			/* ensure we see ring contents up to prod */
> +	for (cons = info->out_cons; cons != prod; cons++) {
> +		union xenfb_out_event *event = &XENFB_OUT_RING_REF(info, cons);
> +
> +		switch (event->type) {
> +		case XENFB_TYPE_UPDATE:
> +			xenfb_update(xenfb, event->update.x, event->update.y,
event->update.width, event->update.height);
> +			break;
> +		}
> +	}
> +	mb();			/* ensure we''re done with ring contents */
> +	info->out_cons = cons;
> +	// FIXME need to notify?
Maybe.  If there''s any possibility of the frontend queuing evetns when
the ring is full, yes.  It doesn''t at the moment, but if you want to
add it in the future and maintain forward compatibility you need it.
> +}
> +
> +static void xenfb_on_kbd_event(struct xenfb_private *xenfb)
> +{
> +	uint32_t prod, cons;
> +	struct xenkbd_info *info = xenfb->kbd_info;
> +
> +	prod = info->out_prod;
> +	rmb();			/* ensure we see ring contents up to prod */
> +	for (cons = info->out_cons; cons != prod; cons++) {
> +		union xenkbd_out_event *event = &XENKBD_OUT_RING_REF(info, cons);
> +
> +		switch (event->type) {
> +		default:
> +			break;
> +		}
> +	}
> +	mb();			/* ensure we''re done with ring contents */
> +	info->out_cons = cons;
> +	// FIXME need to notify?
> +}
I''d replace this with

+static void xenfb_on_kbd_event(struct xenfb_private *xenfb)
+{
+	struct xenkbd_info *info = xenfb->kbd_info;
+       /* We don''t understand any keyboard events, so just ignore
them. */
+	info->out_cons = info->out_prod;
+}

It''s smaller, easier to understand, and more efficient.

As for the FIXME, the protocol spec says you need it, but I''m not sure
it''s actually all that useful.  It will only make any difference if
the frontend starts queueing keyboard events if it finds the queue to
be full when it tries to send one.
> +
> +int xenfb_on_incoming(struct xenfb *xenfb_pub)
> +{
> +	struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
> +	evtchn_port_t port;
> +
> +	port = xc_evtchn_pending(xenfb->evt_xch);
> +	if (port == -1)
> +		return -1;
> +
> +	if (port == xenfb->fbdev_port) {
> +		xenfb_on_fb_event(xenfb);
> +	} else if (port == xenfb->kbd_port) {
> +		xenfb_on_kbd_event(xenfb);
> +	}
> +
> +	if (xc_evtchn_unmask(xenfb->evt_xch, port) == -1)
> +		return -1;
> +
> +	return 0;
> +}
> +
> +int xenfb_send_key(struct xenfb *xenfb_pub, bool down, int keycode)
> +{
> +	struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
> +	union xenkbd_in_event event;
> +
> +	event.type = XENKBD_TYPE_KEY;
> +	event.key.pressed = down ? 1 : 0;
> +	event.key.keycode = keycode;
> +
> +	return xenfb_kbd_event(xenfb, &event);
> +}
> +
> +int xenfb_send_motion(struct xenfb *xenfb_pub, int rel_x, int rel_y)
Is this sending XENKBD_TYPE_MOTION or XENFB_TYPE_MOTION?
> +{
> +	struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
> +	union xenkbd_in_event event;
> +
> +	event.type = XENKBD_TYPE_MOTION;
> +	event.motion.rel_x = rel_x;
> +	event.motion.rel_y = rel_y;
> +
> +	return xenfb_kbd_event(xenfb, &event);
> +}
> +
> +int xenfb_send_button(struct xenfb *xenfb_pub, bool down, int button)
> +{
> +	struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
> +	union xenkbd_in_event event;
> +
> +	event.type = XENKBD_TYPE_BUTTON;
> +	event.button.pressed = down ? 1 : 0;
> +	event.button.button = button;
> +
> +	return xenfb_kbd_event(xenfb, &event);
> +}
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/tools/xenfb/xenfb.h	Sat Sep 02 15:19:25 2006 -0400
> @@ -0,0 +1,33 @@
> +#ifndef _XENFB_H_
> +#define _XENFB_H_
> +
> +#include <stdbool.h>
> +#include <stdint.h>
> +
> +struct xenfb
> +{
> +	uint8_t *pixels;
> +
> +	int row_stride;
> +	int depth;
> +	int width;
> +	int height;
> +
> +	void *user_data;
> +
> +	void (*update)(struct xenfb *xenfb, int x, int y, int width, int height);
> +};
> +
> +struct xenfb *xenfb_new(void);
> +void xenfb_delete(struct xenfb *xenfb);
> +
> +bool xenfb_attach_dom(struct xenfb *xenfb, int domid);
> +
> +int xenfb_get_fileno(struct xenfb *xenfb);
> +int xenfb_on_incoming(struct xenfb *xenfb);
> +
> +int xenfb_send_key(struct xenfb *xenfb, bool down, int keycode);
> +int xenfb_send_motion(struct xenfb *xenfb, int rel_x, int rel_y);
> +int xenfb_send_button(struct xenfb *xenfb, bool down, int button);
> +
> +#endif
Steven.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Steven Smith wrote:
[...]
>> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tools/xenfb/keymapping.c	Sat Sep 02 15:19:25 2006 -0400
>> @@ -0,0 +1,141 @@
>> +#include <stdint.h>
>> +#include <gdk/gdkkeysyms.h>
>> +#include <linux/input.h>
>> +
>> +uint32_t gdk_linux_mapping[0x10000] = {
>> +	[GDK_a] = KEY_A,
> This is kind of ugly.  Is there any chance it could be autogenerated?
> Also, where did 0x10000 come from?
> 
> Also, depending on GTK just for the keymap table is a real pain.  Or
> is it already required for libvncserver?
> 
For the VNC part, as it depends on libvncserver, it should use
/usr/include/rfb/keysym.h:

+uint32_t gdk_linux_mapping[0x10000] = {
+	[XK_a] = KEY_A,

For the SDL part, I''m sorry to repeat it should use scancode instead of
symbol
id ...

Regards,
Laurent
-- 
                Laurent.Vivier@bull.net
         Bull, Architect of an Open World (TM)
+----- "Any sufficiently advanced technology is ----+
| indistinguishable from magic." - Arthur C. Clarke |



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Mon, 2006-09-04 at 10:01 +0100, Steven Smith wrote:
> > +CFLAGS += -g -Wall
> You shouldn''t need to add -g here; Rules.mk handles it for you if
> debug is set.
*nod*  -Wall gets set in Config.mk as well -- will nuke.
> > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> > +++ b/tools/xenfb/keymapping.c	Sat Sep 02 15:19:25 2006 -0400
> > @@ -0,0 +1,141 @@
> > +#include <stdint.h>
> > +#include <gdk/gdkkeysyms.h>
> > +#include <linux/input.h>
> > +
> > +uint32_t gdk_linux_mapping[0x10000] = {
> > +	[GDK_a] = KEY_A,
> This is kind of ugly.  Is there any chance it could be autogenerated?
> Also, where did 0x10000 come from?
> 
> Also, depending on GTK just for the keymap table is a real pain.  Or
> is it already required for libvncserver?
libvncserver requires GTK.  And I don''t know that there''s
really any
good way to auto-generate it unfortunately.  I somehow expect that
0x10000 came from "it''ll be big enough" but Anthony would
have to
confirm :-)

The mappings are unfortunately a bit of a fact of life since we have to
convert from what the X layer gets to what the kernel expects.  And the
two couldn''t be farther from the same.  And then it''s even
more fun when
toolkits get involved.
> > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> > +++ b/tools/xenfb/sdlfb.c	Sat Sep 02 15:19:25 2006 -0400
> > @@ -0,0 +1,191 @@
> > +#include <SDL.h>
> > +#include <sys/types.h>
> > +#include <sys/select.h>
> > +#include <stdlib.h>
> > +#include <linux/input.h>
> > +#include <getopt.h>
> > +#include <string.h>
> > +#include "xenfb.h"
> > +
> > +struct data
> That''s a really wonderful name.
Sold one SDLFBData :-)
> > +int sdl2linux[1024] = {
> > +	[SDLK_a] = KEY_A,
> Another really ugly mapping table, although not quite as bad as the
> GTK one.
[snip]
> Where''d the magic 1024 come from?
Same basic idea
> > +
> > +	while ((opt = getopt_long(argc, argv, "d:t:", options,
> > +				  NULL)) != -1) {
> > +		switch (opt) {
> > +                case ''d'':
> > +                    domid = strtol(optarg, NULL, 10);
> It''d be nice to check for a malformed argument here.
Can change the check to be for domid <= 0 below which will catch the
case of no domid being passed or strtol failing (it''ll return 0)
> > +                    break;
> > +                case ''t'':
> > +                    title = strdup(optarg);
> This can fail.
In which case title will still be NULL and we fall back to the default
which is sane.
> > +                    break;
> > +                }
> > +        }
> > +        if (optind != argc) {
> > +            fprintf(stderr, "Invalid options!\n");
> > +            exit(1);
> errx() maybe?
I tend to prefer being explicit.
> > +        }
> > +        if (domid == -1) {
> > +            fprintf(stderr, "Domain ID must be
specified!\n");
> > +            exit(1);
> > +        }
> > +
> > +	xenfb = xenfb_new();
> > +	if (xenfb == NULL)
> > +		return 1;
> Why have you used exit(1) in some places and return 1 in others?
I''m guessing that some are Anthony''s checks and some are later
added by
Markus and/or myself.
> Also, an error message here would be a good idea.
Agreed on the error messages, will add
> > +        if (title == NULL)
> > +            title = strdup("xen-sdlfb");
> This can fail.
At which point you just end up without a window title.

[snip]
> > +	while (!do_quit && select(fd + 1, &readfds, NULL, NULL,
&tv) != -1) {
> Select can say -1 because of EINTR (e.g. when strace attaches). 
It''s
> not clear to me whether you want to exit or retry in that case.
> 
> Also, if you quit because select returns -1, you need an error
> message.
Reasonable enough and easy to add

[snip]
> > +		tv = (struct timeval){0, 500};
> I think 500us is a little short here.  About ten milliseconds sounds
> more plausible.  This is a bit of a bikeshed.
> 
> It''s a pity SDL doesn''t allow you to wait for either an
SDL event or
> an fd to become readable.  Could you do something like spawn a thread
> which does the selects in a loop, and then SDL_PushEvent()s a user
> event when the fd becomes readable?
>
> Admittedly, SDL_WaitEvent also contains this kind of loop-with-sleep,
> but it''d reduce the number of magic tunables a bit.
I don''t see why we wouldn''t just want to use SDL_WaitEvent
unless I''m
just not awake enough...
> > --- b/tools/xenfb/vncfb.c	Sat Sep 02 15:19:25 2006 -0400
> > +++ b/tools/xenfb/vncfb.c	Sat Sep 02 15:22:19 2006 -0400
> 
> Minor nit: generally, putting a vnc_ prefix on these functions
> confused me, since it looks like they should be in libvncserver.  This
> may just be because I''m not paying enough attention.
Maybe use xenvnc for the things here just to make it more clear?
Although I think the vncserver stuff is actually prefixed with rfb just
to make things extra exciting :)
> > +static void on_kbd_event(rfbBool down, rfbKeySym keycode,
rfbClientPtr cl)
> > +{
> > +	extern uint32_t gdk_linux_mapping[0x10000];
> Is there any chance of moving this into a header file somewhere?
I don''t see any reason why keymapping.c couldn''t be
keymapping.h and
just #include''d

[snip]
> > +		for (i = 0; i < 8; i++) {
> > +			if ((last_button & (1 << i)) !> > +			   
(buttonMask & (1 << i))) {
> > +				printf("%d %d\n", buttonMask & (1 << i), i);
> Umm?
Debug creeping in
> > +
> > +    buf = malloc(256);
> Could fail.  Also, consider using asprintf.
Yeah, asprintf makes lots of sense, changed
> > +    if (snprintf(buf, 256, "%s/console/vnc-port", path) ==
-1)
> > +	goto out;
> > +
> > +    portstr = malloc(10);
> Why is this on the heap rather than the stack?
I''ve been bitten too many times with variables on the stack and calling
conventions on bizarre arches
> > +static int vnc_start_viewer(int port) 
> I''m not convinced the backend server process is the best place to
> start the viewer from.  Perhaps xend would be a better choice?  Not
> sure about this.
I did it here to keep similarity with how FV works -- the viewer is
spawned there by qemu.  So while I also could go either way, I think the
important thing is having it be the same for FV and PV.

[snip]
> This is ignored.  Also, the parent process makes no attempt to check
> whether the child was exec()ed successfully or anything along those
> lines.  This is enough of a pain to fix that I''d probably just
ignore
> it, though.
Yeah -- and even if the parent noticed, it''s not going to actually do
much useful.  This is pretty much purely from qemu
> > +	struct xenfb *xenfb;
> > +	fd_set readfds;
> > +	int fd;
> > +	char buffer[1024];
> Could do with a better name, and is larger than it needs to be.
Will rename to portstr and make it smaller
> > +        int opt;
> > +        bool unused = FALSE;
> You''re inconsistent about the capitalisation of bools.
Consistent on a per-file basis -- have a preference?
> > +        bool viewer = FALSE;
> > +
> > +	while ((opt = getopt_long(argc, argv, "d:p:t:u", options,
> > +				  NULL)) != -1) {
> > +		switch (opt) {
> > +                case ''d'':
> > +                    domid = strtol(optarg, NULL, 10);
> It would be nice to sanity check the argument here.
Will do sanity checking on the argument handling the same as with sdlfb
> > +        if (listen != NULL)
> > +            fake_argv[6] = listen;
> > +
> > +        if (listen != NULL)
> > +            fake_argv[6] = listen;
> Umm... What''s going on here?
Merge error between a few trees

[snip]
> > +
> > +struct xenfb_private
> > +{
> > +	struct xenfb pub;
> > +	int domid;
> > +	unsigned long fbdev_mfn, kbd_mfn;
> > +	int fbdev_evtchn, kbd_evtchn;
> > +	evtchn_port_t fbdev_port, kbd_port;
> How do {fbdev,kbd}_port differ from {fbdev,kbd}_evtchn?
_evtchn the actual remote port ({vfb,vkbd}/event-channel) and _port is
the event port
> The _evtchn fields are only ever accessed from xenfb_attach_dom.  Could
> they be locals to that function?
They could be -- not sure what it actually buys

[snip]
> > +	struct xenfb_private *xenfb = malloc(sizeof(*xenfb));
> > +
> > +	if (xenfb == NULL)
> > +		return NULL;
> > +
> > +	memset(xenfb, 0, sizeof(*xenfb));
> Use calloc instead of malloc, perhaps?
I have some vague recollection of calloc() bad, but my neurons aren''t
firing to tell me why I remember that
> > +	if (xenfb->xc == -1) {
> > +		int serrno = errno;
> > +		xc_evtchn_close(xenfb->evt_xch);
> > +		free(xenfb);
> > +		errno = serrno;
> > +		return NULL;
> It''s a pity we don''t have a macro which hides this
ugliness.  Perhaps
> 
> #define PRESERVING_ERRNO(x) do {
> 	int tmp = errno;
> 	x;
> 	errno = tmp;
> } while (0)
> 
> You could then do something like
> 
> if (xenfb_evt_sch == -1) {
> 	PRESERVING_ERRNO(xc_evtchn_close(xenfb->evt_xch);free(xenfb));
> 	return NULL;
> }
> 
> Not sure whether that''s more or less ugly, to be honest.
I think it makes it a little bit less clear what''s going on.  But
that''s
just me
> > +static char *xenfb_path_in_dom(struct xs_handle *h,
> > +			 unsigned domid, const char *path,
> > +			 char *buffer, size_t size)
> > +{
> > +	char *domp = xs_get_domain_path(h, domid);
> Can fail.
Catching that case
> > +static int xenfb_xs_scanf1(struct xs_handle *xsh, unsigned domid,
> > +			   const char *path, const char *fmt,
> > +			   void *dest)
> > +{
> > +	char buffer[1024];
> > +	char *p;
> > +	int ret;
> > +
> > +	p = xenfb_path_in_dom(xsh, domid, path, buffer, sizeof(buffer));
> What happens if this fails?
I think we should probably do the wait and loop -- changed so that it
actually happens
> > +	p = xs_read(xsh, XBT_NULL, p, NULL);
> > +	if (!p)
> > +		return -ENOENT;
> > +	ret = sscanf(p, fmt, dest);
> > +	free(p);
> > +	if (ret != 1)
> > +		return -EDOM;
> > +	return 0;
> > +}
> You''re somewhat inconsistent about returning error numbers as
negative
> return values or through errno.  I''d prefer the latter in
userspace
> code, but it doesn''t matter too much, privided you pick one.
Switched to the latter -- I think it''s actually on here where
it''s
inconsistent.
> > +	for (;;) {
> > +		ret = xenfb_xs_scanf1(xsh, domid, "vfb/page-ref",
"%lu",
> > +				      &xenfb->fbdev_mfn);
> > +		if (ret == -ENOENT || ret == -EAGAIN)
> xenfb_xs_scanf can''t return -EAGAIN.  What are you trying to
achieve
> here?
I can''t really see anything either -- simplified
> > +	wait:
> > +		printf("Waiting...\n");
> Where does this message go?
With it being spawned from xend, it''ll go to xend-debug.log
> > +	xenfb->fb = xc_map_foreign_batch(xenfb->xc, domid, PROT_READ |
PROT_WRITE, xenfb->fbmfns, xenfb->n_fbmfns);
> > +	if (xenfb->fb == NULL)
> > +		goto error_fbmfns;
> > +
> > +	event.type = XENFB_TYPE_SET_EVENTS;
> > +	event.set_events.flags = XENFB_FLAG_UPDATE;
> > +	if (xenfb_fb_event(xenfb, &event))
> > +		goto error_fb;
> > +
> > +	munmap(xenfb->fbmfns, xenfb->n_fbdirs * XC_PAGE_SIZE);
> Please make fbmfns a local rather than putting it in the info
> structure.
Done
> > +
> > +	xenfb->domid = domid;
> > +
> > +	xenfb->pub.pixels = xenfb->fb;
> > +
> > +	xenfb->pub.row_stride = xenfb->fb_info->line_length;
> > +	xenfb->pub.depth = xenfb->fb_info->depth;
> > +	xenfb->pub.width = xenfb->fb_info->width;
> > +	xenfb->pub.height = xenfb->fb_info->height;
> > +
> > +	return true;
> > +
> > + error_fb:
> The error path here is utterly revolting.  Perhaps something like this:
Heh, indeed :-)  I don''t see why that can''t work
> > +		xs_daemon_close(xsh);
> I think you may end up closing the connection to the daemon twice here.
I don''t see how we could close it twice -- and if we do
> > +static void xenfb_update(struct xenfb_private *xenfb, int x, int y,
int width, int height)
> > +{
> > +	if (xenfb->pub.update)
> > +		xenfb->pub.update(&xenfb->pub, x, y, width, height);
> > +}
> I''m not convinced this wrapper is actually needed, given that
it''s
> utterly trivial and only called from one place.
Sure! 
> > +static void xenfb_on_kbd_event(struct xenfb_private *xenfb)
[snip]
> I''d replace this with
Doesn''t this go back to "we need to consume events out of the ring
to
remain compatible if there are later events that could be understood"?
> > +
> > +int xenfb_on_incoming(struct xenfb *xenfb_pub)
> > +{
> > +	struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
> > +	evtchn_port_t port;
> > +
> > +	port = xc_evtchn_pending(xenfb->evt_xch);
> > +	if (port == -1)
> > +		return -1;
> > +
> > +	if (port == xenfb->fbdev_port) {
> > +		xenfb_on_fb_event(xenfb);
> > +	} else if (port == xenfb->kbd_port) {
> > +		xenfb_on_kbd_event(xenfb);
> > +	}
> > +
> > +	if (xc_evtchn_unmask(xenfb->evt_xch, port) == -1)
> > +		return -1;
> > +
> > +	return 0;
> > +}
> > +
> > +int xenfb_send_key(struct xenfb *xenfb_pub, bool down, int keycode)
> > +{
> > +	struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
> > +	union xenkbd_in_event event;
> > +
> > +	event.type = XENKBD_TYPE_KEY;
> > +	event.key.pressed = down ? 1 : 0;
> > +	event.key.keycode = keycode;
> > +
> > +	return xenfb_kbd_event(xenfb, &event);
> > +}
> > +
> > +int xenfb_send_motion(struct xenfb *xenfb_pub, int rel_x, int rel_y)
> Is this sending XENKBD_TYPE_MOTION or XENFB_TYPE_MOTION?
It''s a "keyboard" event, so XENKBD_TYPE_MOTION

Jeremy


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jeremy Katz wrote:
> On Mon, 2006-09-04 at 10:01 +0100, Steven Smith wrote:
>   
>>> +CFLAGS += -g -Wall
>>>       
>> You shouldn''t need to add -g here; Rules.mk handles it for you
if
>> debug is set.
>>     
>
> *nod*  -Wall gets set in Config.mk as well -- will nuke.
>
>   
>>> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>>> +++ b/tools/xenfb/keymapping.c	Sat Sep 02 15:19:25 2006 -0400
>>> @@ -0,0 +1,141 @@
>>> +#include <stdint.h>
>>> +#include <gdk/gdkkeysyms.h>
>>> +#include <linux/input.h>
>>> +
>>> +uint32_t gdk_linux_mapping[0x10000] = {
>>> +	[GDK_a] = KEY_A,
>>>       
>> This is kind of ugly.  Is there any chance it could be autogenerated?
>> Also, where did 0x10000 come from?
>>
>> Also, depending on GTK just for the keymap table is a real pain.  Or
>> is it already required for libvncserver?
>>     
>
> libvncserver requires GTK.  And I don''t know that there''s
really any
> good way to auto-generate it unfortunately.  I somehow expect that
> 0x10000 came from "it''ll be big enough" but Anthony
would have to
> confirm :-)
>   
That''s the biggest that a GDK scan code can currently be.  That way, we
can use a simple indexed table.

Regards,

Anthony Liguori
> The mappings are unfortunately a bit of a fact of life since we have to
> convert from what the X layer gets to what the kernel expects.  And the
> two couldn''t be farther from the same.  And then it''s
even more fun when
> toolkits get involved.
>   

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> > > +CFLAGS += -g -Wall
> > You shouldn''t need to add -g here; Rules.mk handles it for
you if
> > debug is set.
> *nod*  -Wall gets set in Config.mk as well -- will nuke.
Thanks.
> > > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> > > +++ b/tools/xenfb/keymapping.c	Sat Sep 02 15:19:25 2006 -0400
> > > @@ -0,0 +1,141 @@
> > > +#include <stdint.h>
> > > +#include <gdk/gdkkeysyms.h>
> > > +#include <linux/input.h>
> > > +
> > > +uint32_t gdk_linux_mapping[0x10000] = {
> > > +	[GDK_a] = KEY_A,
> > This is kind of ugly.  Is there any chance it could be autogenerated?
> > Also, where did 0x10000 come from?
> > 
> > Also, depending on GTK just for the keymap table is a real pain.  Or
> > is it already required for libvncserver?
> libvncserver requires GTK.  And I don''t know that there''s
really any
> good way to auto-generate it unfortunately.  I somehow expect that
> 0x10000 came from "it''ll be big enough" but Anthony
would have to
> confirm :-)
> 
> The mappings are unfortunately a bit of a fact of life since we have to
> convert from what the X layer gets to what the kernel expects.  And the
> two couldn''t be farther from the same.  And then it''s
even more fun when
> toolkits get involved.
Yep.
> 
> > > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> > > +++ b/tools/xenfb/sdlfb.c	Sat Sep 02 15:19:25 2006 -0400
> > > @@ -0,0 +1,191 @@
> > > +#include <SDL.h>
> > > +#include <sys/types.h>
> > > +#include <sys/select.h>
> > > +#include <stdlib.h>
> > > +#include <linux/input.h>
> > > +#include <getopt.h>
> > > +#include <string.h>
> > > +#include "xenfb.h"
> > > +
> > > +struct data
> > That''s a really wonderful name.
> Sold one SDLFBData :-)
Thanks.
> > > +int sdl2linux[1024] = {
> > > +	[SDLK_a] = KEY_A,
> > Another really ugly mapping table, although not quite as bad as the
> > GTK one.
> [snip]
> > Where''d the magic 1024 come from?
> Same basic idea
You could use SDLK_LAST here, instead.
> > > +
> > > +	while ((opt = getopt_long(argc, argv, "d:t:",
options,
> > > +				  NULL)) != -1) {
> > > +		switch (opt) {
> > > +                case ''d'':
> > > +                    domid = strtol(optarg, NULL, 10);
> > It''d be nice to check for a malformed argument here.
> 
> Can change the check to be for domid <= 0 below which will catch the
> case of no domid being passed or strtol failing (it''ll return 0)
If strtol is passed a string like "10aaa" it''ll return 10
rather than
0.  The correct way to check for errors here is to pass in an endp and
make sure it points at a nul when you''re finished.

(You may think I''m being overly pedantic here.  You may well be right,
but this is an easy thing to fix)
> 
> > > +                    break;
> > > +                case ''t'':
> > > +                    title = strdup(optarg);
> > This can fail.
> In which case title will still be NULL and we fall back to the default
> which is sane.
Fair enough.
> > > +                    break;
> > > +                }
> > > +        }
> > > +        if (optind != argc) {
> > > +            fprintf(stderr, "Invalid options!\n");
> > > +            exit(1);
> > errx() maybe?
> I tend to prefer being explicit.
Okay.
> > > +        }
> > > +        if (domid == -1) {
> > > +            fprintf(stderr, "Domain ID must be
specified!\n");
> > > +            exit(1);
> > > +        }
> > > +
> > > +	xenfb = xenfb_new();
> > > +	if (xenfb == NULL)
> > > +		return 1;
> > Why have you used exit(1) in some places and return 1 in others?
> I''m guessing that some are Anthony''s checks and some are
later added by
> Markus and/or myself.
Easily done, but I''d prefer to have one consistent way of doing it.
> > Also, an error message here would be a good idea.
> Agreed on the error messages, will add
Thanks.
> > > +        if (title == NULL)
> > > +            title = strdup("xen-sdlfb");
> > This can fail.
> At which point you just end up without a window title.
Okay.  I didn''t think SDL_WM_SetCaption allowed NULLs, but I was
apparently wrong.  Sorry about that.
> [snip]
> > > +	while (!do_quit && select(fd + 1, &readfds, NULL,
NULL, &tv) != -1) {
> > Select can say -1 because of EINTR (e.g. when strace attaches). 
It''s
> > not clear to me whether you want to exit or retry in that case.
> > 
> > Also, if you quit because select returns -1, you need an error
> > message.
> Reasonable enough and easy to add
Thanks.
> [snip]
> > > +		tv = (struct timeval){0, 500};
> > I think 500us is a little short here.  About ten milliseconds sounds
> > more plausible.  This is a bit of a bikeshed.
> > 
> > It''s a pity SDL doesn''t allow you to wait for either
an SDL event or
> > an fd to become readable.  Could you do something like spawn a thread
> > which does the selects in a loop, and then SDL_PushEvent()s a user
> > event when the fd becomes readable?
> >
> > Admittedly, SDL_WaitEvent also contains this kind of loop-with-sleep,
> > but it''d reduce the number of magic tunables a bit.
> I don''t see why we wouldn''t just want to use
SDL_WaitEvent unless I''m
> just not awake enough...
We need to wait for either event channel messages or SDL events to
come in, which means we need to be select()ing on the event channel fd
while we''re waiting.  SDL_WaitEvent doesn''t allow that, unless
I''m
missing something.
> > > --- b/tools/xenfb/vncfb.c	Sat Sep 02 15:19:25 2006 -0400
> > > +++ b/tools/xenfb/vncfb.c	Sat Sep 02 15:22:19 2006 -0400
> > 
> > Minor nit: generally, putting a vnc_ prefix on these functions
> > confused me, since it looks like they should be in libvncserver.  This
> > may just be because I''m not paying enough attention.
> Maybe use xenvnc for the things here just to make it more clear?
> Although I think the vncserver stuff is actually prefixed with rfb just
> to make things extra exciting :)
Whatever''s easier.
> > > +static void on_kbd_event(rfbBool down, rfbKeySym keycode,
rfbClientPtr cl)
> > > +{
> > > +	extern uint32_t gdk_linux_mapping[0x10000];
> > Is there any chance of moving this into a header file somewhere?
> I don''t see any reason why keymapping.c couldn''t be
keymapping.h and
> just #include''d
Yes, if it''s only used from one place.
> > > +
> > > +    buf = malloc(256);
> > Could fail.  Also, consider using asprintf.
> Yeah, asprintf makes lots of sense, changed
Thanks.
> > > +    if (snprintf(buf, 256, "%s/console/vnc-port",
path) == -1)
> > > +	goto out;
> > > +
> > > +    portstr = malloc(10);
> > Why is this on the heap rather than the stack?
> I''ve been bitten too many times with variables on the stack and
calling
> conventions on bizarre arches
What do you think will go wrong if this is on the stack?  Why doesn''t
it apply to portstr in main()?
> > > +static int vnc_start_viewer(int port) 
> > I''m not convinced the backend server process is the best
place to
> > start the viewer from.  Perhaps xend would be a better choice?  Not
> > sure about this.
> I did it here to keep similarity with how FV works -- the viewer is
> spawned there by qemu.  So while I also could go either way, I think the
> important thing is having it be the same for FV and PV.
I''m not entirely convinced qemu is the right place to start vncviewer
from, either. :) We''re not going to change that now, though, and
consistency is a legitimate reason to do this here.
> 
> [snip]
> > This is ignored.  Also, the parent process makes no attempt to check
> > whether the child was exec()ed successfully or anything along those
> > lines.  This is enough of a pain to fix that I''d probably
just ignore
> > it, though.
> Yeah -- and even if the parent noticed, it''s not going to actually
do
> much useful.  This is pretty much purely from qemu
True.
> > > +	struct xenfb *xenfb;
> > > +	fd_set readfds;
> > > +	int fd;
> > > +	char buffer[1024];
> > Could do with a better name, and is larger than it needs to be.
> Will rename to portstr and make it smaller
Thanks.
> 
> > > +        int opt;
> > > +        bool unused = FALSE;
> > You''re inconsistent about the capitalisation of bools.
> Consistent on a per-file basis -- have a preference?
stdbool.h uses lowercase, so lets go with that.
> > > +        bool viewer = FALSE;
> > > +
> > > +	while ((opt = getopt_long(argc, argv, "d:p:t:u",
options,
> > > +				  NULL)) != -1) {
> > > +		switch (opt) {
> > > +                case ''d'':
> > > +                    domid = strtol(optarg, NULL, 10);
> > It would be nice to sanity check the argument here.
> Will do sanity checking on the argument handling the same as with sdlfb
Thanks.
> > > +        if (listen != NULL)
> > > +            fake_argv[6] = listen;
> > > +
> > > +        if (listen != NULL)
> > > +            fake_argv[6] = listen;
> > Umm... What''s going on here?
> Merge error between a few trees
Okay.
> > > +
> > > +struct xenfb_private
> > > +{
> > > +	struct xenfb pub;
> > > +	int domid;
> > > +	unsigned long fbdev_mfn, kbd_mfn;
> > > +	int fbdev_evtchn, kbd_evtchn;
> > > +	evtchn_port_t fbdev_port, kbd_port;
> > How do {fbdev,kbd}_port differ from {fbdev,kbd}_evtchn?
> _evtchn the actual remote port ({vfb,vkbd}/event-channel) and _port is
> the event port
A comment to that effect would be nice.
> > The _evtchn fields are only ever accessed from xenfb_attach_dom. 
Could
> > they be locals to that function?
> They could be -- not sure what it actually buys
Making global variables local usually makes things easier to
understand.  I''d expect that would probably be the case here.
> > > +	struct xenfb_private *xenfb = malloc(sizeof(*xenfb));
> > > +
> > > +	if (xenfb == NULL)
> > > +		return NULL;
> > > +
> > > +	memset(xenfb, 0, sizeof(*xenfb));
> > Use calloc instead of malloc, perhaps?
> I have some vague recollection of calloc() bad, but my neurons
aren''t
> firing to tell me why I remember that
Well, alright then, although I can''t immediately see what the problem
is.
> > > +	if (xenfb->xc == -1) {
> > > +		int serrno = errno;
> > > +		xc_evtchn_close(xenfb->evt_xch);
> > > +		free(xenfb);
> > > +		errno = serrno;
> > > +		return NULL;
> > It''s a pity we don''t have a macro which hides this
ugliness.  Perhaps
> > 
> > #define PRESERVING_ERRNO(x) do {
> > 	int tmp = errno;
> > 	x;
> > 	errno = tmp;
> > } while (0)
> > 
> > You could then do something like
> > 
> > if (xenfb_evt_sch == -1) {
> > 	PRESERVING_ERRNO(xc_evtchn_close(xenfb->evt_xch);free(xenfb));
> > 	return NULL;
> > }
> > 
> > Not sure whether that''s more or less ugly, to be honest.
> I think it makes it a little bit less clear what''s going on.  But
that''s
> just me
Yeah, I''m not sold on it either.  Forget I mentioned it.
> > > +static char *xenfb_path_in_dom(struct xs_handle *h,
> > > +			 unsigned domid, const char *path,
> > > +			 char *buffer, size_t size)
> > > +{
> > > +	char *domp = xs_get_domain_path(h, domid);
> > Can fail.
> Catching that case
Thanks.
> > > +static int xenfb_xs_scanf1(struct xs_handle *xsh, unsigned
domid,
> > > +			   const char *path, const char *fmt,
> > > +			   void *dest)
> > > +{
> > > +	char buffer[1024];
> > > +	char *p;
> > > +	int ret;
> > > +
> > > +	p = xenfb_path_in_dom(xsh, domid, path, buffer,
sizeof(buffer));
> > What happens if this fails?
> I think we should probably do the wait and loop -- changed so that it
> actually happens
Thanks.
> > > +	p = xs_read(xsh, XBT_NULL, p, NULL);
> > > +	if (!p)
> > > +		return -ENOENT;
> > > +	ret = sscanf(p, fmt, dest);
> > > +	free(p);
> > > +	if (ret != 1)
> > > +		return -EDOM;
> > > +	return 0;
> > > +}
> > You''re somewhat inconsistent about returning error numbers as
negative
> > return values or through errno.  I''d prefer the latter in
userspace
> > code, but it doesn''t matter too much, privided you pick one.
> Switched to the latter -- I think it''s actually on here where
it''s
> inconsistent.
Thanks.
> > > +	for (;;) {
> > > +		ret = xenfb_xs_scanf1(xsh, domid, "vfb/page-ref",
"%lu",
> > > +				      &xenfb->fbdev_mfn);
> > > +		if (ret == -ENOENT || ret == -EAGAIN)
> > xenfb_xs_scanf can''t return -EAGAIN.  What are you trying to
achieve
> > here?
> I can''t really see anything either -- simplified
Thanks.
> 
> > > +	wait:
> > > +		printf("Waiting...\n");
> > Where does this message go?
> With it being spawned from xend, it''ll go to xend-debug.log
It''d be nice if it was a bit more specific, then, so as it
doesn''t get
mixed up with messages from other stuff.
> > > +	xenfb->fb = xc_map_foreign_batch(xenfb->xc, domid,
PROT_READ | PROT_WRITE, xenfb->fbmfns, xenfb->n_fbmfns);
> > > +	if (xenfb->fb == NULL)
> > > +		goto error_fbmfns;
> > > +
> > > +	event.type = XENFB_TYPE_SET_EVENTS;
> > > +	event.set_events.flags = XENFB_FLAG_UPDATE;
> > > +	if (xenfb_fb_event(xenfb, &event))
> > > +		goto error_fb;
> > > +
> > > +	munmap(xenfb->fbmfns, xenfb->n_fbdirs * XC_PAGE_SIZE);
> > Please make fbmfns a local rather than putting it in the info
> > structure.
> Done
Thanks.
> > > +
> > > +	xenfb->domid = domid;
> > > +
> > > +	xenfb->pub.pixels = xenfb->fb;
> > > +
> > > +	xenfb->pub.row_stride = xenfb->fb_info->line_length;
> > > +	xenfb->pub.depth = xenfb->fb_info->depth;
> > > +	xenfb->pub.width = xenfb->fb_info->width;
> > > +	xenfb->pub.height = xenfb->fb_info->height;
> > > +
> > > +	return true;
> > > +
> > > + error_fb:
> > The error path here is utterly revolting.  Perhaps something like
this:
> Heh, indeed :-)  I don''t see why that can''t work
Thanks.
> > > +		xs_daemon_close(xsh);
> > I think you may end up closing the connection to the daemon twice
here.
> 
> I don''t see how we could close it twice -- and if we do
I assume you meant ``if we do it doesn''t make any
difference'''', which
is true, since xs_daemon_close does nothing if applied to NULL
handles.

Anyway, what I was thinking of was this bit earlier in the function
> +     xs_daemon_close(xsh);
> +     xsh = NULL;
> +
> +     xenfb->fbdev_port = xc_evtchn_bind_interdomain(xenfb->evt_xch,
domid,
> +                                                   
xenfb->fbdev_evtchn);
> +     if (xenfb->fbdev_port == -1)
> +             goto error;
But, as you point out, it doesn''t actually matter.
> > > +static void xenfb_update(struct xenfb_private *xenfb, int x, int
y, int width, int height)
> > > +{
> > > +	if (xenfb->pub.update)
> > > +		xenfb->pub.update(&xenfb->pub, x, y, width, height);
> > > +}
> > I''m not convinced this wrapper is actually needed, given that
it''s
> > utterly trivial and only called from one place.
> Sure! 
> 
> > > +static void xenfb_on_kbd_event(struct xenfb_private *xenfb)
> [snip]
> > I''d replace this with
> Doesn''t this go back to "we need to consume events out of the
ring to
> remain compatible if there are later events that could be understood"?
Not really.  The replacement I suggested was intended to be
functionally identical to the code that you''ve got there at the
moment.
> > > +
> > > +int xenfb_on_incoming(struct xenfb *xenfb_pub)
> > > +{
> > > +	struct xenfb_private *xenfb = (struct xenfb_private
*)xenfb_pub;
> > > +	evtchn_port_t port;
> > > +
> > > +	port = xc_evtchn_pending(xenfb->evt_xch);
> > > +	if (port == -1)
> > > +		return -1;
> > > +
> > > +	if (port == xenfb->fbdev_port) {
> > > +		xenfb_on_fb_event(xenfb);
> > > +	} else if (port == xenfb->kbd_port) {
> > > +		xenfb_on_kbd_event(xenfb);
> > > +	}
> > > +
> > > +	if (xc_evtchn_unmask(xenfb->evt_xch, port) == -1)
> > > +		return -1;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +int xenfb_send_key(struct xenfb *xenfb_pub, bool down, int
keycode)
> > > +{
> > > +	struct xenfb_private *xenfb = (struct xenfb_private
*)xenfb_pub;
> > > +	union xenkbd_in_event event;
> > > +
> > > +	event.type = XENKBD_TYPE_KEY;
> > > +	event.key.pressed = down ? 1 : 0;
> > > +	event.key.keycode = keycode;
> > > +
> > > +	return xenfb_kbd_event(xenfb, &event);
> > > +}
> > > +
> > > +int xenfb_send_motion(struct xenfb *xenfb_pub, int rel_x, int
rel_y)
> > Is this sending XENKBD_TYPE_MOTION or XENFB_TYPE_MOTION?
> It''s a "keyboard" event, so XENKBD_TYPE_MOTION
It could do with a less deceptive name, then.

Steven.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> >>>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> >>>+++ b/tools/xenfb/keymapping.c	Sat Sep 02 15:19:25 2006 -0400
> >>>@@ -0,0 +1,141 @@
> >>>+#include <stdint.h>
> >>>+#include <gdk/gdkkeysyms.h>
> >>>+#include <linux/input.h>
> >>>+
> >>>+uint32_t gdk_linux_mapping[0x10000] = {
> >>>+	[GDK_a] = KEY_A,
> >>>      
> >>This is kind of ugly.  Is there any chance it could be
autogenerated?
> >>Also, where did 0x10000 come from?
> >>
> >>Also, depending on GTK just for the keymap table is a real pain. 
Or
> >>is it already required for libvncserver?
> >>    
> >
> >libvncserver requires GTK.  And I don''t know that
there''s really any
> >good way to auto-generate it unfortunately.  I somehow expect that
> >0x10000 came from "it''ll be big enough" but Anthony
would have to
> >confirm :-)
> That''s the biggest that a GDK scan code can currently be.
Do you have a reference for that? Could the table grow in the future?

Steven
(who just spent a whole day tracking down a bug which turned out to be
an undersized lookup table combined with a lack of bounds checking)
> That way, we can use a simple indexed table.
> 
> Regards,
> 
> Anthony Liguori
> 
> >The mappings are unfortunately a bit of a fact of life since we have to
> >convert from what the X layer gets to what the kernel expects.  And the
> >two couldn''t be farther from the same.  And then it''s
even more fun when
> >toolkits get involved.
> >  
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> Steven Smith wrote:
> [...]
> >> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> >> +++ b/tools/xenfb/keymapping.c	Sat Sep 02 15:19:25 2006 -0400
> >> @@ -0,0 +1,141 @@
> >> +#include <stdint.h>
> >> +#include <gdk/gdkkeysyms.h>
> >> +#include <linux/input.h>
> >> +
> >> +uint32_t gdk_linux_mapping[0x10000] = {
> >> +	[GDK_a] = KEY_A,
> > This is kind of ugly.  Is there any chance it could be autogenerated?
> > Also, where did 0x10000 come from?
> > 
> > Also, depending on GTK just for the keymap table is a real pain.  Or
> > is it already required for libvncserver?
> > 
> For the VNC part, as it depends on libvncserver, it should use
> /usr/include/rfb/keysym.h:
> 
> +uint32_t gdk_linux_mapping[0x10000] = {
> +	[XK_a] = KEY_A,
Yes, you''re right.  The dependency on GTK is unimportant, but
libvncserver thinks it''s generating X keysyms rather than GDK keys, so
XK_* is the correct thing to use, I think.
> For the SDL part, I''m sorry to repeat it should use scancode
instead
> of symbol id ...
I think that would imply that the frontend would need to maintain its
own keymap, yes?  What do you think should happen if the system
running the SDL viewer has e.g. a French keyboard but the virtual
machine is configured with a US keymap?  Or have I misunderstood you?

Steven.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Steven Smith wrote:
>> Steven Smith wrote:
>> [...]
>>>> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>>>> +++ b/tools/xenfb/keymapping.c	Sat Sep 02 15:19:25 2006 -0400
>>>> @@ -0,0 +1,141 @@
>>>> +#include <stdint.h>
>>>> +#include <gdk/gdkkeysyms.h>
>>>> +#include <linux/input.h>
>>>> +
>>>> +uint32_t gdk_linux_mapping[0x10000] = {
>>>> +	[GDK_a] = KEY_A,
>>> This is kind of ugly.  Is there any chance it could be
autogenerated?
>>> Also, where did 0x10000 come from?
>>>
>>> Also, depending on GTK just for the keymap table is a real pain. 
Or
>>> is it already required for libvncserver?
>>>
>> For the VNC part, as it depends on libvncserver, it should use
>> /usr/include/rfb/keysym.h:
>>
>> +uint32_t gdk_linux_mapping[0x10000] = {
>> +	[XK_a] = KEY_A,
> Yes, you''re right.  The dependency on GTK is unimportant, but
> libvncserver thinks it''s generating X keysyms rather than GDK
keys, so
> XK_* is the correct thing to use, I think.
Thanks
>> For the SDL part, I''m sorry to repeat it should use scancode
instead
>> of symbol id ...
> I think that would imply that the frontend would need to maintain its
> own keymap, yes?  What do you think should happen if the system
Yes, frontend (domU kernel or X11) should maintain it''s own keymap.
> running the SDL viewer has e.g. a French keyboard but the virtual
> machine is configured with a US keymap?  Or have I misunderstood you?
If the SDL viewer is using X11 configured with french keyboard, and the virtual
machine is configured with US keyboard, the used keymap will be the US one. So
when I press ''A'' on my keyboard I will have
''Q''.
In fact, with scancode the used keymap will always be the virtual machine one.

We can compare the two solutions:

1- GDK symbol based:

* keymap is keymap of the backend
* we can''t translate all symbols

for instance, look at these GIF:

http://riley.slc.k12.ut.us/images/program_images/Type%20Keyboard%20with%20lower%20letters.gif
http://www.sussex.ac.uk/its/facilities/pc/keyboards/french.gif

On my french keyboard I want to produce "&", so I press
"&", GDK table of sdlfb
translates it to  nothing... (it''s true for
&é"{(-è_çà)=<> ). If I want to
produce "1", I press shift + "&" and I obtain nothing
too.. so all symbols and
digits are missing !
But if you add "&" in your table, it will be good for french
keyboard but bad
for US keyboard (you will generate "7" instead of "1"...)

And how do you manage this one:

http://jcmc.indiana.edu/vol9/issue1/Japanese_Keyboard.gif

2- GDK scancode based:

* keymap is keymap of the frontend
* frontend knows how to translate ALL scancodes to a symbols.

This is the method generally used in emulators (I took scancode from basiliskII)
basiliskII:
http://www.cebix.net/viewcvs/cebix/BasiliskII/src/SDL/keycodes?rev=1.4
Aranym:
http://www.sophics.cz/cgi-bin/viewcvs.cgi/aranym/src/input.cpp

But if you don''t want to use scancode, you should manage all keyboard
internally
as in E-UAE (file e-uae-0.8.29-WIP3/src/gfx-sdl/sdlkeys.c):
http://www.rcdrummond.net/uae/e-uae-0.8.29-WIP3/e-uae-0.8.29-WIP3.tar.bz2
> Steven.
Laurent
-- 
                Laurent.Vivier@bull.net
         Bull, Architect of an Open World (TM)
+----- "Any sufficiently advanced technology is ----+
| indistinguishable from magic." - Arthur C. Clarke |



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> >> For the SDL part, I''m sorry to repeat it should use
scancode instead
> >> of symbol id ...
> > I think that would imply that the frontend would need to maintain its
> > own keymap, yes?  What do you think should happen if the system
> Yes, frontend (domU kernel or X11) should maintain it''s own
keymap.
> 
> > running the SDL viewer has e.g. a French keyboard but the virtual
> > machine is configured with a US keymap?  Or have I misunderstood you?
> If the SDL viewer is using X11 configured with french keyboard, and the
virtual
> machine is configured with US keyboard, the used keymap will be the US one.
So
> when I press ''A'' on my keyboard I will have
''Q''.
That kind of sucks.  Not being able to produce every character sucks
more, but having to configure the keymap in every VM isn''t much
better.  It''s even better if you consider that with VNC clients the
client keymap may change while the virtual machine is running.
> In fact, with scancode the used keymap will always be the virtual machine
one.
> 
> We can compare the two solutions:
> 
> 1- GDK symbol based:
> 
> * keymap is keymap of the backend
> * we can''t translate all symbols
> 
> for instance, look at these GIF:
> 
>
http://riley.slc.k12.ut.us/images/program_images/Type%20Keyboard%20with%20lower%20letters.gif
> http://www.sussex.ac.uk/its/facilities/pc/keyboards/french.gif
> 
> On my french keyboard I want to produce "&", so I press
"&", GDK table of sdlfb
> translates it to  nothing... (it''s true for
&é"{(-è_çà)=<> ). If I want to
> produce "1", I press shift + "&" and I obtain
nothing too.. so all symbols and
> digits are missing !
> But if you add "&" in your table, it will be good for french
keyboard but bad
> for US keyboard (you will generate "7" instead of
"1"...)
I can''t see why just adding SDLK_AMPERSAND to the table will break US
keyboards.  Surely SDL doesn''t expect every application to track the
effects of the shift key?
> 
> And how do you manage this one:
> 
> http://jcmc.indiana.edu/vol9/issue1/Japanese_Keyboard.gif
Perhaps we should be using the keysym->unicode rather than
keysym->sym?  I have to admit I''m not sure exactly how the more
exotic
keymaps are actually encoded in Linux, but there must be some way of
mapping them to keysyms, or such keyboards wouldn''t work at all.

Actually, it might be nice to send unicode characters over the
protocol directly, rather than Linux key codes, and then do the
translation in the front end.
> 
> 2- GDK scancode based:
> 
> * keymap is keymap of the frontend
And has potentially no relationship to the ``correct'''' keymap,
which
is the one seen by the client.
> * frontend knows how to translate ALL scancodes to a symbols.
But it occasionally gets it wrong.
> This is the method generally used in emulators (I took scancode from
basiliskII)
> basiliskII:
> http://www.cebix.net/viewcvs/cebix/BasiliskII/src/SDL/keycodes?rev=1.4
> Aranym:
> http://www.sophics.cz/cgi-bin/viewcvs.cgi/aranym/src/input.cpp
If you''re doing full emulation then the guest expects to see a real
keyboard with real scancodes, and so you don''t have a great deal of
choice.
> But if you don''t want to use scancode, you should manage all
keyboard internally
> as in E-UAE (file e-uae-0.8.29-WIP3/src/gfx-sdl/sdlkeys.c):
> http://www.rcdrummond.net/uae/e-uae-0.8.29-WIP3/e-uae-0.8.29-WIP3.tar.bz2
That''s kind of icky as well.  Needing to know the user''s exact
keyboard type is really quite unpleasant.

Steven.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Steven Smith wrote:
>> If the SDL viewer is using X11 configured with french keyboard, and the
virtual
>> machine is configured with US keyboard, the used keymap will be the US
one. So
>> when I press ''A'' on my keyboard I will have
''Q''.
> That kind of sucks.
There is no way around that.
> Not being able to produce every character sucks
> more, but having to configure the keymap in every VM isn''t much
> better.
For sane i18n kbd support some translation must be done.

Make the guest think he has an US keyboard and do the translation on the
host isn''t going to work.  An US keyboard hasn''t an
''Ä'' key for example,
so you wouldn''t be able to type that character.  Also some characters
are arranged in a different way, for example on a us keyboard you have
'';'' and '':'' sharing one key, whereas on a
german one '';'' lives together
with '',''.  That isn''t going to work with host-side
translation too.

Thus we *must* do the translation in the guest.  And we must do it in
the guest *only*, otherwise we''ll end up with the translation happening
twice which becomes even more messy.

Sending scancodes not keysyms is the only solution to that problem.  Really.

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Steven Smith wrote:
>>>> For the SDL part, I''m sorry to repeat it should use
scancode instead
>>>> of symbol id ...
>>> I think that would imply that the frontend would need to maintain
its
>>> own keymap, yes?  What do you think should happen if the system
>> Yes, frontend (domU kernel or X11) should maintain it''s own
keymap.
>>
>>> running the SDL viewer has e.g. a French keyboard but the virtual
>>> machine is configured with a US keymap?  Or have I misunderstood
you?
>> If the SDL viewer is using X11 configured with french keyboard, and the
virtual
>> machine is configured with US keyboard, the used keymap will be the US
one. So
>> when I press ''A'' on my keyboard I will have
''Q''.
> That kind of sucks.  Not being able to produce every character sucks
> more, but having to configure the keymap in every VM isn''t much
> better.  It''s even better if you consider that with VNC clients
the
> client keymap may change while the virtual machine is running.
The keymap may not change while the virtual machine is running:
"loadkeys" acts
only on the current console, and for X11, "xmodmap" modifies only the
current
display. A desktop like GNOME can manage that too...
>> In fact, with scancode the used keymap will always be the virtual
machine one.
>>
>> We can compare the two solutions:
>>
>> 1- GDK symbol based:
>>
>> * keymap is keymap of the backend
>> * we can''t translate all symbols
>>
>> for instance, look at these GIF:
>>
>>
http://riley.slc.k12.ut.us/images/program_images/Type%20Keyboard%20with%20lower%20letters.gif
>> http://www.sussex.ac.uk/its/facilities/pc/keyboards/french.gif
>>
>> On my french keyboard I want to produce "&", so I press
"&", GDK table of sdlfb
>> translates it to  nothing... (it''s true for
&é"{(-è_çà)=<> ). If I want to
>> produce "1", I press shift + "&" and I obtain
nothing too.. so all symbols and
>> digits are missing !
>> But if you add "&" in your table, it will be good for
french keyboard but bad
>> for US keyboard (you will generate "7" instead of
"1"...)
> I can''t see why just adding SDLK_AMPERSAND to the table will break
US
> keyboards.  Surely SDL doesn''t expect every application to track
the
> effects of the shift key?
Look at the GIFs...
if you think "US Keyboard", SDLK_AMPERSAND will be translated to
KEY_7, if you
think "French Keyboard", SDLK_AMPERSAND should be translated to KEY_1.
>> And how do you manage this one:
>>
>> http://jcmc.indiana.edu/vol9/issue1/Japanese_Keyboard.gif
> Perhaps we should be using the keysym->unicode rather than
> keysym->sym?  I have to admit I''m not sure exactly how the more
exotic
> keymaps are actually encoded in Linux, but there must be some way of
> mapping them to keysyms, or such keyboards wouldn''t work at all.
Are you ready to manage a table of 2^16 entries ???? ;-)
> Actually, it might be nice to send unicode characters over the
> protocol directly, rather than Linux key codes, and then do the
> translation in the front end.
Perhaps...
>> 2- GDK scancode based:
>>
>> * keymap is keymap of the frontend
> And has potentially no relationship to the ``correct''''
keymap, which
> is the one seen by the client.
> 
>> * frontend knows how to translate ALL scancodes to a symbols.
> But it occasionally gets it wrong.
Really ? In which cases ?
>> This is the method generally used in emulators (I took scancode from
basiliskII)
>> basiliskII:
>> http://www.cebix.net/viewcvs/cebix/BasiliskII/src/SDL/keycodes?rev=1.4
>> Aranym:
>> http://www.sophics.cz/cgi-bin/viewcvs.cgi/aranym/src/input.cpp
> If you''re doing full emulation then the guest expects to see a
real
> keyboard with real scancodes, and so you don''t have a great deal
of
> choice.
Yes, but linux kernel expects a scancode too...
>> But if you don''t want to use scancode, you should manage all
keyboard internally
>> as in E-UAE (file e-uae-0.8.29-WIP3/src/gfx-sdl/sdlkeys.c):
>>
http://www.rcdrummond.net/uae/e-uae-0.8.29-WIP3/e-uae-0.8.29-WIP3.tar.bz2
> That''s kind of icky as well.  Needing to know the user''s
exact
> keyboard type is really quite unpleasant.
I agree.
> Steven.
Laurent

-- 
                Laurent.Vivier@bull.net
         Bull, Architect of an Open World (TM)
+----- "Any sufficiently advanced technology is ----+
| indistinguishable from magic." - Arthur C. Clarke |



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Gerd Hoffmann wrote:
> Steven Smith wrote:
>>> If the SDL viewer is using X11 configured with french keyboard, and
the virtual
>>> machine is configured with US keyboard, the used keymap will be the
US one. So
>>> when I press ''A'' on my keyboard I will have
''Q''.
>> That kind of sucks.
> 
> There is no way around that.
> 
>> Not being able to produce every character sucks
>> more, but having to configure the keymap in every VM isn''t
much
>> better.
> 
> For sane i18n kbd support some translation must be done.
> 
> Make the guest think he has an US keyboard and do the translation on the
> host isn''t going to work.  An US keyboard hasn''t an
''Ä'' key for example,
> so you wouldn''t be able to type that character.  Also some
characters
> are arranged in a different way, for example on a us keyboard you have
> '';'' and '':'' sharing one key, whereas on
a german one '';'' lives together
> with '',''.  That isn''t going to work with
host-side translation too.
> 
> Thus we *must* do the translation in the guest.  And we must do it in
> the guest *only*, otherwise we''ll end up with the translation
happening
> twice which becomes even more messy.
> 
> Sending scancodes not keysyms is the only solution to that problem. 
Really.
Thank you ;-)
> cheers,
>   Gerd
> 
Laurent

-- 
                Laurent.Vivier@bull.net
         Bull, Architect of an Open World (TM)
+----- "Any sufficiently advanced technology is ----+
| indistinguishable from magic." - Arthur C. Clarke |



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> >> If the SDL viewer is using X11 configured with french keyboard,
and the virtual
> >> machine is configured with US keyboard, the used keymap will be
the US one. So
> >> when I press ''A'' on my keyboard I will have
''Q''.
> > That kind of sucks.
> There is no way around that.
> > Not being able to produce every character sucks
> > more, but having to configure the keymap in every VM isn''t
much
> > better.
> 
> For sane i18n kbd support some translation must be done.
> 
> Make the guest think he has an US keyboard and do the translation on the
> host isn''t going to work.  An US keyboard hasn''t an
''Ä'' key for example,
> so you wouldn''t be able to type that character.
> Also some characters
> are arranged in a different way, for example on a us keyboard you have
> '';'' and '':'' sharing one key, whereas on
a german one '';'' lives together
> with '',''.  That isn''t going to work with
host-side translation too.
Alright, I''m convinced: we need to send scancodes rather than keysyms,
since that''s what input core''s expecting and there''s
no sane way to do
the reverse mapping.

Perhaps the protocol should include some means by which the backend
can tell the frontend to use a particular keymap?  Pushing that
through to X is going to be a pain, but getting the console keymap
shouldn''t be too bad.  It''ll get more fun when we support
multiple
frontends in a domain, but that''s a problem for another day.

Someone also needs to think about what the vnc backend''s going to do,
since it receives X keysyms rather than scancodes from the client.

Steven.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

First: I now agree with you that scancodes are a better choice than
keysyms, and that I was wrong initially.  However, scancodes have
their own problems, and I''d like to make sure they''re
understood
before we go too far down this path.
> >>>> For the SDL part, I''m sorry to repeat it should
use scancode instead
> >>>> of symbol id ...
> >>> I think that would imply that the frontend would need to
maintain its
> >>> own keymap, yes?  What do you think should happen if the
system
> >> Yes, frontend (domU kernel or X11) should maintain it''s
own keymap.
> >>
> >>> running the SDL viewer has e.g. a French keyboard but the
virtual
> >>> machine is configured with a US keymap?  Or have I
misunderstood you?
> >> If the SDL viewer is using X11 configured with french keyboard,
and the virtual
> >> machine is configured with US keyboard, the used keymap will be
the US one. So
> >> when I press ''A'' on my keyboard I will have
''Q''.
> > That kind of sucks.  Not being able to produce every character sucks
> > more, but having to configure the keymap in every VM isn''t
much
> > better.  It''s even better if you consider that with VNC
clients the
> > client keymap may change while the virtual machine is running.
> The keymap may not change while the virtual machine is running:
> "loadkeys" acts only on the current console, and for X11,
"xmodmap"
> modifies only the current display.
What happens if you connect vncviewer from a machine with a US
keyboard, then disconnect, and then reconnect from a machine with a
French keyboard?

It''d be nice if, from both machines, pressing the key labelled
''w'' on
the keyboard resulted in a ''w'' being sent to whatever
application is
reading from the keyboard at the time.
> A desktop like GNOME can manage that too...
Most Gnome apps care more about keysyms than scancodes, though, so
they''ll just pick up whatever keymap is set with xmodmap.
> >> In fact, with scancode the used keymap will always be the virtual
machine one.
> >>
> >> We can compare the two solutions:
> >>
> >> 1- GDK symbol based:
> >>
> >> * keymap is keymap of the backend
> >> * we can''t translate all symbols
> >>
> >> for instance, look at these GIF:
> >>
> >>
http://riley.slc.k12.ut.us/images/program_images/Type%20Keyboard%20with%20lower%20letters.gif
> >> http://www.sussex.ac.uk/its/facilities/pc/keyboards/french.gif
> >>
> >> On my french keyboard I want to produce "&", so I
press "&", GDK table of sdlfb
> >> translates it to  nothing... (it''s true for
&?"{(-?_??)=<> ). If I want to
> >> produce "1", I press shift + "&" and I
obtain nothing too.. so all symbols and
> >> digits are missing !
> >> But if you add "&" in your table, it will be good
for french keyboard but bad
> >> for US keyboard (you will generate "7" instead of
"1"...)
> > I can''t see why just adding SDLK_AMPERSAND to the table will
break US
> > keyboards.  Surely SDL doesn''t expect every application to
track the
> > effects of the shift key?
> Look at the GIFs...  if you think "US Keyboard", SDLK_AMPERSAND
will
> be translated to KEY_7, if you think "French Keyboard",
> SDLK_AMPERSAND should be translated to KEY_1.
Ack, sorry, not awake enough.
> >> 2- GDK scancode based:
> >>
> >> * keymap is keymap of the frontend
> > And has potentially no relationship to the
``correct'''' keymap, which
> > is the one seen by the client.
> > 
> >> * frontend knows how to translate ALL scancodes to a symbols.
> > But it occasionally gets it wrong.
> Really ? In which cases ?
Well, if the backend is attached to a French keyboard and the frontend
has a US keymap loaded.  When the user presses ''a'', scancode
16 (say)
will be sent, and the frontend will then run that through its keymap
and get a ''q''.  It''d be good if pressing
''a'' led to an ''a'' appearing
on the screen.

Given that the backend knows exactly what each scancode is supposed to
map to, we should in principle be able to avoid this sort of problem.
It''s just a matter of connecting everything up correctly. :)

Steven.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Steven Smith wrote:
> First: I now agree with you that scancodes are a better choice than
> keysyms, and that I was wrong initially.  However, scancodes have
> their own problems, and I''d like to make sure they''re
understood
> before we go too far down this path.
Good.
>>>>>> For the SDL part, I''m sorry to repeat it
should use scancode instead
>>>>>> of symbol id ...
>>>>> I think that would imply that the frontend would need to
maintain its
>>>>> own keymap, yes?  What do you think should happen if the
system
>>>> Yes, frontend (domU kernel or X11) should maintain
it''s own keymap.
>>>>
>>>>> running the SDL viewer has e.g. a French keyboard but the
virtual
>>>>> machine is configured with a US keymap?  Or have I
misunderstood you?
>>>> If the SDL viewer is using X11 configured with french keyboard,
and the virtual
>>>> machine is configured with US keyboard, the used keymap will be
the US one. So
>>>> when I press ''A'' on my keyboard I will have
''Q''.
>>> That kind of sucks.  Not being able to produce every character
sucks
>>> more, but having to configure the keymap in every VM isn''t
much
>>> better.  It''s even better if you consider that with VNC
clients the
>>> client keymap may change while the virtual machine is running.
>> The keymap may not change while the virtual machine is running:
>> "loadkeys" acts only on the current console, and for X11,
"xmodmap"
>> modifies only the current display.
> What happens if you connect vncviewer from a machine with a US
> keyboard, then disconnect, and then reconnect from a machine with a
> French keyboard?
Nothing :-P
I mean the virtual server always use the same keymap...
If it was US keymap it continues with US keymap, if it was french keymap it
continues with french keymap.
It is possible to use a german keymap on a french keyboard, or belgium keymap on
a spanish keyboard, but you should be aware that the symbol on the key will not
be the symbol on the display !!!!
> It''d be nice if, from both machines, pressing the key labelled
''w'' on
> the keyboard resulted in a ''w'' being sent to whatever
application is
> reading from the keyboard at the time.
--->_I think it''s better to have bad mapping than missing
symbols._<---

I prefer to have to press "A" to display "Q" than to not be
able to use "|" or
"&" because backend is not able to translate it and transfer it to
frontend (see
below).
>> A desktop like GNOME can manage that too...
> Most Gnome apps care more about keysyms than scancodes, though, so
> they''ll just pick up whatever keymap is set with xmodmap.
Yes, I agree.
But GNOME desktop is able to use xmodmap to set the good keymap.
>>>> In fact, with scancode the used keymap will always be the
virtual machine one.
>>>>
>>>> We can compare the two solutions:
>>>>
>>>> 1- GDK symbol based:
>>>>
>>>> * keymap is keymap of the backend
>>>> * we can''t translate all symbols
>>>>
>>>> for instance, look at these GIF:
>>>>
>>>>
http://riley.slc.k12.ut.us/images/program_images/Type%20Keyboard%20with%20lower%20letters.gif
>>>> http://www.sussex.ac.uk/its/facilities/pc/keyboards/french.gif
>>>>
>>>> On my french keyboard I want to produce "&", so I
press "&", GDK table of sdlfb
>>>> translates it to  nothing... (it''s true for
&?"{(-?_??)=<> ). If I want to
>>>> produce "1", I press shift + "&" and I
obtain nothing too.. so all symbols and
>>>> digits are missing !
>>>> But if you add "&" in your table, it will be good
for french keyboard but bad
>>>> for US keyboard (you will generate "7" instead of
"1"...)
>>> I can''t see why just adding SDLK_AMPERSAND to the table
will break US
>>> keyboards.  Surely SDL doesn''t expect every application to
track the
>>> effects of the shift key?
>> Look at the GIFs...  if you think "US Keyboard",
SDLK_AMPERSAND will
>> be translated to KEY_7, if you think "French Keyboard",
>> SDLK_AMPERSAND should be translated to KEY_1.
> Ack, sorry, not awake enough.
I know that too ;-)
>>>> 2- GDK scancode based:
>>>>
>>>> * keymap is keymap of the frontend
>>> And has potentially no relationship to the
``correct'''' keymap, which
>>> is the one seen by the client.
>>>
>>>> * frontend knows how to translate ALL scancodes to a symbols.
>>> But it occasionally gets it wrong.
>> Really ? In which cases ?
> Well, if the backend is attached to a French keyboard and the frontend
> has a US keymap loaded.  When the user presses ''a'',
scancode 16 (say)
> will be sent, and the frontend will then run that through its keymap
Right
> and get a ''q''.  It''d be good if pressing
''a'' led to an ''a'' appearing
> on the screen.
Yes, but I think it is not possible because keyboard driver in kernel is only
able to manage scancodes (KEY_*) and not symbols (like UTF-8).

There are two problems you can''t solve using symbols instead of
scancode:
1- a symbols can be on two different keys when you use two different keyboards
(i.e. like AMPERSAND), so the translation from symbol to scancode is not
possible.
2- the "shift" can act differently on keyboard: for instance on US
keyboard you
don''t have to press shift to have "0" when you press KEY_0,
on french keyboard
you must press shift to have "0" when you press KEY_0 otherwise you
will have
"à". Moreover, for instance, ALTGR and "0" produce
"@".

And I think french keyboard is not the worst case, think about Japanese,
Chinese, Korean, Russian, Greek keyboards (and more). It''s a big deal
to map all
this "exotic" (I mean non latin ;-) ) symbols to a linux KEY_* . How
big will be
the translation array ???
> Given that the backend knows exactly what each scancode is supposed to
> map to, we should in principle be able to avoid this sort of problem.
> It''s just a matter of connecting everything up correctly. :)
No, I''m sorry, I don''t think so.

Except, perhaps, if you send UTF-8 directly from backend to frontend without
translating it. But it supposes that frontend keyboard driver is able to manage
UTF-8 instead of linux scancode. But it will work with console not with X11. I
think X11 is working with scancode too. It will be very messy...

Regards,
Laurent
-- 
                Laurent.Vivier@bull.net
         Bull, Architect of an Open World (TM)
+----- "Any sufficiently advanced technology is ----+
| indistinguishable from magic." - Arthur C. Clarke |



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> > It''d be nice if, from both machines, pressing the key
labelled ''w'' on
> > the keyboard resulted in a ''w'' being sent to
whatever application is
> > reading from the keyboard at the time.
> --->_I think it''s better to have bad mapping than missing
symbols._<---
Agreed.
> > Given that the backend knows exactly what each scancode is supposed to
> > map to, we should in principle be able to avoid this sort of problem.
> > It''s just a matter of connecting everything up correctly. :)
> No, I''m sorry, I don''t think so.
Okay, dumbest possible solution: the backend queries the local X
server for its keymap, and publishes that in xenbus.  You then have
some userspace application running in the guest domain which looks at
that and does xmodmap or loadkeys as appropriate.

This is really rather distasteful, but I think it allows every key
which can be typed on the backend keyboard to be transmitted to the
frontend and rendered correctly there.

You have to come up with some way of representing the keymap, but
at least for Linux that''s already done for us.

Does this break anything?
> Except, perhaps, if you send UTF-8 directly from backend to frontend
> without translating it. But it supposes that frontend keyboard
> driver is able to manage UTF-8 instead of linux scancode. But it
> will work with console not with X11. I think X11 is working with
> scancode too. It will be very messy...
Yeah, we don''t want to go down that path.

Steven.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Steven Smith wrote:
>>> It''d be nice if, from both machines, pressing the key
labelled ''w'' on
>>> the keyboard resulted in a ''w'' being sent to
whatever application is
>>> reading from the keyboard at the time.
>> --->_I think it''s better to have bad mapping than missing
symbols._<---
> Agreed.
> 
>>> Given that the backend knows exactly what each scancode is supposed
to
>>> map to, we should in principle be able to avoid this sort of
problem.
>>> It''s just a matter of connecting everything up correctly.
:)
>> No, I''m sorry, I don''t think so.
> Okay, dumbest possible solution: the backend queries the local X
> server for its keymap, and publishes that in xenbus.  You then have
> some userspace application running in the guest domain which looks at
> that and does xmodmap or loadkeys as appropriate.
> 
> This is really rather distasteful, but I think it allows every key
> which can be typed on the backend keyboard to be transmitted to the
> frontend and rendered correctly there.
> 
> You have to come up with some way of representing the keymap, but
> at least for Linux that''s already done for us.
> 
> Does this break anything?
I think it''s a good idea.

But perhaps not really needed.

Why ?

Because generally people always uses same keyboard and you can set directly in
the logging environment to load a default keyboard mapping by account (like a
"loadkeys" in .bash_profile). For instance, french for my account, US
for your
account or german for Gerd''s account. If I have to connect to other one
account
(like "root", but the command "sudo" should allow to avoid
that), I think it is
acceptable to have bad mapping.

OK, it doesn''t work for the logging screen (console login or xdm), but
generally, on a site, all people uses same keyboard, so we can also set default
keyboard mapping at server level.

An other solution is to force everyone on earth to use french keyboard ;-)
(it''s fine for me)

Regards,
Laurent
-- 
                Laurent.Vivier@bull.net
         Bull, Architect of an Open World (TM)
+----- "Any sufficiently advanced technology is ----+
| indistinguishable from magic." - Arthur C. Clarke |



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Steven Smith wrote:
> First: I now agree with you that scancodes are a better choice than
> keysyms, and that I was wrong initially.
The problem with scancodes is that you cannot always get scancodes from 
the viewer.  You can get scancodes from SDL but you can only get keysyms 
from VNC.  We would have to map VNC keysyms (which are just Xk keysyms) 
to scancodes?

I''m a bit surprised here.  If we generate a KEY_Q event in Linux that 
may show up as a KEY_A key?  There are keysyms for all the extended keys 
I thought.

Regards,

Anthony Liguori
>   However, scancodes have
> their own problems, and I''d like to make sure they''re
understood
> before we go too far down this path.
>
>   
>>>>>> For the SDL part, I''m sorry to repeat it
should use scancode instead
>>>>>> of symbol id ...
>>>>>>             
>>>>> I think that would imply that the frontend would need to
maintain its
>>>>> own keymap, yes?  What do you think should happen if the
system
>>>>>           
>>>> Yes, frontend (domU kernel or X11) should maintain
it''s own keymap.
>>>>
>>>>         
>>>>> running the SDL viewer has e.g. a French keyboard but the
virtual
>>>>> machine is configured with a US keymap?  Or have I
misunderstood you?
>>>>>           
>>>> If the SDL viewer is using X11 configured with french keyboard,
and the virtual
>>>> machine is configured with US keyboard, the used keymap will be
the US one. So
>>>> when I press ''A'' on my keyboard I will have
''Q''.
>>>>         
>>> That kind of sucks.  Not being able to produce every character
sucks
>>> more, but having to configure the keymap in every VM isn''t
much
>>> better.  It''s even better if you consider that with VNC
clients the
>>> client keymap may change while the virtual machine is running.
>>>       
>> The keymap may not change while the virtual machine is running:
>> "loadkeys" acts only on the current console, and for X11,
"xmodmap"
>> modifies only the current display.
>>     
> What happens if you connect vncviewer from a machine with a US
> keyboard, then disconnect, and then reconnect from a machine with a
> French keyboard?
>
> It''d be nice if, from both machines, pressing the key labelled
''w'' on
> the keyboard resulted in a ''w'' being sent to whatever
application is
> reading from the keyboard at the time.
>
>   
>> A desktop like GNOME can manage that too...
>>     
> Most Gnome apps care more about keysyms than scancodes, though, so
> they''ll just pick up whatever keymap is set with xmodmap.
>
>   
>>>> In fact, with scancode the used keymap will always be the
virtual machine one.
>>>>
>>>> We can compare the two solutions:
>>>>
>>>> 1- GDK symbol based:
>>>>
>>>> * keymap is keymap of the backend
>>>> * we can''t translate all symbols
>>>>
>>>> for instance, look at these GIF:
>>>>
>>>>
http://riley.slc.k12.ut.us/images/program_images/Type%20Keyboard%20with%20lower%20letters.gif
>>>> http://www.sussex.ac.uk/its/facilities/pc/keyboards/french.gif
>>>>
>>>> On my french keyboard I want to produce "&", so I
press "&", GDK table of sdlfb
>>>> translates it to  nothing... (it''s true for
&?"{(-?_??)=<> ). If I want to
>>>> produce "1", I press shift + "&" and I
obtain nothing too.. so all symbols and
>>>> digits are missing !
>>>> But if you add "&" in your table, it will be good
for french keyboard but bad
>>>> for US keyboard (you will generate "7" instead of
"1"...)
>>>>         
>>> I can''t see why just adding SDLK_AMPERSAND to the table
will break US
>>> keyboards.  Surely SDL doesn''t expect every application to
track the
>>> effects of the shift key?
>>>       
>> Look at the GIFs...  if you think "US Keyboard",
SDLK_AMPERSAND will
>> be translated to KEY_7, if you think "French Keyboard",
>> SDLK_AMPERSAND should be translated to KEY_1.
>>     
> Ack, sorry, not awake enough.
>
>   
>>>> 2- GDK scancode based:
>>>>
>>>> * keymap is keymap of the frontend
>>>>         
>>> And has potentially no relationship to the
``correct'''' keymap, which
>>> is the one seen by the client.
>>>
>>>       
>>>> * frontend knows how to translate ALL scancodes to a symbols.
>>>>         
>>> But it occasionally gets it wrong.
>>>       
>> Really ? In which cases ?
>>     
> Well, if the backend is attached to a French keyboard and the frontend
> has a US keymap loaded.  When the user presses ''a'',
scancode 16 (say)
> will be sent, and the frontend will then run that through its keymap
> and get a ''q''.  It''d be good if pressing
''a'' led to an ''a'' appearing
> on the screen.
>
> Given that the backend knows exactly what each scancode is supposed to
> map to, we should in principle be able to avoid this sort of problem.
> It''s just a matter of connecting everything up correctly. :)
>
> Steven.
>   

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Anthony Liguori wrote:
> Steven Smith wrote:
>> First: I now agree with you that scancodes are a better choice than
>> keysyms, and that I was wrong initially.
> 
> The problem with scancodes is that you cannot always get scancodes from
> the viewer.  You can get scancodes from SDL but you can only get keysyms
Right.
> from VNC.  We would have to map VNC keysyms (which are just Xk keysyms)
> to scancodes?
It''s what we already did: KEY_* are scancodes.
It''s why it should be better to get scancodes from the viewer, not
keysyms.

Currently:
we get keysyms from VNC and SDL and we translate to scancode for linux kernel.

What I propose:
we get scancode from SDL and we translate to scancode for linux kernel.
We must keep keysyms from VNC because we can''t have scancode from VNC
client.
> I''m a bit surprised here.  If we generate a KEY_Q event in Linux
that
> may show up as a KEY_A key?  There are keysyms for all the extended keys
> I thought.
I''m sorry but I think you didn''t understand the issue: we must
provide scancode
to frontend keyboard driver (KEY_*), and the current issue is in translating
keysyms to scancode. Please have look at all previous e-mails...

Regards,
Laurent
-- 
                Laurent.Vivier@bull.net
         Bull, Architect of an Open World (TM)
+----- "Any sufficiently advanced technology is ----+
| indistinguishable from magic." - Arthur C. Clarke |



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> >First: I now agree with you that scancodes are a better choice than
> >keysyms, and that I was wrong initially.
> The problem with scancodes is that you cannot always get scancodes from 
> the viewer.  You can get scancodes from SDL but you can only get keysyms 
> from VNC.  We would have to map VNC keysyms (which are just Xk keysyms) 
> to scancodes?
Yeah.  That''s going to be a problem, and that''s why I
initially wanted
to avoid this approach, but I can''t see any sane way around it.  The
mapping table you''ve got there at the moment is a pretty reasonable
first step, even if it is limited to a US keyboard layout.

We could potentially do a bit better if we go with the pushing keymaps
across xenbus thing, in that we can just send over a keymap which can
generate every possible keysym using compose keys and so forth, but
it''d be pretty distasteful.
> I''m a bit surprised here.  If we generate a KEY_Q event in Linux
that
> may show up as a KEY_A key?  There are keysyms for all the extended keys 
> I thought.
I''m glad I''m not the only one who finds this confusing. :)

The KEY_ constants in linux/input.h are a lot closer to scancodes than
to traditional X-style keysyms.  This becomes obvious once you notice
that there''s no KEY_COLON, and there''s only one KEY_A rather
than a
KEY_A and a KEY_a.  While there is some small amount of normalisation,
so that you can mostly use the same keymaps for (say) AT and USB
keyboards, input core key events are best thought of as scan codes.
Calling them KEY_A etc. is more than a little bit deceptive, since
they actually refer more to places on the keyboard than actual
characters.

The actual key mapping is implemented in drivers/char/keyboard.c (look
for key_maps), and we''re going to have to go through that whether we
want to or not.

Of course, I completely failed to understand what was going on here a
couple of days ago, so there''s a decent chance I''ve failed to
understand this time as well.

Steven.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Steven Smith wrote:
>> I''m a bit surprised here.  If we generate a KEY_Q event in
Linux that
>> may show up as a KEY_A key?  There are keysyms for all the extended
keys
>> I thought.
>>     
> I''m glad I''m not the only one who finds this confusing.
:)
>   
And I''ve been trying to figure this out longer :-)
> The KEY_ constants in linux/input.h are a lot closer to scancodes than
> to traditional X-style keysyms.  This becomes obvious once you notice
> that there''s no KEY_COLON, and there''s only one KEY_A
rather than a
> KEY_A and a KEY_a.  While there is some small amount of normalisation,
> so that you can mostly use the same keymaps for (say) AT and USB
> keyboards, input core key events are best thought of as scan codes.
> Calling them KEY_A etc. is more than a little bit deceptive, since
> they actually refer more to places on the keyboard than actual
> characters.
>   
Okay, I didn''t realize the KEY_ constants weren''t keycodes.

I guess we''re stuck using a keymap :-/

Regards,

Anthony Liguori
> The actual key mapping is implemented in drivers/char/keyboard.c (look
> for key_maps), and we''re going to have to go through that whether
we
> want to or not.
>
> Of course, I completely failed to understand what was going on here a
> couple of days ago, so there''s a decent chance I''ve
failed to
> understand this time as well.
>
> Steven.
>   

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> >First: I now agree with you that scancodes are a better choice than
> >keysyms, and that I was wrong initially.
> The problem with scancodes is that you cannot always get scancodes from 
> the viewer.  You can get scancodes from SDL but you can only get keysyms 
> from VNC.  We would have to map VNC keysyms (which are just Xk keysyms) 
> to scancodes?
Ian just pointed out that there''s some code in rdesktop to do just
that.  xkeymap.c contains stuff to pull the keymap out of the X
server, parse it up, and then do keysym->scancode translation.  Of
course, choosing a suitable keymap isn''t entirely trivial, since the
server can''t see what keymap the client''s using.

Steven.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Steven Smith wrote:
>>> First: I now agree with you that scancodes are a better choice than
>>> keysyms, and that I was wrong initially.
>>>       
>> The problem with scancodes is that you cannot always get scancodes from
>> the viewer.  You can get scancodes from SDL but you can only get
keysyms
>> from VNC.  We would have to map VNC keysyms (which are just Xk keysyms)
>> to scancodes?
>>     
> Ian just pointed out that there''s some code in rdesktop to do just
> that.  xkeymap.c contains stuff to pull the keymap out of the X
> server, parse it up, and then do keysym->scancode translation.  Of
> course, choosing a suitable keymap isn''t entirely trivial, since
the
> server can''t see what keymap the client''s using.
>   
There is also code in QEMU that does the translation.  In fact, the QEMU 
code is based on the rdesktop tables :-)

Since the QEMU tables are already installed (in /usr/share/qemu/keymaps) 
I suppose it''s best to use those.

Regards,

Anthony Liguori
> Steven.
>   

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Sat, Sep 02, 2006 at 03:58:22PM -0400, Jeremy Katz
wrote:
> Initially from Anthony Liguori and then modified for
> * Integration with xenstore (armrbu)
> * Adding option parsing (katzj)
> * Integration with Xen makefiles 
> * Error handling (armbru)
> * Future-proof layout of shared page (armbru)
> * Memory barriers (armbru)
> * Support for vncunused and vnclisten as support in qemu-dm vnc (katzj)
> 
> Signed-off-by: Jeremy Katz <katzj@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Anthony Liguori <aliguori@us.ibm.com>
I''ve been stress testing the xen-vncfb daemon and found some very nasty
problems. First of all the code in  LibVNCServer which deals with pthreads
condition variables is utterly broken. It passes unlocked mutexes into
pthread_cond_wait, and in some places also tries to lock the mutex just
after existing the wait call. The former leads to a race condition & missed
conditions, the latter completely deadlocks the daemon. To make matters
worse the library fails to check return values form pthread_* calls pretty
much everywhere. Carrying on, the code uses mutexes to synchronize changes
to the struct members in the client state structure, but then fails to do
any locking for reads, leading to yet more race conditions & death by 
SIGBUS in a number of places.

Basically LibVNCServer should be considered extremely harmful :-(

the QEMU VNC code for HVM guests doesn''t use LibVNCServer, instead
following
a more sane select() reactor model multiplexing all I/O in a single process.
I think it would be highly desirable to replace the LibVNCServer code with
Anthony''s code from QEMU, because I don''t have any confidence
in running
something based on LibVNCServer as an unprivileged daemon, let alone root.

FWIW, I''m attaching a patch to LibVNCServer which fixes as many of the
issues I''ve found so far as possible. If you want a stable xen-vncfb 
daemon then I recommend applying these before trying to test the VNC
bits of paravirt framebuffer.

   Signed-off by: Daniel berrange <berrange@redhat.com>

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Steven Smith <sos22-xen@srcf.ucam.org> writes:
>> --- a/tools/Makefile	Sat Sep 02 15:11:17 2006 -0400
>> +++ b/tools/Makefile	Sat Sep 02 15:19:25 2006 -0400
>> @@ -18,6 +18,7 @@ SUBDIRS-y += xenstat
>>  SUBDIRS-y += xenstat
>>  SUBDIRS-y += libaio
>>  SUBDIRS-y += blktap
>> +SUBDIRS-y += xenfb
>>  
>>  # These don''t cross-compile
>>  ifeq ($(XEN_COMPILE_ARCH),$(XEN_TARGET_ARCH))
>> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tools/xenfb/Makefile	Sat Sep 02 15:19:25 2006 -0400
>> @@ -0,0 +1,36 @@
>> +XEN_ROOT=../..
>> +include $(XEN_ROOT)/tools/Rules.mk
>> +
>> +CFLAGS += -g -Wall
> You shouldn''t need to add -g here; Rules.mk handles it for you if
> debug is set.
Jeremy took care of this.
>> +CFLAGS += -I$(XEN_LIBXC) -I$(XEN_XENSTORE)
-I$(XEN_ROOT)/linux-2.6-xen-sparse/include
>> +LDFLAGS += -L$(XEN_LIBXC) -L$(XEN_XENSTORE)
>> +
>> +INSTALL         = install
>> +INSTALL_PROG    = $(INSTALL) -m0755
>> +INSTALL_DIR     = $(INSTALL) -d -m0755
>> +
>> +.PHONY: all
>> +all: build
>> +
>> +.PHONY: build
>> +build: mk-symlinks
>> +	$(MAKE) vncfb sdlfb
>> +
>> +install: all
>> +	$(INSTALL_DIR) -p $(DESTDIR)/usr/$(LIBDIR)/xen/bin
>> +	$(INSTALL_PROG) vncfb $(DESTDIR)/usr/$(LIBDIR)/xen/bin/xen-vncfb
>> +	$(INSTALL_PROG) sdlfb $(DESTDIR)/usr/$(LIBDIR)/xen/bin/xen-sdlfb
>> +
>> +sdlfb: sdlfb.o xenfb.o
>> +
>> +sdlfb.o: CFLAGS += $(shell sdl-config --cflags)
>> +sdlfb: LDLIBS += $(shell sdl-config --libs) -lxenctrl -lxenstore
>> +
>> +clean:
>> +	$(RM) *.o *~ vncfb sdlfb
>> +
>> +keymapping.o: CFLAGS += $(shell pkg-config --cflags gtk+-2.0)
>> +
>> +vncfb: vncfb.o xenfb.o keymapping.o
>> +vncfb.o: CFLAGS += $(shell libvncserver-config --cflags)
>> +vncfb: LDLIBS += $(shell libvncserver-config --libs) -lxenctrl
-lxenstore
>> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tools/xenfb/keymapping.c	Sat Sep 02 15:19:25 2006 -0400
>> @@ -0,0 +1,141 @@
>> +#include <stdint.h>
>> +#include <gdk/gdkkeysyms.h>
>> +#include <linux/input.h>
>> +
>> +uint32_t gdk_linux_mapping[0x10000] = {
>> +	[GDK_a] = KEY_A,
> This is kind of ugly.  Is there any chance it could be autogenerated?
> Also, where did 0x10000 come from?
>
> Also, depending on GTK just for the keymap table is a real pain.  Or
> is it already required for libvncserver?
Jeremy answered this one.

There was quite a bit of discussion on how to best encode keys (thanks
to Laurent for patiently explaining the issue).  I''m *not* ignoring
that problem.  However, the patch is large enough as it is, so let''s
get the things it addresses nailed down before we address the key code
problem.
> <snip>
>
>> +	[GDK_plus] = KEY_EQUAL,
>> +};
>> +
>> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tools/xenfb/sdlfb.c	Sat Sep 02 15:19:25 2006 -0400
>> @@ -0,0 +1,191 @@
>> +#include <SDL.h>
>> +#include <sys/types.h>
>> +#include <sys/select.h>
>> +#include <stdlib.h>
>> +#include <linux/input.h>
>> +#include <getopt.h>
>> +#include <string.h>
>> +#include "xenfb.h"
>> +
>> +struct data
> That''s a really wonderful name.
Jeremy took care of this.
>> +{
>> +	SDL_Surface *dst;
>> +	SDL_Surface *src;
>> +};
>> +
>> +void sdl_update(struct xenfb *xenfb, int x, int y, int width, int
height)
>> +{
>> +	struct data *data = xenfb->user_data;
>> +	SDL_Rect r = { x, y, width, height };
>> +	SDL_BlitSurface(data->src, &r, data->dst, &r);
>> +	SDL_UpdateRect(data->dst, x, y, width, height);
>> +}
>> +
>> +int sdl2linux[1024] = {
>> +	[SDLK_a] = KEY_A,
> Another really ugly mapping table, although not quite as bad as the
> GTK one.
>
> Where''d the magic 1024 come from?
Jeremy answered this one.
>> +	[SDLK_RALT] = KEY_RIGHTALT,
>> +};
>> +
>> +static struct option options[] = {
>> +    { "domid", 1, NULL, ''d'' },
>> +    { "title", 1, NULL, ''t'' },
>> +};
>> +
>> +int main(int argc, char **argv)
>> +{
>> +	struct xenfb *xenfb;
>> +	int fd;
>> +	int domid = -1;
>> +        char * title = NULL;
>> +	fd_set readfds;
>> +	struct data data;
>> +	SDL_Rect r;
>> +	struct timeval tv = { 0, 500 };
>> +	int do_quit = 0;
>> +        int opt;
> Slightly strange whitespace, but nevermind.
Fixed anyway.
>> +
>> +	while ((opt = getopt_long(argc, argv, "d:t:", options,
>> +				  NULL)) != -1) {
>> +		switch (opt) {
>> +                case ''d'':
>> +                    domid = strtol(optarg, NULL, 10);
> It''d be nice to check for a malformed argument here.
Done.
>> +                    break;
>> +                case ''t'':
>> +                    title = strdup(optarg);
> This can fail.
Jeremy answered this one.
>> +                    break;
>> +                }
>> +        }
>> +        if (optind != argc) {
>> +            fprintf(stderr, "Invalid options!\n");
>> +            exit(1);
> errx() maybe?
Jeremy answered this one.
>> +        }
>> +        if (domid == -1) {
>> +            fprintf(stderr, "Domain ID must be
specified!\n");
>> +            exit(1);
>> +        }
>> +
>> +	xenfb = xenfb_new();
>> +	if (xenfb == NULL)
>> +		return 1;
> Why have you used exit(1) in some places and return 1 in others?
It''s all exit(1) now.
> Also, an error message here would be a good idea.
Done.
>> +
>> +	if (!xenfb_attach_dom(xenfb, domid))
>> +		return 1;
> An error mesasge would be good.
Done.
>> +
>> +	SDL_Init(SDL_INIT_VIDEO);
>> +
>> +	fd = xenfb_get_fileno(xenfb);
>> +
>> +	data.dst = SDL_SetVideoMode(xenfb->width, xenfb->height,
xenfb->depth,
>> +				    SDL_SWSURFACE);
>> +
>> +	data.src = SDL_CreateRGBSurfaceFrom(xenfb->pixels,
>> +					    xenfb->width, xenfb->height,
>> +					    xenfb->depth, xenfb->row_stride,
>> +					    0xFF0000, 0xFF00, 0xFF, 0);
>> +
>> +        if (title == NULL)
>> +            title = strdup("xen-sdlfb");
> This can fail.
Jeremy answered this one.
>> +        SDL_WM_SetCaption(title, title);
>> +
>> +	r.x = r.y = 0;
>> +	r.w = xenfb->width;
>> +	r.h = xenfb->height;
>> +	SDL_BlitSurface(data.src, &r, data.dst, &r);
>> +	SDL_UpdateRect(data.dst, 0, 0, xenfb->width, xenfb->height);
>> +
>> +	xenfb->update = sdl_update;
>> +	xenfb->user_data = &data;
>> +
>> +	FD_ZERO(&readfds);
>> +	FD_SET(fd, &readfds);
>> +
>> +	SDL_ShowCursor(0);
>> +
>> +	while (!do_quit && select(fd + 1, &readfds, NULL, NULL,
&tv) != -1) {
> Select can say -1 because of EINTR (e.g. when strace attaches). 
It''s
> not clear to me whether you want to exit or retry in that case.
>
> Also, if you quit because select returns -1, you need an error
> message.
Reworked the loop.
>> +		SDL_Event event;
>> +
>> +		while (SDL_PollEvent(&event)) {
>> +			switch (event.type) {
>> +			case SDL_KEYDOWN:
>> +			case SDL_KEYUP:
>> +				xenfb_send_key(xenfb,
>> +					       event.type == SDL_KEYDOWN,
>> +					       sdl2linux[event.key.keysym.sym]);
>> +				break;
>> +			case SDL_MOUSEMOTION: {
>> +				int x, y;
>> +				Uint8 button;
>> +
>> +				button = SDL_GetRelativeMouseState(&x, &y);
>> +				xenfb_send_motion(xenfb, x, y);
>> +			}	break;
>> +			case SDL_MOUSEBUTTONDOWN:
>> +			case SDL_MOUSEBUTTONUP:
>> +				xenfb_send_button(xenfb,
>> +						  event.type==SDL_MOUSEBUTTONDOWN,
>> +						  3 - event.button.button);
>> +				break;
>> +			case SDL_QUIT:
>> +				do_quit = 1;
>> +				break;
>> +			}
>> +		}
>> +		if (FD_ISSET(fd, &readfds))
>> +			xenfb_on_incoming(xenfb);
>> +
>> +		FD_ZERO(&readfds);
>> +		FD_SET(fd, &readfds);
>> +
>> +		tv = (struct timeval){0, 500};
> I think 500us is a little short here.  About ten milliseconds sounds
> more plausible.  This is a bit of a bikeshed.
Changed to 10ms.
> It''s a pity SDL doesn''t allow you to wait for either an
SDL event or
> an fd to become readable.  Could you do something like spawn a thread
> which does the selects in a loop, and then SDL_PushEvent()s a user
> event when the fd becomes readable?
>
> Admittedly, SDL_WaitEvent also contains this kind of loop-with-sleep,
> but it''d reduce the number of magic tunables a bit.
Added a comment explaining the clunkiness and sketching the solution
you proposed.
>> +	}
>> +
>> +	xenfb_delete(xenfb);
>> +
>> +	SDL_Quit();
>> +
>> +	return 0;
>> +}
>> --- b/tools/xenfb/vncfb.c	Sat Sep 02 15:19:25 2006 -0400
>> +++ b/tools/xenfb/vncfb.c	Sat Sep 02 15:22:19 2006 -0400
>
> Minor nit: generally, putting a vnc_ prefix on these functions
> confused me, since it looks like they should be in libvncserver.  This
> may just be because I''m not paying enough attention.
>
>> @@ -0,0 +1,245 @@
>> +#define _GNU_SOURCE
>> +#include <errno.h>
>> +#include <getopt.h>
>> +#include <stdlib.h>
>> +#include <unistd.h>
>> +#include <malloc.h>
>> +#include <rfb/rfb.h>
>> +#include <xs.h>
>> +#include "xenfb.h"
>> +
>> +static void on_kbd_event(rfbBool down, rfbKeySym keycode, rfbClientPtr
cl)
>> +{
>> +	extern uint32_t gdk_linux_mapping[0x10000];
> Is there any chance of moving this into a header file somewhere?
I moved the table into this file instead.
>> +	rfbScreenInfoPtr server = cl->screen;
>> +	struct xenfb *xenfb = server->screenData;
>> +	xenfb_send_key(xenfb, down, gdk_linux_mapping[keycode & 0xFFFF]);
>> +}
>> +
>> +static void on_ptr_event(int buttonMask, int x, int y, rfbClientPtr
cl)
>> +{
>> +	static int last_x = -1, last_y = -1;
>> +	static int last_button = -1;
>> +	rfbScreenInfoPtr server = cl->screen;
>> +	struct xenfb *xenfb = server->screenData;
>> +
>> +	if (last_button != -1) {
>> +		int i;
>> +
>> +		for (i = 0; i < 8; i++) {
>> +			if ((last_button & (1 << i)) !>> +			   
(buttonMask & (1 << i))) {
>> +				printf("%d %d\n", buttonMask & (1 << i), i);
> Umm?
Anthony''s debug code.  Gone.
>> +				xenfb_send_button(xenfb, buttonMask & (1 << i),
>> +						  2 - i);
>> +			}
>> +		}
>> +	}
>> +
>> +	if (last_x != -1)
>> +		xenfb_send_motion(xenfb, x - last_x, y - last_y);
>> +
>> +	last_button = buttonMask;
>> +
>> +	last_x = x;
>> +	last_y = y;
>> +}
>> +
>> +static void xenstore_write_vncport(int port, int domid)
>> +{
>> +    char *buf = NULL, *path;
>> +    char *portstr = NULL;
>> +    struct xs_handle *xsh = NULL;
>> +
>> +    xsh = xs_daemon_open();
>> +    if (xsh == NULL)
>> +	return;
>> +
>> +    path = xs_get_domain_path(xsh, domid);
>> +    if (path == NULL) {
>> +        fprintf(stderr, "xs_get_domain_path() error\n");
>> +        goto out;
>> +    }
>> +
>> +    buf = malloc(256);
> Could fail.  Also, consider using asprintf.
Jeremy took care of this.
>> +    if (snprintf(buf, 256, "%s/console/vnc-port", path) ==
-1)
>> +	goto out;
>> +
>> +    portstr = malloc(10);
> Why is this on the heap rather than the stack?
Jeremy took care of this.
>> +    if (snprintf(portstr, 10, "%d", port) == -1)
>> +	goto out;
>> +
>> +    if (xs_write(xsh, XBT_NULL, buf, portstr, strlen(portstr)) == 0)
>> +        fprintf(stderr, "xs_write() vncport failed\n");
>> +
>> + out:
>> +    free(portstr);
>> +    free(buf);
>> +}
>> +
>> +
>> +static void vnc_update(struct xenfb *xenfb, int x, int y, int w, int
h)
>> +{
>> +	rfbScreenInfoPtr server = xenfb->user_data;
>> +	rfbMarkRectAsModified(server, x, y, x + w, y + h);
>> +}
>> +
>> +static int vnc_start_viewer(int port) 
> I''m not convinced the backend server process is the best place to
> start the viewer from.  Perhaps xend would be a better choice?  Not
> sure about this.
Jeremy answered this one.
>> +{
>> +    int pid;
>> +    char s[16];
>> +
>> +    snprintf(s, 16, ":%d", port);
>> +    switch (pid = fork()) {
>> +    case -1:
>> +	fprintf(stderr, "vncviewer failed fork\n");
>> +	exit(1);
> err()?
>
>> +
>> +    case 0:	/* child */
>> +	execlp("vncviewer", "vncviewer", s, 0);
>> +	fprintf(stderr, "vncviewer execlp failed\n");
>> +	exit(1);
> err()?
>
>> +
>> +    default:
>> +	return pid;
> This is ignored.  Also, the parent process makes no attempt to check
> whether the child was exec()ed successfully or anything along those
> lines.  This is enough of a pain to fix that I''d probably just
ignore
> it, though.
Agreed.
>> +    }
>> +}
>> +
>> +static struct option options[] = {
>> +    { "domid", 1, NULL, ''d'' },
>> +    { "vncport", 1, NULL, ''p'' },
>> +    { "title", 1, NULL, ''t'' },
>> +    { "unused", 0, NULL, ''u'' },
> What does this do?
Jeremy?
>> +    { "listen", 1, NULL, ''l'' },
>> +    { "vncviewer", 0, NULL, ''v'' },
>> +};
>> +
>> +int main(int argc, char **argv)
>> +{
>> +	rfbScreenInfoPtr server;
>> +	char *fake_argv[7] = { "vncfb", "-rfbport",
"5901",
>> +                               "-desktop",
"xen-vncfb",
>> +                               "-listen",
"0.0.0.0" };
>> +	int fake_argc = sizeof(fake_argv) / sizeof(fake_argv[0]);
>> +	int domid = -1, port = -1;
>> +        char * title = NULL;
>> +        char * listen = NULL;
> Whitespace is a bit funny again.
Fixed.
>> +	struct xenfb *xenfb;
>> +	fd_set readfds;
>> +	int fd;
>> +	char buffer[1024];
> Could do with a better name, and is larger than it needs to be.
Jeremy took care of this.
>> +        int opt;
>> +        bool unused = FALSE;
> You''re inconsistent about the capitalisation of bools.
Jeremy took care of this.
>> +        bool viewer = FALSE;
>> +
>> +	while ((opt = getopt_long(argc, argv, "d:p:t:u", options,
>> +				  NULL)) != -1) {
>> +		switch (opt) {
>> +                case ''d'':
>> +                    domid = strtol(optarg, NULL, 10);
> It would be nice to sanity check the argument here.
Done.
>> +                    break;
>> +                case ''p'':
>> +                    port = strtol(optarg, NULL, 10);
> Again.
Done.
>> +                    break;
>> +                case ''t'':
>> +                    title = strdup(optarg);
> Can fail.
>
>> +                    break;
>> +                case ''u'':
>> +                    unused = TRUE;
>> +                    break;
>> +                case ''l'':
>> +                    listen = strdup(optarg);
> Can fail.
>
>> +                    break;
>> +                case ''v'':
>> +                    viewer = TRUE;
>> +                    break;
>> +                case ''l'':
>> +                    listen = strdup(optarg);
> Can fail.
>
>> +                    break;
>> +                }
>> +        }
>> +        if (optind != argc) {
>> +            fprintf(stderr, "Invalid options!\n");
>> +            exit(1);
>> +        }
>> +        if (domid == -1) {
>> +            fprintf(stderr, "Domain ID must be
specified!\n");
>> +            exit(1);
>> +        }
>> +            
>> +        if (port == -1)
>> +            port = 5900 + domid;
>> +	snprintf(buffer, sizeof(buffer), "%d", port);
>> +	fake_argv[2] = buffer;
>> +
>> +        if (title != NULL)
>> +            fake_argv[4] = title;
>> +
>> +        if (listen != NULL)
>> +            fake_argv[6] = listen;
>> +
>> +        if (listen != NULL)
>> +            fake_argv[6] = listen;
> Umm... What''s going on here?
Jeremy took care of this.
>> +
>> +	xenfb = xenfb_new();
>> +	if (xenfb == NULL) {
>> +		fprintf(stderr, "Could not create framebuffer (%s)\n",
>> +			strerror(errno));
>> +		exit(1);
>> +	}
>> +
>> +	if (!xenfb_attach_dom(xenfb, domid)) {
>> +		fprintf(stderr, "Could not connect to domain (%s)\n",
>> +			strerror(errno));
>> +		exit(1);
>> +	}
>> +
>> +	server = rfbGetScreen(&fake_argc, fake_argv, 
>> +			      xenfb->width, xenfb->height,
>> +			      8, 3, xenfb->depth / 8);
>> +	if (server == NULL) {
>> +		fprintf(stderr, "Could not create VNC server\n");
>> +		exit(1);
>> +	}
>> +
>> +	xenfb->user_data = server;
>> +	xenfb->update = vnc_update;
>> +
>> +        if (unused)
>> +            server->autoPort = TRUE;
>> +
>> +	server->serverFormat.redShift = 16;
>> +	server->serverFormat.greenShift = 8;
>> +	server->serverFormat.blueShift = 0;
>> +	server->kbdAddEvent = on_kbd_event;
>> +	server->ptrAddEvent = on_ptr_event;
>> +	server->frameBuffer = (char *)xenfb->pixels;
>> +	server->screenData = xenfb;
>> +	rfbInitServer(server);
>> +
>> +	rfbRunEventLoop(server, -1, TRUE);
>> +
>> +	fd = xenfb_get_fileno(xenfb);
>> +
>> +	FD_ZERO(&readfds);
>> +	FD_SET(fd, &readfds);
>> +
>> +        xenstore_write_vncport(server->port, domid);
>> +
>> +        if (viewer)
>> +            vnc_start_viewer(server->port);
>> +
>> +	while (select(fd + 1, &readfds, NULL, NULL, NULL) != -1) {
>> +		if (FD_ISSET(fd, &readfds)) {
>> +			xenfb_on_incoming(xenfb);
>> +		}
>> +		
>> +		FD_ZERO(&readfds);
>> +		FD_SET(fd, &readfds);
>> +	}
>> +
>> +	rfbScreenCleanup(server);
>> +	xenfb_delete(xenfb);
>> +
>> +	return 0;
>> +}
>> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tools/xenfb/xenfb.c	Sat Sep 02 15:19:25 2006 -0400
>> @@ -0,0 +1,434 @@
>> +#include <malloc.h>
>> +#include <stdlib.h>
>> +#include <sys/types.h>
>> +#include <fcntl.h>
>> +#include <unistd.h>
>> +#include <xenctrl.h>
>> +#include <linux/xenfb.h>
>> +#include <linux/xenkbd.h>
>> +#include <sys/select.h>
>> +#include <stdbool.h>
>> +#include <xen/linux/evtchn.h>
>> +#include <xen/event_channel.h>
>> +#include <sys/mman.h>
>> +#include <errno.h>
>> +#include <stdio.h>
>> +#include <string.h>
>> +#include <time.h>
>> +#include <xs.h>
>> +
>> +#include "xenfb.h"
>> +
>> +// FIXME defend against malicous backend?
> Well, this is the backend, so defending against a malicious frontend
> would be a better choice.
Thanks.
>> +
>> +struct xenfb_private
>> +{
>> +	struct xenfb pub;
>> +	int domid;
>> +	unsigned long fbdev_mfn, kbd_mfn;
>> +	int fbdev_evtchn, kbd_evtchn;
>> +	evtchn_port_t fbdev_port, kbd_port;
> How do {fbdev,kbd}_port differ from {fbdev,kbd}_evtchn?
Jeremy answered this one.
> The _evtchn fields are only ever accessed from xenfb_attach_dom.  Could
> they be locals to that function?
Done.
>> +	int evt_xch;
>> +	int xc;
>> +	unsigned char *fb;
>> +	struct xenfb_page *fb_info;
>> +	struct xenkbd_info *kbd_info;
>> +	unsigned long *fbmfns;
>> +	int n_fbmfns, n_fbdirs;
>> +};
>> +
>> +struct xenfb *xenfb_new(void)
>> +{
>> +	struct xenfb_private *xenfb = malloc(sizeof(*xenfb));
>> +
>> +	if (xenfb == NULL)
>> +		return NULL;
>> +
>> +	memset(xenfb, 0, sizeof(*xenfb));
> Use calloc instead of malloc, perhaps?
Hate it.
>> +
>> +	xenfb->domid = -1;
>> +
>> +	xenfb->evt_xch = xc_evtchn_open();
>> +	if (xenfb->evt_xch == -1) {
>> +		int serrno = errno;
>> +		free(xenfb);
>> +		errno = serrno;
>> +		return NULL;
>> +	}
>> +
>> +	xenfb->xc = xc_interface_open();
>> +	if (xenfb->xc == -1) {
>> +		int serrno = errno;
>> +		xc_evtchn_close(xenfb->evt_xch);
>> +		free(xenfb);
>> +		errno = serrno;
>> +		return NULL;
> It''s a pity we don''t have a macro which hides this
ugliness.  Perhaps
>
> #define PRESERVING_ERRNO(x) do {
> 	int tmp = errno;
> 	x;
> 	errno = tmp;
> } while (0)
>
> You could then do something like
>
> if (xenfb_evt_sch == -1) {
> 	PRESERVING_ERRNO(xc_evtchn_close(xenfb->evt_xch);free(xenfb));
> 	return NULL;
> }
>
> Not sure whether that''s more or less ugly, to be honest.
I think I prefer my poison straight here, without macro wrapping :)
>> +	}
>> +
>> +	return &xenfb->pub;
>> +}
>> +
>> +int xenfb_get_fileno(struct xenfb *xenfb_pub)
>> +{
>> +	struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
>> +
>> +	return xc_evtchn_fd(xenfb->evt_xch);
>> +}
>> +
>> +static void xenfb_detach_dom(struct xenfb_private *xenfb)
>> +{
>> +	xenfb->domid = -1;
>> +	munmap(xenfb->fb, xenfb->fb_info->mem_length);
>> +	munmap(xenfb->fb_info, XC_PAGE_SIZE);
>> +	munmap(xenfb->kbd_info, XC_PAGE_SIZE);
>> +	xc_evtchn_unbind(xenfb->evt_xch, xenfb->fbdev_port);
>> +	xc_evtchn_unbind(xenfb->evt_xch, xenfb->kbd_port);
>> +}
>> +
>> +void xenfb_delete(struct xenfb *xenfb_pub)
>> +{
>> +	struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
>> +	if (xenfb->domid != -1)
>> +		xenfb_detach_dom(xenfb);
>> +	free(xenfb);
>> +}
>> +
>> +static int xenfb_fb_event(struct xenfb_private *xenfb, union
xenfb_in_event *event)
>> +{
>> +	uint32_t prod;
>> +	struct xenfb_page *info = xenfb->fb_info;
>> +
>> +	prod = info->in_prod;
>> +	if (prod - info->in_cons == XENFB_IN_RING_LEN) {
>> +	    errno = EAGAIN;
>> +	    return -1;
>> +	}
>> +
>> +	mb();			/* ensure ring space available */
>> +	XENFB_IN_RING_REF(info, prod) = *event;
>> +	wmb();			/* ensure ring contents visible */
>> +	info->in_prod = prod + 1;
>> +	return xc_evtchn_notify(xenfb->evt_xch, xenfb->fbdev_port);
>> +}
>> +
>> +static int xenfb_kbd_event(struct xenfb_private *xenfb, union
xenkbd_in_event *event)
>> +{
>> +	uint32_t prod;
>> +	struct xenkbd_info *info = xenfb->kbd_info;
>> +
>> +	prod = info->in_prod;
>> +	if (prod - info->in_cons == XENKBD_IN_RING_LEN) {
>> +	    errno = EAGAIN;
>> +	    return -1;
>> +	}
>> +
>> +	mb();			/* ensure ring space available */
>> +	XENKBD_IN_RING_REF(info, prod) = *event;
>> +	wmb();			/* ensure ring contents visible */
>> +	info->in_prod = prod + 1;
>> +	return xc_evtchn_notify(xenfb->evt_xch, xenfb->kbd_port);
>> +}
>> +
>> +static char *xenfb_path_in_dom(struct xs_handle *h,
>> +			 unsigned domid, const char *path,
>> +			 char *buffer, size_t size)
>> +{
>> +	char *domp = xs_get_domain_path(h, domid);
> Can fail.
Jeremy took care of this.
>> +	int n = snprintf(buffer, size, "%s/%s", domp, path);
>> +	free(domp);
>> +	if (n >= size)
>> +		return NULL;
>> +	return buffer;
>> +}
>> +
>> +static int xenfb_xs_scanf1(struct xs_handle *xsh, unsigned domid,
>> +			   const char *path, const char *fmt,
>> +			   void *dest)
>> +{
>> +	char buffer[1024];
>> +	char *p;
>> +	int ret;
>> +
>> +	p = xenfb_path_in_dom(xsh, domid, path, buffer, sizeof(buffer));
> What happens if this fails?
Catched.
>> +	p = xs_read(xsh, XBT_NULL, p, NULL);
>> +	if (!p)
>> +		return -ENOENT;
>> +	ret = sscanf(p, fmt, dest);
>> +	free(p);
>> +	if (ret != 1)
>> +		return -EDOM;
>> +	return 0;
>> +}
> You''re somewhat inconsistent about returning error numbers as
negative
> return values or through errno.  I''d prefer the latter in
userspace
> code, but it doesn''t matter too much, privided you pick one.
Jeremy took care of this.
>> +
>> +bool xenfb_attach_dom(struct xenfb *xenfb_pub, int domid)
>> +{
>> +	struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
>> +	char buffer[1024];
>> +	struct xs_handle *xsh;
>> +	unsigned dummy;
>> +	int ret;
>> +	char *p, **vec;
>> +	union xenfb_in_event event;
>> +
>> +	if (xenfb->domid != -1) {
>> +		xenfb_detach_dom(xenfb);
>> +		if (domid == -1)
>> +			return true;
>> +	}
>> +
>> +	xsh = xs_daemon_open_readonly();
>> +	if (!xsh)
>> +	    goto error;
>> +
>> +	p = xenfb_path_in_dom(xsh, domid, "vfb", buffer,
sizeof(buffer));
>> +	if (!xs_watch(xsh, p, ""))
>> +		goto error;
>> +	p = xenfb_path_in_dom(xsh, domid, "vkbd", buffer,
sizeof(buffer));
>> +	if (!xs_watch(xsh, p, ""))
>> +		goto error;
>> +
>> +	for (;;) {
>> +		ret = xenfb_xs_scanf1(xsh, domid, "vfb/page-ref",
"%lu",
>> +				      &xenfb->fbdev_mfn);
>> +		if (ret == -ENOENT || ret == -EAGAIN)
> xenfb_xs_scanf can''t return -EAGAIN.  What are you trying to
achieve
> here?
Jeremy took care of this.
>> +			goto wait;
>> +		if (ret < 0)
>> +			goto error;
>> +		ret = xenfb_xs_scanf1(xsh, domid, "vfb/event-channel",
"%u",
>> +				      &xenfb->fbdev_evtchn);
>> +		if (ret == -ENOENT || ret == -EAGAIN)
>> +			goto wait;
>> +		if (ret < 0)
>> +			goto error;
>> +		ret = xenfb_xs_scanf1(xsh, domid, "vkbd/page-ref",
"%lu",
>> +				      &xenfb->kbd_mfn);
>> +		if (ret == -ENOENT || ret == -EAGAIN)
>> +			goto wait;
>> +		if (ret < 0)
>> +			goto error;
>> +		ret = xenfb_xs_scanf1(xsh, domid, "vkbd/event-channel",
"%u",
>> +				      &xenfb->kbd_evtchn);
>> +		if (ret == -ENOENT || ret == -EAGAIN)
>> +			goto wait;
>> +		if (ret < 0)
>> +			goto error;
>> +		break;
>> +
>> +	wait:
>> +		printf("Waiting...\n");
> Where does this message go?
Jeremy answered this one.
>> +		vec = xs_read_watch(xsh, &dummy);
>> +		if (!vec)
>> +			goto error;
>> +		free(vec);
>> +	}
>> +	xs_daemon_close(xsh);
>> +	xsh = NULL;
>> +
>> +	xenfb->fbdev_port = xc_evtchn_bind_interdomain(xenfb->evt_xch,
domid,
>> +						       xenfb->fbdev_evtchn);
>> +	if (xenfb->fbdev_port == -1)
>> +		goto error;
>> +
>> +	xenfb->kbd_port = xc_evtchn_bind_interdomain(xenfb->evt_xch,
domid,
>> +						     xenfb->kbd_evtchn);
>> +	if (xenfb->kbd_port == -1) 
>> +		goto error_fbdev;
>> +
>> +	xenfb->fb_info = xc_map_foreign_range(xenfb->xc, domid,
XC_PAGE_SIZE,
>> +					      PROT_READ | PROT_WRITE,
>> +					      xenfb->fbdev_mfn);
>> +	if (xenfb->fb_info == NULL)
>> +		goto error_kbd;
>> +
>> +	xenfb->kbd_info = xc_map_foreign_range(xenfb->xc, domid,
XC_PAGE_SIZE,
>> +					       PROT_READ | PROT_WRITE,
>> +					       xenfb->kbd_mfn);
>> +	if (xenfb->kbd_info == NULL)
>> +		goto error_fbinfo;
>> +
>> +	xenfb->n_fbmfns = (xenfb->fb_info->mem_length +
(XC_PAGE_SIZE - 1)) / XC_PAGE_SIZE;
>> +	xenfb->n_fbdirs = xenfb->n_fbmfns * sizeof(unsigned long);
>> +	xenfb->n_fbdirs = (xenfb->n_fbdirs + (XC_PAGE_SIZE - 1)) /
XC_PAGE_SIZE;
>> +
>> +	xenfb->fbmfns = xc_map_foreign_batch(xenfb->xc, domid,
PROT_READ, xenfb->fb_info->pd, xenfb->n_fbdirs);
>> +	if (xenfb->fbmfns == NULL)
>> +		goto error_kbdinfo;
>> +
>> +	xenfb->fb = xc_map_foreign_batch(xenfb->xc, domid, PROT_READ |
PROT_WRITE, xenfb->fbmfns, xenfb->n_fbmfns);
>> +	if (xenfb->fb == NULL)
>> +		goto error_fbmfns;
>> +
>> +	event.type = XENFB_TYPE_SET_EVENTS;
>> +	event.set_events.flags = XENFB_FLAG_UPDATE;
>> +	if (xenfb_fb_event(xenfb, &event))
>> +		goto error_fb;
>> +
>> +	munmap(xenfb->fbmfns, xenfb->n_fbdirs * XC_PAGE_SIZE);
> Please make fbmfns a local rather than putting it in the info
> structure.
Done.
>> +
>> +	xenfb->domid = domid;
>> +
>> +	xenfb->pub.pixels = xenfb->fb;
>> +
>> +	xenfb->pub.row_stride = xenfb->fb_info->line_length;
>> +	xenfb->pub.depth = xenfb->fb_info->depth;
>> +	xenfb->pub.width = xenfb->fb_info->width;
>> +	xenfb->pub.height = xenfb->fb_info->height;
>> +
>> +	return true;
>> +
>> + error_fb:
> The error path here is utterly revolting.  Perhaps something like this:
>
> error:
> 	serrno = errno;
> 	if (xenfb->fb)
> 		munmap(xenfb->fb, xenfb->fb_info->mem_length);
> 	if (fbmfns)
> 		munmap(fbmfns, xenfb->fb_info->mem_length);
> 	...
> 	errno = serrno;
>
> 	return false;
>
> Or would that be too easy?
Done.
>> +	printf("%d\n", __LINE__);
>> +	{
>> +		int serrno = errno;
>> +		munmap(xenfb->fb, xenfb->fb_info->mem_length);
>> +		errno = serrno;
>> +	}
>> + error_fbmfns:
>> +	printf("%d\n", __LINE__);
>> +	{
>> +		int serrno = errno;
>> +		munmap(xenfb->fbmfns, xenfb->n_fbdirs * XC_PAGE_SIZE);
>> +		errno = serrno;
>> +	}
>> + error_kbdinfo:
>> +	printf("%d\n", __LINE__);
>> +	{
>> +		int serrno = errno;
>> +		munmap(xenfb->kbd_info, XC_PAGE_SIZE);
>> +		errno = serrno;
>> +	}
>> + error_fbinfo:
>> +	printf("%d\n", __LINE__);
>> +	{
>> +		int serrno = errno;
>> +		munmap(xenfb->fb_info, XC_PAGE_SIZE);
>> +		errno = serrno;
>> +	}
>> + error_kbd:
>> +	printf("%d\n", __LINE__);
>> +	{
>> +		int serrno = errno;
>> +		xc_evtchn_unbind(xenfb->evt_xch, xenfb->kbd_port);
>> +		errno = serrno;
>> +	}
>> + error_fbdev:
>> +	printf("%d\n", __LINE__);
>> +	{
>> +		int serrno = errno;
>> +		xc_evtchn_unbind(xenfb->evt_xch, xenfb->fbdev_port);
>> +		errno = serrno;
>> +	}
>> + error:
>> +	printf("%d\n", __LINE__);
>> +	if (xsh) {
>> +		int serrno = errno;
>> +		xs_daemon_close(xsh);
> I think you may end up closing the connection to the daemon twice here.
Don''t think so, because xsh is cleared on the other close.
>> +		errno = serrno;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>> +static void xenfb_update(struct xenfb_private *xenfb, int x, int y,
int width, int height)
>> +{
>> +	if (xenfb->pub.update)
>> +		xenfb->pub.update(&xenfb->pub, x, y, width, height);
>> +}
> I''m not convinced this wrapper is actually needed, given that
it''s
> utterly trivial and only called from one place.
It''s gone.
>> +
>> +static void xenfb_on_fb_event(struct xenfb_private *xenfb)
>> +{
>> +	uint32_t prod, cons;
>> +	struct xenfb_page *info = xenfb->fb_info;
>> +
>> +	prod = info->out_prod;
>> +	rmb();			/* ensure we see ring contents up to prod */
>> +	for (cons = info->out_cons; cons != prod; cons++) {
>> +		union xenfb_out_event *event = &XENFB_OUT_RING_REF(info, cons);
>> +
>> +		switch (event->type) {
>> +		case XENFB_TYPE_UPDATE:
>> +			xenfb_update(xenfb, event->update.x, event->update.y,
event->update.width, event->update.height);
>> +			break;
>> +		}
>> +	}
>> +	mb();			/* ensure we''re done with ring contents */
>> +	info->out_cons = cons;
>> +	// FIXME need to notify?
> Maybe.  If there''s any possibility of the frontend queuing evetns
when
> the ring is full, yes.  It doesn''t at the moment, but if you want
to
> add it in the future and maintain forward compatibility you need it.
Added the notify.
>> +}
>> +
>> +static void xenfb_on_kbd_event(struct xenfb_private *xenfb)
>> +{
>> +	uint32_t prod, cons;
>> +	struct xenkbd_info *info = xenfb->kbd_info;
>> +
>> +	prod = info->out_prod;
>> +	rmb();			/* ensure we see ring contents up to prod */
>> +	for (cons = info->out_cons; cons != prod; cons++) {
>> +		union xenkbd_out_event *event = &XENKBD_OUT_RING_REF(info,
cons);
>> +
>> +		switch (event->type) {
>> +		default:
>> +			break;
>> +		}
>> +	}
>> +	mb();			/* ensure we''re done with ring contents */
>> +	info->out_cons = cons;
>> +	// FIXME need to notify?
>> +}
>
> I''d replace this with
>
> +static void xenfb_on_kbd_event(struct xenfb_private *xenfb)
> +{
> +	struct xenkbd_info *info = xenfb->kbd_info;
> +       /* We don''t understand any keyboard events, so just ignore
them. */
> +	info->out_cons = info->out_prod;
> +}
>
> It''s smaller, easier to understand, and more efficient.
Taken.
> As for the FIXME, the protocol spec says you need it, but I''m not
sure
> it''s actually all that useful.  It will only make any difference
if
> the frontend starts queueing keyboard events if it finds the queue to
> be full when it tries to send one.
Added the notify.
>> +
>> +int xenfb_on_incoming(struct xenfb *xenfb_pub)
>> +{
>> +	struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
>> +	evtchn_port_t port;
>> +
>> +	port = xc_evtchn_pending(xenfb->evt_xch);
>> +	if (port == -1)
>> +		return -1;
>> +
>> +	if (port == xenfb->fbdev_port) {
>> +		xenfb_on_fb_event(xenfb);
>> +	} else if (port == xenfb->kbd_port) {
>> +		xenfb_on_kbd_event(xenfb);
>> +	}
>> +
>> +	if (xc_evtchn_unmask(xenfb->evt_xch, port) == -1)
>> +		return -1;
>> +
>> +	return 0;
>> +}
>> +
>> +int xenfb_send_key(struct xenfb *xenfb_pub, bool down, int keycode)
>> +{
>> +	struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
>> +	union xenkbd_in_event event;
>> +
>> +	event.type = XENKBD_TYPE_KEY;
>> +	event.key.pressed = down ? 1 : 0;
>> +	event.key.keycode = keycode;
>> +
>> +	return xenfb_kbd_event(xenfb, &event);
>> +}
>> +
>> +int xenfb_send_motion(struct xenfb *xenfb_pub, int rel_x, int rel_y)
> Is this sending XENKBD_TYPE_MOTION or XENFB_TYPE_MOTION?
XENFB_TYPE_MOTION doesn''t exist anymore.
>> +{
>> +	struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
>> +	union xenkbd_in_event event;
>> +
>> +	event.type = XENKBD_TYPE_MOTION;
>> +	event.motion.rel_x = rel_x;
>> +	event.motion.rel_y = rel_y;
>> +
>> +	return xenfb_kbd_event(xenfb, &event);
>> +}
>> +
>> +int xenfb_send_button(struct xenfb *xenfb_pub, bool down, int button)
>> +{
>> +	struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
>> +	union xenkbd_in_event event;
>> +
>> +	event.type = XENKBD_TYPE_BUTTON;
>> +	event.button.pressed = down ? 1 : 0;
>> +	event.button.button = button;
>> +
>> +	return xenfb_kbd_event(xenfb, &event);
>> +}
>> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tools/xenfb/xenfb.h	Sat Sep 02 15:19:25 2006 -0400
>> @@ -0,0 +1,33 @@
>> +#ifndef _XENFB_H_
>> +#define _XENFB_H_
>> +
>> +#include <stdbool.h>
>> +#include <stdint.h>
>> +
>> +struct xenfb
>> +{
>> +	uint8_t *pixels;
>> +
>> +	int row_stride;
>> +	int depth;
>> +	int width;
>> +	int height;
>> +
>> +	void *user_data;
>> +
>> +	void (*update)(struct xenfb *xenfb, int x, int y, int width, int
height);
>> +};
>> +
>> +struct xenfb *xenfb_new(void);
>> +void xenfb_delete(struct xenfb *xenfb);
>> +
>> +bool xenfb_attach_dom(struct xenfb *xenfb, int domid);
>> +
>> +int xenfb_get_fileno(struct xenfb *xenfb);
>> +int xenfb_on_incoming(struct xenfb *xenfb);
>> +
>> +int xenfb_send_key(struct xenfb *xenfb, bool down, int keycode);
>> +int xenfb_send_motion(struct xenfb *xenfb, int rel_x, int rel_y);
>> +int xenfb_send_button(struct xenfb *xenfb, bool down, int button);
>> +
>> +#endif
>
> Steven.
Okay, here''s the next patch.  It doesn''t yet take care of
shadow
translate mode, just because I want to get it out of the door.  Next
week, I hope...

Oh, and it also doesn''t include Daniel Berrange''s locking
fixes to
LibVNCServer, which you really, really need:
http://lists.xensource.com/archives/html/xen-devel/2006-09/msg00371.html

diff -r 9977b8785570 tools/Makefile
--- a/tools/Makefile	Fri Sep 29 19:12:15 2006 +0100
+++ b/tools/Makefile	Sat Sep 30 09:29:38 2006 +0200
@@ -18,6 +18,7 @@ SUBDIRS-y += xenstat
 SUBDIRS-y += xenstat
 SUBDIRS-y += libaio
 SUBDIRS-y += blktap
+SUBDIRS-y += xenfb
 
 # These don''t cross-compile
 ifeq ($(XEN_COMPILE_ARCH),$(XEN_TARGET_ARCH))
diff -r 9977b8785570 tools/xenfb/Makefile
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/xenfb/Makefile	Sat Sep 30 09:29:38 2006 +0200
@@ -0,0 +1,33 @@
+XEN_ROOT=../..
+include $(XEN_ROOT)/tools/Rules.mk
+
+CFLAGS += -I$(XEN_LIBXC) -I$(XEN_XENSTORE)
-I$(XEN_ROOT)/linux-2.6-xen-sparse/include
+LDFLAGS += -L$(XEN_LIBXC) -L$(XEN_XENSTORE)
+
+INSTALL         = install
+INSTALL_PROG    = $(INSTALL) -m0755
+INSTALL_DIR     = $(INSTALL) -d -m0755
+
+.PHONY: all
+all: build
+
+.PHONY: build
+build: mk-symlinks
+	$(MAKE) vncfb sdlfb
+
+install: all
+	$(INSTALL_DIR) -p $(DESTDIR)/usr/$(LIBDIR)/xen/bin
+	$(INSTALL_PROG) vncfb $(DESTDIR)/usr/$(LIBDIR)/xen/bin/xen-vncfb
+	$(INSTALL_PROG) sdlfb $(DESTDIR)/usr/$(LIBDIR)/xen/bin/xen-sdlfb
+
+sdlfb: sdlfb.o xenfb.o
+
+sdlfb.o: CFLAGS += $(shell sdl-config --cflags)
+sdlfb: LDLIBS += $(shell sdl-config --libs) -lxenctrl -lxenstore
+
+clean:
+	$(RM) *.o *~ vncfb sdlfb
+
+vncfb: vncfb.o xenfb.o
+vncfb.o: CFLAGS += $(shell libvncserver-config --cflags) $(shell pkg-config
--cflags gtk+-2.0)
+vncfb: LDLIBS += $(shell libvncserver-config --libs) -lxenctrl -lxenstore
diff -r 9977b8785570 tools/xenfb/sdlfb.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/xenfb/sdlfb.c	Sat Sep 30 09:29:38 2006 +0200
@@ -0,0 +1,216 @@
+#include <SDL.h>
+#include <errno.h>
+#include <sys/types.h>
+#include <sys/select.h>
+#include <stdlib.h>
+#include <linux/input.h>
+#include <getopt.h>
+#include <string.h>
+#include "xenfb.h"
+
+struct SDLFBData
+{
+	SDL_Surface *dst;
+	SDL_Surface *src;
+};
+
+void sdl_update(struct xenfb *xenfb, int x, int y, int width, int height)
+{
+	struct SDLFBData *data = xenfb->user_data;
+	SDL_Rect r = { x, y, width, height };
+	SDL_BlitSurface(data->src, &r, data->dst, &r);
+	SDL_UpdateRect(data->dst, x, y, width, height);
+}
+
+int sdl2linux[1024] = {
+	[SDLK_a] = KEY_A,
+	[SDLK_b] = KEY_B,
+	[SDLK_c] = KEY_C,
+	[SDLK_d] = KEY_D,
+	[SDLK_e] = KEY_E,
+	[SDLK_f] = KEY_F,
+	[SDLK_g] = KEY_G,
+	[SDLK_h] = KEY_H,
+	[SDLK_i] = KEY_I,
+	[SDLK_j] = KEY_J,
+	[SDLK_k] = KEY_K,
+	[SDLK_l] = KEY_L,
+	[SDLK_m] = KEY_M,
+	[SDLK_n] = KEY_N,
+	[SDLK_o] = KEY_O,
+	[SDLK_p] = KEY_P,
+	[SDLK_q] = KEY_Q,
+	[SDLK_r] = KEY_R,
+	[SDLK_s] = KEY_S,
+	[SDLK_t] = KEY_T,
+	[SDLK_u] = KEY_U,
+	[SDLK_v] = KEY_V,
+	[SDLK_w] = KEY_W,
+	[SDLK_x] = KEY_X,
+	[SDLK_y] = KEY_Y,
+	[SDLK_z] = KEY_Z,
+	[SDLK_0] = KEY_0,
+	[SDLK_1] = KEY_1,
+	[SDLK_2] = KEY_2,
+	[SDLK_3] = KEY_3,
+	[SDLK_4] = KEY_4,
+	[SDLK_5] = KEY_5,
+	[SDLK_6] = KEY_6,
+	[SDLK_7] = KEY_7,
+	[SDLK_8] = KEY_8,
+	[SDLK_9] = KEY_9,
+	[SDLK_SPACE] = KEY_SPACE,
+	[SDLK_RETURN] = KEY_ENTER,
+	[SDLK_PERIOD] = KEY_DOT,
+	[SDLK_SLASH] = KEY_SLASH,
+	[SDLK_BACKSPACE] = KEY_BACKSPACE,
+	[SDLK_TAB] = KEY_TAB,
+	[SDLK_LSHIFT] = KEY_LEFTSHIFT,
+	[SDLK_RSHIFT] = KEY_RIGHTSHIFT,
+	[SDLK_LALT] = KEY_LEFTALT,
+	[SDLK_RALT] = KEY_RIGHTALT,
+};
+
+static struct option options[] = {
+	{ "domid", 1, NULL, ''d'' },
+	{ "title", 1, NULL, ''t'' },
+};
+
+int main(int argc, char **argv)
+{
+	struct xenfb *xenfb;
+	int fd;
+	int domid = -1;
+        char * title = NULL;
+	fd_set readfds;
+	struct SDLFBData data;
+	SDL_Rect r;
+	struct timeval tv;
+	SDL_Event event;
+	int do_quit = 0;
+        int opt;
+        char *endp = NULL;
+
+	while ((opt = getopt_long(argc, argv, "d:t:", options,
+				  NULL)) != -1) {
+		switch (opt) {
+                case ''d'':
+			domid = strtol(optarg, &endp, 10);
+			if (endp == optarg || *endp || errno) {
+				fprintf(stderr, "Invalid domain id specified\n");
+				exit(1);
+			}
+			break;
+                case ''t'':
+			title = strdup(optarg);
+			break;
+                }
+        }
+        if (optind != argc) {
+		fprintf(stderr, "Invalid options!\n");
+		exit(1);
+        }
+        if (domid <= 0) {
+		fprintf(stderr, "Domain ID must be specified!\n");
+		exit(1);
+        }
+
+	xenfb = xenfb_new();
+	if (xenfb == NULL) {
+		fprintf(stderr, "Could not create framebuffer (%s)\n",
+			strerror(errno));
+		exit(1);
+        }
+
+	if (!xenfb_attach_dom(xenfb, domid)) {
+		fprintf(stderr, "Could not connect to domain (%s)\n",
+			strerror(errno));
+		exit(1);
+        }
+
+	SDL_Init(SDL_INIT_VIDEO);
+
+	fd = xenfb_get_fileno(xenfb);
+
+	data.dst = SDL_SetVideoMode(xenfb->width, xenfb->height,
xenfb->depth,
+				    SDL_SWSURFACE);
+
+	data.src = SDL_CreateRGBSurfaceFrom(xenfb->pixels,
+					    xenfb->width, xenfb->height,
+					    xenfb->depth, xenfb->row_stride,
+					    0xFF0000, 0xFF00, 0xFF, 0);
+
+        if (title == NULL)
+		title = strdup("xen-sdlfb");
+        SDL_WM_SetCaption(title, title);
+
+	r.x = r.y = 0;
+	r.w = xenfb->width;
+	r.h = xenfb->height;
+	SDL_BlitSurface(data.src, &r, data.dst, &r);
+	SDL_UpdateRect(data.dst, 0, 0, xenfb->width, xenfb->height);
+
+	xenfb->update = sdl_update;
+	xenfb->user_data = &data;
+
+	SDL_ShowCursor(0);
+
+	/*
+	 * We need to wait for fd becoming ready or SDL events to
+	 * arrive.  We time out the select after 10ms to poll for SDL
+	 * events.  Clunky, but works.  Could avoid the clunkiness
+	 * with a separate thread.
+	 */
+	for (;;) {
+		FD_ZERO(&readfds);
+		FD_SET(fd, &readfds);
+		tv = (struct timeval){0, 10000};
+
+		if (select(fd + 1, &readfds, NULL, NULL, &tv) < 0) {
+			if (errno == EINTR)
+				continue;
+			fprintf(stderr, "Connection to domain broke (%s)\n",
+				strerror(errno));
+			break;
+		}
+
+		while (SDL_PollEvent(&event)) {
+			switch (event.type) {
+			case SDL_KEYDOWN:
+			case SDL_KEYUP:
+				xenfb_send_key(xenfb,
+					event.type == SDL_KEYDOWN,
+					sdl2linux[event.key.keysym.sym]);
+				break;
+			case SDL_MOUSEMOTION: {
+				int x, y;
+				Uint8 button;
+
+				button = SDL_GetRelativeMouseState(&x, &y);
+				xenfb_send_motion(xenfb, x, y);
+			}	break;
+			case SDL_MOUSEBUTTONDOWN:
+			case SDL_MOUSEBUTTONUP:
+				xenfb_send_button(xenfb,
+					event.type == SDL_MOUSEBUTTONDOWN,
+					3 - event.button.button);
+				break;
+			case SDL_QUIT:
+				do_quit = 1;
+				break;
+			}
+		}
+
+                if (do_quit)
+			break;
+
+		if (FD_ISSET(fd, &readfds))
+			xenfb_on_incoming(xenfb);
+	}
+
+	xenfb_delete(xenfb);
+
+	SDL_Quit();
+
+	return 0;
+}
diff -r 9977b8785570 tools/xenfb/vncfb.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/xenfb/vncfb.c	Sat Sep 30 09:29:38 2006 +0200
@@ -0,0 +1,402 @@
+#define _GNU_SOURCE
+#include <errno.h>
+#include <getopt.h>
+#include <stdlib.h>
+#include <signal.h>
+#include <unistd.h>
+#include <malloc.h>
+#include <rfb/rfb.h>
+#include <gdk/gdkkeysyms.h>
+#include <linux/input.h>
+#include <xs.h>
+#include "xenfb.h"
+
+static uint32_t gdk_linux_mapping[0x10000] = {
+	[GDK_a] = KEY_A,
+	[GDK_b] = KEY_B,
+	[GDK_c] = KEY_C,
+	[GDK_d] = KEY_D,
+	[GDK_e] = KEY_E,
+	[GDK_f] = KEY_F,
+	[GDK_g] = KEY_G,
+	[GDK_h] = KEY_H,
+	[GDK_i] = KEY_I,
+	[GDK_j] = KEY_J,
+	[GDK_k] = KEY_K,
+	[GDK_l] = KEY_L,
+	[GDK_m] = KEY_M,
+	[GDK_n] = KEY_N,
+	[GDK_o] = KEY_O,
+	[GDK_p] = KEY_P,
+	[GDK_q] = KEY_Q,
+	[GDK_r] = KEY_R,
+	[GDK_s] = KEY_S,
+	[GDK_t] = KEY_T,
+	[GDK_u] = KEY_U,
+	[GDK_v] = KEY_V,
+	[GDK_w] = KEY_W,
+	[GDK_x] = KEY_X,
+	[GDK_y] = KEY_Y,
+	[GDK_z] = KEY_Z,
+	[GDK_A] = KEY_A,
+	[GDK_B] = KEY_B,
+	[GDK_C] = KEY_C,
+	[GDK_D] = KEY_D,
+	[GDK_E] = KEY_E,
+	[GDK_F] = KEY_F,
+	[GDK_G] = KEY_G,
+	[GDK_H] = KEY_H,
+	[GDK_I] = KEY_I,
+	[GDK_J] = KEY_J,
+	[GDK_K] = KEY_K,
+	[GDK_L] = KEY_L,
+	[GDK_M] = KEY_M,
+	[GDK_N] = KEY_N,
+	[GDK_O] = KEY_O,
+	[GDK_P] = KEY_P,
+	[GDK_Q] = KEY_Q,
+	[GDK_R] = KEY_R,
+	[GDK_S] = KEY_S,
+	[GDK_T] = KEY_T,
+	[GDK_U] = KEY_U,
+	[GDK_V] = KEY_V,
+	[GDK_W] = KEY_W,
+	[GDK_X] = KEY_X,
+	[GDK_Y] = KEY_Y,
+	[GDK_Z] = KEY_Z,
+	[GDK_0] = KEY_0,
+	[GDK_1] = KEY_1,
+	[GDK_2] = KEY_2,
+	[GDK_3] = KEY_3,
+	[GDK_4] = KEY_4,
+	[GDK_5] = KEY_5,
+	[GDK_6] = KEY_6,
+	[GDK_7] = KEY_7,
+	[GDK_8] = KEY_8,
+	[GDK_9] = KEY_9,
+	[GDK_Return] = KEY_ENTER,
+	[GDK_BackSpace] = KEY_BACKSPACE,
+	[GDK_Tab] = KEY_TAB,
+	[GDK_Pause] = KEY_PAUSE,
+	[GDK_Delete] = KEY_DELETE,
+	[GDK_slash] = KEY_SLASH,
+	[GDK_minus] = KEY_MINUS,
+	[GDK_equal] = KEY_EQUAL,
+	[GDK_Escape] = KEY_ESC,
+	[GDK_braceleft] = KEY_LEFTBRACE,
+	[GDK_braceright] = KEY_RIGHTBRACE,
+	[GDK_bracketleft] = KEY_LEFTMETA,
+	[GDK_bracketright] = KEY_RIGHTMETA,
+	[GDK_Control_L] = KEY_LEFTCTRL,
+	[GDK_Control_R] = KEY_RIGHTCTRL,
+	[GDK_Shift_L] = KEY_LEFTSHIFT,
+	[GDK_Shift_R] = KEY_RIGHTSHIFT,
+	[GDK_Alt_L] = KEY_LEFTALT,
+	[GDK_Alt_R] = KEY_RIGHTALT,
+	[GDK_semicolon] = KEY_SEMICOLON, 
+	[GDK_apostrophe] = KEY_APOSTROPHE,
+	[GDK_grave] = KEY_GRAVE,
+	[GDK_backslash] = KEY_BACKSLASH,
+	[GDK_comma] = KEY_COMMA,
+	[GDK_period] = KEY_DOT,
+	[GDK_space] = KEY_SPACE,
+	[GDK_Caps_Lock] = KEY_CAPSLOCK,
+	[GDK_Num_Lock] = KEY_NUMLOCK,
+	[GDK_Scroll_Lock] = KEY_SCROLLLOCK,
+	[GDK_Sys_Req] = KEY_SYSRQ,
+	[GDK_Linefeed] = KEY_LINEFEED,
+	[GDK_Home] = KEY_HOME,
+	[GDK_Pause] = KEY_PAUSE,
+	[GDK_F1] = KEY_F1,
+	[GDK_F2] = KEY_F2,
+	[GDK_F3] = KEY_F3,
+	[GDK_F4] = KEY_F4,
+	[GDK_F5] = KEY_F5,
+	[GDK_F6] = KEY_F6,
+	[GDK_F7] = KEY_F7,
+	[GDK_F8] = KEY_F8,
+	[GDK_F9] = KEY_F9,
+	[GDK_F10] = KEY_F10,
+	[GDK_F11] = KEY_F11,
+	[GDK_F12] = KEY_F12,
+	[GDK_Up] = KEY_UP,
+	[GDK_Page_Up] = KEY_PAGEUP,
+	[GDK_Left] = KEY_LEFT,
+	[GDK_Right] = KEY_RIGHT,
+	[GDK_End] = KEY_END,
+	[GDK_Down] = KEY_DOWN,
+	[GDK_Page_Down] = KEY_PAGEDOWN,
+	[GDK_Insert] = KEY_INSERT, 
+	[GDK_colon] = KEY_SEMICOLON,
+	[GDK_quotedbl] = KEY_APOSTROPHE,
+	[GDK_less] = KEY_COMMA,
+	[GDK_greater] = KEY_DOT,
+	[GDK_question] = KEY_SLASH,
+	[GDK_bar] = KEY_BACKSLASH,
+	[GDK_asciitilde] = KEY_GRAVE,
+	[GDK_exclam] = KEY_1,
+	[GDK_at] = KEY_2,
+	[GDK_numbersign] = KEY_3,
+	[GDK_dollar] = KEY_4,
+	[GDK_percent] = KEY_5,
+	[GDK_asciicircum] = KEY_6,
+	[GDK_ampersand] = KEY_7,
+	[GDK_asterisk] = KEY_8,
+	[GDK_parenleft] = KEY_9,
+	[GDK_parenright] = KEY_0,
+	[GDK_underscore] = KEY_MINUS,
+	[GDK_plus] = KEY_EQUAL,
+};
+
+static void on_kbd_event(rfbBool down, rfbKeySym keycode, rfbClientPtr cl)
+{
+	rfbScreenInfoPtr server = cl->screen;
+	struct xenfb *xenfb = server->screenData;
+	xenfb_send_key(xenfb, down, gdk_linux_mapping[keycode & 0xFFFF]);
+}
+
+static void on_ptr_event(int buttonMask, int x, int y, rfbClientPtr cl)
+{
+	static int last_x = -1, last_y = -1;
+	static int last_button = -1;
+	rfbScreenInfoPtr server = cl->screen;
+	struct xenfb *xenfb = server->screenData;
+
+	if (last_button != -1) {
+		int i;
+
+		for (i = 0; i < 8; i++) {
+			if ((last_button & (1 << i)) !+			    (buttonMask & (1
<< i))) {
+				xenfb_send_button(xenfb, buttonMask & (1 << i),
+						  2 - i);
+			}
+		}
+	}
+
+	if (last_x != -1)
+		xenfb_send_motion(xenfb, x - last_x, y - last_y);
+
+	last_button = buttonMask;
+
+	last_x = x;
+	last_y = y;
+}
+
+static void xenstore_write_vncport(int port, int domid)
+{
+	char *buf = NULL, *path;
+	char portstr[10];
+	struct xs_handle *xsh = NULL;
+
+	xsh = xs_daemon_open();
+	if (xsh == NULL)
+		return;
+
+	path = xs_get_domain_path(xsh, domid);
+	if (path == NULL) {
+		fprintf(stderr, "Can''t get domain path (%s)\n",
+			strerror(errno));
+		goto out;
+	}
+
+	if (asprintf(&buf, "%s/console/vnc-port", path) == -1) {
+		fprintf(stderr, "Can''t make vncport path\n");
+		goto out;
+	}
+
+	if (snprintf(portstr, sizeof(portstr), "%d", port) == -1) {
+		fprintf(stderr, "Can''t make vncport value\n");
+		goto out;
+	}
+
+	if (!xs_write(xsh, XBT_NULL, buf, portstr, strlen(portstr)))
+		fprintf(stderr, "Can''t set vncport (%s)\n",
+			strerror(errno));
+
+out:
+	free(buf);
+}
+
+
+static void vnc_update(struct xenfb *xenfb, int x, int y, int w, int h)
+{
+	rfbScreenInfoPtr server = xenfb->user_data;
+	rfbMarkRectAsModified(server, x, y, x + w, y + h);
+}
+
+static int vnc_start_viewer(int port) 
+{
+	int pid;
+	char s[16];
+
+	snprintf(s, sizeof(s), ":%d", port);
+	switch (pid = fork()) {
+	case -1:
+		fprintf(stderr, "vncviewer failed fork\n");
+		exit(1);
+
+	case 0:	/* child */
+		execlp("vncviewer", "vncviewer", s, NULL);
+		fprintf(stderr, "vncviewer execlp failed\n");
+		exit(1);
+
+	default:
+		return pid;
+	}
+}
+
+static struct option options[] = {
+	{ "domid", 1, NULL, ''d'' },
+	{ "vncport", 1, NULL, ''p'' },
+	{ "title", 1, NULL, ''t'' },
+	{ "unused", 0, NULL, ''u'' },
+	{ "listen", 1, NULL, ''l'' },
+	{ "vncviewer", 0, NULL, ''v'' },
+};
+
+int main(int argc, char **argv)
+{
+	rfbScreenInfoPtr server;
+	char *fake_argv[7] = { "vncfb", "-rfbport",
"5901",
+                               "-desktop", "xen-vncfb", 
+                               "-listen", "0.0.0.0" };
+	int fake_argc = sizeof(fake_argv) / sizeof(fake_argv[0]);
+	int domid = -1, port = -1;
+        char *title = NULL;
+        char *listen = NULL;
+	struct xenfb *xenfb;
+	fd_set readfds;
+	int fd;
+	char portstr[10];
+        int opt;
+        bool unused = false;
+        bool viewer = false;
+        char *endp = NULL;
+
+	while ((opt = getopt_long(argc, argv, "d:p:t:u", options,
+				  NULL)) != -1) {
+		switch (opt) {
+                case ''d'':
+			errno = 0;
+			domid = strtol(optarg, &endp, 10);
+			if (endp == optarg || *endp || errno) {
+				fprintf(stderr, "Invalid domain id specified\n");
+				exit(1);
+			}
+			break;
+                case ''p'':
+			errno = 0;
+			port = strtol(optarg, &endp, 10);
+			if (endp == optarg || *endp || errno) {
+				fprintf(stderr, "Invalid port specified\n");
+				exit(1);
+			}
+			break;
+                case ''t'':
+			title = strdup(optarg);
+			break;
+                case ''u'':
+			unused = true;
+			break;
+                case ''l'':
+			listen = strdup(optarg);
+			break;
+                case ''v'':
+			viewer = true;
+			break;
+                }
+        }
+        if (optind != argc) {
+		fprintf(stderr, "Invalid options!\n");
+		exit(1);
+        }
+        if (domid <= 0) {
+		fprintf(stderr, "Domain ID must be specified!\n");
+		exit(1);
+        }
+            
+        if (port <= 0)
+		port = 5900 + domid;
+	if (snprintf(portstr, sizeof(portstr), "%d", port) == -1) {
+		fprintf(stderr, "Invalid port specified\n");
+		exit(1);
+        }
+            
+	fake_argv[2] = portstr;
+
+        if (title != NULL)
+		fake_argv[4] = title;
+
+        if (listen != NULL)
+		fake_argv[6] = listen;
+
+	signal(SIGPIPE, SIG_IGN);
+
+	xenfb = xenfb_new();
+	if (xenfb == NULL) {
+		fprintf(stderr, "Could not create framebuffer (%s)\n",
+			strerror(errno));
+		exit(1);
+	}
+
+	if (!xenfb_attach_dom(xenfb, domid)) {
+		fprintf(stderr, "Could not connect to domain (%s)\n",
+			strerror(errno));
+		exit(1);
+	}
+
+	server = rfbGetScreen(&fake_argc, fake_argv, 
+			      xenfb->width, xenfb->height,
+			      8, 3, xenfb->depth / 8);
+	if (server == NULL) {
+		fprintf(stderr, "Could not create VNC server\n");
+		exit(1);
+	}
+
+	xenfb->user_data = server;
+	xenfb->update = vnc_update;
+
+        if (unused)
+		server->autoPort = true;
+
+	server->serverFormat.redShift = 16;
+	server->serverFormat.greenShift = 8;
+	server->serverFormat.blueShift = 0;
+	server->kbdAddEvent = on_kbd_event;
+	server->ptrAddEvent = on_ptr_event;
+	server->frameBuffer = (char *)xenfb->pixels;
+	server->screenData = xenfb;
+	server->cursor = NULL;
+	rfbInitServer(server);
+
+	rfbRunEventLoop(server, -1, true);
+
+	fd = xenfb_get_fileno(xenfb);
+
+        xenstore_write_vncport(server->port, domid);
+
+        if (viewer)
+		vnc_start_viewer(server->port);
+
+	for (;;) {
+		FD_ZERO(&readfds);
+		FD_SET(fd, &readfds);
+
+		if (select(fd + 1, &readfds, NULL, NULL, NULL) < 0) {
+			if (errno == EINTR)
+				continue;
+			fprintf(stderr, "Connection to domain broke (%s)\n",
+				strerror(errno));
+			break;
+		}
+
+		if (FD_ISSET(fd, &readfds))
+			xenfb_on_incoming(xenfb);
+	}
+
+	rfbScreenCleanup(server);
+	xenfb_delete(xenfb);
+
+	return 0;
+}
diff -r 9977b8785570 tools/xenfb/xenfb.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/xenfb/xenfb.c	Sat Sep 30 09:29:38 2006 +0200
@@ -0,0 +1,404 @@
+#include <malloc.h>
+#include <stdlib.h>
+#include <sys/types.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <xenctrl.h>
+#include <linux/xenfb.h>
+#include <linux/xenkbd.h>
+#include <sys/select.h>
+#include <stdbool.h>
+#include <xen/linux/evtchn.h>
+#include <xen/event_channel.h>
+#include <sys/mman.h>
+#include <errno.h>
+#include <stdio.h>
+#include <string.h>
+#include <time.h>
+#include <xs.h>
+
+#include "xenfb.h"
+
+// FIXME defend against malicous frontend?
+
+struct xenfb_private
+{
+	struct xenfb pub;
+	int domid;
+	evtchn_port_t fbdev_port, kbd_port;
+	int evt_xch;
+	int xc;
+	unsigned char *fb;
+	struct xenfb_page *fb_info;
+	struct xenkbd_info *kbd_info;
+};
+
+struct xenfb *xenfb_new(void)
+{
+	struct xenfb_private *xenfb = malloc(sizeof(*xenfb));
+
+	if (xenfb == NULL)
+		return NULL;
+
+	memset(xenfb, 0, sizeof(*xenfb));
+
+	xenfb->domid = -1;
+
+	xenfb->evt_xch = xc_evtchn_open();
+	if (xenfb->evt_xch == -1) {
+		int serrno = errno;
+		free(xenfb);
+		errno = serrno;
+		return NULL;
+	}
+
+	xenfb->xc = xc_interface_open();
+	if (xenfb->xc == -1) {
+		int serrno = errno;
+		xc_evtchn_close(xenfb->evt_xch);
+		free(xenfb);
+		errno = serrno;
+		return NULL;
+	}
+
+	return &xenfb->pub;
+}
+
+int xenfb_get_fileno(struct xenfb *xenfb_pub)
+{
+	struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
+
+	return xc_evtchn_fd(xenfb->evt_xch);
+}
+
+static void xenfb_detach_dom(struct xenfb_private *xenfb)
+{
+	xenfb->domid = -1;
+	munmap(xenfb->fb, xenfb->fb_info->mem_length);
+	munmap(xenfb->fb_info, XC_PAGE_SIZE);
+	munmap(xenfb->kbd_info, XC_PAGE_SIZE);
+	xc_evtchn_unbind(xenfb->evt_xch, xenfb->fbdev_port);
+	xc_evtchn_unbind(xenfb->evt_xch, xenfb->kbd_port);
+}
+
+void xenfb_delete(struct xenfb *xenfb_pub)
+{
+	struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
+	if (xenfb->domid != -1)
+		xenfb_detach_dom(xenfb);
+	free(xenfb);
+}
+
+static int xenfb_fb_event(struct xenfb_private *xenfb, union xenfb_in_event
*event)
+{
+	uint32_t prod;
+	struct xenfb_page *info = xenfb->fb_info;
+
+	prod = info->in_prod;
+	if (prod - info->in_cons == XENFB_IN_RING_LEN) {
+	    errno = EAGAIN;
+	    return -1;
+	}
+
+	mb();			/* ensure ring space available */
+	XENFB_IN_RING_REF(info, prod) = *event;
+	wmb();			/* ensure ring contents visible */
+	info->in_prod = prod + 1;
+	return xc_evtchn_notify(xenfb->evt_xch, xenfb->fbdev_port);
+}
+
+static int xenfb_kbd_event(struct xenfb_private *xenfb, union xenkbd_in_event
*event)
+{
+	uint32_t prod;
+	struct xenkbd_info *info = xenfb->kbd_info;
+
+	prod = info->in_prod;
+	if (prod - info->in_cons == XENKBD_IN_RING_LEN) {
+		errno = EAGAIN;
+		return -1;
+	}
+
+	mb();			/* ensure ring space available */
+	XENKBD_IN_RING_REF(info, prod) = *event;
+	wmb();			/* ensure ring contents visible */
+	info->in_prod = prod + 1;
+	return xc_evtchn_notify(xenfb->evt_xch, xenfb->kbd_port);
+}
+
+static char *xenfb_path_in_dom(struct xs_handle *h,
+			 unsigned domid, const char *path,
+			 char *buffer, size_t size)
+{
+	char *domp = xs_get_domain_path(h, domid);
+	int n;
+
+        if (domp == NULL)
+		return NULL;
+        n = snprintf(buffer, size, "%s/%s", domp, path);
+	free(domp);
+	if (n >= size)
+		return NULL;
+	return buffer;
+}
+
+static int xenfb_xs_scanf1(struct xs_handle *xsh, unsigned domid,
+			   const char *path, const char *fmt,
+			   void *dest)
+{
+	char buffer[1024];
+	char *p;
+	int ret;
+
+	p = xenfb_path_in_dom(xsh, domid, path, buffer, sizeof(buffer));
+        if (!p) {
+		errno = -ENOENT;
+		return -1;
+        }
+	p = xs_read(xsh, XBT_NULL, p, NULL);
+	if (!p) {
+		errno = -ENOENT;
+		return -1;
+        }
+	ret = sscanf(p, fmt, dest);
+	free(p);
+	if (ret != 1) {
+		errno = -EDOM;
+		return -1;
+        }
+	return 0;
+}
+
+bool xenfb_attach_dom(struct xenfb *xenfb_pub, int domid)
+{
+	struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
+	char buffer[1024];
+	struct xs_handle *xsh;
+	unsigned dummy;
+	int ret;
+	char *p, **vec;
+	union xenfb_in_event event;
+	unsigned long *fbmfns = NULL;
+	int n_fbmfns, n_fbdirs;
+        int serrno;
+        int fbdev_evtchn, kbd_evtchn;
+	unsigned long fbdev_mfn, kbd_mfn;
+
+	if (xenfb->domid != -1) {
+		xenfb_detach_dom(xenfb);
+		if (domid == -1)
+			return true;
+	}
+
+	xsh = xs_daemon_open_readonly();
+	if (!xsh)
+		goto error;
+
+	p = xenfb_path_in_dom(xsh, domid, "vfb", buffer, sizeof(buffer));
+	if (!p)
+		goto error;
+	if (!xs_watch(xsh, p, ""))
+		goto error;
+	p = xenfb_path_in_dom(xsh, domid, "vkbd", buffer, sizeof(buffer));
+	if (!p)
+		goto error;
+	if (!xs_watch(xsh, p, ""))
+		goto error;
+
+	for (;;) {
+		ret = xenfb_xs_scanf1(xsh, domid, "vfb/0/page-ref",
"%lu",
+				      &fbdev_mfn);
+		if (ret == -1 && errno == -ENOENT)
+			goto wait;
+		if (ret < 0)
+			goto error;
+		ret = xenfb_xs_scanf1(xsh, domid, "vfb/0/event-channel",
"%u",
+				      &fbdev_evtchn);
+		if (ret == -1 && errno == -ENOENT)
+			goto wait;
+		if (ret < 0)
+			goto error;
+		ret = xenfb_xs_scanf1(xsh, domid, "vkbd/0/page-ref",
"%lu",
+				      &kbd_mfn);
+		if (ret == -1 && errno == -ENOENT)
+			goto wait;
+		if (ret < 0)
+			goto error;
+		ret = xenfb_xs_scanf1(xsh, domid, "vkbd/0/event-channel",
"%u",
+				      &kbd_evtchn);
+		if (ret == -1 && errno == -ENOENT)
+			goto wait;
+		if (ret < 0)
+			goto error;
+		break;
+
+	wait:
+		printf("Waiting for guest framebuffer to connect...\n");
+		vec = xs_read_watch(xsh, &dummy);
+		if (!vec)
+			goto error;
+		free(vec);
+	}
+	xs_daemon_close(xsh);
+	xsh = NULL;
+
+	xenfb->fbdev_port = xc_evtchn_bind_interdomain(xenfb->evt_xch, domid,
+						       fbdev_evtchn);
+	if (xenfb->fbdev_port == -1)
+		goto error;
+
+	xenfb->kbd_port = xc_evtchn_bind_interdomain(xenfb->evt_xch, domid,
+						     kbd_evtchn);
+	if (xenfb->kbd_port == -1) 
+		goto error;
+
+	xenfb->fb_info = xc_map_foreign_range(xenfb->xc, domid, XC_PAGE_SIZE,
+					      PROT_READ | PROT_WRITE,
+					      fbdev_mfn);
+	if (xenfb->fb_info == NULL)
+		goto error;
+
+	xenfb->kbd_info = xc_map_foreign_range(xenfb->xc, domid, XC_PAGE_SIZE,
+					       PROT_READ | PROT_WRITE,
+					       kbd_mfn);
+	if (xenfb->kbd_info == NULL)
+		goto error;
+
+	n_fbmfns = (xenfb->fb_info->mem_length + (XC_PAGE_SIZE - 1)) /
XC_PAGE_SIZE;
+	n_fbdirs = n_fbmfns * sizeof(unsigned long);
+	n_fbdirs = (n_fbdirs + (XC_PAGE_SIZE - 1)) / XC_PAGE_SIZE;
+
+	fbmfns = xc_map_foreign_batch(xenfb->xc, domid, PROT_READ,
xenfb->fb_info->pd, n_fbdirs);
+	if (fbmfns == NULL)
+		goto error;
+
+	xenfb->fb = xc_map_foreign_batch(xenfb->xc, domid, PROT_READ |
PROT_WRITE, fbmfns, n_fbmfns);
+	if (xenfb->fb == NULL)
+		goto error;
+
+	event.type = XENFB_TYPE_SET_EVENTS;
+	event.set_events.flags = XENFB_FLAG_UPDATE;
+	if (xenfb_fb_event(xenfb, &event))
+		goto error;
+
+	munmap(fbmfns, n_fbdirs * XC_PAGE_SIZE);
+
+	xenfb->domid = domid;
+
+	xenfb->pub.pixels = xenfb->fb;
+
+	xenfb->pub.row_stride = xenfb->fb_info->line_length;
+	xenfb->pub.depth = xenfb->fb_info->depth;
+	xenfb->pub.width = xenfb->fb_info->width;
+	xenfb->pub.height = xenfb->fb_info->height;
+
+	return true;
+
+error:        
+	serrno = errno;
+ 	if (xenfb->fb)
+ 		munmap(xenfb->fb, xenfb->fb_info->mem_length);
+ 	if (fbmfns)
+ 		munmap(fbmfns, xenfb->fb_info->mem_length);
+        if (xenfb->kbd_info)
+		munmap(xenfb->kbd_info, XC_PAGE_SIZE);
+        if (xenfb->fb_info)
+		munmap(xenfb->fb_info, XC_PAGE_SIZE);
+        if (xenfb->kbd_port);
+	xc_evtchn_unbind(xenfb->evt_xch, xenfb->kbd_port);
+        if (xenfb->fbdev_port)
+		xc_evtchn_unbind(xenfb->evt_xch, xenfb->fbdev_port);
+        if (xsh) 
+		xs_daemon_close(xsh);
+        errno = serrno;
+        return false;
+}
+
+static void xenfb_on_fb_event(struct xenfb_private *xenfb)
+{
+	uint32_t prod, cons;
+	struct xenfb_page *info = xenfb->fb_info;
+
+	prod = info->out_prod;
+	rmb();			/* ensure we see ring contents up to prod */
+	for (cons = info->out_cons; cons != prod; cons++) {
+		union xenfb_out_event *event = &XENFB_OUT_RING_REF(info, cons);
+
+		switch (event->type) {
+		case XENFB_TYPE_UPDATE:
+                    if (xenfb->pub.update)
+			xenfb->pub.update(&xenfb->pub,
+					  event->update.x, event->update.y,
+					  event->update.width, event->update.height);
+                    break;
+		}
+	}
+	mb();			/* ensure we''re done with ring contents */
+	info->out_cons = cons;
+	xc_evtchn_notify(xenfb->evt_xch, xenfb->fbdev_port);
+}
+
+static void xenfb_on_kbd_event(struct xenfb_private *xenfb)
+{
+	struct xenkbd_info *info = xenfb->kbd_info;
+
+	/* We don''t understand any keyboard events, so just ignore them. */
+	info->out_cons = info->out_prod;
+	xc_evtchn_notify(xenfb->evt_xch, xenfb->kbd_port);
+}
+
+int xenfb_on_incoming(struct xenfb *xenfb_pub)
+{
+	struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
+	evtchn_port_t port;
+
+	port = xc_evtchn_pending(xenfb->evt_xch);
+	if (port == -1)
+		return -1;
+
+	if (port == xenfb->fbdev_port) {
+		xenfb_on_fb_event(xenfb);
+	} else if (port == xenfb->kbd_port) {
+		xenfb_on_kbd_event(xenfb);
+	}
+
+	if (xc_evtchn_unmask(xenfb->evt_xch, port) == -1)
+		return -1;
+
+	return 0;
+}
+
+int xenfb_send_key(struct xenfb *xenfb_pub, bool down, int keycode)
+{
+	struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
+	union xenkbd_in_event event;
+
+	event.type = XENKBD_TYPE_KEY;
+	event.key.pressed = down ? 1 : 0;
+	event.key.keycode = keycode;
+
+	return xenfb_kbd_event(xenfb, &event);
+}
+
+int xenfb_send_motion(struct xenfb *xenfb_pub, int rel_x, int rel_y)
+{
+	struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
+	union xenkbd_in_event event;
+
+	event.type = XENKBD_TYPE_MOTION;
+	event.motion.rel_x = rel_x;
+	event.motion.rel_y = rel_y;
+
+	return xenfb_kbd_event(xenfb, &event);
+}
+
+int xenfb_send_button(struct xenfb *xenfb_pub, bool down, int button)
+{
+	struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
+	union xenkbd_in_event event;
+
+	event.type = XENKBD_TYPE_BUTTON;
+	event.button.pressed = down ? 1 : 0;
+	event.button.button = button;
+
+	return xenfb_kbd_event(xenfb, &event);
+}
diff -r 9977b8785570 tools/xenfb/xenfb.h
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/xenfb/xenfb.h	Sat Sep 30 09:29:38 2006 +0200
@@ -0,0 +1,33 @@
+#ifndef _XENFB_H_
+#define _XENFB_H_
+
+#include <stdbool.h>
+#include <stdint.h>
+
+struct xenfb
+{
+	uint8_t *pixels;
+
+	int row_stride;
+	int depth;
+	int width;
+	int height;
+
+	void *user_data;
+
+	void (*update)(struct xenfb *xenfb, int x, int y, int width, int height);
+};
+
+struct xenfb *xenfb_new(void);
+void xenfb_delete(struct xenfb *xenfb);
+
+bool xenfb_attach_dom(struct xenfb *xenfb, int domid);
+
+int xenfb_get_fileno(struct xenfb *xenfb);
+int xenfb_on_incoming(struct xenfb *xenfb);
+
+int xenfb_send_key(struct xenfb *xenfb, bool down, int keycode);
+int xenfb_send_motion(struct xenfb *xenfb, int rel_x, int rel_y);
+int xenfb_send_button(struct xenfb *xenfb, bool down, int button);
+
+#endif

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> >> +#include <linux/input.h>
> >> +
> >> +uint32_t gdk_linux_mapping[0x10000] = {
> >> +	[GDK_a] = KEY_A,
> > This is kind of ugly.  Is there any chance it could be autogenerated?
> > Also, where did 0x10000 come from?
> >
> > Also, depending on GTK just for the keymap table is a real pain.  Or
> > is it already required for libvncserver?
> Jeremy answered this one.
> 
> There was quite a bit of discussion on how to best encode keys (thanks
> to Laurent for patiently explaining the issue).  I''m *not*
ignoring
> that problem.  However, the patch is large enough as it is, so
let''s
> get the things it addresses nailed down before we address the key code
> problem.
Fair enough.
> >> +	xenfb = xenfb_new();
> >> +	if (xenfb == NULL)
> >> +		return 1;
> > Why have you used exit(1) in some places and return 1 in others?
> It''s all exit(1) now.
Thanks.
> >> +		if (FD_ISSET(fd, &readfds))
> >> +			xenfb_on_incoming(xenfb);
> >> +
> >> +		FD_ZERO(&readfds);
> >> +		FD_SET(fd, &readfds);
> >> +
> >> +		tv = (struct timeval){0, 500};
> > I think 500us is a little short here.  About ten milliseconds sounds
> > more plausible.  This is a bit of a bikeshed.
> Changed to 10ms.
Thanks.
> > It''s a pity SDL doesn''t allow you to wait for either
an SDL event or
> > an fd to become readable.  Could you do something like spawn a thread
> > which does the selects in a loop, and then SDL_PushEvent()s a user
> > event when the fd becomes readable?
> >
> > Admittedly, SDL_WaitEvent also contains this kind of loop-with-sleep,
> > but it''d reduce the number of magic tunables a bit.
> Added a comment explaining the clunkiness and sketching the solution
> you proposed.
Thanks.
> >> +static void on_kbd_event(rfbBool down, rfbKeySym keycode,
rfbClientPtr cl)
> >> +{
> >> +	extern uint32_t gdk_linux_mapping[0x10000];
> > Is there any chance of moving this into a header file somewhere?
> I moved the table into this file instead.
Thanks.
> >> +
> >> +	if (xenfb == NULL)
> >> +		return NULL;
> >> +
> >> +	memset(xenfb, 0, sizeof(*xenfb));
> > Use calloc instead of malloc, perhaps?
> Hate it.
Umm... well, okay, then, leave it as malloc.
> >> +		errno = serrno;
> >> +		return NULL;
> > It''s a pity we don''t have a macro which hides this
ugliness.  Perhaps
> >
> > #define PRESERVING_ERRNO(x) do {
> > 	int tmp = errno;
> > 	x;
> > 	errno = tmp;
> > } while (0)
> >
...
> > Not sure whether that''s more or less ugly, to be honest.
> I think I prefer my poison straight here, without macro wrapping :)
Fair enough.
> >> +{
> >> +	char buffer[1024];
> >> +	char *p;
> >> +	int ret;
> >> +
> >> +	p = xenfb_path_in_dom(xsh, domid, path, buffer, sizeof(buffer));
> > What happens if this fails?
> Catched.
Thanks.
> >> +
> >> +	munmap(xenfb->fbmfns, xenfb->n_fbdirs * XC_PAGE_SIZE);
> > Please make fbmfns a local rather than putting it in the info
> > structure.
> Done.
Thanks.
> >> + error:
> >> +	printf("%d\n", __LINE__);
> >> +	if (xsh) {
> >> +		int serrno = errno;
> >> +		xs_daemon_close(xsh);
> > I think you may end up closing the connection to the daemon twice
here.
> Don''t think so, because xsh is cleared on the other close.
Yeah, sorry, I''d forgotten that xs_daemon_close(NULL) is a safe no-op.
> >> +static void xenfb_update(struct xenfb_private *xenfb, int x, int
y, int width, int height)
> >> +{
> >> +	if (xenfb->pub.update)
> >> +		xenfb->pub.update(&xenfb->pub, x, y, width, height);
> >> +}
> > I''m not convinced this wrapper is actually needed, given that
it''s
> > utterly trivial and only called from one place.
> It''s gone.
Thanks.
> >> +}
> >> +
> >> +int xenfb_send_motion(struct xenfb *xenfb_pub, int rel_x, int
rel_y)
> > Is this sending XENKBD_TYPE_MOTION or XENFB_TYPE_MOTION?
> XENFB_TYPE_MOTION doesn''t exist anymore.
Excellent.
> Okay, here''s the next patch.  It doesn''t yet take care of
shadow
> translate mode, just because I want to get it out of the door.  Next
> week, I hope...
Okay.
> Oh, and it also doesn''t include Daniel Berrange''s locking
fixes to
> LibVNCServer, which you really, really need:
> http://lists.xensource.com/archives/html/xen-devel/2006-09/msg00371.html
Did you look at how much work it would be to use the qemu VNC server
instead of libVNCServer?  I suspect quite a lot, but it might be
necessary if libVNCServer really is as bad as it appears.


This new patch is really pretty good.  Here''s the summary of the bits
which might actually matter:

-- The backend still isn''t proof against a malicious frontend.  This
   (might) mean that root in an unprivileged domain can get root in
   dom0, which is a fairly major problem.  Fixing this should be
   fairly easy.
-- The setup protocol doesn''t look much like the normal xenbus state
   machine.  There may be a good reason for this, but I haven''t heard
   one yet.  I know the standard way is badly documented and
   non-trivial to understand from the existing implementations; sorry
   about that.
-- You don''t seem to support mice with more than three buttons, or
   scroll wheels.  It''d be perfectly acceptable to just push this
   to a later version, though.
-- As you say, the keymaping and shadow problems are still there.

Other than that, there''s nothing here which would take more than
an hour or so to fix.

> +}
> +
> +int sdl2linux[1024] = {
SDLK_LAST, maybe?
> +	[SDLK_a] = KEY_A,
> +	[SDLK_b] = KEY_B,
...
> +int main(int argc, char **argv)
> +{
...
> +	while ((opt = getopt_long(argc, argv, "d:t:", options,
> +				  NULL)) != -1) {
> +		switch (opt) {
> +                case ''d'':
> +			domid = strtol(optarg, &endp, 10);
> +			if (endp == optarg || *endp || errno) {
Is testing errno really a good idea here?  strtol doesn''t zero it on
success, so you''re relying on it already being zero, which happens to
be true this time, as far as I can see, but is a somewhat dubious
assumption.  The other two tests should be enough, unless I''m
confused.

...
> +	if (!xenfb_attach_dom(xenfb, domid)) {
> +		fprintf(stderr, "Could not connect to domain (%s)\n",
> +			strerror(errno));
> +		exit(1);
> +        }
> +
> +	SDL_Init(SDL_INIT_VIDEO);
This can fail.

I''m aware that a lot of code currently in the tree is less than
perfect about checking return values and so forth, especially the
userspace tools, and that it doesn''t actually make that much
difference in practice.  It''d still be good not to introduce any more
of this kind of bug without good reason.

(And, yeah, I know this one''s been there for ages and I''ve
missed it
twice.  Sorry about that. )
> +
> +	fd = xenfb_get_fileno(xenfb);
> +
> +	data.dst = SDL_SetVideoMode(xenfb->width, xenfb->height,
xenfb->depth,
> +				    SDL_SWSURFACE);
This can fail.
> +
> +	data.src = SDL_CreateRGBSurfaceFrom(xenfb->pixels,
> +					    xenfb->width, xenfb->height,
> +					    xenfb->depth, xenfb->row_stride,
> +					    0xFF0000, 0xFF00, 0xFF, 0);
This can fail.
> +
> +        if (title == NULL)
> +		title = strdup("xen-sdlfb");
> +        SDL_WM_SetCaption(title, title);
> +
> +	r.x = r.y = 0;
> +	r.w = xenfb->width;
> +	r.h = xenfb->height;
> +	SDL_BlitSurface(data.src, &r, data.dst, &r);
> +	SDL_UpdateRect(data.dst, 0, 0, xenfb->width, xenfb->height);
> +
> +	xenfb->update = sdl_update;
> +	xenfb->user_data = &data;
> +
> +	SDL_ShowCursor(0);
> +
> +	/*
> +	 * We need to wait for fd becoming ready or SDL events to
> +	 * arrive.  We time out the select after 10ms to poll for SDL
> +	 * events.  Clunky, but works.  Could avoid the clunkiness
> +	 * with a separate thread.
> +	 */
Thanks for this.
> +	for (;;) {
> +		FD_ZERO(&readfds);
> +		FD_SET(fd, &readfds);
> +		tv = (struct timeval){0, 10000};
> +
> +		if (select(fd + 1, &readfds, NULL, NULL, &tv) < 0) {
> +			if (errno == EINTR)
> +				continue;
> +			fprintf(stderr, "Connection to domain broke (%s)\n",
> +				strerror(errno));
It''s not really the connection to the domain that''s broken as
the
event channel device.  Not terribly important.

...
> +		while (SDL_PollEvent(&event)) {
...
> +			case SDL_MOUSEBUTTONDOWN:
> +			case SDL_MOUSEBUTTONUP:
> +				xenfb_send_button(xenfb,
> +					event.type == SDL_MOUSEBUTTONDOWN,
> +					3 - event.button.button);
Why 3 - button?  What happens if someone has a four, five, six button
mouse?
> +				break;
...
> diff -r 9977b8785570 tools/xenfb/vncfb.c
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/tools/xenfb/vncfb.c	Sat Sep 30 09:29:38 2006 +0200
...
> +static void on_kbd_event(rfbBool down, rfbKeySym keycode, rfbClientPtr cl)
> +{
> +	rfbScreenInfoPtr server = cl->screen;
> +	struct xenfb *xenfb = server->screenData;
> +	xenfb_send_key(xenfb, down, gdk_linux_mapping[keycode & 0xFFFF]);
I probably just don''t understand the rfbKeySym format, but why is it
safe to ignore the high bits of the keycode?
> +}
> +
> +static void on_ptr_event(int buttonMask, int x, int y, rfbClientPtr cl)
> +{
> +	static int last_x = -1, last_y = -1;
> +	static int last_button = -1;
> +	rfbScreenInfoPtr server = cl->screen;
> +	struct xenfb *xenfb = server->screenData;
> +
> +	if (last_button != -1) {
> +		int i;
> +
> +		for (i = 0; i < 8; i++) {
> +			if ((last_button & (1 << i)) !> +			    (buttonMask &
(1 << i))) {
> +				xenfb_send_button(xenfb, buttonMask & (1 << i),
> +						  2 - i);
Why 2 - i?  What if button three gets pressed?  We send button 0xff to
the frontend and it does ...?

And if VNC can''t generate button 3, why do we scan for eight buttons?
> +			}
> +		}
> +	}
> +
> +	if (last_x != -1)
> +		xenfb_send_motion(xenfb, x - last_x, y - last_y);
> +
> +	last_button = buttonMask;
> +
> +	last_x = x;
> +	last_y = y;
> +}
> +
...
> +static int vnc_start_viewer(int port) 
> +{
> +	int pid;
> +	char s[16];
> +
> +	snprintf(s, sizeof(s), ":%d", port);
> +	switch (pid = fork()) {
> +	case -1:
> +		fprintf(stderr, "vncviewer failed fork\n");
> +		exit(1);
> +
> +	case 0:	/* child */
> +		execlp("vncviewer", "vncviewer", s, NULL);
> +		fprintf(stderr, "vncviewer execlp failed\n");
Getting strerror(errno) out here would be good.
> +		exit(1);
> +
> +	default:
> +		return pid;
> +	}
> +}
> +
...
> +int main(int argc, char **argv)
> +{
> +	rfbScreenInfoPtr server;
> +	char *fake_argv[7] = { "vncfb", "-rfbport",
"5901",
> +                               "-desktop",
"xen-vncfb",
> +                               "-listen", "0.0.0.0" };
> +	int fake_argc = sizeof(fake_argv) / sizeof(fake_argv[0]);
The initialisation for fake_argv is spread out over quite a lot of
code.  It''d be a bit easier to follow if it was all in one place.  Not
a big deal, though.

...
> diff -r 9977b8785570 tools/xenfb/xenfb.c
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/tools/xenfb/xenfb.c	Sat Sep 30 09:29:38 2006 +0200
...
> +#include <xs.h>
> +
> +#include "xenfb.h"
> +
> +// FIXME defend against malicous frontend?
Actually, you''re mostly there already.  I''ve pointed out a few
things
in line, but it seems pretty good for the most part.

...
> +static void xenfb_detach_dom(struct xenfb_private *xenfb)
> +{
> +	xenfb->domid = -1;
> +	munmap(xenfb->fb, xenfb->fb_info->mem_length);
You can''t trust mem_length to be the same as it was when you mapped
the page, since it''s in the shared area.
> +	munmap(xenfb->fb_info, XC_PAGE_SIZE);
> +	munmap(xenfb->kbd_info, XC_PAGE_SIZE);
> +	xc_evtchn_unbind(xenfb->evt_xch, xenfb->fbdev_port);
> +	xc_evtchn_unbind(xenfb->evt_xch, xenfb->kbd_port);
> +}
...
> +bool xenfb_attach_dom(struct xenfb *xenfb_pub, int domid)
> +{
...
> +	if (xenfb->kbd_info == NULL)
> +		goto error;
> +
> +	n_fbmfns = (xenfb->fb_info->mem_length + (XC_PAGE_SIZE - 1)) /
XC_PAGE_SIZE;
If mem_length is near UINT_MAX, this could overflow and lead to you
not actually mapping any memory, and then crashing later.  Not that
big a deal, I guess.
> +	n_fbdirs = n_fbmfns * sizeof(unsigned long);
...
> +	n_fbdirs = (n_fbdirs + (XC_PAGE_SIZE - 1)) / XC_PAGE_SIZE;
> +
> +	fbmfns = xc_map_foreign_batch(xenfb->xc, domid, PROT_READ,
xenfb->fb_info->pd, n_fbdirs);
> +	if (fbmfns == NULL)
> +		goto error;
> +
> +	xenfb->fb = xc_map_foreign_batch(xenfb->xc, domid, PROT_READ |
PROT_WRITE, fbmfns, n_fbmfns);
> +	if (xenfb->fb == NULL)
> +		goto error;
Irritatingly, map_foreign_batch doesn''t actually return success or
failure through its return value, but by setting the high bits on the
failed entry in the array you pass in.  If the array is mapped
readonly, or is shared with a remote domain, there''s no way to detect
failure.

You might also want to consider using xc_map_foreign_ranges, since
that has a useful return value, but it would mean copying the pfn
arrays and translating them to a slightly different format.
> +
> +	event.type = XENFB_TYPE_SET_EVENTS;
> +	event.set_events.flags = XENFB_FLAG_UPDATE;
> +	if (xenfb_fb_event(xenfb, &event))
> +		goto error;
Would it make sense to negotiate this via xenbus at connection setup
time?  It''s not like it''s likely to change during the life of
the
buffer.
> +
> +	munmap(fbmfns, n_fbdirs * XC_PAGE_SIZE);
> +
> +	xenfb->domid = domid;
> +
> +	xenfb->pub.pixels = xenfb->fb;
> +
> +	xenfb->pub.row_stride = xenfb->fb_info->line_length;
> +	xenfb->pub.depth = xenfb->fb_info->depth;
> +	xenfb->pub.width = xenfb->fb_info->width;
> +	xenfb->pub.height = xenfb->fb_info->height;
It''d be good if you could check that this information was plausible
for mem_length.
> +
> +	return true;
> +
> +error:        
> +	serrno = errno;
> + 	if (xenfb->fb)
> + 		munmap(xenfb->fb, xenfb->fb_info->mem_length);
mem_length is shared with the frontend, and so could have changed
between mapping the area and unmapping it.
> + 	if (fbmfns)
> + 		munmap(fbmfns, xenfb->fb_info->mem_length);
Eh?  I think you mean n_fbdirs * XC_PAGE_SIZE.
> +        if (xenfb->kbd_info)
> +		munmap(xenfb->kbd_info, XC_PAGE_SIZE);
> +        if (xenfb->fb_info)
> +		munmap(xenfb->fb_info, XC_PAGE_SIZE);
> +        if (xenfb->kbd_port);
> +	xc_evtchn_unbind(xenfb->evt_xch, xenfb->kbd_port);
> +        if (xenfb->fbdev_port)
> +		xc_evtchn_unbind(xenfb->evt_xch, xenfb->fbdev_port);
> +        if (xsh) 
> +		xs_daemon_close(xsh);
> +        errno = serrno;
> +        return false;
> +}
Steven.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Steven Smith <sos22-xen@srcf.ucam.org> writes:

[...]
>> Okay, here''s the next patch.  It doesn''t yet take
care of shadow
>> translate mode, just because I want to get it out of the door.  Next
>> week, I hope...
> Okay.
>
>> Oh, and it also doesn''t include Daniel Berrange''s
locking fixes to
>> LibVNCServer, which you really, really need:
>>
http://lists.xensource.com/archives/html/xen-devel/2006-09/msg00371.html
> Did you look at how much work it would be to use the qemu VNC server
> instead of libVNCServer?  I suspect quite a lot, but it might be
> necessary if libVNCServer really is as bad as it appears.
Not yet, sorry.
> This new patch is really pretty good.  Here''s the summary of the
bits
> which might actually matter:
>
> -- The backend still isn''t proof against a malicious frontend. 
This
>    (might) mean that root in an unprivileged domain can get root in
>    dom0, which is a fairly major problem.  Fixing this should be
>    fairly easy.
Yes, this needs to be done.
> -- The setup protocol doesn''t look much like the normal xenbus
state
>    machine.  There may be a good reason for this, but I haven''t
heard
>    one yet.  I know the standard way is badly documented and
>    non-trivial to understand from the existing implementations; sorry
>    about that.
Anthony wrote it that way.  I don''t know why, but it''s simple,
and it
works.

I''m most reluctant to throw out working code while there''s so
much
code that needs fixing.  That situation is improving, though.

There''s a TODO comment in xenfb.c to remind me that I want to try the
xenbus_device API.
> -- You don''t seem to support mice with more than three buttons, or
>    scroll wheels.  It''d be perfectly acceptable to just push this
>    to a later version, though.
See mouse discussion below.
> -- As you say, the keymaping and shadow problems are still there.
One step at a time.
> Other than that, there''s nothing here which would take more than
> an hour or so to fix.
>
>
>> +}
>> +
>> +int sdl2linux[1024] = {
> SDLK_LAST, maybe?
Done, plus bounds check on access.
>> +	[SDLK_a] = KEY_A,
>> +	[SDLK_b] = KEY_B,
> ...
>> +int main(int argc, char **argv)
>> +{
> ...
>> +	while ((opt = getopt_long(argc, argv, "d:t:", options,
>> +				  NULL)) != -1) {
>> +		switch (opt) {
>> +                case ''d'':
>> +			domid = strtol(optarg, &endp, 10);
>> +			if (endp == optarg || *endp || errno) {
> Is testing errno really a good idea here?  strtol doesn''t zero it
on
> success, so you''re relying on it already being zero, which happens
to
> be true this time, as far as I can see, but is a somewhat dubious
> assumption.  The other two tests should be enough, unless I''m
> confused.
Testing errno isn''t so useful here, because we silently truncate the
value anyway.  Removed.
> ...
>> +	if (!xenfb_attach_dom(xenfb, domid)) {
>> +		fprintf(stderr, "Could not connect to domain (%s)\n",
>> +			strerror(errno));
>> +		exit(1);
>> +        }
>> +
>> +	SDL_Init(SDL_INIT_VIDEO);
> This can fail.
>
> I''m aware that a lot of code currently in the tree is less than
> perfect about checking return values and so forth, especially the
> userspace tools, and that it doesn''t actually make that much
> difference in practice.  It''d still be good not to introduce any
more
> of this kind of bug without good reason.
>
> (And, yeah, I know this one''s been there for ages and
I''ve missed it
> twice.  Sorry about that. )
Fixing.  Agreed on the wisdom of checking return values.
>> +
>> +	fd = xenfb_get_fileno(xenfb);
>> +
>> +	data.dst = SDL_SetVideoMode(xenfb->width, xenfb->height,
xenfb->depth,
>> +				    SDL_SWSURFACE);
> This can fail.
Fixing.
>> +
>> +	data.src = SDL_CreateRGBSurfaceFrom(xenfb->pixels,
>> +					    xenfb->width, xenfb->height,
>> +					    xenfb->depth, xenfb->row_stride,
>> +					    0xFF0000, 0xFF00, 0xFF, 0);
> This can fail.
Fixing.
>> +
>> +        if (title == NULL)
>> +		title = strdup("xen-sdlfb");
>> +        SDL_WM_SetCaption(title, title);
>> +
>> +	r.x = r.y = 0;
>> +	r.w = xenfb->width;
>> +	r.h = xenfb->height;
>> +	SDL_BlitSurface(data.src, &r, data.dst, &r);
>> +	SDL_UpdateRect(data.dst, 0, 0, xenfb->width, xenfb->height);
>> +
>> +	xenfb->update = sdl_update;
>> +	xenfb->user_data = &data;
>> +
>> +	SDL_ShowCursor(0);
>> +
>> +	/*
>> +	 * We need to wait for fd becoming ready or SDL events to
>> +	 * arrive.  We time out the select after 10ms to poll for SDL
>> +	 * events.  Clunky, but works.  Could avoid the clunkiness
>> +	 * with a separate thread.
>> +	 */
> Thanks for this.
>
>> +	for (;;) {
>> +		FD_ZERO(&readfds);
>> +		FD_SET(fd, &readfds);
>> +		tv = (struct timeval){0, 10000};
>> +
>> +		if (select(fd + 1, &readfds, NULL, NULL, &tv) < 0) {
>> +			if (errno == EINTR)
>> +				continue;
>> +			fprintf(stderr, "Connection to domain broke (%s)\n",
>> +				strerror(errno));
> It''s not really the connection to the domain that''s
broken as the
> event channel device.  Not terribly important.
Happy to reword it; suggestions?
> ...
>> +		while (SDL_PollEvent(&event)) {
> ...
>> +			case SDL_MOUSEBUTTONDOWN:
>> +			case SDL_MOUSEBUTTONUP:
>> +				xenfb_send_button(xenfb,
>> +					event.type == SDL_MOUSEBUTTONDOWN,
>> +					3 - event.button.button);
> Why 3 - button?
Anthony speedcoding?  %-}
>                  What happens if someone has a four, five, six button
> mouse?
The extra buttons get mapped to negative values here (ugh), truncated
to __u8 in xenfb_send_button() (double-ugh) and thrown away in the
frontend.  The 256th button would get misinterpreted as the first one,
though :)

The xenkbd protocol provides for three mouse buttons.  From xenkbd.h:

	__u8 button;       /* the button (0, 1, 2 is right, middle, left) */

This is extensible as long as we handle unknown buttons gracefully.  I
do hate the weird encoding of buttons, though.  Too hebrew for my
taste.

SDL provides for five buttons (SDL_BUTTON_LEFT, SDL_BUTTON_MIDDLE,
SDL_BUTTON_RIGHT, SDL_BUTTON_WHEELUP, SDL_BUTTON_WHEELDOWN).

LibVNCServer provides for 8 buttons.

The kernel input layer provides for 8 mouse buttons (BTN_LEFT,
BTN_RIGHT, BTN_MIDDLE, BTN_SIDE, BTN_EXTRA, BTN_FORWARD, BTN_BACK,
BTN_TASK).

How to best map buttons from VNC and SDL to input beyond the first
three?  If we can find a workable answer for that, providing for more
buttons should be trivial.
>> +				break;
> ...
>> diff -r 9977b8785570 tools/xenfb/vncfb.c
>> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tools/xenfb/vncfb.c	Sat Sep 30 09:29:38 2006 +0200
> ...
>> +static void on_kbd_event(rfbBool down, rfbKeySym keycode, rfbClientPtr
cl)
>> +{
>> +	rfbScreenInfoPtr server = cl->screen;
>> +	struct xenfb *xenfb = server->screenData;
>> +	xenfb_send_key(xenfb, down, gdk_linux_mapping[keycode & 0xFFFF]);
> I probably just don''t understand the rfbKeySym format, but why is
it
> safe to ignore the high bits of the keycode?
I suspect this is a rather sloppy and confusing bounds check.

I further suspect that rfbKeySym is really just an X11 keysym.  These
are 29 bits, as far as I know.

We''ll dig this up when we address the key encoding issue.  I''d
like to
leave it broken for now.
>> +}
>> +
>> +static void on_ptr_event(int buttonMask, int x, int y, rfbClientPtr
cl)
>> +{
>> +	static int last_x = -1, last_y = -1;
>> +	static int last_button = -1;
>> +	rfbScreenInfoPtr server = cl->screen;
>> +	struct xenfb *xenfb = server->screenData;
>> +
>> +	if (last_button != -1) {
>> +		int i;
>> +
>> +		for (i = 0; i < 8; i++) {
>> +			if ((last_button & (1 << i)) !>> +			   
(buttonMask & (1 << i))) {
>> +				xenfb_send_button(xenfb, buttonMask & (1 << i),
>> +						  2 - i);
> Why 2 - i?  What if button three gets pressed?  We send button 0xff to
> the frontend and it does ...?
>
> And if VNC can''t generate button 3, why do we scan for eight
buttons?
See button discussion above.
>> +			}
>> +		}
>> +	}
>> +
>> +	if (last_x != -1)
>> +		xenfb_send_motion(xenfb, x - last_x, y - last_y);
>> +
>> +	last_button = buttonMask;
>> +
>> +	last_x = x;
>> +	last_y = y;
>> +}
>> +
> ...
>> +static int vnc_start_viewer(int port) 
>> +{
>> +	int pid;
>> +	char s[16];
>> +
>> +	snprintf(s, sizeof(s), ":%d", port);
>> +	switch (pid = fork()) {
>> +	case -1:
>> +		fprintf(stderr, "vncviewer failed fork\n");
>> +		exit(1);
>> +
>> +	case 0:	/* child */
>> +		execlp("vncviewer", "vncviewer", s, NULL);
>> +		fprintf(stderr, "vncviewer execlp failed\n");
> Getting strerror(errno) out here would be good.
Sure.
>> +		exit(1);
>> +
>> +	default:
>> +		return pid;
>> +	}
>> +}
>> +
> ...
>> +int main(int argc, char **argv)
>> +{
>> +	rfbScreenInfoPtr server;
>> +	char *fake_argv[7] = { "vncfb", "-rfbport",
"5901",
>> +                               "-desktop",
"xen-vncfb",
>> +                               "-listen",
"0.0.0.0" };
>> +	int fake_argc = sizeof(fake_argv) / sizeof(fake_argv[0]);
> The initialisation for fake_argv is spread out over quite a lot of
> code.  It''d be a bit easier to follow if it was all in one place. 
Not
> a big deal, though.
>
> ...
>> diff -r 9977b8785570 tools/xenfb/xenfb.c
>> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tools/xenfb/xenfb.c	Sat Sep 30 09:29:38 2006 +0200
> ...
>> +#include <xs.h>
>> +
>> +#include "xenfb.h"
>> +
>> +// FIXME defend against malicous frontend?
> Actually, you''re mostly there already.  I''ve pointed out
a few things
> in line, but it seems pretty good for the most part.
I''d like to keep the FIXME for now, to remind me I want a full audit.
> ...
>> +static void xenfb_detach_dom(struct xenfb_private *xenfb)
>> +{
>> +	xenfb->domid = -1;
>> +	munmap(xenfb->fb, xenfb->fb_info->mem_length);
> You can''t trust mem_length to be the same as it was when you
mapped
> the page, since it''s in the shared area.
Malicious frontend.  It should be read just once, checked for
plausibility, and saved for later use.  Same for the other values
there.  Done except for the checks.
>> +	munmap(xenfb->fb_info, XC_PAGE_SIZE);
>> +	munmap(xenfb->kbd_info, XC_PAGE_SIZE);
>> +	xc_evtchn_unbind(xenfb->evt_xch, xenfb->fbdev_port);
>> +	xc_evtchn_unbind(xenfb->evt_xch, xenfb->kbd_port);
>> +}
> ...
>> +bool xenfb_attach_dom(struct xenfb *xenfb_pub, int domid)
>> +{
> ...
>> +	if (xenfb->kbd_info == NULL)
>> +		goto error;
>> +
>> +	n_fbmfns = (xenfb->fb_info->mem_length + (XC_PAGE_SIZE - 1)) /
XC_PAGE_SIZE;
> If mem_length is near UINT_MAX, this could overflow and lead to you
> not actually mapping any memory, and then crashing later.  Not that
> big a deal, I guess.
Yes.  See above.
>> +	n_fbdirs = n_fbmfns * sizeof(unsigned long);
> ...
>> +	n_fbdirs = (n_fbdirs + (XC_PAGE_SIZE - 1)) / XC_PAGE_SIZE;
>> +
>> +	fbmfns = xc_map_foreign_batch(xenfb->xc, domid, PROT_READ,
xenfb->fb_info->pd, n_fbdirs);
>> +	if (fbmfns == NULL)
>> +		goto error;
>> +
>> +	xenfb->fb = xc_map_foreign_batch(xenfb->xc, domid, PROT_READ |
PROT_WRITE, fbmfns, n_fbmfns);
>> +	if (xenfb->fb == NULL)
>> +		goto error;
> Irritatingly, map_foreign_batch doesn''t actually return success or
> failure through its return value, but by setting the high bits on the
> failed entry in the array you pass in.  If the array is mapped
> readonly, or is shared with a remote domain, there''s no way to
detect
> failure.
Sounds like a design flaw to me.

xc_map_foreign_batch() returns NULL on obvious failures, but the
ioctl() might cause the behavior you described.

I looked for other users of xc_map_foreign_batch() and I can''t see
them checking success other than by examining the return value.

fbmfns[] is mapped PROT_READ from the frontend''s domain.
> You might also want to consider using xc_map_foreign_ranges, since
> that has a useful return value, but it would mean copying the pfn
> arrays and translating them to a slightly different format.
Isn''t that function private?

What do you want me to do here?
>> +
>> +	event.type = XENFB_TYPE_SET_EVENTS;
>> +	event.set_events.flags = XENFB_FLAG_UPDATE;
>> +	if (xenfb_fb_event(xenfb, &event))
>> +		goto error;
> Would it make sense to negotiate this via xenbus at connection setup
> time?  It''s not like it''s likely to change during the
life of the
> buffer.
Can you point to an example of such a negotiation between a frontend
and a backend via xenbus?
>> +
>> +	munmap(fbmfns, n_fbdirs * XC_PAGE_SIZE);
>> +
>> +	xenfb->domid = domid;
>> +
>> +	xenfb->pub.pixels = xenfb->fb;
>> +
>> +	xenfb->pub.row_stride = xenfb->fb_info->line_length;
>> +	xenfb->pub.depth = xenfb->fb_info->depth;
>> +	xenfb->pub.width = xenfb->fb_info->width;
>> +	xenfb->pub.height = xenfb->fb_info->height;
> It''d be good if you could check that this information was
plausible
> for mem_length.
Yes.  See above.
>> +
>> +	return true;
>> +
>> +error:        
>> +	serrno = errno;
>> + 	if (xenfb->fb)
>> + 		munmap(xenfb->fb, xenfb->fb_info->mem_length);
> mem_length is shared with the frontend, and so could have changed
> between mapping the area and unmapping it.
It''s read exactly once now.
>> + 	if (fbmfns)
>> + 		munmap(fbmfns, xenfb->fb_info->mem_length);
> Eh?  I think you mean n_fbdirs * XC_PAGE_SIZE.
Good catch.
>> +        if (xenfb->kbd_info)
>> +		munmap(xenfb->kbd_info, XC_PAGE_SIZE);
>> +        if (xenfb->fb_info)
>> +		munmap(xenfb->fb_info, XC_PAGE_SIZE);
>> +        if (xenfb->kbd_port);
>> +	xc_evtchn_unbind(xenfb->evt_xch, xenfb->kbd_port);
>> +        if (xenfb->fbdev_port)
>> +		xc_evtchn_unbind(xenfb->evt_xch, xenfb->fbdev_port);
>> +        if (xsh) 
>> +		xs_daemon_close(xsh);
>> +        errno = serrno;
>> +        return false;
>> +}
>
> Steven.
I have a few more things to fix for the next patch.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Wed, Oct 04, 2006 at 04:04:29PM +0200, Markus Armbruster
wrote:
> 
> The xenkbd protocol provides for three mouse buttons.  From xenkbd.h:
> 
> 	__u8 button;       /* the button (0, 1, 2 is right, middle, left) */
> 
> This is extensible as long as we handle unknown buttons gracefully.  I
> do hate the weird encoding of buttons, though.  Too hebrew for my
> taste.
> 
> SDL provides for five buttons (SDL_BUTTON_LEFT, SDL_BUTTON_MIDDLE,
> SDL_BUTTON_RIGHT, SDL_BUTTON_WHEELUP, SDL_BUTTON_WHEELDOWN).
> 
> LibVNCServer provides for 8 buttons.
> 
> The kernel input layer provides for 8 mouse buttons (BTN_LEFT,
> BTN_RIGHT, BTN_MIDDLE, BTN_SIDE, BTN_EXTRA, BTN_FORWARD, BTN_BACK,
> BTN_TASK).
> 
> How to best map buttons from VNC and SDL to input beyond the first
> three?  If we can find a workable answer for that, providing for more
> buttons should be trivial.
Not sure why LibVNCServer bothers to go to the trouble of defining
named constants for mouse buttons, since interpretation is really
dependant on the mouse the user has plugged in & VNC protocol
doesn''t
care about named roles for buttons.

VNC rfb protocol allows for 8 buttons, and just documents the regular
X11 conventions[1]

  "On a conventional mouse, buttons 1, 2 and 3 correspond to the 
   left, middle and right buttons on the mouse. On a wheel mouse, 
   each step of the wheel upwards is represented by a press and 
   release of button 4, and each step downwards is represented by
   a press and release of button 5."

For VNC, it would seem we should/can just pass buttons 1-8 straight
though the whole stack with no need for funky mappings. The wire
protocol for VNC is just a U8 bitmask of buttons which are pressed.

Regards,
Dan.

[1] Page 23  http://realvnc.com/docs/rfbproto.pdf

-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Markus Armbruster wrote:
>> -- The backend still isn''t proof against a malicious frontend.
This
>>    (might) mean that root in an unprivileged domain can get root in
>>    dom0, which is a fairly major problem.  Fixing this should be
>>    fairly easy.
>>     
>
> Yes, this needs to be done.
>   
Sorry if I missed this previously, but how could a malicious frontend 
attack a backend?  And where else in Xen are we safe from this? :-)
>> -- The setup protocol doesn''t look much like the normal xenbus
state
>>    machine.  There may be a good reason for this, but I
haven''t heard
>>    one yet.  I know the standard way is badly documented and
>>    non-trivial to understand from the existing implementations; sorry
>>    about that.
This was written before we even had the xenbus state machine.
>>> +			case SDL_MOUSEBUTTONDOWN:
>>> +			case SDL_MOUSEBUTTONUP:
>>> +				xenfb_send_button(xenfb,
>>> +					event.type == SDL_MOUSEBUTTONDOWN,
>>> +					3 - event.button.button);
>>>       
>> Why 3 - button?
>>     
>
> Anthony speedcoding?  %-}
>   
I never expected this code to see the light of day :-)

Seems like every UI toolkit uses a different ordering for mouse 
buttons.  In this case, SDL stores them backwards :-)
>>                  What happens if someone has a four, five, six button
>> mouse?
>>     
>
>   
>> Irritatingly, map_foreign_batch doesn''t actually return
success or
>> failure through its return value, but by setting the high bits on the
>> failed entry in the array you pass in.  If the array is mapped
>> readonly, or is shared with a remote domain, there''s no way to
detect
>> failure.
>>     
>
> Sounds like a design flaw to me.
>   
Wow.

Thanks again Markus for taking on this code!  I hope it''s not too 
painful :-)

Regards,

Anthony Liguori

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> > -- The setup protocol doesn''t look much like the normal
xenbus state
> >    machine.  There may be a good reason for this, but I
haven''t heard
> >    one yet.  I know the standard way is badly documented and
> >    non-trivial to understand from the existing implementations; sorry
> >    about that.
> Anthony wrote it that way.  I don''t know why, but it''s
simple, and it
> works.
> 
> I''m most reluctant to throw out working code while
there''s so much
> code that needs fixing.  That situation is improving, though.
Okay.
> >> +	for (;;) {
> >> +		FD_ZERO(&readfds);
> >> +		FD_SET(fd, &readfds);
> >> +		tv = (struct timeval){0, 10000};
> >> +
> >> +		if (select(fd + 1, &readfds, NULL, NULL, &tv) < 0) {
> >> +			if (errno == EINTR)
> >> +				continue;
> >> +			fprintf(stderr, "Connection to domain broke (%s)\n",
> >> +				strerror(errno));
> > It''s not really the connection to the domain that''s
broken as the
> > event channel device.  Not terribly important.
> Happy to reword it; suggestions?
I dunno; maybe something like ``select() failed on event channel
device (%s)\n"?  I''d rather have a message which most users would
find
meaningless than one which is actually misleading.  Given that this
should almost never happen I don''t think it''s worth worrying
about
that much.
> >                  What happens if someone has a four, five, six button
> > mouse?
> The extra buttons get mapped to negative values here (ugh), truncated
> to __u8 in xenfb_send_button() (double-ugh) and thrown away in the
> frontend.  The 256th button would get misinterpreted as the first one,
> though :)
I''d prefer to discard invalid events here in the backend if possible,
just on the general principle that sending garbage across the ring
buffer is a bad idea even if you know it''s going to get discarded.
> The xenkbd protocol provides for three mouse buttons.  From xenkbd.h:
> 
> 	__u8 button;       /* the button (0, 1, 2 is right, middle, left) */
> 
> This is extensible as long as we handle unknown buttons gracefully.  I
> do hate the weird encoding of buttons, though.  Too hebrew for my
> taste.
It''s certainly not how I would have done it, but I don''t think
it
makes much difference.
> SDL provides for five buttons (SDL_BUTTON_LEFT, SDL_BUTTON_MIDDLE,
> SDL_BUTTON_RIGHT, SDL_BUTTON_WHEELUP, SDL_BUTTON_WHEELDOWN).
> 
> LibVNCServer provides for 8 buttons.
> 
> The kernel input layer provides for 8 mouse buttons (BTN_LEFT,
> BTN_RIGHT, BTN_MIDDLE, BTN_SIDE, BTN_EXTRA, BTN_FORWARD, BTN_BACK,
> BTN_TASK).
> 
> How to best map buttons from VNC and SDL to input beyond the first
> three?  If we can find a workable answer for that, providing for more
> buttons should be trivial.
*shrug* We might as well just send input layer codes across the ring
buffer and do the translation in the backend.  That makes the Linux
frontend easier and doesn''t make other operating systems any harder.
If we later find that some other operating system supports buttons
which the input layer doesn''t then we can figure out what to do with
them then; provided we make the frontend discard buttons it doesn''t
understand it should be trivial to do this in a backwards compatible
way.
> >> +				break;
> > ...
> >> diff -r 9977b8785570 tools/xenfb/vncfb.c
> >> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> >> +++ b/tools/xenfb/vncfb.c	Sat Sep 30 09:29:38 2006 +0200
> > ...
> >> +static void on_kbd_event(rfbBool down, rfbKeySym keycode,
rfbClientPtr cl)
> >> +{
> >> +	rfbScreenInfoPtr server = cl->screen;
> >> +	struct xenfb *xenfb = server->screenData;
> >> +	xenfb_send_key(xenfb, down, gdk_linux_mapping[keycode &
0xFFFF]);
> > I probably just don''t understand the rfbKeySym format, but
why is it
> > safe to ignore the high bits of the keycode?
> I suspect this is a rather sloppy and confusing bounds check.
> 
> I further suspect that rfbKeySym is really just an X11 keysym.  These
> are 29 bits, as far as I know.
> 
> We''ll dig this up when we address the key encoding issue. 
I''d like to
> leave it broken for now.
Fair enough.
> >> +#include <xs.h>
> >> +
> >> +#include "xenfb.h"
> >> +
> >> +// FIXME defend against malicous frontend?
> > Actually, you''re mostly there already.  I''ve pointed
out a few things
> > in line, but it seems pretty good for the most part.
> I''d like to keep the FIXME for now, to remind me I want a full
audit.
Okay.
> > ...
> >> +static void xenfb_detach_dom(struct xenfb_private *xenfb)
> >> +{
> >> +	xenfb->domid = -1;
> >> +	munmap(xenfb->fb, xenfb->fb_info->mem_length);
> > You can''t trust mem_length to be the same as it was when you
mapped
> > the page, since it''s in the shared area.
> Malicious frontend.  It should be read just once, checked for
> plausibility, and saved for later use.  Same for the other values
> there.
Yep.
> Done except for the checks.
Thanks.
> >> +bool xenfb_attach_dom(struct xenfb *xenfb_pub, int domid)
> >> +{
> > ...
> >> +	if (xenfb->kbd_info == NULL)
> >> +		goto error;
> >> +
> >> +	n_fbmfns = (xenfb->fb_info->mem_length + (XC_PAGE_SIZE -
1)) / XC_PAGE_SIZE;
> > If mem_length is near UINT_MAX, this could overflow and lead to you
> > not actually mapping any memory, and then crashing later.  Not that
> > big a deal, I guess.
> Yes.  See above.
Okay.
> >> +	n_fbdirs = n_fbmfns * sizeof(unsigned long);
> > ...
> >> +	n_fbdirs = (n_fbdirs + (XC_PAGE_SIZE - 1)) / XC_PAGE_SIZE;
> >> +
> >> +	fbmfns = xc_map_foreign_batch(xenfb->xc, domid, PROT_READ,
xenfb->fb_info->pd, n_fbdirs);
> >> +	if (fbmfns == NULL)
> >> +		goto error;
> >> +
> >> +	xenfb->fb = xc_map_foreign_batch(xenfb->xc, domid,
PROT_READ | PROT_WRITE, fbmfns, n_fbmfns);
> >> +	if (xenfb->fb == NULL)
> >> +		goto error;
> > Irritatingly, map_foreign_batch doesn''t actually return
success or
> > failure through its return value, but by setting the high bits on the
> > failed entry in the array you pass in.  If the array is mapped
> > readonly, or is shared with a remote domain, there''s no way
to detect
> > failure.
> Sounds like a design flaw to me.
That''s one way of putting it, certainly. :)
> xc_map_foreign_batch() returns NULL on obvious failures, but the
> ioctl() might cause the behavior you described.
> 
> I looked for other users of xc_map_foreign_batch() and I can''t see
> them checking success other than by examining the return value.
> 
> fbmfns[] is mapped PROT_READ from the frontend''s domain.
I think the correct fix here is probably just to fix up
xc_map_foreign_batch() to have a more sane calling convention.  As you
say, most of its existing users seem to make the same assumptions as
have happened here.  I can''t actually see any good way of dealing with
this in the standard libxc API.
> > You might also want to consider using xc_map_foreign_ranges, since
> > that has a useful return value, but it would mean copying the pfn
> > arrays and translating them to a slightly different format.
> Isn''t that function private?
Oops, yes, sorry, forgot about that.
> What do you want me to do here?
I''d leave it as a known bug for now.  We''ll add a new
interface to
libxc once 3.0.3''s dealt with.
> >> +
> >> +	event.type = XENFB_TYPE_SET_EVENTS;
> >> +	event.set_events.flags = XENFB_FLAG_UPDATE;
> >> +	if (xenfb_fb_event(xenfb, &event))
> >> +		goto error;
> > Would it make sense to negotiate this via xenbus at connection setup
> > time?  It''s not like it''s likely to change during
the life of the
> > buffer.
> Can you point to an example of such a negotiation between a frontend
> and a backend via xenbus?
The netif feature flags are probably the most obvious example.  For
instance, to turn on copy rx mode, you first have the backend put
feature-rx-copy=1 in its xenstore area, and then when the frontend
connects it notices this and puts request-rx-copy=1 in its area.  The
backend reads this out as part of connection setup, and rx copy mode
is turned on.

The equivalent in this case would be for the backend to set
request-update=1 in its area when it starts, and then for the frontend
to set provides-update=1 if appropriate.

Of course, this sort of thing only works well for flags which don''t
change while the buffer is live.  I''d certainly expect
XENFB_FLAG_UPDATE to fit into that category, but perhaps you have some
future plans which wouldn''t work well with this?
> >> +
> >> +	return true;
> >> +
> >> +error:        
> >> +	serrno = errno;
> >> + 	if (xenfb->fb)
> >> + 		munmap(xenfb->fb, xenfb->fb_info->mem_length);
> > mem_length is shared with the frontend, and so could have changed
> > between mapping the area and unmapping it.
> It''s read exactly once now.
Excellent, thank you.

Steven.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> >>-- The backend still isn''t proof against a malicious
frontend.  This
> >>   (might) mean that root in an unprivileged domain can get root in
> >>   dom0, which is a fairly major problem.  Fixing this should be
> >>   fairly easy.
> >>    
> >Yes, this needs to be done.
> Sorry if I missed this previously, but how could a malicious frontend 
> attack a backend?
This protocol places various bits of control data in a page shared
between the the frontend and backend.  The backend continues to read
things like the resolution out of this buffer while it''s operating,
and I was worried that the frontend could cause the backend to
misbehave by changing them while the buffer was live.
> And where else in Xen are we safe from this? :-)
In theory, everywhere, since frontends are explicitly not trusted
code.  If you know of any places in which a frontend can cause a
backend to run arbitrary code, or panic, or anything like that then
please report it as a bug.
> >>-- The setup protocol doesn''t look much like the normal
xenbus state
> >>   machine.  There may be a good reason for this, but I
haven''t heard
> >>   one yet.  I know the standard way is badly documented and
> >>   non-trivial to understand from the existing implementations;
sorry
> >>   about that.
> This was written before we even had the xenbus state machine.
Okay.
> >>>+			case SDL_MOUSEBUTTONDOWN:
> >>>+			case SDL_MOUSEBUTTONUP:
> >>>+				xenfb_send_button(xenfb,
> >>>+					event.type == SDL_MOUSEBUTTONDOWN,
> >>>+					3 - event.button.button);
> >>>      
> >>Why 3 - button?
> >>    
> >
> >Anthony speedcoding?  %-}
> >  
> I never expected this code to see the light of day :-)
> 
> Seems like every UI toolkit uses a different ordering for mouse 
> buttons.  In this case, SDL stores them backwards :-)
Okay.

Steven.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Steven Smith <sos22-xen@srcf.ucam.org> writes:
>> > -- The setup protocol doesn''t look much like the normal
xenbus state
>> >    machine.  There may be a good reason for this, but I
haven''t heard
>> >    one yet.  I know the standard way is badly documented and
>> >    non-trivial to understand from the existing implementations;
sorry
>> >    about that.
>> Anthony wrote it that way.  I don''t know why, but
it''s simple, and it
>> works.
>> 
>> I''m most reluctant to throw out working code while
there''s so much
>> code that needs fixing.  That situation is improving, though.
> Okay.
>
>> >> +	for (;;) {
>> >> +		FD_ZERO(&readfds);
>> >> +		FD_SET(fd, &readfds);
>> >> +		tv = (struct timeval){0, 10000};
>> >> +
>> >> +		if (select(fd + 1, &readfds, NULL, NULL, &tv) <
0) {
>> >> +			if (errno == EINTR)
>> >> +				continue;
>> >> +			fprintf(stderr, "Connection to domain broke
(%s)\n",
>> >> +				strerror(errno));
>> > It''s not really the connection to the domain
that''s broken as the
>> > event channel device.  Not terribly important.
>> Happy to reword it; suggestions?
> I dunno; maybe something like ``select() failed on event channel
> device (%s)\n"?  I''d rather have a message which most users
would find
> meaningless than one which is actually misleading.  Given that this
> should almost never happen I don''t think it''s worth
worrying about
> that much.
Works for me.
>> >                  What happens if someone has a four, five, six
button
>> > mouse?
>> The extra buttons get mapped to negative values here (ugh), truncated
>> to __u8 in xenfb_send_button() (double-ugh) and thrown away in the
>> frontend.  The 256th button would get misinterpreted as the first one,
>> though :)
> I''d prefer to discard invalid events here in the backend if
possible,
> just on the general principle that sending garbage across the ring
> buffer is a bad idea even if you know it''s going to get discarded.
Agreed.
>> The xenkbd protocol provides for three mouse buttons.  From xenkbd.h:
>> 
>> 	__u8 button;       /* the button (0, 1, 2 is right, middle, left) */
>> 
>> This is extensible as long as we handle unknown buttons gracefully.  I
>> do hate the weird encoding of buttons, though.  Too hebrew for my
>> taste.
> It''s certainly not how I would have done it, but I don''t
think it
> makes much difference.
>
>> SDL provides for five buttons (SDL_BUTTON_LEFT, SDL_BUTTON_MIDDLE,
>> SDL_BUTTON_RIGHT, SDL_BUTTON_WHEELUP, SDL_BUTTON_WHEELDOWN).
>> 
>> LibVNCServer provides for 8 buttons.
>> 
>> The kernel input layer provides for 8 mouse buttons (BTN_LEFT,
>> BTN_RIGHT, BTN_MIDDLE, BTN_SIDE, BTN_EXTRA, BTN_FORWARD, BTN_BACK,
>> BTN_TASK).
>> 
>> How to best map buttons from VNC and SDL to input beyond the first
>> three?  If we can find a workable answer for that, providing for more
>> buttons should be trivial.
> *shrug* We might as well just send input layer codes across the ring
> buffer and do the translation in the backend.  That makes the Linux
> frontend easier and doesn''t make other operating systems any
harder.
> If we later find that some other operating system supports buttons
> which the input layer doesn''t then we can figure out what to do
with
> them then; provided we make the frontend discard buttons it
doesn''t
> understand it should be trivial to do this in a backwards compatible
> way.
The input layer treats mouse buttons just like keys.  If we use its
button encoding, we can just as well use XENKBD_TYPE_KEY and drop
XENKBD_TYPE_BUTTON.  Any objections?

[...]
>> >> +	n_fbdirs = n_fbmfns * sizeof(unsigned long);
>> > ...
>> >> +	n_fbdirs = (n_fbdirs + (XC_PAGE_SIZE - 1)) / XC_PAGE_SIZE;
>> >> +
>> >> +	fbmfns = xc_map_foreign_batch(xenfb->xc, domid,
PROT_READ, xenfb->fb_info->pd, n_fbdirs);
>> >> +	if (fbmfns == NULL)
>> >> +		goto error;
>> >> +
>> >> +	xenfb->fb = xc_map_foreign_batch(xenfb->xc, domid,
PROT_READ | PROT_WRITE, fbmfns, n_fbmfns);
>> >> +	if (xenfb->fb == NULL)
>> >> +		goto error;
>> > Irritatingly, map_foreign_batch doesn''t actually return
success or
>> > failure through its return value, but by setting the high bits on
the
>> > failed entry in the array you pass in.  If the array is mapped
>> > readonly, or is shared with a remote domain, there''s no
way to detect
>> > failure.
>> Sounds like a design flaw to me.
> That''s one way of putting it, certainly. :)
>
>> xc_map_foreign_batch() returns NULL on obvious failures, but the
>> ioctl() might cause the behavior you described.
>> 
>> I looked for other users of xc_map_foreign_batch() and I can''t
see
>> them checking success other than by examining the return value.
>> 
>> fbmfns[] is mapped PROT_READ from the frontend''s domain.
> I think the correct fix here is probably just to fix up
> xc_map_foreign_batch() to have a more sane calling convention.  As you
> say, most of its existing users seem to make the same assumptions as
> have happened here.  I can''t actually see any good way of dealing
with
> this in the standard libxc API.
>
>> > You might also want to consider using xc_map_foreign_ranges, since
>> > that has a useful return value, but it would mean copying the pfn
>> > arrays and translating them to a slightly different format.
>> Isn''t that function private?
> Oops, yes, sorry, forgot about that.
>
>> What do you want me to do here?
> I''d leave it as a known bug for now.  We''ll add a new
interface to
> libxc once 3.0.3''s dealt with.
Excellent.
>> >> +
>> >> +	event.type = XENFB_TYPE_SET_EVENTS;
>> >> +	event.set_events.flags = XENFB_FLAG_UPDATE;
>> >> +	if (xenfb_fb_event(xenfb, &event))
>> >> +		goto error;
>> > Would it make sense to negotiate this via xenbus at connection
setup
>> > time?  It''s not like it''s likely to change
during the life of the
>> > buffer.
>> Can you point to an example of such a negotiation between a frontend
>> and a backend via xenbus?
> The netif feature flags are probably the most obvious example.  For
> instance, to turn on copy rx mode, you first have the backend put
> feature-rx-copy=1 in its xenstore area, and then when the frontend
> connects it notices this and puts request-rx-copy=1 in its area.  The
> backend reads this out as part of connection setup, and rx copy mode
> is turned on.
>
> The equivalent in this case would be for the backend to set
> request-update=1 in its area when it starts, and then for the frontend
> to set provides-update=1 if appropriate.
I''ll look into this when/if I find the time.
> Of course, this sort of thing only works well for flags which
don''t
> change while the buffer is live.  I''d certainly expect
> XENFB_FLAG_UPDATE to fit into that category, but perhaps you have some
> future plans which wouldn''t work well with this?
I can''t guarantee we won''t need a mechanism to switch things
during
operation at some time, but neither can I guarantee that
XENFB_TYPE_SET_EVENTS as it is now will do for that then.

Instead of attempting to cover all possible future cases of option
negotiation / mode switching, let''s just provide for what we need now
in a simple and reasonably general way.

[...]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> >> How to best map buttons from VNC and SDL to input beyond the first
> >> three?  If we can find a workable answer for that, providing for
more
> >> buttons should be trivial.
> > *shrug* We might as well just send input layer codes across the ring
> > buffer and do the translation in the backend.  That makes the Linux
> > frontend easier and doesn''t make other operating systems any
harder.
> > If we later find that some other operating system supports buttons
> > which the input layer doesn''t then we can figure out what to
do with
> > them then; provided we make the frontend discard buttons it
doesn''t
> > understand it should be trivial to do this in a backwards compatible
> > way.
> The input layer treats mouse buttons just like keys.  If we use its
> button encoding, we can just as well use XENKBD_TYPE_KEY and drop
> XENKBD_TYPE_BUTTON.  Any objections?
Choosing the correct encoding for key events is (as we''ve so recently
demonstrated) non-trivial, so I think I''d like to avoid tying it too
closely to the encoding for mouse events, if it''s all the same to you.
> >> >> +
> >> >> +	event.type = XENFB_TYPE_SET_EVENTS;
> >> >> +	event.set_events.flags = XENFB_FLAG_UPDATE;
> >> >> +	if (xenfb_fb_event(xenfb, &event))
> >> >> +		goto error;
> >> > Would it make sense to negotiate this via xenbus at
connection setup
> >> > time?  It''s not like it''s likely to change
during the life of the
> >> > buffer.
> >> Can you point to an example of such a negotiation between a
frontend
> >> and a backend via xenbus?
> > The netif feature flags are probably the most obvious example.  For
> > instance, to turn on copy rx mode, you first have the backend put
> > feature-rx-copy=1 in its xenstore area, and then when the frontend
> > connects it notices this and puts request-rx-copy=1 in its area.  The
> > backend reads this out as part of connection setup, and rx copy mode
> > is turned on.
> >
> > The equivalent in this case would be for the backend to set
> > request-update=1 in its area when it starts, and then for the frontend
> > to set provides-update=1 if appropriate.
> I''ll look into this when/if I find the time.
Thanks.
> > Of course, this sort of thing only works well for flags which
don''t
> > change while the buffer is live.  I''d certainly expect
> > XENFB_FLAG_UPDATE to fit into that category, but perhaps you have some
> > future plans which wouldn''t work well with this?
> I can''t guarantee we won''t need a mechanism to switch
things during
> operation at some time, but neither can I guarantee that
> XENFB_TYPE_SET_EVENTS as it is now will do for that then.
> 
> Instead of attempting to cover all possible future cases of option
> negotiation / mode switching, let''s just provide for what we need
now
> in a simple and reasonably general way.
Good plan.

Steven.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

I think I understand the problem of key encoding, but I''m not sure I
fully understand the solutions proposed here, so let me summarize and
ask (possibly stupid) questions.

The backend gets keys in an encoding that depends on the software used
to access the keyboard:

* with SDL, struct SDL_keysym (consists of scancode, SDL keysym,
current modifiers and optionally Unicode translation),

* with LibVNCServer: rfbKeySym (which is just an X11 keysym).

The frontend needs to stuff Linux keycodes into the input layer.

Our job is to translate from frontend keysyms to Linux keycodes, and
the question is to find out how and where.

Translating SDL_keysym.scancode into Linux keycode should be
straightforward, but translating X11 keysyms isn''t.  Steven mentioned
code in rdesktop that does that, and Anthony pointed to QEMU.

It would be nice to encapsulate all knowledge about frontend keysyms
in the frontends, i.e. to translate to a common wire encoding there.
Any reason why that''s not the way to go?  Any reason why the wire
encoding can''t (or shouldn''t) be simply Linux keycodes?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Hi,

On Sat, 2006-10-07 at 18:48 +0200, Markus Armbruster wrote:
> The backend gets keys in an encoding that depends on the software used
> to access the keyboard:
> 
> * with SDL, struct SDL_keysym (consists of scancode, SDL keysym,
> current modifiers and optionally Unicode translation),
> 
> * with LibVNCServer: rfbKeySym (which is just an X11 keysym).
> 
> The frontend needs to stuff Linux keycodes into the input layer.
> 
> Our job is to translate from frontend keysyms to Linux keycodes, and
> the question is to find out how and where.
Bear in mind that it may simply not be possible.  :-(

An example is my UK keyboard connecting to a guest with a US keymap.
For me, "#" is a key on its own (with "~" as the
shift-modified key.)
When I type "#", with no modifier pressed, there is simply no way to
translate that to a single keycode on a US keymap and get the "#" ksym
out --- on a US keyboard, "#" requires shift to be held down.

The only way you''ll get such a keypress through is by faking modifiers
on the fly.

--Stephen



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Stephen C. Tweedie wrote:
> Hi,
> 
> Bear in mind that it may simply not be possible.  :-(
Right, this is my understanding of the problem.

I think the best we can do is use a keymap in the host to translate from 
whatever keycodes we''re getting to PC scancodes and then leave it up to
the guest to convert the scancodes based on whatever keymap they''re
using.

This is what rdesktop and qemu do.

Regards,

Anthony Liguori
> An example is my UK keyboard connecting to a guest with a US keymap.
> For me, "#" is a key on its own (with "~" as the
shift-modified key.)
> When I type "#", with no modifier pressed, there is simply no way
to
> translate that to a single keycode on a US keymap and get the "#"
ksym
> out --- on a US keyboard, "#" requires shift to be held down.
> 
> The only way you''ll get such a keypress through is by faking
modifiers
> on the fly.
> 
> --Stephen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Stephen C. Tweedie wrote:
> Hi,
> 
> Bear in mind that it may simply not be possible.  :-(
Right, this is my understanding of the problem.

I think the best we can do is use a keymap in the host to translate from 
whatever keycodes we''re getting to PC scancodes and then leave it up to
the guest to convert the scancodes based on whatever keymap they''re
using.

This is what rdesktop and qemu do.

Regards,

Anthony Liguori
> An example is my UK keyboard connecting to a guest with a US keymap.
> For me, "#" is a key on its own (with "~" as the
shift-modified key.)
> When I type "#", with no modifier pressed, there is simply no way
to
> translate that to a single keycode on a US keymap and get the "#"
ksym
> out --- on a US keyboard, "#" requires shift to be held down.
> 
> The only way you''ll get such a keypress through is by faking
modifiers
> on the fly.
> 
> --Stephen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> The backend gets keys in an encoding that depends on the software used
> to access the keyboard:
...
> The frontend needs to stuff Linux keycodes into the input layer.
> 
> Our job is to translate from frontend keysyms to Linux keycodes, and
> the question is to find out how and where.
I think you mean backend keysyms, but, yes, that''s the basic issue.
> Translating SDL_keysym.scancode into Linux keycode should be
> straightforward, but translating X11 keysyms isn''t.  Steven
mentioned
> code in rdesktop that does that, and Anthony pointed to QEMU.
> 
> It would be nice to encapsulate all knowledge about frontend keysyms
> in the frontends, i.e. to translate to a common wire encoding there.
> Any reason why that''s not the way to go?  Any reason why the wire
> encoding can''t (or shouldn''t) be simply Linux keycodes?
The main objection to sending Linux keycodes over the wire is that
their meaning depends on the keymap currently loaded.  This could
cause some problems if the machine running VNC viewer is using a
different keymap to the one loaded in the frontend.  However, every
other solution that''s been suggested has lead to some keys being
unrepresentable, which is even worse.

Steven.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

"Stephen C. Tweedie" <sct@redhat.com> writes:
> Hi,
>
> On Sat, 2006-10-07 at 18:48 +0200, Markus Armbruster wrote:
>
>> The backend gets keys in an encoding that depends on the software used
>> to access the keyboard:
>> 
>> * with SDL, struct SDL_keysym (consists of scancode, SDL keysym,
>> current modifiers and optionally Unicode translation),
>> 
>> * with LibVNCServer: rfbKeySym (which is just an X11 keysym).
>> 
>> The frontend needs to stuff Linux keycodes into the input layer.
>> 
>> Our job is to translate from frontend keysyms to Linux keycodes, and
>> the question is to find out how and where.
>
> Bear in mind that it may simply not be possible.  :-(
>
> An example is my UK keyboard connecting to a guest with a US keymap.
> For me, "#" is a key on its own (with "~" as the
shift-modified key.)
> When I type "#", with no modifier pressed, there is simply no way
to
> translate that to a single keycode on a US keymap and get the "#"
ksym
> out --- on a US keyboard, "#" requires shift to be held down.
>
> The only way you''ll get such a keypress through is by faking
modifiers
> on the fly.
Arguably, `#'' is not a raw key, its something bound to a key by
keymapping software.  Let me call that a mapped key, for lack of a
better word.

Options I can see:

(1) Start with the host''s mapped keys.  Fake raw keys in the guest so
    that guest keymapping maps them to the mapped keys we see in the
    host.

    Nice: host key map just works across all guests.

    However, I fear this is a game we can''t win: we can hardly assume
    that every keymap can produce every conceivable mapped key, no
    matter how much we fool around with modifiers.  Perhaps we could
    be good enough at the game to make things work well enough for
    most users.  I don''t know.  Smells like a swamp to me.

(2) Simply use the host''s raw keys.

    The guest needs to understand the host''s raw keys, and map them if
    they differ from its own idea of raw keys.  At this time, the only
    idea of raw keys in town is Linux keycodes, so no mapping is
    necessary.

    Nice: simple & stupid.

    Unless I misunderstand Anthony, that''s what rdesktop and qemu do.

(3) Define some abstract key encoding, map from host raw keys to that,
    then to guest raw keys.

    If we simply choose the current Linux keycodes as abstract
    encoding, we get (3) for the price of (2) now, and pay the rest
    only when Linux keycodes change incompatibly.  Which I figure is
    rather unlikely.

What do you want me to code?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Hi,
> (2) Simply use the host''s raw keys.
> 
>     The guest needs to understand the host''s raw keys, and map
them if
>     they differ from its own idea of raw keys.  At this time, the only
>     idea of raw keys in town is Linux keycodes, so no mapping is
>     necessary.
> 
>     Nice: simple & stupid.
> 
>     Unless I misunderstand Anthony, that''s what rdesktop and qemu
do.
<unimportant side node>
  Well, there are some corner cases where you can''t easily get the
hosts
  raw keys.  In that case qemu tries to translate the keysyms back to
  raw keycodes using the maps in /usr/share/qemu/keymaps.  See also qemu
  manpage, "-k" switch.
</>

But, yes, using linux keycodes as wire encoding IMHO is the way to go.

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>
http://www.suse.de/~kraxel/julika-dora.jpeg

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Gerd Hoffmann wrote:
>   Hi,
> 
>> (2) Simply use the host''s raw keys.
>>
>>     The guest needs to understand the host''s raw keys, and map
them if
>>     they differ from its own idea of raw keys.  At this time, the only
>>     idea of raw keys in town is Linux keycodes, so no mapping is
>>     necessary.
>>
>>     Nice: simple & stupid.
>>
>>     Unless I misunderstand Anthony, that''s what rdesktop and
qemu do.
> 
> <unimportant side node>
>   Well, there are some corner cases where you can''t easily get the
hosts
>   raw keys.  In that case qemu tries to translate the keysyms back to
>   raw keycodes using the maps in /usr/share/qemu/keymaps.  See also qemu
>   manpage, "-k" switch.
> </>
> 
> But, yes, using linux keycodes as wire encoding IMHO is the way to go.
> 
> cheers,
>   Gerd
> 
So, perhaps the patch I post sometime ago on this subject should be useful ?

Laurent
-- 
                Laurent.Vivier@bull.net
+----- "Any sufficiently advanced technology is ----+
| indistinguishable from magic." - Arthur C. Clarke |



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel