Re: [PATCH v2 6/7] qemu: Refactor virtio-input capabilities checks

[Ján Tomko <jtomko@xxxxxxxxxx>]

On Thu, Sep 06, 2018 at 02:22:18PM +0200, Andrea Bolognani wrote:
> The checks and error messages are mostly the same across
> all virtio-input devices, so we can avoid having multiple
> copies of the same code.
>
> Signed-off-by: Andrea Bolognani <abologna@xxxxxxxxxx>
> ---
> src/qemu/qemu_domain.c | 51 +++++++++++++++++++++---------------------
> 1 file changed, 25 insertions(+), 26 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 05e90c3615..cd4e78993f 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5732,43 +5732,33 @@ qemuDomainDeviceDefValidateInput(const virDomainInputDef *input,
>                                  const virDomainDef *def ATTRIBUTE_UNUSED,
>                                  virQEMUCapsPtr qemuCaps)
> {
> +    const char *baseName;
> +    int cap;
> +    int ccwCap;
> +
>     if (input->bus != VIR_DOMAIN_INPUT_BUS_VIRTIO)
>         return 0;
>
>     switch ((virDomainInputType)input->type) {
>     case VIR_DOMAIN_INPUT_TYPE_MOUSE:
> -        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_MOUSE) ||
> -            (input->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW &&
> -             !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW))) {
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                           _("virtio-mouse is not supported by this QEMU binary"));
> -            return -1;
> -        }
> +        baseName = "virtio-mouse";
> +        cap = QEMU_CAPS_VIRTIO_MOUSE;
> +        ccwCap = QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW;
>         break;
>     case VIR_DOMAIN_INPUT_TYPE_TABLET:
> -        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_TABLET) ||
> -            (input->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW &&
> -             !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW))) {
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                           _("virtio-tablet is not supported by this QEMU binary"));
> -            return -1;
> -        }
> +        baseName = "virtio-tablet";
> +        cap = QEMU_CAPS_VIRTIO_TABLET;
> +        ccwCap = QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW;
>         break;
>     case VIR_DOMAIN_INPUT_TYPE_KBD:
> -        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_KEYBOARD) ||
> -            (input->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW &&
> -             !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_KEYBOARD_CCW))) {
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                           _("virtio-keyboard is not supported by this QEMU binary"));
> -            return -1;
> -        }
> +        baseName = "virtio-keyboard";
> +        cap = QEMU_CAPS_VIRTIO_KEYBOARD;
> +        ccwCap = QEMU_CAPS_DEVICE_VIRTIO_KEYBOARD_CCW;
>         break;
>     case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH:
> -        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_INPUT_HOST)) {
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                           _("virtio-input-host is not supported by this QEMU binary"));
> -            return -1;
> -        }
> +        baseName = "virtio-input-host";
> +        cap = QEMU_CAPS_VIRTIO_INPUT_HOST;
> +        ccwCap = QEMU_CAPS_VIRTIO_INPUT_HOST;

This is incorrect.

I see there is a 'virtio-input-host-device' for the 's390-ccw-virtio'
machine, but it is the only device prefixed with 'virtio-' that does not
have a -ccw counterpart.

Maybe (ab)use 'QEMU_CAPS_LAST' here as a capability that is never set?

>         break;
>     case VIR_DOMAIN_INPUT_TYPE_LAST:
>     default:
> @@ -5777,6 +5767,15 @@ qemuDomainDeviceDefValidateInput(const virDomainInputDef *input,
>         return -1;
>     }
>
> +    if (!virQEMUCapsGet(qemuCaps, cap) ||
> +        (input->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW &&
> +         !virQEMUCapsGet(qemuCaps, ccwCap))) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("%s is not supported by this QEMU binary"),

Consider '%s'

> +                       baseName);
> +        return -1;
> +    }

Also, the data itself might look nicer in an array:

struct qemuDomainVirtioInputCapsType {
   const char *baseName;
   int cap;
   int ccwCap;
};

static const struct qemuDomainVirtioInputCapsType qemuDomainVirtioInputCaps[] = {
   [VIR_DOMAIN_INPUT_TYPE_MOUSE] = {
       "virtio-mouse",
       QEMU_CAPS_VIRTIO_MOUSE,
       QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW
   },
   [VIR_DOMAIN_INPUT_TYPE_TABLET] = {
       "virtio-tablet",
       QEMU_CAPS_VIRTIO_TABLET,
       QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW,
   },
   [VIR_DOMAIN_INPUT_TYPE_KBD] = {
       "virtio-keyboard",
       QEMU_CAPS_VIRTIO_KEYBOARD,
       QEMU_CAPS_DEVICE_VIRTIO_KEYBOARD_CCW
   },
   [VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH] = {
       "virtio-input-host",
       QEMU_CAPS_VIRTIO_INPUT_HOST,
       QEMU_CAPS_LAST
   },
};
verify(ARRAY_CARDINALITY(qemuDomainVirtioInputCaps) == VIR_DOMAIN_INPUT_TYPE_LAST);


With missing virtio-input-host-ccw addressed:

Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx>

Jano

Attachment: signature.asc
Description: Digital signature

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