Re: [libvirt] [PATCH 2/2] Avoid starting a PowerPC VM with floppy disk

On Fri, Jul 24, 2015 at 03:30:49PM -0400, Kothapally Madhu Pavan wrote: > PowerPC pseries based VMs do not support a floppy disk controller. > This prohibits libvirt from creating qemu command with floppy device. > > Signed-off-by: Kothapally Madhu Pavan <kmp(a)linux.vnet.ibm.com&gt; > --- > src/qemu/qemu_command.c | 47 +++++++++++++++++++++++++++++++++++++++++++++-- Extending the qemuxml2argvtest with a test case that fails on such config would be nice. > 1 file changed, 45 insertions(+), 2 deletions(-) > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 42906a8..93f84e2 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -9486,6 +9486,12 @@ qemuBuildCommandLine(virConnectPtr conn, > boot[i] = 'd'; > break; > case VIR_DOMAIN_BOOT_FLOPPY: > + /* PowerPC pseries based VMs do not support floppy device */ > + if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, "pseries")) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("PowerPC pseries machines do not support floppy device")); > + goto error; > + } There is no need to error out earlier for machines that do not use deviceboot. This error can be dropped, we will hit the next one. > boot[i] = 'a'; > break; > case VIR_DOMAIN_BOOT_DISK: > @@ -9769,6 +9775,12 @@ qemuBuildCommandLine(virConnectPtr conn, > bootCD = 0; > break; > case VIR_DOMAIN_DISK_DEVICE_FLOPPY: > + /* PowerPC pseries based VMs do not support floppy device */ > + if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, "pseries")) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("PowerPC pseries machines do not support floppy device")); > + goto error; > + } This is more appropriate place for the error message. Personally, I would move it above the switch() and let the switch only deal with boot indexes. > bootindex = bootFloppy; > bootFloppy = 0; > break; > @@ -9812,6 +9824,12 @@ qemuBuildCommandLine(virConnectPtr conn, > > if (withDeviceArg) { > if (disk->bus == VIR_DOMAIN_DISK_BUS_FDC) { > + /* PowerPC pseries based VMs do not support floppy device */ > + if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, "pseries")) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("PowerPC pseries machines do not support floppy device")); > + goto error; > + } This is dead code, the condition will never be true here, we already matched the error above. > if (virAsprintf(&optstr, "drive%c=drive-%s", > disk->info.addr.drive.unit ? 'B' : 'A', > disk->info.alias) < 0) > @@ -9854,6 +9872,12 @@ qemuBuildCommandLine(virConnectPtr conn, > /* Newer Q35 machine types require an explicit FDC controller */ > virBufferTrim(&fdc_opts, ",", -1); > if ((fdc_opts_str = virBufferContentAndReset(&fdc_opts))) { > + /* PowerPC pseries based VMs do not support floppy device */ > + if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, "pseries")) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("PowerPC pseries machines do not support floppy device")); > + goto error; > + } Same here. > virCommandAddArg(cmd, "-device"); > virCommandAddArgFormat(cmd, "isa-fdc,%s", fdc_opts_str); > VIR_FREE(fdc_opts_str); > @@ -9918,10 +9942,17 @@ qemuBuildCommandLine(virConnectPtr conn, > _("cannot create virtual FAT disks in read-write mode")); > goto error; > } > - if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) > + if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) { > + /* PowerPC pseries based VMs do not support floppy device */ > + if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, "pseries")) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("PowerPC pseries machines do not support floppy device")); > + goto error; > + } > fmt = "fat:floppy:%s"; > - else > + } else { > fmt = "fat:%s"; > + } > This code path can only be taken for QEMUs not having QEMU_CAPS_DRIVE, i.e. older than at least 7 years. I do not think touching this part of code is worth it. > if (virAsprintf(&file, fmt, disk->src) < 0) > goto error; > @@ -11674,6 +11705,12 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, > def->device = VIR_DOMAIN_DISK_DEVICE_CDROM; > def->src->readonly = true; > } else if (STREQ(values[i], "floppy")) { > + /* PowerPC pseries based VMs do not support floppy device */ > + if (ARCH_IS_PPC64(dom->os.arch) && STRPREFIX(dom->os.machine, "pseries")) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("PowerPC pseries machines do not support floppy device")); > + goto error; > + } This function is used to parse the command line of a running QEMU. If QEMU rejects the floppy drive on the command line, such a machine would not be running. If it quietly ignores it, we should do that too to get a matching config. > def->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY; > } > } else if (STREQ(keywords[i], "format")) { > @@ -12908,6 +12945,12 @@ qemuParseCommandLine(virCapsPtr qemuCaps, > disk->src->readonly = true; > } else { > if (STRPREFIX(arg, "-fd")) { > + /* PowerPC pseries based VMs do not support floppy device */ > + if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, "pseries")) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("PowerPC pseries machines do not support floppy device")); > + goto error; > + } Same here.