[libvirt] [PATCH v2 0/7] qemu: Unify generation of command line for virtio devices
Or, how to turn a single patch into a seven patches series using the astounding power of review comments! Andrea Bolognani (7): tests: Add missing virtio-input capabilities qemu: Check type range for virtio-input devices qemu: Remove duplicated option formatting for virtio devices qemu: Always format iothread for virtio-blk qemu: Check for virtio-input capabilites at validate time qemu: Refactor virtio-input capabilities checks qemu: Unify generation of command line for virtio devices src/qemu/qemu_command.c | 227 +++++++++++++++++----------------------- src/qemu/qemu_domain.c | 58 +++++++++- tests/qemuxml2xmltest.c | 24 ++++- 3 files changed, 173 insertions(+), 136 deletions(-) -- 2.17.1
A few qemuxml2xml tests for virtio-input devices are missing the capabilities used for the corresponding qemuxml2argv tests: this wasn't a problem until now because capabilities were only checked at command line generation time, but we're going to change that later. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemuxml2xmltest.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 47da7a7201..caf79625d4 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1098,12 +1098,30 @@ mymain(void) DO_TEST("video-virtio-gpu-virgl", NONE); DO_TEST("video-virtio-gpu-spice-gl", NONE); DO_TEST("video-virtio-gpu-sdl-gl", NONE); - DO_TEST("virtio-input", NONE); - DO_TEST("virtio-input-passthrough", NONE); + + DO_TEST("virtio-input", + QEMU_CAPS_VIRTIO_KEYBOARD, + QEMU_CAPS_VIRTIO_MOUSE, + QEMU_CAPS_VIRTIO_TABLET); + DO_TEST("virtio-input-passthrough", + QEMU_CAPS_VIRTIO_INPUT_HOST); DO_TEST("memorybacking-set", NONE); DO_TEST("memorybacking-unset", NONE); - DO_TEST("virtio-options", QEMU_CAPS_VIRTIO_SCSI); + + DO_TEST("virtio-options", + QEMU_CAPS_VIRTIO_SCSI, + QEMU_CAPS_VIRTIO_KEYBOARD, + QEMU_CAPS_VIRTIO_MOUSE, + QEMU_CAPS_VIRTIO_TABLET, + QEMU_CAPS_VIRTIO_INPUT_HOST, + QEMU_CAPS_DEVICE_VIRTIO_GPU, + QEMU_CAPS_VIRTIO_GPU_VIRGL, + QEMU_CAPS_DEVICE_VIRTIO_RNG, + QEMU_CAPS_OBJECT_RNG_RANDOM, + QEMU_CAPS_DEVICE_VIDEO_PRIMARY, + QEMU_CAPS_VIRTIO_PCI_IOMMU_PLATFORM, + QEMU_CAPS_VIRTIO_PCI_ATS); virObjectUnref(cfg); -- 2.17.1
On Thu, Sep 06, 2018 at 02:22:13PM +0200, Andrea Bolognani wrote:
A few qemuxml2xml tests for virtio-input devices are missing the capabilities used for the corresponding qemuxml2argv tests: this wasn't a problem until now because capabilities were only checked at command line generation time, but we're going to change that later.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemuxml2xmltest.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 877269f52e..72d8bb4cf6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4096,7 +4096,10 @@ qemuBuildVirtioInputDevStr(const virDomainDef *def, virQEMUBuildBufferEscapeComma(&buf, dev->source.evdev); break; case VIR_DOMAIN_INPUT_TYPE_LAST: - break; + default: + virReportEnumRangeError(virDomainInputType, + dev->type); + goto error; } if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) -- 2.17.1
On Thu, Sep 06, 2018 at 02:22:14PM +0200, Andrea Bolognani wrote:
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 877269f52e..72d8bb4cf6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4096,7 +4096,10 @@ qemuBuildVirtioInputDevStr(const virDomainDef *def, virQEMUBuildBufferEscapeComma(&buf, dev->source.evdev); break; case VIR_DOMAIN_INPUT_TYPE_LAST: - break; + default: + virReportEnumRangeError(virDomainInputType, + dev->type);
This would comfortably fit on one line. Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
There are several functions where we pointlessly duplicate parts of the format string and pass the same arguments: refactor them so that the common parts are formatted separately from the variable parts. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 42 ++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 72d8bb4cf6..b283f7ca83 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2004,8 +2004,6 @@ qemuBuildDiskDeviceStr(const virDomainDef *def, case VIR_DOMAIN_DISK_BUS_VIRTIO: if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { virBufferAddLit(&opt, "virtio-blk-ccw"); - if (disk->iothread) - virBufferAsprintf(&opt, ",iothread=iothread%u", disk->iothread); } else if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) { virBufferAddLit(&opt, "virtio-blk-s390"); @@ -2014,9 +2012,14 @@ qemuBuildDiskDeviceStr(const virDomainDef *def, virBufferAddLit(&opt, "virtio-blk-device"); } else { virBufferAddLit(&opt, "virtio-blk-pci"); - if (disk->iothread) - virBufferAsprintf(&opt, ",iothread=iothread%u", disk->iothread); } + + if (disk->iothread && + (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW || + disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) { + virBufferAsprintf(&opt, ",iothread=iothread%u", disk->iothread); + } + qemuBuildIoEventFdStr(&opt, disk->ioeventfd, qemuCaps); if (disk->event_idx && virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_EVENT_IDX)) { @@ -4064,7 +4067,7 @@ qemuBuildVirtioInputDevStr(const virDomainDef *def, _("virtio-mouse is not supported by this QEMU binary")); goto error; } - virBufferAsprintf(&buf, "virtio-mouse%s,id=%s", suffix, dev->info.alias); + virBufferAsprintf(&buf, "virtio-mouse%s", suffix); break; case VIR_DOMAIN_INPUT_TYPE_TABLET: if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_TABLET) || @@ -4074,7 +4077,7 @@ qemuBuildVirtioInputDevStr(const virDomainDef *def, _("virtio-tablet is not supported by this QEMU binary")); goto error; } - virBufferAsprintf(&buf, "virtio-tablet%s,id=%s", suffix, dev->info.alias); + virBufferAsprintf(&buf, "virtio-tablet%s", suffix); break; case VIR_DOMAIN_INPUT_TYPE_KBD: if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_KEYBOARD) || @@ -4084,7 +4087,7 @@ qemuBuildVirtioInputDevStr(const virDomainDef *def, _("virtio-keyboard is not supported by this QEMU binary")); goto error; } - virBufferAsprintf(&buf, "virtio-keyboard%s,id=%s", suffix, dev->info.alias); + virBufferAsprintf(&buf, "virtio-keyboard%s", suffix); break; case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH: if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_INPUT_HOST)) { @@ -4092,8 +4095,7 @@ qemuBuildVirtioInputDevStr(const virDomainDef *def, _("virtio-input-host is not supported by this QEMU binary")); goto error; } - virBufferAsprintf(&buf, "virtio-input-host%s,id=%s,evdev=", suffix, dev->info.alias); - virQEMUBuildBufferEscapeComma(&buf, dev->source.evdev); + virBufferAsprintf(&buf, "virtio-input-host%s", suffix); break; case VIR_DOMAIN_INPUT_TYPE_LAST: default: @@ -4102,6 +4104,13 @@ qemuBuildVirtioInputDevStr(const virDomainDef *def, goto error; } + virBufferAsprintf(&buf, ",id=%s", dev->info.alias); + + if (dev->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) { + virBufferAddLit(&buf, ",evdev="); + virQEMUBuildBufferEscapeComma(&buf, dev->source.evdev); + } + if (qemuBuildDeviceAddressStr(&buf, def, &dev->info, qemuCaps) < 0) goto error; @@ -5834,17 +5843,16 @@ qemuBuildRNGDevStr(const virDomainDef *def, goto error; if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) - virBufferAsprintf(&buf, "virtio-rng-ccw,rng=obj%s,id=%s", - dev->info.alias, dev->info.alias); + virBufferAddLit(&buf, "virtio-rng-ccw"); else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) - virBufferAsprintf(&buf, "virtio-rng-s390,rng=obj%s,id=%s", - dev->info.alias, dev->info.alias); + virBufferAddLit(&buf, "virtio-rng-s390"); else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) - virBufferAsprintf(&buf, "virtio-rng-device,rng=obj%s,id=%s", - dev->info.alias, dev->info.alias); + virBufferAddLit(&buf, "virtio-rng-device"); else - virBufferAsprintf(&buf, "virtio-rng-pci,rng=obj%s,id=%s", - dev->info.alias, dev->info.alias); + virBufferAddLit(&buf, "virtio-rng-pci"); + + virBufferAsprintf(&buf, ",rng=obj%s,id=%s", + dev->info.alias, dev->info.alias); if (dev->rate > 0) { virBufferAsprintf(&buf, ",max-bytes=%u", dev->rate); -- 2.17.1
On Thu, Sep 06, 2018 at 02:22:15PM +0200, Andrea Bolognani wrote:
There are several functions where we pointlessly duplicate parts of the format string and pass the same arguments: refactor them so that the common parts are formatted separately from the variable parts.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 42 ++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
So far we've only formatted it for virtio-blk-pci and virtio-blk-ccw, but other virtio-blk devices also support the corresponding option; moreover, we've always formatted it for all virtio-scsi devices. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b283f7ca83..e5743fad9d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2014,11 +2014,8 @@ qemuBuildDiskDeviceStr(const virDomainDef *def, virBufferAddLit(&opt, "virtio-blk-pci"); } - if (disk->iothread && - (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW || - disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) { + if (disk->iothread) virBufferAsprintf(&opt, ",iothread=iothread%u", disk->iothread); - } qemuBuildIoEventFdStr(&opt, disk->ioeventfd, qemuCaps); if (disk->event_idx && -- 2.17.1
On Thu, Sep 06, 2018 at 02:22:16PM +0200, Andrea Bolognani wrote:
So far we've only formatted it for virtio-blk-pci and virtio-blk-ccw, but other virtio-blk devices also support the corresponding option; moreover, we've always formatted it for all virtio-scsi devices.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The appropriate time to ensure the required capabilities are present is validate rather than command line generation: add a new qemuDomainDeviceDefValidateInput() function and move all existing checks there. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 26 ------------------ src/qemu/qemu_domain.c | 59 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 58 insertions(+), 27 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e5743fad9d..db7c3ad698 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4057,41 +4057,15 @@ qemuBuildVirtioInputDevStr(const virDomainDef *def, switch ((virDomainInputType)dev->type) { case VIR_DOMAIN_INPUT_TYPE_MOUSE: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_MOUSE) || - (dev->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")); - goto error; - } virBufferAsprintf(&buf, "virtio-mouse%s", suffix); break; case VIR_DOMAIN_INPUT_TYPE_TABLET: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_TABLET) || - (dev->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")); - goto error; - } virBufferAsprintf(&buf, "virtio-tablet%s", suffix); break; case VIR_DOMAIN_INPUT_TYPE_KBD: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_KEYBOARD) || - (dev->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")); - goto error; - } virBufferAsprintf(&buf, "virtio-keyboard%s", suffix); 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")); - goto error; - } virBufferAsprintf(&buf, "virtio-input-host%s", suffix); break; case VIR_DOMAIN_INPUT_TYPE_LAST: diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f161cf6c84..05e90c3615 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5727,6 +5727,60 @@ qemuDomainDeviceDefValidateGraphics(const virDomainGraphicsDef *graphics, } +static int +qemuDomainDeviceDefValidateInput(const virDomainInputDef *input, + const virDomainDef *def ATTRIBUTE_UNUSED, + virQEMUCapsPtr qemuCaps) +{ + 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; + } + 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; + } + 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; + } + 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; + } + break; + case VIR_DOMAIN_INPUT_TYPE_LAST: + default: + virReportEnumRangeError(virDomainInputType, + input->type); + return -1; + } + + return 0; +} + + static int qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, const virDomainDef *def, @@ -5796,9 +5850,12 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, qemuCaps); break; + case VIR_DOMAIN_DEVICE_INPUT: + ret = qemuDomainDeviceDefValidateInput(dev->data.input, def, qemuCaps); + break; + case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: - case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: case VIR_DOMAIN_DEVICE_HUB: case VIR_DOMAIN_DEVICE_MEMBALLOON: -- 2.17.1
s/ites/ities/ On Thu, Sep 06, 2018 at 02:22:17PM +0200, Andrea Bolognani wrote:
The appropriate time to ensure the required capabilities are present is validate rather than command line generation: add a new qemuDomainDeviceDefValidateInput() function and move all existing checks there.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 26 ------------------ src/qemu/qemu_domain.c | 59 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 58 insertions(+), 27 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@redhat.com> --- 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; 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"), + baseName); + return -1; + } + return 0; } -- 2.17.1
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@redhat.com> --- 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@redhat.com> Jano
A virtio device such as <controller type='scsi' model='virtio-scsi'/> will be translated to one of four different QEMU devices based on the address type. This behavior is the same for all virtio devices, but unfortunately we have separate ad-hoc code dealing with each and every one of them: not only this is pointless duplication, but it turns out that most of that code is not robust against new address types being introduced and some of it is outright buggy. Introduce a new function, qemuBuildVirtioDevStr(), which deals with the issue in a generic fashion, and rewrite all existing code to use it. This fixes a bunch of issues such as virtio-serial-pci being used with virtio-mmio addresses and virtio-gpu not being usable at all with virtio-mmio addresses. It also introduces a couple of minor regressions, namely no longer erroring out when attempting to use virtio-balloon and virtio-input devices with virtio-s390 addresses; that said, virtio-s390 has been superseded by virtio-ccw such a long time ago that recent QEMU releases have dropped support for the former entirely, so re-implementing such device-specific validation is not worth it. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 181 ++++++++++++++++++---------------------- 1 file changed, 81 insertions(+), 100 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index db7c3ad698..041e4bd7fb 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -397,6 +397,59 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, return ret; } + +static int +qemuBuildVirtioDevStr(virBufferPtr buf, + const char *baseName, + virDomainDeviceAddressType type) +{ + const char *implName = NULL; + int ret = -1; + + switch (type) { + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: + implName = "pci"; + break; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO: + implName = "device"; + break; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW: + implName = "ccw"; + break; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390: + implName = "s390"; + break; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected address type for '%s'"), baseName); + goto done; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST: + default: + virReportEnumRangeError(virDomainDeviceAddressType, type); + goto done; + } + + virBufferAsprintf(buf, "%s-%s", baseName, implName); + + ret = 0; + + done: + return ret; +} + + static int qemuBuildVirtioOptionsStr(virBufferPtr buf, virDomainVirtioOptionsPtr virtio, @@ -2002,17 +2055,8 @@ qemuBuildDiskDeviceStr(const virDomainDef *def, break; case VIR_DOMAIN_DISK_BUS_VIRTIO: - if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { - virBufferAddLit(&opt, "virtio-blk-ccw"); - } else if (disk->info.type == - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) { - virBufferAddLit(&opt, "virtio-blk-s390"); - } else if (disk->info.type == - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) { - virBufferAddLit(&opt, "virtio-blk-device"); - } else { - virBufferAddLit(&opt, "virtio-blk-pci"); - } + if (qemuBuildVirtioDevStr(&opt, "virtio-blk", disk->info.type) < 0) + goto error; if (disk->iothread) virBufferAsprintf(&opt, ",iothread=iothread%u", disk->iothread); @@ -2546,10 +2590,8 @@ qemuBuildFSDevStr(const virDomainDef *def, goto error; } - if (fs->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) - virBufferAddLit(&opt, "virtio-9p-ccw"); - else - virBufferAddLit(&opt, "virtio-9p-pci"); + if (qemuBuildVirtioDevStr(&opt, "virtio-9p", fs->info.type) < 0) + goto error; virBufferAsprintf(&opt, ",id=%s", fs->info.alias); virBufferAsprintf(&opt, ",fsdev=%s%s", @@ -2744,7 +2786,6 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, int *nusbcontroller) { virBuffer buf = VIR_BUFFER_INITIALIZER; - int address_type = def->info.type; *devstr = NULL; @@ -2752,19 +2793,8 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: switch ((virDomainControllerModelSCSI) def->model) { case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: - switch (address_type) { - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW: - virBufferAddLit(&buf, "virtio-scsi-ccw"); - break; - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390: - virBufferAddLit(&buf, "virtio-scsi-s390"); - break; - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO: - virBufferAddLit(&buf, "virtio-scsi-device"); - break; - default: - virBufferAddLit(&buf, "virtio-scsi-pci"); - } + if (qemuBuildVirtioDevStr(&buf, "virtio-scsi", def->info.type) < 0) + goto error; if (def->iothread) { virBufferAsprintf(&buf, ",iothread=iothread%u", @@ -2804,22 +2834,9 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, break; case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: - switch (address_type) { - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: - virBufferAddLit(&buf, "virtio-serial-pci"); - break; - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW: - virBufferAddLit(&buf, "virtio-serial-ccw"); - break; - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390: - virBufferAddLit(&buf, "virtio-serial-s390"); - break; - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO: - virBufferAddLit(&buf, "virtio-serial-device"); - break; - default: - virBufferAddLit(&buf, "virtio-serial"); - } + if (qemuBuildVirtioDevStr(&buf, "virtio-serial", def->info.type) < 0) + goto error; + virBufferAsprintf(&buf, ",id=%s", def->info.alias); if (def->opts.vioserial.ports != -1) { virBufferAsprintf(&buf, ",max_ports=%d", @@ -3517,24 +3534,18 @@ qemuBuildNicDevStr(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; - const char *nic = net->model; bool usingVirtio = false; char macaddr[VIR_MAC_STRING_BUFLEN]; if (STREQ(net->model, "virtio")) { - if (net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) - nic = "virtio-net-ccw"; - else if (net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) - nic = "virtio-net-s390"; - else if (net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) - nic = "virtio-net-device"; - else - nic = "virtio-net-pci"; + if (qemuBuildVirtioDevStr(&buf, "virtio-net", net->info.type) < 0) + goto error; usingVirtio = true; + } else { + virBufferAddStr(&buf, net->model); } - virBufferAdd(&buf, nic, -1); if (usingVirtio && net->driver.virtio.txmode) { if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_TX_ALG)) { virBufferAddLit(&buf, ",tx="); @@ -3929,21 +3940,9 @@ qemuBuildMemballoonCommandLine(virCommandPtr cmd, return -1; } - switch (def->memballoon->info.type) { - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: - virBufferAddLit(&buf, "virtio-balloon-pci"); - break; - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW: - virBufferAddLit(&buf, "virtio-balloon-ccw"); - break; - case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO: - virBufferAddLit(&buf, "virtio-balloon-device"); - break; - default: - virReportError(VIR_ERR_XML_ERROR, - _("memballoon unsupported with address type '%s'"), - virDomainDeviceAddressTypeToString(def->memballoon->info.type)); - goto error; + if (qemuBuildVirtioDevStr(&buf, "virtio-balloon", + def->memballoon->info.type) < 0) { + goto error; } virBufferAsprintf(&buf, ",id=%s", def->memballoon->info.alias); @@ -4040,33 +4039,23 @@ qemuBuildVirtioInputDevStr(const virDomainDef *def, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; - const char *suffix; - - if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - suffix = "-pci"; - } else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { - suffix = "-ccw"; - } else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) { - suffix = "-device"; - } else { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unsupported address type %s for virtio input device"), - virDomainDeviceAddressTypeToString(dev->info.type)); - goto error; - } switch ((virDomainInputType)dev->type) { case VIR_DOMAIN_INPUT_TYPE_MOUSE: - virBufferAsprintf(&buf, "virtio-mouse%s", suffix); + if (qemuBuildVirtioDevStr(&buf, "virtio-mouse", dev->info.type) < 0) + goto error; break; case VIR_DOMAIN_INPUT_TYPE_TABLET: - virBufferAsprintf(&buf, "virtio-tablet%s", suffix); + if (qemuBuildVirtioDevStr(&buf, "virtio-tablet", dev->info.type) < 0) + goto error; break; case VIR_DOMAIN_INPUT_TYPE_KBD: - virBufferAsprintf(&buf, "virtio-keyboard%s", suffix); + if (qemuBuildVirtioDevStr(&buf, "virtio-keyboard", dev->info.type) < 0) + goto error; break; case VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH: - virBufferAsprintf(&buf, "virtio-input-host%s", suffix); + if (qemuBuildVirtioDevStr(&buf, "virtio-input-host", dev->info.type) < 0) + goto error; break; case VIR_DOMAIN_INPUT_TYPE_LAST: default: @@ -4379,10 +4368,8 @@ qemuBuildDeviceVideoStr(const virDomainDef *def, } if (STREQ(model, "virtio-gpu")) { - if (video->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) - virBufferAsprintf(&buf, "%s-ccw", model); - else - virBufferAsprintf(&buf, "%s-pci", model); + if (qemuBuildVirtioDevStr(&buf, "virtio-gpu", video->info.type) < 0) + goto error; } else { virBufferAsprintf(&buf, "%s", model); } @@ -5813,14 +5800,8 @@ qemuBuildRNGDevStr(const virDomainDef *def, dev->source.file)) goto error; - if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) - virBufferAddLit(&buf, "virtio-rng-ccw"); - else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) - virBufferAddLit(&buf, "virtio-rng-s390"); - else if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) - virBufferAddLit(&buf, "virtio-rng-device"); - else - virBufferAddLit(&buf, "virtio-rng-pci"); + if (qemuBuildVirtioDevStr(&buf, "virtio-rng", dev->info.type) < 0) + goto error; virBufferAsprintf(&buf, ",rng=obj%s,id=%s", dev->info.alias, dev->info.alias); -- 2.17.1
On Thu, Sep 06, 2018 at 02:22:19PM +0200, Andrea Bolognani wrote:
A virtio device such as
<controller type='scsi' model='virtio-scsi'/>
will be translated to one of four different QEMU devices based on the address type. This behavior is the same for all virtio devices, but unfortunately we have separate ad-hoc code dealing with each and every one of them: not only this is pointless duplication, but it turns out that most of that code is not robust against new address types being introduced and some of it is outright buggy.
Introduce a new function, qemuBuildVirtioDevStr(), which deals with the issue in a generic fashion, and rewrite all existing code to use it.
This fixes a bunch of issues such as virtio-serial-pci being used with virtio-mmio addresses and virtio-gpu not being usable at all with virtio-mmio addresses.
It also introduces a couple of minor regressions, namely no longer erroring out when attempting to use virtio-balloon and virtio-input devices with virtio-s390 addresses; that said, virtio-s390 has been superseded by virtio-ccw such a long time ago that recent QEMU releases have dropped support for the former entirely,
I approve of such disregard for antique virtio.
so re-implementing such device-specific validation is not worth it.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 181 ++++++++++++++++++---------------------- 1 file changed, 81 insertions(+), 100 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index db7c3ad698..041e4bd7fb 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -397,6 +397,59 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, return ret; }
+ +static int +qemuBuildVirtioDevStr(virBufferPtr buf, + const char *baseName, + virDomainDeviceAddressType type) +{ + const char *implName = NULL; + int ret = -1; + + switch (type) { + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: + implName = "pci"; + break; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO: + implName = "device"; + break; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW: + implName = "ccw"; + break; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390: + implName = "s390"; + break; + + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected address type for '%s'"), baseName); + goto done;
Just return -1;
+ + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE: + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST: + default: + virReportEnumRangeError(virDomainDeviceAddressType, type); + goto done;
Here too. It's unlikely we'll end up having to do something worth cleaning up before we even validate the supplied parameters.
+ } + + virBufferAsprintf(buf, "%s-%s", baseName, implName); +
+ ret = 0; + + done: + return ret;
Then this can be simply: return 0; Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Andrea Bolognani -
Ján Tomko