[libvirt] [PATCH 2/3] qemu: add two qemu caps for lsi and virtio-scsi SCSI controllers

[Eric Blake]

On 08/07/2012 11:21 AM, Guannan Ren wrote:
> Rename qemuDefaultScsiControllerModel to qemuCheckScsiControllerModel.
> When scsi model is given explicitly in XML(model > 0) checking if the
> underlying QEMU supports it or not first, raise an error on checking
> failure.
> When the model is not given(mode <= 0), return LSI by default, if
> the QEMU doesn't support it, raise an error.
> ---
>  src/qemu/qemu_command.c |  106 +++++++++++++++++++++++++++++++++++-----------
>  src/qemu/qemu_command.h |    3 +-
>  src/qemu/qemu_process.c |    9 +++-
>  3 files changed, 88 insertions(+), 30 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 9999a05..d4791c6 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -469,19 +469,58 @@ static int qemuAssignDeviceDiskAliasFixed(virDomainDiskDefPtr disk)
>  }
>  
>  static int
> -qemuDefaultScsiControllerModel(virDomainDefPtr def) {
> -    if (STREQ(def->os.arch, "ppc64") &&
> -        STREQ(def->os.machine, "pseries")) {
> -        return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI;
> +qemuCheckScsiControllerModel(virDomainDefPtr def,
> +                             virBitmapPtr qemuCaps,
> +                             int *model)
> +{
> +    if (*model > 0) {
> +        switch (*model) {

This looks a bit odd; why not just:

switch (*model) {

...

> +        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
> +            if (!qemuCapsGet(qemuCaps, QEMU_CAPS_SCSI_LSI)) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                               _("This QEMU doesn't support "
> +                                 "lsi scsi controller"));
> +                return -1;
> +            }
> +            break;
> +        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
> +            if (!qemuCapsGet(qemuCaps, QEMU_CAPS_VIRIO_SCSI_PCI)) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                               _("This QEMU doesn't support "
> +                                 "virtio scsi controller"));
> +                return -1;
> +            }
> +            break;
> +        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI:
> +            /*TODO: need checking work here if necessary */
> +            break;
> +        default:
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("Unsupported controller model: %s"),
> +                           virDomainControllerModelSCSITypeToString(*model));
> +            return -1;
> +        }
>      } else {

...then fold this else branch into:

case 0:

> -        return VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC;
> +        if (STREQ(def->os.arch, "ppc64") &&
> +            STREQ(def->os.machine, "pseries")) {
> +            *model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI;
> +        } else if (qemuCapsGet(qemuCaps, QEMU_CAPS_SCSI_LSI)) {
> +            *model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC;
> +        } else {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("Unable to determine model for scsi controller"));
> +            return -1;
> +        }
>      }
> +
> +    return 0;
>  }

But that's just a formatting recommendation, and not a bug in your code,
so I'm okay if you leave it as written.

> +int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def,
> +                                      virBitmapPtr qemuCaps)
>  {
>      int i, rc;
>      int model;
> +    virBitmapPtr localCaps = NULL;
>  
>      /* Default values match QEMU. See spapr_(llan|vscsi|vty).c */
>  
> +    if (!qemuCaps) {
> +        /* need to get information from real environment */
> +        if (qemuCapsExtractVersionInfo(def->emulator, def->os.arch,
> +                                       false, NULL,
> +                                       &localCaps) < 0)
> +            goto cleanup;
> +        qemuCaps = localCaps;
> +    }

Yet more evidence why we need to rewrite the capabilities collection
code to be caching, but that's a project for another day.

ACK.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120807/c27af20d/attachment-0001.sig>