Re: [Xen-devel] [PATCH 2/2] PV framebuffer

[Steven Smith <sos22-xen@xxxxxxxxxxxxx>]

> PV framebuffer frontend.  Derived from http://hg.codemonkey.ws/vncfb
This is the backend, isn't it?

> diff -r 2e35cf028ff0 tools/python/xen/xend/XendDevices.py
> --- a/tools/python/xen/xend/XendDevices.py    Thu Nov 09 15:43:24 2006 +0000
> +++ b/tools/python/xen/xend/XendDevices.py    Thu Nov 09 17:58:26 2006 +0100
> @@ -41,6 +41,8 @@ class XendDevices:
>          'irq': irqif.IRQController,
>          'usb': usbif.UsbifController,
>          'tap': BlktapController,
> +        'vfb': vfbif.VfbifController,
> +        'vkbd': vfif.VkbdifController,
Typo?  Should this be:

  +        'vkbd': vfbif.VkbdifController,

?

> diff -r 2e35cf028ff0 tools/xenfb/vncfb.c
> --- /dev/null   Thu Jan 01 00:00:00 1970 +0000
> +++ b/tools/xenfb/vncfb.c       Thu Nov 09 10:18:58 2006 +0100

> +int main(int argc, char **argv)
> +{
> +     rfbScreenInfoPtr server;
> +     char *fake_argv[7] = { "vncfb", "-rfbport", "5901", 
> +                               "-desktop", "xen-vncfb", 
> +                               "-listen", "0.0.0.0" };
HVM VNC server defaults to listening on 127.0.0.1 now.  You might want
to make this do the same thing.

> diff -r 2e35cf028ff0 tools/xenfb/xenfb.c
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/tools/xenfb/xenfb.c     Thu Nov 09 10:22:36 2006 +0100

> +static int xenfb_hotplug(struct xs_handle *xsh, const char *backdir)
> +{
> +     if (xenfb_xs_printf(xsh, backdir, "hotplug-status", "connected"))
> +             return -1;
> +
> +     if (!xs_watch(xsh, backdir, ""))
> +             return -1;
> +
> +     switch (xenfb_wait_for_state(xsh, backdir,
> +#if 1 /* TODO fudging state to permit restarting; to be removed */
> +                     (1 << XenbusStateInitialising)
> +                     | (1 << XenbusStateInitWait)
> +                     | (1 << XenbusStateConnected)
> +#else
> +                     1 << XenbusStateInitialising,
> +#endif
I'm not sure what you're trying to do here.  It looks like you're
waiting for the backend to go to state Initialising (or InitWait, or
Connected), but this is the backend, so if it isn't there already
you're going to wait forever.

> +bool xenfb_attach_dom(struct xenfb *xenfb_pub, int domid)
> +{
...
> +     if (xenfb_hotplug(xsh, vfb_backdir) < 0)
> +             goto error;
> +     if (xenfb_hotplug(xsh, vkbd_backdir) < 0)
> +             goto error;
> +
> +     if (xenfb_xs_printf(xsh, vkbd_backdir, "feature-abs-pointer", "1"))
> +             goto error;
> +     if (xenfb_xs_printf(xsh, vfb_backdir, "state", "%d",
> +                         XenbusStateInitWait))
> +             goto error;
> +     if (xenfb_xs_printf(xsh, vkbd_backdir, "state", "%d",
> +                         XenbusStateInitWait))
> +             goto error;
> +
I'd probably reorder this a little to look more like this:

(1) Set feature-abs-pointer
(2) Set state to InitWait
(3) Set hotplug status

The only actual *required* constraint is (1) before (2), so that the
frontend doesn't initialise before we've set the feature and
potentially miss it.

(2) before (3) is kind of nice, in that it makes sure that the backend
is ready before xend unpauses the domain, and so the frontend'll be
able to connect the first time it tries, but that's a lot less
important here than for e.g. block devices.

> + error:
> +     serrno = errno;
> +     if (xenfb->fb)
> +             munmap(xenfb->fb, xenfb->fb_len);
> +     if (fbmfns)
> +             munmap(fbmfns, 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;
It'd be good to set an error in xenbus when this fails.  Look at
xenbus_dev_fatal and _dev_error in drivers/xen/xenbus/xenbus_client.c
to see how to do this.

> +        return false;
> +}
> +

Steven.

Attachment: signature.asc
Description: Digital signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel