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

Re: [libvirt] [PATCHv3 1/5] qemu: Extended qemuDomainAssignAddresses to be callable from everywhere.



On 29.06.2012 17:02, Viktor Mihajlovski wrote:
> This is in preparation of the enablement of s390 guests with virtio devices.
> 
> The assignment of device addresses happens in different places, i.e. the
> qemu driver and process modules as well as in the unit tests in slightly
> different flavors. Currently, these are PPC spapr-vio and PCI
> devices, virtio-s390 (not PCI based) will follow.
> 
> By optionally passing to qemuDomainAssignAddresses the domain
> object and the capabilities it is now possible to call the function
> from most of the places (except for hotplug) where address assignment
> is done.
> 
> Signed-off-by: Viktor Mihajlovski <mihajlov linux vnet ibm com>
> ---
>  src/qemu/qemu_command.c |   41 ++++++++++++++++++++++++++++++++---------
>  src/qemu/qemu_command.h |    6 ++++--
>  src/qemu/qemu_driver.c  |   14 +++++++-------
>  src/qemu/qemu_process.c |   42 ++++--------------------------------------
>  4 files changed, 47 insertions(+), 56 deletions(-)

Nice cleanup.

> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 6549f57..5edf915 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -942,16 +942,22 @@ cleanup:
>  
>  
>  int
> -qemuDomainAssignPCIAddresses(virDomainDefPtr def)
> +qemuDomainAssignPCIAddresses(virDomainDefPtr def, virBitmapPtr qemuCaps,
> +                             virDomainObjPtr obj)

Even though this is still less than 80 characters, I think we could have each argument per separate line.

>  {
>      int ret = -1;
> -    virBitmapPtr qemuCaps = NULL;
> +    virBitmapPtr localCaps = NULL;
>      qemuDomainPCIAddressSetPtr addrs = NULL;
> +    qemuDomainObjPrivatePtr priv = NULL;
>  
> -    if (qemuCapsExtractVersionInfo(def->emulator, def->os.arch,
> -                                   NULL,
> -                                   &qemuCaps) < 0)
> -        goto cleanup;
> +    if (!qemuCaps) {
> +        /* need to get information from real environment */
> +        if (qemuCapsExtractVersionInfo(def->emulator, def->os.arch,
> +                                       NULL,
> +                                       &localCaps) < 0)
> +            goto cleanup;
> +        qemuCaps = localCaps;
> +    }
>  
>      if (qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
>          if (!(addrs = qemuDomainPCIAddressSetCreate(def)))
> @@ -961,16 +967,33 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def)
>              goto cleanup;
>      }
>  
> +    if (obj && obj->privateData) {
> +        priv = obj->privateData;
> +        if (addrs) {
> +            /* if this is the live domain object, we persist the PCI addresses*/
> +            if (priv->pciaddrs) {
> +                qemuDomainPCIAddressSetFree(priv->pciaddrs);
> +                priv->pciaddrs = NULL;
> +            }

qemuDomainPCIAddressSetFree() handles passed NULL, so this check is redundant.

> +            priv->persistentAddrs = 1;
> +            priv->pciaddrs = addrs;
> +            addrs = NULL;
> +        } else {
> +            priv->persistentAddrs = 0;
> +        }
> +    }
> +
>      ret = 0;
>  
>  cleanup:
> -    qemuCapsFree(qemuCaps);
> +    qemuCapsFree(localCaps);
>      qemuDomainPCIAddressSetFree(addrs);
>  
>      return ret;
>  }
>  
> -int qemuDomainAssignAddresses(virDomainDefPtr def)
> +int qemuDomainAssignAddresses(virDomainDefPtr def, virBitmapPtr qemuCaps,
> +                              virDomainObjPtr obj)

Again. I'd prefer to have each argument on a separate line...

