After commit c95f50cb021ea9a297 we always set a disk format in the post parse callback so the code that madates use of explicit format for shareable disks no longer makes sense. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 964fe97963..e29d01c828 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4642,20 +4642,12 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk, const char *driverName = virDomainDiskGetDriver(disk); virStorageSourcePtr n; - if (disk->src->shared && !disk->src->readonly) { - if (disk->src->format <= VIR_STORAGE_FILE_AUTO) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("shared access for disk '%s' requires use of " - "explicitly specified disk format"), disk->dst); - return -1; - } - - if (!qemuBlockStorageSourceSupportsConcurrentAccess(disk->src)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("shared access for disk '%s' requires use of " - "supported storage format"), disk->dst); - return -1; - } + if (disk->src->shared && !disk->src->readonly && + !qemuBlockStorageSourceSupportsConcurrentAccess(disk->src)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("shared access for disk '%s' requires use of " + "supported storage format"), disk->dst); + return -1; } if (disk->geometry.cylinders > 0 && -- 2.16.2

The sectors read from the backing image need to be written to the top level image. If a disk is marked read-only the image can't be written. QEMU handled that by disabling copy_on_read and reporting a warning: -drive file=/var/lib/libvirt/images/c,format=qcow2,if=none, id=drive-scsi0-0-1,readonly=on,copy-on-read=on: warning: disabling copy-on-read on read-only drive Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e29d01c828..0821ce769b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4650,6 +4650,13 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk, return -1; } + if (disk->src->readonly && disk->copy_on_read == VIR_TRISTATE_SWITCH_ON) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("copy_on_read is not compatible with read-only disk '%s'"), + disk->dst); + return -1; + } + if (disk->geometry.cylinders > 0 && disk->geometry.heads > 0 && disk->geometry.sectors > 0) { -- 2.16.2

We are testing character devices so the disk is not necessary. Minimize the configuration. This will prevent changes when switching to blockdev. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- tests/qemuxml2argvdata/serial-unix-chardev.args | 2 -- tests/qemuxml2argvdata/serial-unix-chardev.x86_64-latest.args | 2 -- tests/qemuxml2argvdata/serial-unix-chardev.xml | 5 ----- 3 files changed, 9 deletions(-) diff --git a/tests/qemuxml2argvdata/serial-unix-chardev.args b/tests/qemuxml2argvdata/serial-unix-chardev.args index 873d3263c6..1977ba5cd7 100644 --- a/tests/qemuxml2argvdata/serial-unix-chardev.args +++ b/tests/qemuxml2argvdata/serial-unix-chardev.args @@ -22,8 +22,6 @@ server,nowait \ -no-acpi \ -boot c \ -usb \ --drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ --device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ -chardev socket,id=charserial0,path=/tmp/serial.sock \ -device isa-serial,chardev=charserial0,id=serial0 \ -chardev socket,id=charserial1,path=/tmp/serial-server.sock,server,nowait \ diff --git a/tests/qemuxml2argvdata/serial-unix-chardev.x86_64-latest.args b/tests/qemuxml2argvdata/serial-unix-chardev.x86_64-latest.args index ce7a7f80d7..e9ce4d3181 100644 --- a/tests/qemuxml2argvdata/serial-unix-chardev.x86_64-latest.args +++ b/tests/qemuxml2argvdata/serial-unix-chardev.x86_64-latest.args @@ -24,8 +24,6 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ -no-acpi \ -boot strict=on \ -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ --drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ --device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \ -chardev socket,id=charserial0,path=/tmp/serial.sock \ -device isa-serial,chardev=charserial0,id=serial0 \ -chardev socket,id=charserial1,fd=1729,server,nowait \ diff --git a/tests/qemuxml2argvdata/serial-unix-chardev.xml b/tests/qemuxml2argvdata/serial-unix-chardev.xml index af513d6445..f977d4d25c 100644 --- a/tests/qemuxml2argvdata/serial-unix-chardev.xml +++ b/tests/qemuxml2argvdata/serial-unix-chardev.xml @@ -14,11 +14,6 @@ <on_crash>destroy</on_crash> <devices> <emulator>/usr/bin/qemu-system-i686</emulator> - <disk type='block' device='disk'> - <source dev='/dev/HostVG/QEMUGuest1'/> - <target dev='hda' bus='ide'/> - <address type='drive' controller='0' bus='0' target='0' unit='0'/> - </disk> <controller type='usb' index='0'/> <controller type='ide' index='0'/> <serial type='unix'> -- 2.16.2

Change the semantics to exactly opposite and rename it to qemuDiskBusNeedsDriveArg. This will be necessary as some devices can't be used with -blockdev. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_command.c | 23 +++++++++++++---------- src/qemu/qemu_command.h | 1 + 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index cea31e6a24..8f5303ed95 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1330,15 +1330,18 @@ qemuCheckFips(void) } -/* Unfortunately it is not possible to use - -device for floppies, or SD - devices. Fortunately, those don't need - static PCI addresses, so we don't really - care that we can't use -device */ -static bool -qemuDiskBusNeedsDeviceArg(int bus) +/** + * qemuDiskBusNeedsDriveArg: + * @bus: disk bus + * + * Unfortunately it is not possible to use -device for SD devices. + * Fortunately, those don't need static PCI addresses, so we can use -drive + * without -device. + */ +bool +qemuDiskBusNeedsDriveArg(int bus) { - return bus != VIR_DOMAIN_DISK_BUS_SD; + return bus == VIR_DOMAIN_DISK_BUS_SD; } @@ -1636,7 +1639,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, if (qemuBuildDriveSourceStr(disk, qemuCaps, &opt) < 0) goto error; - if (qemuDiskBusNeedsDeviceArg(disk->bus)) { + if (!qemuDiskBusNeedsDriveArg(disk->bus)) { char *drivealias = qemuAliasDiskDriveFromDisk(disk); if (!drivealias) goto error; @@ -2250,7 +2253,7 @@ qemuBuildDiskCommandLine(virCommandPtr cmd, qemuBlockStorageSourceAttachDataFree(data); - if (qemuDiskBusNeedsDeviceArg(disk->bus)) { + if (!qemuDiskBusNeedsDriveArg(disk->bus)) { if (disk->bus == VIR_DOMAIN_DISK_BUS_FDC) { if (qemuBuildFloppyCommandLineOptions(cmd, def, disk, bootindex) < 0) diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 4f1b360130..4452c98e4b 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -99,6 +99,7 @@ char *qemuBuildNicDevStr(virDomainDefPtr def, virQEMUCapsPtr qemuCaps); char *qemuDeviceDriveHostAlias(virDomainDiskDefPtr disk); +bool qemuDiskBusNeedsDriveArg(int bus); qemuBlockStorageSourceAttachDataPtr qemuBuildStorageSourceAttachPrepareDrive(virDomainDiskDefPtr disk, -- 2.16.2

SD card hotplug should not be implemented until they can be used via -blockdev. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_hotplug.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 23f6d1daba..69f599f575 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -725,6 +725,9 @@ qemuDomainAttachDeviceDiskLive(virQEMUDriverPtr driver, case VIR_DOMAIN_DISK_BUS_UML: case VIR_DOMAIN_DISK_BUS_SATA: case VIR_DOMAIN_DISK_BUS_SD: + /* Note that SD card hotplug support should be added only once + * they support '-device' (don't require -drive only). + * See also: qemuDiskBusNeedsDriveArg */ case VIR_DOMAIN_DISK_BUS_LAST: virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("disk bus '%s' cannot be hotplugged."), -- 2.16.2

Further split qemuBuildDiskCommandLine to separate formatting of the source part. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_command.c | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8f5303ed95..48e463c3c9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2231,27 +2231,42 @@ qemuBuildBlockStorageSourceAttachDataCommandline(virCommandPtr cmd, static int -qemuBuildDiskCommandLine(virCommandPtr cmd, - const virDomainDef *def, - virDomainDiskDefPtr disk, - virQEMUCapsPtr qemuCaps, - unsigned int bootindex, - bool driveBoot) +qemuBuildDiskSourceCommandLine(virCommandPtr cmd, + virDomainDiskDefPtr disk, + virQEMUCapsPtr qemuCaps, + bool driveBoot) { qemuBlockStorageSourceAttachDataPtr data = NULL; - char *optstr; + int ret = -1; if (!(data = qemuBuildStorageSourceAttachPrepareDrive(disk, qemuCaps, driveBoot))) return -1; if (qemuBuildStorageSourceAttachPrepareCommon(disk->src, data, qemuCaps) < 0 || - qemuBuildBlockStorageSourceAttachDataCommandline(cmd, data) < 0) { - qemuBlockStorageSourceAttachDataFree(data); - return -1; - } + qemuBuildBlockStorageSourceAttachDataCommandline(cmd, data) < 0) + goto cleanup; + ret = 0; + + cleanup: qemuBlockStorageSourceAttachDataFree(data); + return ret; +} + + +static int +qemuBuildDiskCommandLine(virCommandPtr cmd, + const virDomainDef *def, + virDomainDiskDefPtr disk, + virQEMUCapsPtr qemuCaps, + unsigned int bootindex, + bool driveBoot) +{ + char *optstr; + + if (qemuBuildDiskSourceCommandLine(cmd, disk, qemuCaps, driveBoot) < 0) + return -1; if (!qemuDiskBusNeedsDriveArg(disk->bus)) { if (disk->bus == VIR_DOMAIN_DISK_BUS_FDC) { -- 2.16.2

qemuBulildFloppyCommandLineOptions built it's own version of the -drive alias. Replace it by qemuAliasDiskDriveFromDisk. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_command.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 48e463c3c9..f756cc7112 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2150,12 +2150,17 @@ qemuBuildFloppyCommandLineOptions(virCommandPtr cmd, virBuffer fdc_opts = VIR_BUFFER_INITIALIZER; char *fdc_opts_str = NULL; char *optstr; + char *backendAlias = NULL; + int ret = -1; - if (virAsprintf(&optstr, "drive%c=drive-%s", - disk->info.addr.drive.unit ? 'B' : 'A', - disk->info.alias) < 0) + if (!(backendAlias = qemuAliasDiskDriveFromDisk(disk))) return -1; + if (virAsprintf(&optstr, "drive%c=%s", + disk->info.addr.drive.unit ? 'B' : 'A', + backendAlias) < 0) + goto cleanup; + if (!qemuDomainNeedsFDC(def)) { virCommandAddArg(cmd, "-global"); virCommandAddArgFormat(cmd, "isa-fdc.%s", optstr); @@ -2169,7 +2174,7 @@ qemuBuildFloppyCommandLineOptions(virCommandPtr cmd, disk->info.addr.drive.unit ? 'B' : 'A', bootindex) < 0) - return -1; + goto cleanup; if (!qemuDomainNeedsFDC(def)) { virCommandAddArg(cmd, "-global"); @@ -2188,7 +2193,11 @@ qemuBuildFloppyCommandLineOptions(virCommandPtr cmd, VIR_FREE(fdc_opts_str); } - return 0; + ret = 0; + + cleanup: + VIR_FREE(backendAlias); + return ret; } -- 2.16.2

Aggregate the code for the two separate formats used according to the machine type and add some supporting code so that the function is actually readable. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_command.c | 58 ++++++++++++++++++++++++------------------------- 1 file changed, 28 insertions(+), 30 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f756cc7112..2e5920e859 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2148,55 +2148,53 @@ qemuBuildFloppyCommandLineOptions(virCommandPtr cmd, { virBuffer fdc_opts = VIR_BUFFER_INITIALIZER; - char *fdc_opts_str = NULL; - char *optstr; + char driveLetter; char *backendAlias = NULL; + char *backendStr = NULL; + char *bootindexStr = NULL; int ret = -1; + if (disk->info.addr.drive.unit) + driveLetter = 'B'; + else + driveLetter = 'A'; + if (!(backendAlias = qemuAliasDiskDriveFromDisk(disk))) return -1; - if (virAsprintf(&optstr, "drive%c=%s", - disk->info.addr.drive.unit ? 'B' : 'A', - backendAlias) < 0) + if (virAsprintf(&backendStr, "drive%c=%s", driveLetter, backendAlias) < 0) goto cleanup; - if (!qemuDomainNeedsFDC(def)) { - virCommandAddArg(cmd, "-global"); - virCommandAddArgFormat(cmd, "isa-fdc.%s", optstr); - } else { - virBufferAsprintf(&fdc_opts, "%s,", optstr); - } - VIR_FREE(optstr); - - if (bootindex) { - if (virAsprintf(&optstr, "bootindex%c=%u", - disk->info.addr.drive.unit - ? 'B' : 'A', - bootindex) < 0) - goto cleanup; + if (bootindex && + virAsprintf(&bootindexStr, "bootindex%c=%u", driveLetter, bootindex) < 0) + goto cleanup; - if (!qemuDomainNeedsFDC(def)) { + if (!qemuDomainNeedsFDC(def)) { + if (backendStr) { virCommandAddArg(cmd, "-global"); - virCommandAddArgFormat(cmd, "isa-fdc.%s", optstr); - } else { - virBufferAsprintf(&fdc_opts, "%s,", optstr); + virCommandAddArgFormat(cmd, "isa-fdc.%s", backendStr); } - VIR_FREE(optstr); - } - /* Newer Q35 machine types require an explicit FDC controller */ - virBufferTrim(&fdc_opts, ",", -1); - if ((fdc_opts_str = virBufferContentAndReset(&fdc_opts))) { + if (bootindexStr) { + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, "isa-fdc.%s", bootindexStr); + } + } else { + /* Newer Q35 machine types require an explicit FDC controller */ + virBufferAddLit(&fdc_opts, "isa-fdc,"); + virBufferStrcat(&fdc_opts, backendStr, ",", NULL); + virBufferStrcat(&fdc_opts, bootindexStr, NULL); + virBufferTrim(&fdc_opts, ",", -1); virCommandAddArg(cmd, "-device"); - virCommandAddArgFormat(cmd, "isa-fdc,%s", fdc_opts_str); - VIR_FREE(fdc_opts_str); + virCommandAddArgBuffer(cmd, &fdc_opts); } ret = 0; cleanup: VIR_FREE(backendAlias); + VIR_FREE(backendStr); + VIR_FREE(bootindexStr); return ret; } -- 2.16.2

On Tue, Jul 10, 2018 at 11:27:51 +0200, Ján Tomko wrote: With -blockdev there is no backend for an empty drive so it. I can remove it but it will need to be put back later.