Re: [Xen-devel] [PATCH] xenfb: make restartable

[Markus Armbruster <armbru@xxxxxxxxxx>]

Samuel Thibault <samuel.thibault@xxxxxxxxxxxxx> writes:

> Markus Armbruster, le Fri 15 Aug 2008 08:39:06 -0400, a écrit :
>> > +  if (!strcmp(dev->devicetype, "vfb")) {
>> > +      if (dev->xenfb->pixels) {
>> > +              munmap(dev->xenfb->pixels, dev->xenfb->fb_len);
>> > +              dev->xenfb->pixels = NULL;
>> > +      }
>> > +  }
>> >  }
>> >  
>> >  
>> 
>> The check for vfb is ugly.  The only alternative I can see is fb ==
>> 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.

struct xenfb_device captures stuff that both devices have.

By "is ugly" I meant to say that checking for vfb by testing its
devicetype string looks ugly to me.  But fb == dev->xenfb->fb isn't
exactly the acme of elegance either.

>> > @@ -653,6 +656,16 @@ static int xenfb_on_state_change(struct 
>> >               not much point in us continuing. */
>> >            return -1;
>> >    case XenbusStateInitialising:
>> > +          if (dev->state != XenbusStateClosed)
>> > +              /* Do not let a domain make us skip the closing state */
>> > +              return 0;
>> 
>> 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.

We need to make up our mind how to handle invalid state transitions.

Is silently ignoring guest misbehavior smart?  I'd rather log it.

[...]

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