>  {
>      int rc;
>  
> @@ -978,7 +1001,7 @@ int qemuDomainAssignAddresses(virDomainDefPtr def)
>      if (rc)
>          return rc;
>  
> -    return qemuDomainAssignPCIAddresses(def);
> +    return qemuDomainAssignPCIAddresses(def, qemuCaps, obj);
>  }
>  
>  static void
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index 1eafeb3..dd104d6 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -175,10 +175,12 @@ virDomainDefPtr qemuParseCommandLinePid(virCapsPtr caps,
>                                          virDomainChrSourceDefPtr *monConfig,
>                                          bool *monJSON);
>  
> -int qemuDomainAssignAddresses(virDomainDefPtr def);
> +int qemuDomainAssignAddresses(virDomainDefPtr def, virBitmapPtr qemuCaps,
> +                              virDomainObjPtr);
>  int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def);
>  
> -int qemuDomainAssignPCIAddresses(virDomainDefPtr def);
> +int qemuDomainAssignPCIAddresses(virDomainDefPtr def, virBitmapPtr qemuCaps,
> +                                 virDomainObjPtr obj);
>  qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def);

... and same here.

>  int qemuDomainPCIAddressReserveFunction(qemuDomainPCIAddressSetPtr addrs,
>                                          int slot, int function);
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 2f93404..ef9983c 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1404,7 +1404,7 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml,
>      if (qemudCanonicalizeMachine(driver, def) < 0)
>          goto cleanup;
>  
> -    if (qemuDomainAssignAddresses(def) < 0)
> +    if (qemuDomainAssignAddresses(def, NULL, NULL) < 0)
>          goto cleanup;
>  
>      if (!(vm = virDomainAssignDef(driver->caps,
> @@ -5070,7 +5070,7 @@ static virDomainPtr qemudDomainDefine(virConnectPtr conn, const char *xml) {
>      if (qemudCanonicalizeMachine(driver, def) < 0)
>          goto cleanup;
>  
> -    if (qemuDomainAssignAddresses(def) < 0)
> +    if (qemuDomainAssignAddresses(def, NULL, NULL) < 0)
>          goto cleanup;
>  
>      if (!(vm = virDomainAssignDef(driver->caps,
> @@ -5548,7 +5548,7 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef,
>          if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO)
>              if (virDomainDefAddImplicitControllers(vmdef) < 0)
>                  return -1;
> -        if (qemuDomainAssignAddresses(vmdef) < 0)
> +        if (qemuDomainAssignAddresses(vmdef, NULL, NULL) < 0)
>              return -1;
>          break;
>  
> @@ -5566,7 +5566,7 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef,
>              return -1;
>          }
>          dev->data.net = NULL;
> -        if (qemuDomainAssignAddresses(vmdef) < 0)
> +        if (qemuDomainAssignAddresses(vmdef, NULL, NULL) < 0)
>              return -1;
>          break;
>  
> @@ -5582,7 +5582,7 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef,
>              return -1;
>          }
>          dev->data.hostdev = NULL;
> -        if (qemuDomainAssignAddresses(vmdef) < 0)
> +        if (qemuDomainAssignAddresses(vmdef, NULL, NULL) < 0)
>              return -1;
>          break;
>  
> @@ -5736,7 +5736,7 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
>          vmdef->nets[pos] = net;
>          dev->data.net = NULL;
>  
> -        if (qemuDomainAssignAddresses(vmdef) < 0)
> +        if (qemuDomainAssignAddresses(vmdef, NULL, NULL) < 0)
>              return -1;
>          break;
>  
> @@ -11734,7 +11734,7 @@ static virDomainPtr qemuDomainAttach(virConnectPtr conn,
>      if (qemudCanonicalizeMachine(driver, def) < 0)
>          goto cleanup;
>  
> -    if (qemuDomainAssignAddresses(def) < 0)
> +    if (qemuDomainAssignAddresses(def, NULL, NULL) < 0)
>          goto cleanup;
>  
>      if (!(vm = virDomainAssignDef(driver->caps,
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index c5140c3..bf32487 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3090,13 +3090,8 @@ qemuProcessReconnect(void *opaque)
>          goto endjob;
>      }
>  
> -    if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
> -        priv->persistentAddrs = 1;
> -
> -        if (!(priv->pciaddrs = qemuDomainPCIAddressSetCreate(obj->def)) ||
> -            qemuAssignDevicePCISlots(obj->def, priv->pciaddrs) < 0)
> -            goto error;
> -    }
> +    if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE))
> +        qemuDomainAssignAddresses(obj->def, priv->qemuCaps, obj);
>  
>      if (virSecurityManagerReserveLabel(driver->securityManager, obj->def, obj->pid) < 0)
>          goto error;
> @@ -3560,22 +3555,7 @@ int qemuProcessStart(virConnectPtr conn,
>       */
>      if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
>          VIR_DEBUG("Assigning domain PCI addresses");
> -        /* Populate cache with current addresses */
> -        if (priv->pciaddrs) {
> -            qemuDomainPCIAddressSetFree(priv->pciaddrs);
> -            priv->pciaddrs = NULL;
> -        }
> -        if (!(priv->pciaddrs = qemuDomainPCIAddressSetCreate(vm->def)))
> -            goto cleanup;
> -
> -
> -        /* Assign any remaining addresses */
> -        if (qemuAssignDevicePCISlots(vm->def, priv->pciaddrs) < 0)
> -            goto cleanup;
> -
> -        priv->persistentAddrs = 1;
> -    } else {
> -        priv->persistentAddrs = 0;
> +        qemuDomainAssignAddresses(vm->def, priv->qemuCaps, vm);
>      }
>  
>      VIR_DEBUG("Building emulator command line");
> @@ -4257,21 +4237,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
>       */
>      if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
>          VIR_DEBUG("Assigning domain PCI addresses");
> -        /* Populate cache with current addresses */
> -        if (priv->pciaddrs) {
> -            qemuDomainPCIAddressSetFree(priv->pciaddrs);
> -            priv->pciaddrs = NULL;
> -        }
> -        if (!(priv->pciaddrs = qemuDomainPCIAddressSetCreate(vm->def)))
> -            goto cleanup;
> -
> -        /* Assign any remaining addresses */
> -        if (qemuAssignDevicePCISlots(vm->def, priv->pciaddrs) < 0)
> -            goto cleanup;
> -
> -        priv->persistentAddrs = 1;
> -    } else {
> -        priv->persistentAddrs = 0;
> +        qemuDomainAssignAddresses(vm->def, priv->qemuCaps, vm);
>      }
>  
>      if ((timestamp = virTimeStringNow()) == NULL) {
> 

ACK with this squashed in:

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index a6fc9ca..bcc2192 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -942,7 +942,8 @@ cleanup:
 
 
 int
-qemuDomainAssignPCIAddresses(virDomainDefPtr def, virBitmapPtr qemuCaps,
+qemuDomainAssignPCIAddresses(virDomainDefPtr def,
+                             virBitmapPtr qemuCaps,
                              virDomainObjPtr obj)
 {
     int ret = -1;
@@ -971,10 +972,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, virBitmapPtr qemuCaps,
         priv = obj->privateData;
         if (addrs) {
             /* if this is the live domain object, we persist the PCI addresses*/
-            if (priv->pciaddrs) {
-                qemuDomainPCIAddressSetFree(priv->pciaddrs);
-                priv->pciaddrs = NULL;
-            }
+            qemuDomainPCIAddressSetFree(priv->pciaddrs);
             priv->persistentAddrs = 1;
             priv->pciaddrs = addrs;
             addrs = NULL;
@@ -992,7 +990,8 @@ cleanup:
     return ret;
 }
 
-int qemuDomainAssignAddresses(virDomainDefPtr def, virBitmapPtr qemuCaps,
+int qemuDomainAssignAddresses(virDomainDefPtr def,
+                              virBitmapPtr qemuCaps,
                               virDomainObjPtr obj)
 {
     int rc;
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index dd104d6..ddf426f 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -175,11 +175,13 @@ virDomainDefPtr qemuParseCommandLinePid(virCapsPtr caps,
                                         virDomainChrSourceDefPtr *monConfig,
                                         bool *monJSON);
 
-int qemuDomainAssignAddresses(virDomainDefPtr def, virBitmapPtr qemuCaps,
+int qemuDomainAssignAddresses(virDomainDefPtr def,
+                              virBitmapPtr qemuCaps,
                               virDomainObjPtr);
 int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def);
 
-int qemuDomainAssignPCIAddresses(virDomainDefPtr def, virBitmapPtr qemuCaps,
+int qemuDomainAssignPCIAddresses(virDomainDefPtr def,
+                                 virBitmapPtr qemuCaps,
                                  virDomainObjPtr obj);
 qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def);
 int qemuDomainPCIAddressReserveFunction(qemuDomainPCIAddressSetPtr addrs,


Michal


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