Re: [PATCH v4 4/4] libxl: fix cd-eject

[Ian Campbell <ian.campbell@citrix.com>]

On Wed, 2016-02-17 at 12:20 +0100, Roger Pau Monné wrote:
> El 16/2/16 a les 18:58, Ian Jackson ha escrit:
> > Roger Pau Monne writes ("[PATCH v4 4/4] libxl: fix cd-eject"):
> > > Current libxl__device_disk_set_backend implementation tried to guess
> > > the
> > > backend of devices with format LIBXL_DISK_FORMAT_EMPTY, which is of
> > > course
> > > doomed to fail since the disk is empty. Instead just return early
> > > from the
> > > function if an empty disk is detected.
> > > 
> > > This fixes cd ejection.
> > 
> > DYK when this was broken ?  Or, to put it another way, how did this
> > ever work ?
> > 
> > ...looking at the code...
> > 
> > AFAICT disk_try_backend should succeed for both LIBXL_DISK_BACKEND_PHY
> > and LIBXL_DISK_BACKEND_QDISK.  So even before your patch:
> > 
> > >          }
> > > -        memset(&a.stab, 0, sizeof(a.stab));
> > > +        /* Disk is empty, so it's useless to try to guess the
> > > backend type. */
> > > +        return 0;
> > >      } else if ((disk->backend == LIBXL_DISK_BACKEND_UNKNOWN ||
> > 
> > libxl__device_disk_set_backend should work.
> 
> I've been looking at the code and I cannot really understand how this is
> supposed to work before, none of the recent changes seem to have broken
> it AFAICT, and the issue has been there for a long time (~1year), which
> IMHO makes no sense, so I'm quite sure I'm missing something.

cd-insert/eject seems to me to either be perpetually broken or is
incredibly prone to regressing (i..e this keeps coming up).

Getting a test step into osstest which used cd-insert/eject would be a good
idea IMHO, either a deliberate test step which checks it works or trying to
use it as a matter of course during e.g. guest install.

Ian.

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