domain_conf: skip boot order check of CD-ROM or floppy device

[libvirt] [PATCH] domain_conf: skip boot order check of CD-ROM or floppy device

Posted by Chen Hanxiao 6 years, 3 months ago

From: Chen Hanxiao <chenhanxiao@gmail.com>

If we insert or eject a CD-ROM/floppy device with a boot order,
we may get:
  unsupported configuration: boot order 2 is already used by another device

This check should be skipped in this case.

Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com>
---
 src/conf/domain_conf.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9a62bc472..885ab88d2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -26880,11 +26880,19 @@ virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def ATTRIBUTE_UNUSED,
 {
     virDomainDeviceInfoPtr newinfo = opaque;
 
+    int disk_device = device->data.disk->device;
     if (info->bootIndex == newinfo->bootIndex) {
-        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                       _("boot order %u is already used by another device"),
-                       newinfo->bootIndex);
-        return -1;
+        /* Skip check for insert or eject CD-ROM device */
+        if (disk_device == VIR_DOMAIN_DISK_DEVICE_FLOPPY ||
+            disk_device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
+            VIR_DEBUG("Skip boot index check for floppy or CDROM");
+            return 0;
+        } else {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                           _("boot order %u is already used by another device"),
+                           newinfo->bootIndex);
+            return -1;
+        }
     }
     return 0;
 }
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] domain_conf: skip boot order check of CD-ROM or floppy device

Posted by John Ferlan 6 years, 3 months ago


On 12/25/2017 06:21 AM, Chen Hanxiao wrote:
> From: Chen Hanxiao <chenhanxiao@gmail.com>
> 
> If we insert or eject a CD-ROM/floppy device with a boot order,
> we may get:
>   unsupported configuration: boot order 2 is already used by another device
> 
> This check should be skipped in this case.
> 
> Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com>
> ---
>  src/conf/domain_conf.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 

Is there a specific reason for your assertion?  Given what scenario?

virDomainDefCompatibleDevice is called from Attach and Update qemu/lxc
code currently.

I dunno, but if someone is trying to attach or update a floppy/cdrom and
wanted to use a boot index in use by something else, then I would think
inhibiting that is a good idea...

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 9a62bc472..885ab88d2 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -26880,11 +26880,19 @@ virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def ATTRIBUTE_UNUSED,
>  {
>      virDomainDeviceInfoPtr newinfo = opaque;
>  
> +    int disk_device = device->data.disk->device;

Typically the deref would be something like virDomainDiskDefPtr disk =
device->data.disk; and then "disk->device"

BTW: @device would then no longer be ATTRIBUTE_UNUSED.


John

>      if (info->bootIndex == newinfo->bootIndex) {
> -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                       _("boot order %u is already used by another device"),
> -                       newinfo->bootIndex);
> -        return -1;
> +        /* Skip check for insert or eject CD-ROM device */
> +        if (disk_device == VIR_DOMAIN_DISK_DEVICE_FLOPPY ||
> +            disk_device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
> +            VIR_DEBUG("Skip boot index check for floppy or CDROM");
> +            return 0;
> +        } else {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("boot order %u is already used by another device"),
> +                           newinfo->bootIndex);
> +            return -1;
> +        }
>      }
>      return 0;
>  }
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] domain_conf: skip boot order check of CD-ROM or floppy device

Posted by Ján Tomko 6 years, 3 months ago

On Fri, Jan 05, 2018 at 10:01:50AM -0500, John Ferlan wrote:
>
>
>On 12/25/2017 06:21 AM, Chen Hanxiao wrote:
>> From: Chen Hanxiao <chenhanxiao@gmail.com>
>>
>> If we insert or eject a CD-ROM/floppy device with a boot order,
>> we may get:
>>   unsupported configuration: boot order 2 is already used by another device
>>
>> This check should be skipped in this case.
>>
>> Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com>
>> ---
>>  src/conf/domain_conf.c | 16 ++++++++++++----
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>
>Is there a specific reason for your assertion?  Given what scenario?
>
>virDomainDefCompatibleDevice is called from Attach and Update qemu/lxc
>code currently.
>
>I dunno, but if someone is trying to attach or update a floppy/cdrom and
>wanted to use a boot index in use by something else, then I would think
>inhibiting that is a good idea...
>

Yes. Also, even though libvirt does not support cdrom/floppy hotplug,
assuming that this is an update based only on the disk type feels wrong.

Jan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] domain_conf: skip boot order check of CD-ROM or floppy device

Posted by Chen Hanxiao 6 years, 3 months ago


At 2018-01-05 23:01:50, "John Ferlan" <jferlan@redhat.com> wrote:
>
>
>On 12/25/2017 06:21 AM, Chen Hanxiao wrote:
>> From: Chen Hanxiao <chenhanxiao@gmail.com>
>> 
>> If we insert or eject a CD-ROM/floppy device with a boot order,
>> we may get:
>>   unsupported configuration: boot order 2 is already used by another device
>> 
>> This check should be skipped in this case.
>> 
>> Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com>
>> ---
>>  src/conf/domain_conf.c | 16 ++++++++++++----
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>> 
>
>Is there a specific reason for your assertion?  Given what scenario?
1) create a VM by virt-manger with a virtio disk and a cd-rom
2) set boot order of them
3) Try to eject a CD-rom device, such as installing an OS with multiple DVDs
    i. e: virsh change-media ...
4) got:
  unsupported configuration: boot order 2 is already used by another device.

>
>virDomainDefCompatibleDevice is called from Attach and Update qemu/lxc
>code currently.
>
>I dunno, but if someone is trying to attach or update a floppy/cdrom and
>wanted to use a boot index in use by something else, then I would think
>inhibiting that is a good idea...

We updated the source file of floppy/cdrom, which is not a hotplug operation,
so the boot order check should be skipped.

Regards,
- Chen

>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 9a62bc472..885ab88d2 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -26880,11 +26880,19 @@ virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def ATTRIBUTE_UNUSED,
>>  {
>>      virDomainDeviceInfoPtr newinfo = opaque;
>>  
>> +    int disk_device = device->data.disk->device;
>
>Typically the deref would be something like virDomainDiskDefPtr disk =
>device->data.disk; and then "disk->device"
>
>BTW: @device would then no longer be ATTRIBUTE_UNUSED.
>
>
>John
>
>>      if (info->bootIndex == newinfo->bootIndex) {
>> -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> -                       _("boot order %u is already used by another device"),
>> -                       newinfo->bootIndex);
>> -        return -1;
>> +        /* Skip check for insert or eject CD-ROM device */
>> +        if (disk_device == VIR_DOMAIN_DISK_DEVICE_FLOPPY ||
>> +            disk_device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
>> +            VIR_DEBUG("Skip boot index check for floppy or CDROM");
>> +            return 0;
>> +        } else {
>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +                           _("boot order %u is already used by another device"),
>> +                           newinfo->bootIndex);
>> +            return -1;
>> +        }
>>      }
>>      return 0;
>>  }
>> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list