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

[Andrea Bolognani]

On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:

[...]

> +          <dt><code>model</code></dt>
> +            <dd>
> +            Indicates the emulated device model of the disk. Typically
> +            this is indicated solely by the <code>bus</code> property but
> +            for <code>bus</code> "virtio" the model can be specified further
> +            with "virtio-transitional", "virtio-non-transitional", or
> +            "virtio" which matches the old behavior. These setting are

s/setting/settings/

[...]

> +++ b/docs/schemas/domaincommon.rng
> @@ -1506,6 +1506,14 @@
>            </interleave>
>          </group>
>        </choice>
> +      <optional>
> +        <attribute name="model">
> +          <choice>

You forgot to add <value>virtio</value> here.

> +            <value>virtio-transitional</value>
> +            <value>virtio-non-transitional</value>
> +          </choice>

[...]

> @@ -24311,6 +24344,11 @@ virDomainDiskDefFormat(virBufferPtr buf,
>      virBufferAsprintf(buf,
>                        "<disk type='%s' device='%s'",
>                        type, device);

Empty line here, please.

> +    if (def->model) {
> +        virBufferAsprintf(buf, " model='%s'",
> +                          virDomainDiskModelTypeToString(def->model));
> +    }

[...]

> +++ b/tests/qemuxml2xmltest.c
> @@ -1265,6 +1265,17 @@ mymain(void)
>      DO_TEST("riscv64-virt",
>              QEMU_CAPS_DEVICE_VIRTIO_MMIO);
>  
> +    DO_TEST("virtio-transitional",
> +            QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
> +            QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE,
> +            QEMU_CAPS_DEVICE_PCIE_ROOT_PORT,
> +            QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY);
> +    DO_TEST("virtio-non-transitional",
> +            QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
> +            QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE,
> +            QEMU_CAPS_DEVICE_PCIE_ROOT_PORT,
> +            QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY);

So you got rid of the macro from RFC's 2/6 completely? Despite the
code duplication issue I've pointed out at the time, I'd still
rather see that macro being used, after renaming it, than yet
another hardcoded list of capabilities...

Either way, with the schema and the other nits fixed,

  Reviewed-by: Andrea Bolognani <abologna at redhat.com>

-- 
Andrea Bolognani / Red Hat / Virtualization