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

Re: [libvirt] [PATCH v2 04/14] libxl: support backend domain setting for disk and net devices



Marek Marczykowski-Górecki wrote:
> This implement handling of <domain name=''/> parameter introduced in
> previous patch.
>
> Lookup on domain name (to get domain ID) requires libxlDriverPrivate
> object, so it must be passed down to libxlMakeDisk and libxlMakeNet from
> top level callers.
>
> Changes in v2:
>  - rebase on 1.0.6+
>  - fix indentation
>  - make libxl_name_to_domid switch more defensive
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek invisiblethingslab com>
> ---
>  src/libxl/libxl_conf.c   | 76 +++++++++++++++++++++++++++++++++++++++++++-----
>  src/libxl/libxl_conf.h   |  4 +--
>  src/libxl/libxl_driver.c | 51 +++++++++++++++++++-------------
>  3 files changed, 101 insertions(+), 30 deletions(-)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index b9cb61e..623956e 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -441,7 +441,9 @@ error:
>  }
>  
>  int
> -libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk *x_disk)
> +libxlMakeDisk(libxlDriverPrivatePtr driver,
>   

After looking at this again, I wonder if it is better to just pass the
libxl_ctx to libxlMakeDisk and libxlMakeNic.  All the callers already
have a libxl_ctx, making for a much smaller patch.  What do you think?

Regards,
Jim

> +              virDomainDiskDefPtr l_disk,
> +              libxl_device_disk *x_disk)
>  {
>      if (VIR_STRDUP(x_disk->pdev_path, l_disk->src) < 0)
>          return -1;
> @@ -509,11 +511,39 @@ libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk *x_disk)
>          return -1;
>      }
>  
> +    if (l_disk->domain_name) {
> +        uint32_t domid;
> +        /* Do not use virDomainObjListFindByName as it causes deadlock here -
> +         * we already have lock on this domain object, but
> +         * virDomainObjListFindByName will try to take it again.
> +         */
> +        switch (libxl_name_to_domid(driver->ctx, l_disk->domain_name, &domid)) {
> +            case 0:
> +                x_disk->backend_domid = domid;
> +                break;
> +            case ERROR_INVAL:
> +                virReportError(VIR_ERR_XML_DETAIL,
> +                        _("Backend domain '%s' does not exists for disk '%s'"),
> +                        l_disk->domain_name, l_disk->dst);
> +                return -1;
> +            case ERROR_NOMEM:
> +                virReportOOMError();
> +                return -1;
> +            default:
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                        _("Failed to get ID of domain '%s'"),
> +                        l_disk->domain_name);
> +                return -1;
> +        }
> +    }
> +
>      return 0;
>  }
>  
>  static int
> -libxlMakeDiskList(virDomainDefPtr def, libxl_domain_config *d_config)
> +libxlMakeDiskList(libxlDriverPrivatePtr driver,
> +                  virDomainDefPtr def,
> +                  libxl_domain_config *d_config)
>  {
>      virDomainDiskDefPtr *l_disks = def->disks;
>      int ndisks = def->ndisks;
> @@ -526,7 +556,7 @@ libxlMakeDiskList(virDomainDefPtr def, libxl_domain_config *d_config)
>      }
>  
>      for (i = 0; i < ndisks; i++) {
> -        if (libxlMakeDisk(l_disks[i], &x_disks[i]) < 0)
> +        if (libxlMakeDisk(driver, l_disks[i], &x_disks[i]) < 0)
>              goto error;
>      }
>  
> @@ -543,7 +573,9 @@ error:
>  }
>  
>  int
> -libxlMakeNic(virDomainNetDefPtr l_nic, libxl_device_nic *x_nic)
> +libxlMakeNic(libxlDriverPrivatePtr driver,
> +             virDomainNetDefPtr l_nic,
> +             libxl_device_nic *x_nic)
>  {
>      /* TODO: Where is mtu stored?
>       *
> @@ -579,11 +611,39 @@ libxlMakeNic(virDomainNetDefPtr l_nic, libxl_device_nic *x_nic)
>              return -1;
>      }
>  
> +    if (l_nic->domain_name) {
> +        uint32_t domid;
> +        /* Do not use virDomainObjListFindByName as it causes deadlock here -
> +         * we already have lock on this domain object, but
> +         * virDomainObjListFindByName will try to take it again.
> +         */
> +        switch (libxl_name_to_domid(driver->ctx, l_nic->domain_name, &domid)) {
> +            case 0:
> +                x_nic->backend_domid = domid;
> +                break;
> +            case ERROR_INVAL:
> +                virReportError(VIR_ERR_XML_DETAIL,
> +                        _("Backend domain '%s' does not exists for nic '%s'"),
> +                        l_nic->domain_name, l_nic->ifname);
> +                return -1;
> +            case ERROR_NOMEM:
> +                virReportOOMError();
> +                return -1;
> +            default:
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                        _("Failed to get ID of domain '%s'"),
> +                        l_nic->domain_name);
> +                return -1;
> +        }
> +    }
> +
>      return 0;
>  }
>  
>  static int
> -libxlMakeNicList(virDomainDefPtr def,  libxl_domain_config *d_config)
> +libxlMakeNicList(libxlDriverPrivatePtr driver,
> +                 virDomainDefPtr def,
> +                 libxl_domain_config *d_config)
>  {
>      virDomainNetDefPtr *l_nics = def->nets;
>      int nnics = def->nnets;
> @@ -598,7 +658,7 @@ libxlMakeNicList(virDomainDefPtr def,  libxl_domain_config *d_config)
>      for (i = 0; i < nnics; i++) {
>          x_nics[i].devid = i;
>  
> -        if (libxlMakeNic(l_nics[i], &x_nics[i]))
> +        if (libxlMakeNic(driver, l_nics[i], &x_nics[i]))
>              goto error;
>      }
>  
> @@ -756,11 +816,11 @@ libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
>          return -1;
>      }
>  
> -    if (libxlMakeDiskList(def, d_config) < 0) {
> +    if (libxlMakeDiskList(driver, def, d_config) < 0) {
>          return -1;
>      }
>  
> -    if (libxlMakeNicList(def, d_config) < 0) {
> +    if (libxlMakeNicList(driver, def, d_config) < 0) {
>          return -1;
>      }
>  
> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
> index e8fb9c4..33126f3 100644
> --- a/src/libxl/libxl_conf.h
> +++ b/src/libxl/libxl_conf.h
> @@ -115,9 +115,9 @@ virCapsPtr
>  libxlMakeCapabilities(libxl_ctx *ctx);
>  
>  int
> -libxlMakeDisk(virDomainDiskDefPtr l_dev, libxl_device_disk *x_dev);
> +libxlMakeDisk(libxlDriverPrivatePtr driver, virDomainDiskDefPtr l_dev, libxl_device_disk *x_dev);
>  int
> -libxlMakeNic(virDomainNetDefPtr l_nic, libxl_device_nic *x_nic);
> +libxlMakeNic(libxlDriverPrivatePtr driver, virDomainNetDefPtr l_nic, libxl_device_nic *x_nic);
>  int
>  libxlMakeVfb(libxlDriverPrivatePtr driver,
>               virDomainGraphicsDefPtr l_vfb, libxl_device_vfb *x_vfb);
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 0a0690d..df31001 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -3233,8 +3233,10 @@ libxlDomainUndefine(virDomainPtr dom)
>  }
>  
>  static int
> -libxlDomainChangeEjectableMedia(libxlDomainObjPrivatePtr priv,
> -                                virDomainObjPtr vm, virDomainDiskDefPtr disk)
> +libxlDomainChangeEjectableMedia(libxlDriverPrivatePtr driver,
> +                                libxlDomainObjPrivatePtr priv,
> +                                virDomainObjPtr vm,
> +                                virDomainDiskDefPtr disk)
>  {
>      virDomainDiskDefPtr origdisk = NULL;
>      libxl_device_disk x_disk;
> @@ -3263,7 +3265,7 @@ libxlDomainChangeEjectableMedia(libxlDomainObjPrivatePtr priv,
>          return -1;
>      }
>  
> -    if (libxlMakeDisk(disk, &x_disk) < 0)
> +    if (libxlMakeDisk(driver, disk, &x_disk) < 0)
>          goto cleanup;
>  
>      if ((ret = libxl_cdrom_insert(priv->ctx, vm->def->id, &x_disk, NULL)) < 0) {
> @@ -3288,8 +3290,10 @@ cleanup:
>  }
>  
>  static int
> -libxlDomainAttachDeviceDiskLive(libxlDomainObjPrivatePtr priv,
> -                                virDomainObjPtr vm, virDomainDeviceDefPtr dev)
> +libxlDomainAttachDeviceDiskLive(libxlDriverPrivatePtr driver,
> +                                libxlDomainObjPrivatePtr priv,
> +                                virDomainObjPtr vm,
> +                                virDomainDeviceDefPtr dev)
>  {
>      virDomainDiskDefPtr l_disk = dev->data.disk;
>      libxl_device_disk x_disk;
> @@ -3297,7 +3301,7 @@ libxlDomainAttachDeviceDiskLive(libxlDomainObjPrivatePtr priv,
>  
>      switch (l_disk->device)  {
>          case VIR_DOMAIN_DISK_DEVICE_CDROM:
> -            ret = libxlDomainChangeEjectableMedia(priv, vm, l_disk);
> +            ret = libxlDomainChangeEjectableMedia(driver, priv, vm, l_disk);
>              break;
>          case VIR_DOMAIN_DISK_DEVICE_DISK:
>              if (l_disk->bus == VIR_DOMAIN_DISK_BUS_XEN) {
> @@ -3318,7 +3322,7 @@ libxlDomainAttachDeviceDiskLive(libxlDomainObjPrivatePtr priv,
>                      goto cleanup;
>                  }
>  
> -                if (libxlMakeDisk(l_disk, &x_disk) < 0)
> +                if (libxlMakeDisk(driver, l_disk, &x_disk) < 0)
>                      goto cleanup;
>  
>                  if ((ret = libxl_device_disk_add(priv->ctx, vm->def->id,
> @@ -3349,8 +3353,10 @@ cleanup:
>  }
>  
>  static int
> -libxlDomainDetachDeviceDiskLive(libxlDomainObjPrivatePtr priv,
> -                                virDomainObjPtr vm, virDomainDeviceDefPtr dev)
> +libxlDomainDetachDeviceDiskLive(libxlDriverPrivatePtr driver,
> +                                libxlDomainObjPrivatePtr priv,
> +                                virDomainObjPtr vm,
> +                                virDomainDeviceDefPtr dev)
>  {
>      virDomainDiskDefPtr l_disk = NULL;
>      libxl_device_disk x_disk;
> @@ -3371,7 +3377,7 @@ libxlDomainDetachDeviceDiskLive(libxlDomainObjPrivatePtr priv,
>  
>                  l_disk = vm->def->disks[i];
>  
> -                if (libxlMakeDisk(l_disk, &x_disk) < 0)
> +                if (libxlMakeDisk(driver, l_disk, &x_disk) < 0)
>                      goto cleanup;
>  
>                  if ((ret = libxl_device_disk_remove(priv->ctx, vm->def->id,
> @@ -3403,14 +3409,15 @@ cleanup:
>  }
>  
>  static int
> -libxlDomainAttachDeviceLive(libxlDomainObjPrivatePtr priv, virDomainObjPtr vm,
> +libxlDomainAttachDeviceLive(libxlDriverPrivatePtr driver,
> +                            libxlDomainObjPrivatePtr priv, virDomainObjPtr vm,
>                              virDomainDeviceDefPtr dev)
>  {
>      int ret = -1;
>  
>      switch (dev->type) {
>          case VIR_DOMAIN_DEVICE_DISK:
> -            ret = libxlDomainAttachDeviceDiskLive(priv, vm, dev);
> +            ret = libxlDomainAttachDeviceDiskLive(driver, priv, vm, dev);
>              if (!ret)
>                  dev->data.disk = NULL;
>              break;
> @@ -3455,14 +3462,16 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev)
>  }
>  
>  static int
> -libxlDomainDetachDeviceLive(libxlDomainObjPrivatePtr priv, virDomainObjPtr vm,
> +libxlDomainDetachDeviceLive(libxlDriverPrivatePtr driver,
> +                            libxlDomainObjPrivatePtr priv,
> +                            virDomainObjPtr vm,
>                              virDomainDeviceDefPtr dev)
>  {
>      int ret = -1;
>  
>      switch (dev->type) {
>          case VIR_DOMAIN_DEVICE_DISK:
> -            ret = libxlDomainDetachDeviceDiskLive(priv, vm, dev);
> +            ret = libxlDomainDetachDeviceDiskLive(driver, priv, vm, dev);
>              break;
>  
>          default:
> @@ -3502,8 +3511,10 @@ libxlDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev)
>  }
>  
>  static int
> -libxlDomainUpdateDeviceLive(libxlDomainObjPrivatePtr priv,
> -                            virDomainObjPtr vm, virDomainDeviceDefPtr dev)
> +libxlDomainUpdateDeviceLive(libxlDriverPrivatePtr driver,
> +                            libxlDomainObjPrivatePtr priv,
> +                            virDomainObjPtr vm,
> +                            virDomainDeviceDefPtr dev)
>  {
>      virDomainDiskDefPtr disk;
>      int ret = -1;
> @@ -3513,7 +3524,7 @@ libxlDomainUpdateDeviceLive(libxlDomainObjPrivatePtr priv,
>              disk = dev->data.disk;
>              switch (disk->device) {
>                  case VIR_DOMAIN_DISK_DEVICE_CDROM:
> -                    ret = libxlDomainChangeEjectableMedia(priv, vm, disk);
> +                    ret = libxlDomainChangeEjectableMedia(driver, priv, vm, disk);
>                      if (ret == 0)
>                          dev->data.disk = NULL;
>                      break;
> @@ -3650,7 +3661,7 @@ libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
>                                              VIR_DOMAIN_XML_INACTIVE)))
>              goto cleanup;
>  
> -        if ((ret = libxlDomainAttachDeviceLive(priv, vm, dev)) < 0)
> +        if ((ret = libxlDomainAttachDeviceLive(driver, priv, vm, dev)) < 0)
>              goto cleanup;
>  
>          /*
> @@ -3755,7 +3766,7 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml,
>                                              VIR_DOMAIN_XML_INACTIVE)))
>              goto cleanup;
>  
> -        if ((ret = libxlDomainDetachDeviceLive(priv, vm, dev)) < 0)
> +        if ((ret = libxlDomainDetachDeviceLive(driver, priv, vm, dev)) < 0)
>              goto cleanup;
>  
>          /*
> @@ -3860,7 +3871,7 @@ libxlDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml,
>                                              VIR_DOMAIN_XML_INACTIVE)))
>              goto cleanup;
>  
> -        if ((ret = libxlDomainUpdateDeviceLive(priv, vm, dev)) < 0)
> +        if ((ret = libxlDomainUpdateDeviceLive(driver, priv, vm, dev)) < 0)
>              goto cleanup;
>  
>          /*
>   


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