Re: [Xen-devel] [PATCH] Paravirt framebuffer series [0/6]

[aliguori@xxxxxxxxxxxxxxx]

Quoting Markus Armbruster <armbru@xxxxxxxxxx>:

> "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.

That's just a macro I use for finding where a problem is when things crash in
weird ways.  It can be removed as there shouldn't be any BUG()s in the
submitted patch.

> >> +
> >> +  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.

LibVNCServer sucks.  This is one of the reasons why.  It pulled a bunch of code
from Xvnc.  Xvnc is completely synchronous so the solution was to stick it in a
thread.  Of course, there's no locks anywhere.

This is one of the many reasons I wrote a new VNC implementation for QEMU. 
Borrying that code for the backend would be a good thing.

> > 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.

With XenBus, the loop is no longer needed.  It was needed before.

> 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.

The idea was for xenfb_* to be a library for writing various front-ends.  I
still think that's a good idea.  Eventually, we'll have gtkfb, qtfb, etc. 
Being a bit more defensive can never hurt.

> > No, because the whole function is completely pointless.

No, it's not.  If you have queues, and you buffer them properly, you absolutely
have to make sure both sides flush the queues regularly.

> 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.

If you remove it, you risk causing breakages with future guests.

Regards,

Anthony Liguori



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