Marek Marczykowski 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.
> ---
> src/libxl/libxl_conf.c | 62 +++++++++++++++++++++++++++++++++++++++++-------
> src/libxl/libxl_conf.h | 4 ++--
> src/libxl/libxl_driver.c | 50 +++++++++++++++++++++-----------------
> 3 files changed, 84 insertions(+), 32 deletions(-)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 4bd62e9..477e46d 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -472,7 +472,9 @@ error:
> }
>
> int
> -libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk *x_disk)
> +libxlMakeDisk(libxlDriverPrivatePtr driver,
> + virDomainDiskDefPtr l_disk,
> + libxl_device_disk *x_disk)
>
Indentation looks off here.
> {
> if (l_disk->src && (x_disk->pdev_path = strdup(l_disk->src)) == NULL) {
> virReportOOMError();
> @@ -549,11 +551,32 @@ 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 ERROR_INVAL:
> + virReportError(VIR_ERR_XML_DETAIL,
> + _("Disk backend domain '%s' does not exists"),
>
I think this error would be better worded with "Backend domain '%s' does
not exist for disk '%s'", using l_disk->dst for the disk name.
> + l_disk->domain_name);
> + return -1;
> + case ERROR_NOMEM:
> + virReportOOMError();
> + return -1;
> + }
> + x_disk->backend_domid = domid;
>
IMO this could be coded a little more defensively, e.g. only assign
backend_domid on "case 0:", and have a default case that will also fail
if libxl_name_to_domid() ever returns something besides 0, ERROR_INVAL,
or ERROR_NOMEM.
> + }
> +
> return 0;
> }
>
> static int
> -libxlMakeDiskList(virDomainDefPtr def, libxl_domain_config *d_config)
> +libxlMakeDiskList(libxlDriverPrivatePtr driver,
> + virDomainDefPtr def,
> + libxl_domain_config *d_config)
>
Indentation.
> {
> virDomainDiskDefPtr *l_disks = def->disks;
> int ndisks = def->ndisks;
> @@ -566,7 +589,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;
> }
>
> @@ -583,7 +606,9 @@ error:
> }
>
> int
> -libxlMakeNic(virDomainNetDefPtr l_nic, libxl_device_nic *x_nic)
> +libxlMakeNic(libxlDriverPrivatePtr driver,
> + virDomainNetDefPtr l_nic,
> + libxl_device_nic *x_nic)
>
Indentation.
> {
> /* TODO: Where is mtu stored?
> *
> @@ -620,11 +645,32 @@ 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 ERROR_INVAL:
> + virReportError(VIR_ERR_XML_DETAIL,
> + _("Network backend domain '%s' does not exists"),
>
Same comment here about the error message.
> + l_nic->domain_name);
> + return -1;
> + case ERROR_NOMEM:
> + virReportOOMError();
> + return -1;
> + }
> + x_nic->backend_domid = domid;
>
And same comment here about the switch statement.
> + }
> +
> return 0;
> }
>
> static int
> -libxlMakeNicList(virDomainDefPtr def, libxl_domain_config *d_config)
> +libxlMakeNicList(libxlDriverPrivatePtr driver,
> + virDomainDefPtr def,
> + libxl_domain_config *d_config)
>
Indentation.
> {
> virDomainNetDefPtr *l_nics = def->nets;
> int nnics = def->nnets;
> @@ -639,7 +685,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;
> }
>
> @@ -878,11 +924,11 @@ libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
> goto error;
> }
>
> - if (libxlMakeDiskList(def, d_config) < 0) {
> + if (libxlMakeDiskList(driver, def, d_config) < 0) {
> goto error;
> }
>
> - if (libxlMakeNicList(def, d_config) < 0) {
> + if (libxlMakeNicList(driver, def, d_config) < 0) {
> goto error;
> }
>
> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
> index b3ab3bf..99948b5 100644
> --- a/src/libxl/libxl_conf.h
> +++ b/src/libxl/libxl_conf.h
> @@ -113,9 +113,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);
>
This is more than 80 columns now.
> int
> -libxlMakeNic(virDomainNetDefPtr l_nic, libxl_device_nic *x_nic);
> +libxlMakeNic(libxlDriverPrivatePtr driver, virDomainNetDefPtr l_nic, libxl_device_nic *x_nic);
>
Same here.
> 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 89546a5..b0f0c6a 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -3166,8 +3166,9 @@ libxlDomainUndefine(virDomainPtr dom)
> }
>
> static int
> -libxlDomainChangeEjectableMedia(libxlDomainObjPrivatePtr priv,
> - virDomainObjPtr vm, virDomainDiskDefPtr disk)
> +libxlDomainChangeEjectableMedia(libxlDriverPrivatePtr driver,
> + libxlDomainObjPrivatePtr priv, virDomainObjPtr vm, virDomainDiskDefPtr
> + disk)
>
Whitespace issues here. There are no strict guidelines for the
whitespace format of function definitions/declarations, but the dominate
pattern in libvirt seems to be return type on first line, then name and
params on next line. If name and params exceeds 80 columns, params 2..n
get their own line. I've tried to move to this pattern in the libxl
driver as I touch offending code.
> {
> virDomainDiskDefPtr origdisk = NULL;
> libxl_device_disk x_disk;
> @@ -3196,7 +3197,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) {
> @@ -3221,8 +3222,9 @@ cleanup:
> }
>
> static int
> -libxlDomainAttachDeviceDiskLive(libxlDomainObjPrivatePtr priv,
> - virDomainObjPtr vm, virDomainDeviceDefPtr dev)
> +libxlDomainAttachDeviceDiskLive(libxlDriverPrivatePtr driver,
> + libxlDomainObjPrivatePtr priv, virDomainObjPtr vm,
> + virDomainDeviceDefPtr dev)
>
Indentation.
> {
> virDomainDiskDefPtr l_disk = dev->data.disk;
> libxl_device_disk x_disk;
> @@ -3230,7 +3232,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) {
> @@ -3251,7 +3253,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,
> @@ -3282,8 +3284,9 @@ cleanup:
> }
>
> static int
> -libxlDomainDetachDeviceDiskLive(libxlDomainObjPrivatePtr priv,
> - virDomainObjPtr vm, virDomainDeviceDefPtr dev)
> +libxlDomainDetachDeviceDiskLive(libxlDriverPrivatePtr driver,
> + libxlDomainObjPrivatePtr priv, virDomainObjPtr vm,
> + virDomainDeviceDefPtr dev)
>
Indentation.
> {
> virDomainDiskDefPtr l_disk = NULL;
> libxl_device_disk x_disk;
> @@ -3304,7 +3307,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,
> @@ -3336,14 +3339,15 @@ cleanup:
> }
>
> static int
> -libxlDomainAttachDeviceLive(libxlDomainObjPrivatePtr priv, virDomainObjPtr vm,
> - virDomainDeviceDefPtr dev)
> +libxlDomainAttachDeviceLive(libxlDriverPrivatePtr driver,
> + libxlDomainObjPrivatePtr priv, virDomainObjPtr vm,
> + virDomainDeviceDefPtr dev)
>
Indentation.
> {
> 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;
> @@ -3388,14 +3392,15 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev)
> }
>
> static int
> -libxlDomainDetachDeviceLive(libxlDomainObjPrivatePtr priv, virDomainObjPtr vm,
> - virDomainDeviceDefPtr dev)
> +libxlDomainDetachDeviceLive(libxlDriverPrivatePtr driver,
> + libxlDomainObjPrivatePtr priv, virDomainObjPtr vm,
> + virDomainDeviceDefPtr dev)
>
Indentation.
> {
> int ret = -1;
>
> switch (dev->type) {
> case VIR_DOMAIN_DEVICE_DISK:
> - ret = libxlDomainDetachDeviceDiskLive(priv, vm, dev);
> + ret = libxlDomainDetachDeviceDiskLive(driver, priv, vm, dev);
> break;
>
> default:
> @@ -3435,8 +3440,9 @@ libxlDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev)
> }
>
> static int
> -libxlDomainUpdateDeviceLive(libxlDomainObjPrivatePtr priv,
> - virDomainObjPtr vm, virDomainDeviceDefPtr dev)
> +libxlDomainUpdateDeviceLive(libxlDriverPrivatePtr driver,
> + libxlDomainObjPrivatePtr priv, virDomainObjPtr vm,
> + virDomainDeviceDefPtr dev)
>
Indentation.
Looks good with the exception of the nits. Depends on patch 7 of course...
Regards,
Jim
> {
> virDomainDiskDefPtr disk;
> int ret = -1;
> @@ -3446,7 +3452,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;
> @@ -3601,13 +3607,13 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml,
>
> switch (action) {
> case LIBXL_DEVICE_ATTACH:
> - ret = libxlDomainAttachDeviceLive(priv, vm, dev);
> + ret = libxlDomainAttachDeviceLive(driver, priv, vm, dev);
> break;
> case LIBXL_DEVICE_DETACH:
> - ret = libxlDomainDetachDeviceLive(priv, vm, dev);
> + ret = libxlDomainDetachDeviceLive(driver, priv, vm, dev);
> break;
> case LIBXL_DEVICE_UPDATE:
> - ret = libxlDomainUpdateDeviceLive(priv, vm, dev);
> + ret = libxlDomainUpdateDeviceLive(driver, priv, vm, dev);
> break;
> default:
> virReportError(VIR_ERR_INTERNAL_ERROR,
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list