"Christian Limpach" <christian.limpach@xxxxxxxxx> writes:
[Review of frontend...]
I replied to that separately.
>> diff -r 6ca424e1867e -r 103a9f38f17f tools/xenfb/vncfb.c
>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tools/xenfb/vncfb.c Fri Aug 18 16:20:27 2006 -0400
>> @@ -0,0 +1,154 @@
>
>> +#if 0
>> +#define BUG() do { printf("%d\n", __LINE__); return 1; } while(0)
>> +#else
>> +extern void abort();
> #include <stdlib.h> ?
My debug code crept into the patch, oops.
Anthony's code had the first BUG() definition. I hate it. I want
code to crash hard when it detects it messed up. Should be cleaned
up. What behavior do you prefer?
>> +#define BUG() abort();
>> +#endif
>> +
>
>> +
>> + rfbRunEventLoop(server, -1, TRUE);
> I'm a little worried that you appear to have a multithreaded program
> with no locks here, but nevermind.
As far as I can see, our client code doesn't share data with
LibVNCServer, it only calls its services. Therefore, I don't think
locking is necessary, unless the LibVNCServer API requires it. And
that would be odd, wouldn't it? I can't tell for sure, as I'm not
familiar with it.
>> +struct xenfb *xenfb_new(void)
>> +{
> ...
>> +
>> + xenfb->fd = open("/dev/xen/evtchn", O_RDWR);
> xc_evtchn_open, maybe.
Certainly.
>> + if (xenfb->fd == -1) {
> ...
>> +}
>> +
>> +int xenfb_get_fileno(struct xenfb *xenfb_pub)
>> +{
>> + struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
>> + if (xenfb->domid == -1)
>> + return -1;
> Eh?
I don't understand why this is necessary or useful either. I'll
remove it and see what happens.
>> +
>> + return xenfb->fd;
>> +}
>> +
>> +static void xenfb_unset_domid(struct xenfb_private *xenfb)
> This does a fair bit more than unset the domid.
Sometimes things outgrow initial names... renamed to
xenfb_detach_dom().
>> +{
>> + struct ioctl_evtchn_unbind unbind;
>> +
>> + xenfb->domid = -1;
>> +
>> + munmap(xenfb->fb, xenfb->fb_info->mem_length);
>> + munmap(xenfb->fb_info, XC_PAGE_SIZE);
>> + munmap(xenfb->kbd_info, XC_PAGE_SIZE);
>> + unbind.port = xenfb->fbdev_port;
>> + ioctl(xenfb->fd, IOCTL_EVTCHN_UNBIND, &unbind);
>> + unbind.port = xenfb->kbd_port;
>> + ioctl(xenfb->fd, IOCTL_EVTCHN_UNBIND, &unbind);
> xc_evtchn_unbind?
Yes.
>> +}
>
>> +static int xenfb_fb_event(struct xenfb_private *xenfb, union xenfb_in_event
>> *event)
>> +{
>> + uint32_t prod;
>> + struct xenfb_page *info = xenfb->fb_info;
>> + struct ioctl_evtchn_notify notify;
>> +
>> + prod = info->in_prod;
>> + if (prod - info->in_cons == XENFB_RING_SIZE(info->in)) {
>> + errno = EAGAIN;
>> + return -1;
>> + }
>> +
>> + XENFB_RING_REF(info->in, prod) = *event;
> Need a memory barrier here.
You didn't ask for memory barriers in similar code in the frontend.
Did you just miss it there, or do I miss something?
>> + info->in_prod = prod + 1;
>> + notify.port = xenfb->fbdev_port;
>> + return ioctl(xenfb->fd, IOCTL_EVTCHN_NOTIFY, ¬ify);
> xc_evtchn_notify()?
Yes.
>> +}
>> +
>> +static int xenfb_kbd_event(struct xenfb_private *xenfb, union
>> xenkbd_in_event *event)
>> +{
>> + uint32_t prod;
>> + struct xenkbd_info *info = xenfb->kbd_info;
>> + struct ioctl_evtchn_notify notify;
>> +
>> + prod = info->in_prod;
>> + if (prod - info->in_cons == XENKBD_RING_SIZE(info->in)) {
>> + errno = EAGAIN;
>> + return -1;
>> + }
>> +
>> + XENKBD_RING_REF(info->in, prod) = *event;
> Need a memory barrier.
Yes, just as in xenfb_fb_event() above.
>> + info->in_prod = prod + 1;
>> + notify.port = xenfb->kbd_port;
>> + return ioctl(xenfb->fd, IOCTL_EVTCHN_NOTIFY, ¬ify);
>> +}
>> +
>> +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);
>> + int n = snprintf(buffer, size, "%s/%s", domp, path);
>> + free(domp);
>> + if (n >= size)
>> + return NULL;
>> + return buffer;
>> +}
>> +
>> +static int xenfb_xs_scanf1(struct xs_handle *xsh, unsigned domid,
>> + const char *path, const char *fmt,
>> + void *dest)
> Not entirely convinced this is a good idea. You could do this
> properly; vscanf isn't hard to use.
Laziness on my part. Always try the most stupid thing that could
possibly solve the problem first ;)
I feel the proper solution belongs into a library anyway.
>> +{
>> + char buffer[1024];
>> + char *p;
>> + int ret;
>> +
>> + p = xenfb_path_in_dom(xsh, domid, path, buffer, sizeof(buffer));
>> + 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;
>> +}
>> +
>> +bool xenfb_set_domid(struct xenfb *xenfb_pub, int domid)
> Perhaps misnamed? This does a bit more than setting the domid.
Renamed to xenfb_attach_dom().
>> +{
> ...
>> + }
>> + xs_daemon_close(xsh);
>> + xsh = NULL;
>> +
>> + printf("%d, %d, %d\n", xenfb->fd, xenfb->fbdev_evtchn, domid);
> Umm?
Debugging leftover, removed.
>> +
>> + bind.remote_port = xenfb->fbdev_evtchn;
>> + bind.remote_domain = domid;
>> + xenfb->fbdev_port = ioctl(xenfb->fd, IOCTL_EVTCHN_BIND_INTERDOMAIN,
>> &bind);
> I'm goign to stop pointing out the xc_evtchn wrappers now.
I think I got them all :)
>> +
>> + xenfb->kbd_info = xc_map_foreign_range(xenfb->xc, domid, XC_PAGE_SIZE,
>> + PROT_READ | PROT_WRITE,
>> + xenfb->kbd_mfn);
>> + if (xenfb->kbd_info == NULL)
>> + goto error_fbinfo;
>> +
>> + while (!xenfb->fb_info->initialized) {
>> + struct timeval tv = { 0, 10000 };
>> + select(0, NULL, NULL, NULL, &tv);
>> + }
> If you ever need to go into this loop there's a problem somewhere.
> You shouldn't be able to connect things up until the shared area is
> initialised.
I think this is a leftover of the pre-xenstore way to connect things.
I'll look into getting rid of the initialized flag.
>> + 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;
>> +
>> + {
>> + union xenfb_in_event event;
> I know we do this in lots of other places, but it's really ugly.
> Can this move to the start of the function, please?
Sure.
>> +
>> + 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);
> Why was fbmfns in xenfb rather than a local?
I guess Anthony felt it logically belongs there. It could be made
local, along with a few other members. I can do that.
>> +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);
> Can pub.update ever be null?
Can't happen because both users (vncfb.c and sdlfb.c) fill it in
before they call xenfb_update(). But xenfb.c doesn't want to know
about that, so it checks.
>> +}
>> +
>> +static void xenfb_on_fb_event(struct xenfb_private *xenfb)
>> +{
>> + uint32_t prod, cons;
>> + struct xenfb_page *info = xenfb->fb_info;
>> +
>> + prod = info->out_prod;
>> + rmb(); /* ensure we see ring contents up to prod */
>> + for (cons = info->out_cons; cons != prod; cons++) {
>> + union xenfb_out_event *event = &XENFB_RING_REF(info->out, cons);
>> +
>> + switch (event->type) {
>> + case XENFB_TYPE_UPDATE:
>> + xenfb_update(xenfb, event->update.x, event->update.y,
>> event->update.width, event->update.height);
>> + break;
>> + }
>> + }
>> + /* FIXME do I need a wmb() here? */
> Maybe. Leaving it out means that the frontend can't reliably test for
> ring overflow.
I'm not sure I understand. Can you give an example of how things can
go wrong?
>> + info->out_cons = cons;
>> +}
>> +
>> +static void xenfb_on_kbd_event(struct xenfb_private *xenfb)
>> +{
>> + uint32_t prod, cons;
>> + struct xenkbd_info *info = xenfb->kbd_info;
>> +
>> + prod = info->out_prod;
>> + rmb(); /* ensure we see ring contents up to prod */
>> + for (cons = info->out_cons; cons != prod; cons++) {
>> + union xenkbd_out_event *event = &XENKBD_RING_REF(info->out,
>> cons);
>> +
>> + switch (event->type) {
>> + default:
>> + break;
>> + }
> Huh?
Skeleton code. There are no output events defined, yet.
>> + }
>> + /* FIXME do I need a wmb() here? */
> No, because the whole function is completely pointless.
It's a skeleton. I can remove it if it bothers you. If we keep it,
it should be as correct as we can make it.
Otherwise, same deal as for xenfb_fb_event().
>> + info->out_cons = cons;
>> +}
>> +
>> +int xenfb_on_incoming(struct xenfb *xenfb_pub)
>> +{
>> + struct xenfb_private *xenfb = (struct xenfb_private *)xenfb_pub;
>> + evtchn_port_t port;
>> +
>> + if (read(xenfb->fd, &port, sizeof(port)) != sizeof(port))
>> + return -1;
>> +
>> + if (port == xenfb->fbdev_port) {
>> + xenfb_on_fb_event(xenfb);
>> + } else if (port == xenfb->kbd_port) {
>> + xenfb_on_kbd_event(xenfb);
> Does this ever happen?
Not with the current frontend.
>> + }
>> +
>> + if (write(xenfb->fd, &port, sizeof(port)) != sizeof(port))
>> + return -1;
>> +
>> + return 0;
>> +}
>> +
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home