From mboxrd@z Thu Jan  1 00:00:00 1970
From: Samuel Thibault <samuel.thibault@eu.citrix.com>
Subject: Re: [PATCH] xenfb: make restartable
Date: Fri, 15 Aug 2008 23:23:46 +0100
Message-ID: <20080815222346.GE5198@implementation>
References: <48A40B17.2090701@redhat.com>
	<20080814111520.GH4590@implementation.uk.xensource.com>
	<48A4232B.6040500@redhat.com>
	<20080814122931.GL4590@implementation.uk.xensource.com>
	<48A43EAD.7040701@redhat.com>
	<20080814142849.GS4590@implementation.uk.xensource.com>
	<48A44967.4060401@redhat.com>
	<20080814151359.GT4590@implementation.uk.xensource.com>
	<20080814180251.GD4590@implementation.uk.xensource.com>
	<m3bpzubggl.fsf@crossbow.pond.sub.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: quoted-printable
Return-path: <xen-devel-bounces@lists.xensource.com>
Content-Disposition: inline
In-Reply-To: <m3bpzubggl.fsf@crossbow.pond.sub.org>
List-Unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xensource.com>
List-Help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-Subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
Sender: xen-devel-bounces@lists.xensource.com
Errors-To: xen-devel-bounces@lists.xensource.com
To: Markus Armbruster <armbru@redhat.com>
Cc: Xen Development Mailing List <xen-devel@lists.xensource.com>, Gerd Hoffmann <kraxel@redhat.com>
List-Id: xen-devel@lists.xenproject.org

Markus Armbruster, le Fri 15 Aug 2008 08:39:06 -0400, a =E9crit :
> > +	if (!strcmp(dev->devicetype, "vfb")) {
> > +	    if (dev->xenfb->pixels) {
> > +		    munmap(dev->xenfb->pixels, dev->xenfb->fb_len);
> > +		    dev->xenfb->pixels =3D NULL;
> > +	    }
> > +	}
> >  }
> > =20
> > =20
>=20
> The check for vfb is ugly.  The only alternative I can see is fb =3D=3D
> dev->xenfb->fb.  Matter of taste.

Hum, that was what I had written previously actually.  I don't see why
it is "ugly", in that pixels is a device resource.  It should probably
be in xenfb->fb actually.

> > @@ -653,6 +656,16 @@ static int xenfb_on_state_change(struct=20
> >  		   not much point in us continuing. */
> >  		return -1;
> >  	case XenbusStateInitialising:
> > +		if (dev->state !=3D XenbusStateClosed)
> > +		    /* Do not let a domain make us skip the closing state */
> > +		    return 0;
>=20
> Why does this work?

It's meant to not work.  If the frontend is not playing well (going
through the closing state), then ignore what it's doing until it plays
well. An alternative could be to just shut down.  In any case, we don't
want to let it drive us to remapping something else without unmapping
the previous fb.

> > +		xenfb_switch_state(dev, XenbusStateInitWait);
> > +		xs_unwatch(dev->xenfb->xsh, dev->otherend, "");
> > +		if (!strcmp(dev->devicetype, "vkbd")) {
> > +		    fprintf(stderr, "FB: Waiting for KBD backend creation\n");
> > +		    xenfb_wait_for_frontend(&dev->xenfb->kbd, xenfb_frontend_initi=
alized_kbd);
> > +		}
> > +		break;
>=20
> Man, I hate this state machine...=20

I agree :)

> Gerd's new one is much clearer (I asked for that).

Then since 3.3 is about to be released, we'd probably have this change
with it.

Samuel