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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home