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

Re: [libvirt] [PATCH RFC 4/5] qemu: Build qemu command line for scsi-generic



On 2013年03月04日 14:01, Han Cheng wrote:
> For scsi-generic, the command line will be like:
> 
>    -drive file=/dev/sg0,if=none,id=drive-hostdev-scsi0-0-0-0 -device
>    scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=8,drive=drive-hostdev-scsi0-0-0-0,id=hostdev-scsi0-0-0-0
> 
> The relationship between the libvirt address attrs and the qdev
> properties are(channel should always be 0):
>    bus=scsi<controller>.0
>    scsi-id=<target>
>    lun=<unit>
> ---
>   src/qemu/qemu_command.c |  156 ++++++++++++++++++++++++++++++++++++++++++++++-
>   1 files changed, 153 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 5eb9999..1d2540e 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -49,6 +49,8 @@
> 
>   #include<sys/stat.h>
>   #include<fcntl.h>
> +#include<dirent.h>
> +#include<sys/types.h>
> 
>   #define VIR_FROM_THIS VIR_FROM_QEMU
> 
> @@ -651,7 +653,16 @@ qemuAssignDeviceHostdevAlias(virDomainDefPtr def, virDomainHostdevDefPtr hostdev
>           }
>       }
> 
> -    if (virAsprintf(&hostdev->info->alias, "hostdev%d", idx)<  0) {
> +    if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) {
> +        if (virAsprintf(&hostdev->info->alias, "hostdev-scsi%d-%d-%d-%d",
> +                        hostdev->source.subsys.u.scsi.adapter,
> +                        hostdev->source.subsys.u.scsi.bus,
> +                        hostdev->source.subsys.u.scsi.target,
> +                        hostdev->source.subsys.u.scsi.unit)<  0) {
> +            virReportOOMError();
> +            return -1;
> +        }
> +    } else if (virAsprintf(&hostdev->info->alias, "hostdev%d", idx)<  0) {
>           virReportOOMError();
>           return -1;
>       }
> @@ -3864,6 +3875,110 @@ qemuBuildUSBHostdevUsbDevStr(virDomainHostdevDefPtr dev)
>       return ret;
>   }
> 
> +static int
> +scsiGetDeviceId(unsigned int adapter,
> +                unsigned int bus,
> +                unsigned int target,
> +                unsigned int unit)

This should be a util function, and note that it's Linux platform
specific.

> +{
> +    DIR *dir = NULL;
> +    struct dirent *entry;
> +    char *path;
> +    int id;

s/int/unsigned int/,

> +
> +    if (virAsprintf(&path,
> +                    "/sys/bus/scsi/devices/target%d:%d:%d/%d:%d:%d:%d/scsi_generic",
> +                    adapter, bus, target, adapter, bus, target, unit)<  0) {
> +        virReportOOMError();
> +        return -1;
> +    }
> +    dir = opendir(path);
> +    VIR_FREE(path);
> +    if (!dir) {

if (!(dir = opendir(path)))

> +        VIR_WARN("Failed to open %s", path);

Any special reason for this to be a warning, but not an error
instead?

> +        return -1;
> +    }
> +
> +    while ((entry = readdir(dir))) {
> +        if (entry->d_name[0] == '.')
> +            continue;
> +
> +        if (sscanf(entry->d_name, "sg%d",&id) != 1) {
> +            VIR_WARN("Failed to analyse sg id");
> +            return -1;

Since here it returns -1, above should be an error.

> +        }
> +    }
> +
> +    return id;
> +}
> +
> +static char *
> +qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev, virQEMUCapsPtr qemuCaps)
> +{
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    int scsiid;
> +
> +    if ((scsiid = scsiGetDeviceId(dev->source.subsys.u.scsi.adapter,
> +                                  dev->source.subsys.u.scsi.bus,
> +                                  dev->source.subsys.u.scsi.target,
> +                                  dev->source.subsys.u.scsi.unit))<  0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("can't get sg* number"));

"*" is magic in the error message. s/sg\*/sg/,

And actually if you throw error in scsiGetDeviceId, there is no need
to report error here anymore.

> +        goto error;
> +    }
> +
> +    virBufferAsprintf(&buf, "file=/dev/sg%d,if=none", scsiid);
> +    virBufferAsprintf(&buf, ",id=%s-%s",
> +                      virDomainDeviceAddressTypeToString(dev->info->type),
> +                      dev->info->alias);
> +    if (dev->info->readonly&&
> +        virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_READONLY))
> +        virBufferAsprintf(&buf, ",readonly=on");
> +
> +    return virBufferContentAndReset(&buf);

