| Message ID | 1460051129-20817-4-git-send-email-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 07, 2016 at 07:45:28PM +0200, Roger Pau Monne wrote: > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Wei Liu <wei.liu2@citrix.com> > --- > 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? Wei. > + rc = ERROR_FAIL; > + goto out; > + } > + > disks = libxl_device_disk_list(ctx, domid, &num); > for (i = 0; i < num; i++) { > if (disks[i].is_cdrom && !strcmp(disk->vdev, disks[i].vdev)) > -- > 2.6.4 (Apple Git-63) >
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@citrix.com> > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > Cc: Wei Liu <wei.liu2@citrix.com> > > --- > > 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). Roger.
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@citrix.com> > > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > > Cc: Wei Liu <wei.liu2@citrix.com> > > > --- > > > 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... Wei. > Roger.
Wei Liu writes ("Re: [PATCH 3/4] libxl: only allow guests with a device model to use cd-{eject/insert}"):
> On Fri, Apr 08, 2016 at 04:17:55PM +0200, Roger Pau Monné wrote:
> > 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...
I think an "empty" cdrom drive is represented to the guest as a
0-length block device.
There seem to be varying views as to whether insert/remove is supposed
to involve a vbd state teardown/restart, or just a capacity change.
From what I think Roger said, Linux blkback doesn't spot changes to
params (obviously - it looks for physdev) and AIUI needs to be
prodded.
Ian.
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@citrix.com> > > > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > > > Cc: Wei Liu <wei.liu2@citrix.com> > > > > --- > > > > 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; } Roger.
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@citrix.com> > > > > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > > > > Cc: Wei Liu <wei.liu2@citrix.com> > > > > > --- > > > > > 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@citrix.com> Wei. > > Roger.
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@citrix.com> > > > > > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > > > > > Cc: Wei Liu <wei.liu2@citrix.com> > > > > > > --- > > > > > > 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@citrix.com> 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). Roger.
On 08/04/16 15:51, 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@citrix.com> >>>>>>> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >>>>>>> Cc: Wei Liu <wei.liu2@citrix.com> >>>>>>> --- >>>>>>> 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@citrix.com> > 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). In principle, PV guests could have a device model providing emulated CD support. FWIW, I prefer the "device model" wording. ~Andrew
On Fri, Apr 8, 2016 at 3:59 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 08/04/16 15:51, 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@citrix.com> >>>>>>>> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >>>>>>>> Cc: Wei Liu <wei.liu2@citrix.com> >>>>>>>> --- >>>>>>>> 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@citrix.com> >> 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). > > In principle, PV guests could have a device model providing emulated CD > support. > > FWIW, I prefer the "device model" wording. Since we're painting bike sheds, I'll toss in my preferred color. This message is for the user, not for developers, and since in other areas we talk about "PV" domains or "HVM" domains (e.g., builder="hvm" in the config file), I think "requires an HVM domain" is probably going to convey the most meaning. I don't think we support PV guests having emulated devices yet; when we do then the message will be inaccurate, but so will the check, so they can both go away at once. :-) -George
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@citrix.com> > > > > > > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > > > > > > Cc: Wei Liu <wei.liu2@citrix.com> > > > > > > > --- > > > > > > > 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@citrix.com> > > 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.
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"); + rc = ERROR_FAIL; + goto out; + } + disks = libxl_device_disk_list(ctx, domid, &num); for (i = 0; i < num; i++) { if (disks[i].is_cdrom && !strcmp(disk->vdev, disks[i].vdev))
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> --- tools/libxl/libxl.c | 6 ++++++ 1 file changed, 6 insertions(+)