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

[George Dunlap <dunlapg@xxxxxxxxx>]

On Fri, Apr 8, 2016 at 3:59 PM, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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@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).
>
> 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

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