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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list