[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]