WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: Markus Armbruster <armbru@xxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] Paravirt framebuffer series [0/6]
From: aliguori@xxxxxxxxxxxxxxx
Date: Wed, 23 Aug 2006 10:10:15 -0500
Cc: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, Amos Waterland <apw@xxxxxxxxxx>, Jeremy Katz <katzj@xxxxxxxxxx>, Ian Pratt <ian.pratt@xxxxxxxxxxxxx>, Anthony Liguori <anthony@xxxxxxxxxxxxx>, Christian Limpach <chris@xxxxxxxxxxxxx>, Christian.Limpach@xxxxxxxxxxxx
Delivery-date: Thu, 24 Aug 2006 02:04:20 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <87ejv7h87b.fsf@xxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <1155935108.11663.48.camel@xxxxxxxxxxxxxx> <3d8eece20608210344t4d860d30sb832df34f156bd53@xxxxxxxxxxxxxx> <87ejv7h87b.fsf@xxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Internet Messaging Program (IMP) 3.2.8
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