Re: [Xen-devel] [PATCH] Paravirt framebuffer backend tools [2/5]

[Jeremy Katz <katzj@xxxxxxxxxx>]

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