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