Should check if there is any error of the buf here. Something like:

    if (virBufferError(&buf)) {
        virReportOOMErrorr();
        goto error;
    }

> +error:
> +    virBufferFreeAndReset(&buf);
> +    return NULL;
> +}
> +
> +
> +static char *
> +qemuBuildSCSIHostdevDevStr(virDomainDefPtr def, virDomainHostdevDefPtr dev,
> +                           virQEMUCapsPtr qemuCaps)
> +{
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    int controllerModel = -1;
> +
> +    controllerModel = virDomainInfoFindControllerModel(def, dev->info,
> +                                                       VIR_DOMAIN_CONTROLLER_TYPE_SCSI);
> +    if (qemuSetScsiControllerModel(def, qemuCaps,&controllerModel)<  0)
> +        goto error;
> +    /* TODO: deal with lsi or ibm controller */
> +    if (controllerModel == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI) {
> +        virBufferAsprintf(&buf, "scsi-generic");
> +        if (dev->info->addr.drive.bus != 0)
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           "%s", _("SCSI controller only supports 1 bus"));

XML_ERROR here might be better.

> +        /* TODO: deal with early version qemu which does not support bus... */
> +        virBufferAsprintf(&buf,
> +                          ",bus=scsi%d.0,channel=0,scsi-id=%d,lun=%d",
> +                          dev->info->addr.drive.controller,
> +                          dev->info->addr.drive.target,
> +                          dev->info->addr.drive.unit);
> +        virBufferAsprintf(&buf, ",drive=%s-%s,id=%s",
> +                          virDomainDeviceAddressTypeToString(dev->info->type),
> +                          dev->info->alias, dev->info->alias);
> +        if (dev->info->bootIndex)
> +            virBufferAsprintf(&buf, ",bootindex=%d", dev->info->bootIndex);
> +    } else {
> +        goto error;

This is magic, you need to give an error message here.

> +    }
> +
> +    return virBufferContentAndReset(&buf);

Need to check if there is error of buf too here.

> +error:
> +    virBufferFreeAndReset(&buf);
> +    return NULL;
> +}
> 
> 
>   /* This function outputs a -chardev command line option which describes only the
> @@ -6953,10 +7068,11 @@ qemuBuildCommandLine(virConnectPtr conn,
>           if (hostdev->info->bootIndex) {
>               if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS ||
>                   (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI&&
> -                 hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)) {
> +                 hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB&&
> +                 hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI)) {
>                   virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                                  _("booting from assigned devices is only"
> -                                 " supported for PCI and USB devices"));
> +                                 " supported for PCI, USB and SCSI devices"));
>                   goto error;
>               } else {
>                   if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI&&
> @@ -6973,6 +7089,40 @@ qemuBuildCommandLine(virConnectPtr conn,
>                                        " supported with this version of qemu"));
>                       goto error;
>                   }
> +                if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI&&
> +                    !virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_HOST_BOOTINDEX)) {
> +                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                                   _("booting from assigned SCSI devices is not"
> +                                     " supported with this version of qemu"));
> +                    goto error;
> +                }
> +            }
> +        }
> +
> +        /* SCSI */
> +        if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS&&
> +            hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) {
> +            /* TODO */
> +            if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE)&&
> +                virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)&&
> +                virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_GENERIC)) {
> +                char *drvstr;
> +
> +                virCommandAddArg(cmd, "-drive");
> +                if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, qemuCaps)))
> +                    goto error;
> +                virCommandAddArg(cmd, drvstr);
> +                VIR_FREE(drvstr);
> +
> +                virCommandAddArg(cmd, "-device");
> +                if (!(devstr = qemuBuildSCSIHostdevDevStr(def, hostdev, qemuCaps)))
> +                    goto error;
> +                virCommandAddArg(cmd, devstr);
> +                VIR_FREE(devstr);
> +            } else {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                               _("SCSI assignment is not supported by this version of qemu"));

In libvirt, we use "passthrough", instead of "assignment".

> +                goto error;
>               }
>           }
> 


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