> --- 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.
signature.asc
Description: Digital signature
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home