[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH V4] xl: create VFB for PV guest when VNC is specified



Ian Campbell writes ("Re: [PATCH V4] xl: create VFB for PV guest when VNC is 
specified"):
> And now George has gone away and left me holding the can, smart move on
> his part ;-)

I have also reviewed this patch again.

Firstly,

Acked-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>

Now, onto the RM questions.

> The potential risk is that this breaks the existing vfb syntax which
> works, or it breaks the hvm stuff. This is of particular concern because
> I don't think any of that is covered by osstest (except perhaps hvm
> console, but that might only be on some other error when osstest takes a
> screenshot), so the probability of finding it before release is reliant
> on manual testing/test days/user testing etc.
> Are you happy that all the existing options keep working?

Wei, could you tell us what configuration(s) you tested ?

> The patch is big enough that it isn't "obviously correct". . THe patch
> is textually large because it contain two refactorings
> (ARRAY_EXTEND_INIT and parse_top_level_vnc_options). ARRAY_EXTEND_INIT
> had pretty comprehensive review from me and Ian J as it was constructed.

More to the point, if ARRAY_EXTEND_INIT and the use site pattern were
broken it would totally break vfb entirely.  I think we'd be likely to
detect this well before release if it hadn't been spotted already.

> parse_top_... is really just code motion (although I'm a bit concerned
> that the hvm callsite has moved too). With my RM hat off and my
> maintainer hat on I've reviewed it and it looks good. (RM hat back on)

The autotester does use vnc, so it should detect any systematic bugs
introduced here.  So so long as the change is sufficiently systematic
I think we are fine.
 
> So, I think I'm a bit border line but slightly on the side of accepting
> it for 4.4 unless anyone has a counter opinion.

Right, I don't disagree, particularly if Wei's reply to my question
about testing is reassuring.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.