[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 3/6] conf: Add <disk model='virtio-{non-}transitional'/>



On Sun, 2019-01-13 at 18:12 -0500, Cole Robinson wrote:
[...]
> @@ -5431,6 +5436,16 @@ virDomainDiskDefValidate(const virDomainDiskDef *disk)
>          return -1;
>      }
>  
> +    if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO &&
> +        (disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL ||
> +         disk->model == VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL)) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("disk model '%s' only supported for bus '%s'"),
> +                       virDomainDiskModelTypeToString(disk->model),
> +                       virDomainDiskBusTypeToString(disk->bus));

s/only supported/not supported/

I also wonder if more thorough validation would be a good idea: this
only ensures that VIR_DOMAIN_DISK_MODEL_VIRTIO_* can't be used with
other bus types, but if later on we add more VIR_DOMAIN_DISK_MODEL_*
we won't be protected against those being use with
VIR_DOMAIN_DISK_BUS_VIRTIO...

[...]
> +typedef enum {
> +    VIR_DOMAIN_DISK_MODEL_DEFAULT = 0,
> +    VIR_DOMAIN_DISK_MODEL_VIRTIO_TRANSITIONAL,
> +    VIR_DOMAIN_DISK_MODEL_VIRTIO_NON_TRANSITIONAL,
> +
> +    VIR_DOMAIN_DISK_MODEL_LAST
> +} virDomainDiskModel;

I think we want to have VIR_DOMAIN_DISK_MODEL_VIRTIO as well, for
consistency's sake if nothing else.

For all other devices, model='virtio' will be (or already is) a
perfectly valid configuration, so not accepting it for disks would
violate the principle of least surprise.

[...]
> +++ b/tests/qemuxml2argvdata/virtio-non-transitional.xml
> @@ -0,0 +1,26 @@
> +<domain type='qemu'>
> +  <name>QEMUGuest1</name>
> +  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> +  <memory unit='KiB'>219136</memory>
> +  <currentMemory unit='KiB'>219136</currentMemory>
> +  <vcpu placement='static'>1</vcpu>
> +  <os>
> +    <type arch='x86_64' machine='q35'>hvm</type>
> +    <boot dev='hd'/>
> +  </os>
> +  <clock offset='utc'/>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>destroy</on_crash>
> +  <devices>
> +    <disk type='block' device='disk' model='virtio-non-transitional'>
> +      <driver name='qemu' type='raw'/>
> +      <source dev='/dev/HostVG/QEMUGuest1'/>
> +      <target dev='vda' bus='virtio'/>
> +    </disk>
> +    <controller type='usb' index='0' model='none'/>
> +    <input type='mouse' bus='ps2'/>
> +    <input type='keyboard' bus='ps2'/>
> +    <memballoon model='none'/>
> +  </devices>
> +</domain>

You can strip this file further by removing the <currentMemory>,
<os><boot>, <clock>, <on_*> and <input> elements at least.

-- 
Andrea Bolognani / Red Hat / Virtualization


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]