On Wed, Aug 27, 2008 at 05:32:21PM -0400, Cole Robinson wrote:

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.

Excellant - that intermediate month can be argued as a bug because people could add multiple CDROMs and the monitor provided no way to select between them. So this check for -drive is sufficient.

+/* 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);

Minor bug here if (idx < 0) return -1;

+ + 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:

Could also add case VIR_DOMAIN_DISK_BUS_USB: case VIR_DOMAIN_DISK_BUS_XEN:

+ 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);

if (virDiskNameToBusDeviceIndex(disk, &busid, &devid) < 0) qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR, _("cannot convertor disk name to bus/device index")); return NULL; }

+ + 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));

s/Unknown/Unsupported/ because we know about Xen / USB, merely don't support them.

+ 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);

You don't want to call qemudReportError here, because qemudDiskDeviceName() will already have raised an error. You need to push this particular error raise up into the 'else { ... }' clause above.

+ 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"));

I know this was here already, but we should change it to the OOM error code qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_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"));

Likewise here..

+ 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 &&

OOpps, nice catch for that typo !

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

Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|