|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] Paravirt framebuffer backend tools [2/5]
> >> +#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.
Attachment:
signature.asc _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxx http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |