Re: [Xen-devel] [PATCH 3/4] libxl: only allow guests with a device model to use cd-{eject/insert}

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Apr 08, 2016 at 04:51:05PM +0200, Roger Pau Monné wrote:
> On Fri, 8 Apr 2016, Wei Liu wrote:
> > On Fri, Apr 08, 2016 at 04:35:22PM +0200, Roger Pau Monné wrote:
> > > On Fri, 8 Apr 2016, Wei Liu wrote:
> > > > On Fri, Apr 08, 2016 at 04:17:55PM +0200, Roger Pau Monné wrote:
> > > > > On Fri, 8 Apr 2016, Wei Liu wrote:
> > > > > > On Thu, Apr 07, 2016 at 07:45:28PM +0200, Roger Pau Monne wrote:
> > > > > > > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > > > > > > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > > > > > > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> > > > > > > ---
> > > > > > >  tools/libxl/libxl.c | 6 ++++++
> > > > > > >  1 file changed, 6 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > > > > > > index 9d785a4..232e2c1 100644
> > > > > > > --- a/tools/libxl/libxl.c
> > > > > > > +++ b/tools/libxl/libxl.c
> > > > > > > @@ -2909,6 +2909,12 @@ int libxl_cdrom_insert(libxl_ctx *ctx, 
> > > > > > > uint32_t domid, libxl_device_disk *disk,
> > > > > > >          goto out;
> > > > > > >      }
> > > > > > >  
> > > > > > > +    if (dm_ver == LIBXL_DEVICE_MODEL_VERSION_NONE) {
> > > > > > > +        LOG(ERROR, "HVM guests without a device model cannot use 
> > > > > > > cd-insert");
> > > > > > 
> > > > > > It's not specific to HVM, right?
> > > > > 
> > > > > It's specific to guests with a device model (those are the only ones 
> > > > > that 
> > > > > can have an emulated CDROM device), and the only guests that can have 
> > > > > a 
> > > > > device model are HVM (although not all of them).
> > > > > 
> > > > 
> > > > Remind me again: how does PV guest CDROM work? ISTR it will also go
> > > > through this function? My memory is vague though...
> > > 
> > > No, AFAICT PV guests use block-attach/detach in order to manage CDROMs, 
> > > the usage of the cd-eject/insert commands is already limited to HVM 
> > > guests, my check is only further limiting it to HVM guests with a device 
> > > model (so it's not used for PVH guests).
> > > 
> > > A little above of the check that I've added:
> > > 
> > >     libxl_domain_type type = libxl__domain_type(gc, domid);
> > >     if (type == LIBXL_DOMAIN_TYPE_INVALID) {
> > >         rc = ERROR_FAIL;
> > >         goto out;
> > >     }
> > >     if (type != LIBXL_DOMAIN_TYPE_HVM) {
> > >         LOG(ERROR, "cdrom-insert requires an HVM domain");
> > >         rc = ERROR_INVAL;
> > >         goto out;
> > >     }
> > 
> > Right. Thanks for checking.  I realise quibbling over an error message
> > seems counter-productive.
> > 
> > Acked-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> 
> Thanks, I don't mind changing the message to "Guests without a device 
> model cannot use cd-insert", I just felt that adding HVM would make it 
> clearer, but it might not. If you or the committer prefer to change the 
> message that's fine for me (and I can send a new version if requested).
> 

I would be probably easier for you to get a branch for Ian to commit.

Wei.

> Roger.


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