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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>,
	"Wei Liu" <wei.liu2@citrix.com>
Cc: xen-devel@lists.xenproject.org, Ian Jackson <ian.jackson@eu.citrix.com>
Subject: Re: [PATCH 3/4] libxl: only allow guests with a device model to use cd-{eject/insert}
Date: Fri, 8 Apr 2016 15:59:11 +0100	[thread overview]
Message-ID: <5707C73F.9090905@citrix.com> (raw)
In-Reply-To: <alpine.OSX.2.20.1604081649020.18723@mac>

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

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