Re: [libvirt] [PATCH] Fix ejecting cdroms with latest qemu syntax

Daniel P. Berrange wrote: > > I think this block could be re-factored a little into one or more helper > methods, because I think we'll need to re-use this for hot-unplug of > disks. I'd suggest a helper to turn the plain integer index into the > (bus,device) index pair > > virDiskNameToBusDeviceIndex(virDomainDiskDefPtr disk, > int *busIdx, > int *devIdx); > > And then a QEMU specific function > > char *virQEMUDeviceName(virDomainDiskDefPtr disk); > > and have this call virDiskNameToBusDeviceIndex() and return the 'ide1-2' > type string. > Okay, second cut attached. I followed your recommendations: virDiskNameToBusDeviceIndex added to domain_conf.c qemudDiskDeviceName added to qemu_driver.c I also hopefully solved the back compatibility issue. Seems -drive was added to qemu in Dec 07 (qemu commit 3759) and the device naming scheme was updated at the end of that month (qemu commit 3846), so checking for -drive should be a sufficient indicator that we need to use the new device name format. So if drive was detected (using the already present qemu_conf flag), we revert to the old style cdrom/fda naming style.

+/* Translates a device name of the form (regex) "[fhv]d[a-z]+" into + * the corresponding bus,index combination (e.g. sda => (0,0), sdi (1,1), + * hdd => (1,1), vdaa => (0,27)) + * @param disk The disk device + * @param busIdx parsed bus number + * @param devIdx parsed device number + * @return 0 on success, -1 on failure + */ +int virDiskNameToBusDeviceIndex(virDomainDiskDefPtr disk, + int *busIdx, + int *devIdx) { + + int idx = virDiskNameToIndex(disk->dst);

+ + switch (disk->bus) { + case VIR_DOMAIN_DISK_BUS_IDE: + *busIdx = ((idx - (idx % 2)) / 2); + *devIdx = (idx % 2); + break; + case VIR_DOMAIN_DISK_BUS_SCSI: + *busIdx = ((idx - (idx % 7)) / 7); + *devIdx = (idx % 7); + break; + case VIR_DOMAIN_DISK_BUS_FDC: + case VIR_DOMAIN_DISK_BUS_VIRTIO:

+ default: + *busIdx = 0; + *devIdx = idx; + break; + } + + return 0; +}

+/* Return the disks name for use in monitor commands */ +static char *qemudDiskDeviceName(virDomainPtr dom, + virDomainDiskDefPtr disk) { + + int busid, devid; + int ret; + char *devname; + + virDiskNameToBusDeviceIndex(disk, &busid, &devid);

+ + switch (disk->bus) { + case VIR_DOMAIN_DISK_BUS_IDE: + ret = asprintf(&devname, "ide%d-cd%d", busid, devid); + break; + case VIR_DOMAIN_DISK_BUS_SCSI: + ret = asprintf(&devname, "scsi%d-cd%d", busid, devid); + break; + case VIR_DOMAIN_DISK_BUS_FDC: + ret = asprintf(&devname, "floppy%d", devid); + break; + case VIR_DOMAIN_DISK_BUS_VIRTIO: + ret = asprintf(&devname, "virtio%d", devid); + break; + default: + qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT, + _("Unknown disk name mapping for bus '%s'"), + virDomainDiskBusTypeToString(disk->bus));

+ return NULL; + } + + if (ret == -1) { + qemudReportError(dom->conn, NULL, NULL, VIR_ERR_NO_MEMORY, NULL); + return NULL; + } + + return devname; +} + +static int qemudDomainChangeEjectableMedia(virDomainPtr dom, + virDomainDeviceDefPtr dev) +{ struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData; + virDomainObjPtr vm = virDomainFindByUUID(driver->domains, dom->uuid); + virDomainDiskDefPtr origdisk, newdisk; char *cmd, *reply, *safe_path; + char *devname = NULL; + int qemuCmdFlags; + + if (!vm) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN, + "%s", _("no domain with matching uuid")); + return -1; + } + + newdisk = dev->data.disk; + origdisk = vm->def->disks; + while (origdisk) { + if (origdisk->bus == newdisk->bus && + STREQ(origdisk->dst, newdisk->dst)) + break; + origdisk = origdisk->next; + } + + if (!origdisk) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR, + _("No device with bus '%s' and target '%s'"), + virDomainDiskBusTypeToString(newdisk->bus), + newdisk->dst); + return -1; + } + + if (qemudExtractVersionInfo(vm->def->emulator, + NULL, + &qemuCmdFlags) < 0) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR, + _("Cannot determine QEMU argv syntax %s"), + vm->def->emulator); + return -1; + } + + if (qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE) { + devname = qemudDiskDeviceName(dom, newdisk); + } else { + /* Back compat for no -drive option */ + if (newdisk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) + devname = strdup(newdisk->dst); + else if (newdisk->device == VIR_DOMAIN_DISK_DEVICE_CDROM && + STREQ(newdisk->dst, "hdc")) + devname = strdup("cdrom"); + else { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR, + _("Emulator version does not support removable " + "media for device '%s' and target '%s'"), + virDomainDiskDeviceTypeToString(newdisk->device), + newdisk->dst); + return -1; + } + } + + if (!devname) { + qemudReportError(dom->conn, NULL, NULL, VIR_ERR_NO_MEMORY, NULL);

+ return -1; + } if (newdisk->src) { safe_path = qemudEscapeMonitorArg(newdisk->src); if (!safe_path) { qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, "%s", _("out of memory")); + VIR_FREE(devname); return -1; } - if (asprintf (&cmd, "change %s \"%s\"", - /* XXX qemu may support multiple CDROM in future */ - /* olddisk->dst */ "cdrom", - safe_path) == -1) { + if (asprintf (&cmd, "change %s \"%s\"", devname, safe_path) == -1) { qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, "%s", _("out of memory"));

VIR_FREE(safe_path); + VIR_FREE(devname); return -1; } VIR_FREE(safe_path); - } else if (asprintf(&cmd, "eject cdrom") == -1) { + } else if (asprintf(&cmd, "eject %s", devname) == -1) { qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, "%s", _("out of memory"));

+ VIR_FREE(devname); return -1; } + VIR_FREE(devname); if (qemudMonitorCommand(driver, vm, cmd, &reply) < 0) { qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, @@ -2984,7 +3082,7 @@ static int qemudDomainChangeCDROM(virDomainPtr dom,

@@ -3186,10 +3251,11 @@ static int qemudDomainAttachDevice(virDomainPtr dom, } if (dev->type == VIR_DOMAIN_DEVICE_DISK && - dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { - ret = qemudDomainAttachCdromDevice(dom, dev); + (dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM || + dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)) { + ret = qemudDomainChangeEjectableMedia(dom, dev); } else if (dev->type == VIR_DOMAIN_DEVICE_DISK && - dev->data.disk->device == VIR_DOMAIN_DEVICE_DISK && + dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK &&

dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_USB) { ret = qemudDomainAttachUsbMassstorageDevice(dom, dev); } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV &&