[libvirt] [PATCH 1/1] Assign spapr-vio address value to VIO devices without hardcode

[Eric Blake]

On 05/17/2012 12:16 AM, Li Zhang wrote:
> Hardcode address will cause conflicts when there are a lot of VIO
> devices.
> 
> This patch is to remove the harcode of the address, and assign
> a variable to it, which is cnt * 0x1000UL. And assign spapr-vio
> address to VIO devices, such as spapr-vlan and spapr-vty. Several
> test cases are modified.

>      /* Default values match QEMU. See spapr_(llan|vscsi|vty).c */
>  
>      for (i = 0 ; i < def->nnets; i++) {
> -        rc = qemuAssignSpaprVIOAddress(def, &def->nets[i]->info,
> -                                       0x1000ul);

Isn't the point of qemuAssignSpaprVIOAddress to start at 0x1000 and
increment until it finds a gap?  If so, then the caller shouldn't also
be incrementing.

> -        if (rc)
> -            return rc;
> +        if (!def->nets[i]->model &&

This says def->nets[i]->model is NULL...

> +            STREQ(def->os.arch, "ppc64") &&
> +            STREQ(def->os.machine, "pseries"))
> +            strcpy(def->nets[i]->model, "spapr-vlan");

and this tells strcpy() to dereference it.  You can't have possibly
tested this line of code, since it will crash.  Did you mean to do a
strdup() instead?

> +        if (def->nets[i]->model &&
> +            STREQ(def->nets[i]->model, "spapr-vlan")) {
> +            cnt ++;

Style nit - no spacing between ++ or -- operator and the variable it
modifies.

> +            def->nets[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO;
> +            rc = qemuAssignSpaprVIOAddress(def, &def->nets[i]->info,
> +                                       cnt * 0x1000ul);

Indentation is off.

> +            if (rc)
> +                return rc;
> +        }
> +
>      }
>  
>      for (i = 0 ; i < def->ncontrollers; i++) {
> @@ -797,19 +808,28 @@ int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def)
>              model = qemuDefaultScsiControllerModel(def);
>          if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI &&
>              def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
> +            cnt ++;
>              def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO;
>              rc = qemuAssignSpaprVIOAddress(def, &def->controllers[i]->info,
> -                                           0x2000ul);
> +                                           cnt * 0x1000ul);

About the only difference I can see is that if you skipped the first
loop entirely, this would let the second loop start at 0x1000 instead of
0x2000.  But again, since qemuAssignSpaprVIOAddress is supposed to
auto-increment until it finds a gap, wouldn't that mean you can just
change this to start at 0x1000 without needing 'cnt'?

>              if (rc)
>                  return rc;
>          }
>      }
>  
>      for (i = 0 ; i < def->nserials; i++) {
> -        rc = qemuAssignSpaprVIOAddress(def, &def->serials[i]->info,
> -                                       0x30000000ul);
> -        if (rc)
> -            return rc;
> +        if (STREQ(def->os.arch, "ppc64") &&
> +            STREQ(def->os.machine, "pseries"))
> +            def->serials[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO;
> +
> +        if (def->serials[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) {
> +            cnt ++;
> +            rc = qemuAssignSpaprVIOAddress(def, &def->serials[i]->info,
> +                                       cnt * 0x1000ul);

This changes the default value from 0x30000000 to the first gap after
0x1000.  Can you please justify this change by pointing to the qemu
commit that shows how qemu assigns addresses, to make sure libvirt is
picking the same defaults as qemu?

-- 
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/20120517/d7371dfc/attachment-0001.sig>