Use virStringSplitCount instead of virStringSplit so that we can drop the call to virStringListLength and use VIR_AUTOSTRINGLIST to declare it and allow removal of the cleanup section. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/domain_conf.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c95bd34fb5..6124c7520c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -31327,7 +31327,8 @@ virDomainDiskAddISCSIPoolSourceHost(virDomainDiskDefPtr def, virStoragePoolDefPtr pooldef) { int ret = -1; - char **tokens = NULL; + VIR_AUTOSTRINGLIST tokens = NULL; + size_t ntokens; /* Only support one host */ if (pooldef->source.nhost != 1) { @@ -31348,10 +31349,10 @@ virDomainDiskAddISCSIPoolSourceHost(virDomainDiskDefPtr def, pooldef->source.hosts[0].port : 3260; /* iscsi volume has name like "unit:0:0:1" */ - if (!(tokens = virStringSplit(def->src->srcpool->volume, ":", 0))) + if (!(tokens = virStringSplitCount(def->src->srcpool->volume, ":", 0, &ntokens))) goto cleanup; - if (virStringListLength((const char * const *)tokens) != 4) { + if (ntokens != 4) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected iscsi volume name '%s'"), def->src->srcpool->volume); @@ -31373,7 +31374,6 @@ virDomainDiskAddISCSIPoolSourceHost(virDomainDiskDefPtr def, ret = 0; cleanup: - virStringListFree(tokens); return ret; } -- 2.24.1

Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/domain_conf.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6124c7520c..7cf555596f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -31339,9 +31339,7 @@ virDomainDiskAddISCSIPoolSourceHost(virDomainDiskDefPtr def, /* iscsi pool only supports one host */ def->src->nhosts = 1; - - if (VIR_ALLOC_N(def->src->hosts, def->src->nhosts) < 0) - goto cleanup; + def->src->hosts = g_new0(virStorageNetHostDef, 1); def->src->hosts[0].name = g_strdup(pooldef->source.hosts[0].name); -- 2.24.1

Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/domain_conf.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7cf555596f..1c52d7f347 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -31326,7 +31326,6 @@ static int virDomainDiskAddISCSIPoolSourceHost(virDomainDiskDefPtr def, virStoragePoolDefPtr pooldef) { - int ret = -1; VIR_AUTOSTRINGLIST tokens = NULL; size_t ntokens; @@ -31334,7 +31333,7 @@ virDomainDiskAddISCSIPoolSourceHost(virDomainDiskDefPtr def, if (pooldef->source.nhost != 1) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Expected exactly 1 host for the storage pool")); - goto cleanup; + return -1; } /* iscsi pool only supports one host */ @@ -31348,13 +31347,13 @@ virDomainDiskAddISCSIPoolSourceHost(virDomainDiskDefPtr def, /* iscsi volume has name like "unit:0:0:1" */ if (!(tokens = virStringSplitCount(def->src->srcpool->volume, ":", 0, &ntokens))) - goto cleanup; + return -1; if (ntokens != 4) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected iscsi volume name '%s'"), def->src->srcpool->volume); - goto cleanup; + return -1; } /* iscsi pool has only one source device path */ @@ -31369,10 +31368,7 @@ virDomainDiskAddISCSIPoolSourceHost(virDomainDiskDefPtr def, def->src->protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI; - ret = 0; - - cleanup: - return ret; + return 0; } -- 2.24.1

Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/domain_conf.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1c52d7f347..5ef12c7ee7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -31342,8 +31342,10 @@ virDomainDiskAddISCSIPoolSourceHost(virDomainDiskDefPtr def, def->src->hosts[0].name = g_strdup(pooldef->source.hosts[0].name); - def->src->hosts[0].port = pooldef->source.hosts[0].port ? - pooldef->source.hosts[0].port : 3260; + if (pooldef->source.hosts[0].port != 0) + def->src->hosts[0].port = pooldef->source.hosts[0].port; + else + def->src->hosts[0].port = 3260; /* iscsi volume has name like "unit:0:0:1" */ if (!(tokens = virStringSplitCount(def->src->srcpool->volume, ":", 0, &ntokens))) -- 2.24.1

Only 'def->src' was ever used in this function. Use the source directly. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/domain_conf.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5ef12c7ee7..bcb17dcc6e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -31323,7 +31323,7 @@ virDomainNetResolveActualType(virDomainNetDefPtr iface) static int -virDomainDiskAddISCSIPoolSourceHost(virDomainDiskDefPtr def, +virDomainDiskAddISCSIPoolSourceHost(virStorageSourcePtr src, virStoragePoolDefPtr pooldef) { VIR_AUTOSTRINGLIST tokens = NULL; @@ -31337,38 +31337,38 @@ virDomainDiskAddISCSIPoolSourceHost(virDomainDiskDefPtr def, } /* iscsi pool only supports one host */ - def->src->nhosts = 1; - def->src->hosts = g_new0(virStorageNetHostDef, 1); + src->nhosts = 1; + src->hosts = g_new0(virStorageNetHostDef, 1); - def->src->hosts[0].name = g_strdup(pooldef->source.hosts[0].name); + src->hosts[0].name = g_strdup(pooldef->source.hosts[0].name); if (pooldef->source.hosts[0].port != 0) - def->src->hosts[0].port = pooldef->source.hosts[0].port; + src->hosts[0].port = pooldef->source.hosts[0].port; else - def->src->hosts[0].port = 3260; + src->hosts[0].port = 3260; /* iscsi volume has name like "unit:0:0:1" */ - if (!(tokens = virStringSplitCount(def->src->srcpool->volume, ":", 0, &ntokens))) + if (!(tokens = virStringSplitCount(src->srcpool->volume, ":", 0, &ntokens))) return -1; if (ntokens != 4) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected iscsi volume name '%s'"), - def->src->srcpool->volume); + src->srcpool->volume); return -1; } /* iscsi pool has only one source device path */ - def->src->path = g_strdup_printf("%s/%s", pooldef->source.devices[0].path, - tokens[3]); + src->path = g_strdup_printf("%s/%s", pooldef->source.devices[0].path, + tokens[3]); /* Storage pool have not supported these 2 attributes yet, * use the defaults. */ - def->src->hosts[0].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; - def->src->hosts[0].socket = NULL; + src->hosts[0].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; + src->hosts[0].socket = NULL; - def->src->protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI; + src->protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI; return 0; } @@ -31411,7 +31411,7 @@ virDomainDiskTranslateISCSIDirect(virDomainDiskDefPtr def, def->src->auth->secrettype = g_strdup(secrettype); } - if (virDomainDiskAddISCSIPoolSourceHost(def, pooldef) < 0) + if (virDomainDiskAddISCSIPoolSourceHost(def->src, pooldef) < 0) return -1; if (!def->src->initiator.iqn && pooldef->source.initiator.iqn && -- 2.24.1

Only 'def->src' was ever used in this function. Use the source directly. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/domain_conf.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bcb17dcc6e..4545c6b314 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -31375,18 +31375,18 @@ virDomainDiskAddISCSIPoolSourceHost(virStorageSourcePtr src, static int -virDomainDiskTranslateSourcePoolAuth(virDomainDiskDefPtr def, +virDomainDiskTranslateSourcePoolAuth(virStorageSourcePtr src, virStoragePoolSourcePtr source) { /* Only necessary when authentication set */ if (!source->auth) return 0; - def->src->auth = virStorageAuthDefCopy(source->auth); - if (!def->src->auth) + src->auth = virStorageAuthDefCopy(source->auth); + if (!src->auth) return -1; /* A <disk> doesn't use <auth type='%s', so clear that out for the disk */ - def->src->auth->authType = VIR_STORAGE_AUTH_TYPE_NONE; + src->auth->authType = VIR_STORAGE_AUTH_TYPE_NONE; return 0; } @@ -31398,7 +31398,7 @@ virDomainDiskTranslateISCSIDirect(virDomainDiskDefPtr def, def->src->srcpool->actualtype = VIR_STORAGE_TYPE_NETWORK; def->src->protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI; - if (virDomainDiskTranslateSourcePoolAuth(def, + if (virDomainDiskTranslateSourcePoolAuth(def->src, &pooldef->source) < 0) return -1; -- 2.24.1

Only 'def->src' was ever used in this function. Use the source directly. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/domain_conf.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4545c6b314..531c765032 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -31392,30 +31392,30 @@ virDomainDiskTranslateSourcePoolAuth(virStorageSourcePtr src, static int -virDomainDiskTranslateISCSIDirect(virDomainDiskDefPtr def, +virDomainDiskTranslateISCSIDirect(virStorageSourcePtr src, virStoragePoolDefPtr pooldef) { - def->src->srcpool->actualtype = VIR_STORAGE_TYPE_NETWORK; - def->src->protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI; + src->srcpool->actualtype = VIR_STORAGE_TYPE_NETWORK; + src->protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI; - if (virDomainDiskTranslateSourcePoolAuth(def->src, + if (virDomainDiskTranslateSourcePoolAuth(src, &pooldef->source) < 0) return -1; /* Source pool may not fill in the secrettype field, * so we need to do so here */ - if (def->src->auth && !def->src->auth->secrettype) { + if (src->auth && !src->auth->secrettype) { const char *secrettype = virSecretUsageTypeToString(VIR_SECRET_USAGE_TYPE_ISCSI); - def->src->auth->secrettype = g_strdup(secrettype); + src->auth->secrettype = g_strdup(secrettype); } - if (virDomainDiskAddISCSIPoolSourceHost(def->src, pooldef) < 0) + if (virDomainDiskAddISCSIPoolSourceHost(src, pooldef) < 0) return -1; - if (!def->src->initiator.iqn && pooldef->source.initiator.iqn && - virStorageSourceInitiatorCopy(&def->src->initiator, + if (!src->initiator.iqn && pooldef->source.initiator.iqn && + virStorageSourceInitiatorCopy(&src->initiator, &pooldef->source.initiator) < 0) { return -1; } @@ -31540,7 +31540,7 @@ virDomainDiskTranslateSourcePool(virDomainDiskDefPtr def) return -1; } - if (virDomainDiskTranslateISCSIDirect(def, pooldef) < 0) + if (virDomainDiskTranslateISCSIDirect(def->src, pooldef) < 0) return -1; break; @@ -31565,7 +31565,7 @@ virDomainDiskTranslateSourcePool(virDomainDiskDefPtr def) break; case VIR_STORAGE_SOURCE_POOL_MODE_DIRECT: - if (virDomainDiskTranslateISCSIDirect(def, pooldef) < 0) + if (virDomainDiskTranslateISCSIDirect(def->src, pooldef) < 0) return -1; break; } -- 2.24.1

Extract all the code setting up one storage source from the rest which sets up the whole disk. This will allow us to prepare the whole backing chain. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/domain_conf.c | 112 ++++++++++++++++++++--------------------- 1 file changed, 55 insertions(+), 57 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 531c765032..7bd86d67e5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -31424,37 +31424,28 @@ virDomainDiskTranslateISCSIDirect(virStorageSourcePtr src, } -int -virDomainDiskTranslateSourcePool(virDomainDiskDefPtr def) +static int +virDomainStorageSourceTranslateSourcePool(virStorageSourcePtr src, + virConnectPtr conn) { virStorageVolInfo info; g_autoptr(virStoragePoolDef) pooldef = NULL; g_autofree char *poolxml = NULL; - g_autoptr(virConnect) conn = NULL; g_autoptr(virStoragePool) pool = NULL; g_autoptr(virStorageVol) vol = NULL; - if (def->src->type != VIR_STORAGE_TYPE_VOLUME) - return 0; - - if (!def->src->srcpool) - return 0; - - if (!(conn = virGetConnectStorage())) - return -1; - - if (!(pool = virStoragePoolLookupByName(conn, def->src->srcpool->pool))) + if (!(pool = virStoragePoolLookupByName(conn, src->srcpool->pool))) return -1; if (virStoragePoolIsActive(pool) != 1) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("storage pool '%s' containing volume '%s' " "is not active"), - def->src->srcpool->pool, def->src->srcpool->volume); + src->srcpool->pool, src->srcpool->volume); return -1; } - if (!(vol = virStorageVolLookupByName(pool, def->src->srcpool->volume))) + if (!(vol = virStorageVolLookupByName(pool, src->srcpool->volume))) return -1; if (virStorageVolGetInfo(vol, &info) < 0) @@ -31466,22 +31457,22 @@ virDomainDiskTranslateSourcePool(virDomainDiskDefPtr def) if (!(pooldef = virStoragePoolDefParseString(poolxml))) return -1; - def->src->srcpool->pooltype = pooldef->type; - def->src->srcpool->voltype = info.type; + src->srcpool->pooltype = pooldef->type; + src->srcpool->voltype = info.type; - if (def->src->srcpool->mode && pooldef->type != VIR_STORAGE_POOL_ISCSI) { + if (src->srcpool->mode && pooldef->type != VIR_STORAGE_POOL_ISCSI) { virReportError(VIR_ERR_XML_ERROR, "%s", _("disk source mode is only valid when " "storage pool is of iscsi type")); return -1; } - VIR_FREE(def->src->path); - virStorageNetHostDefFree(def->src->nhosts, def->src->hosts); - def->src->nhosts = 0; - def->src->hosts = NULL; - virStorageAuthDefFree(def->src->auth); - def->src->auth = NULL; + VIR_FREE(src->path); + virStorageNetHostDefFree(src->nhosts, src->hosts); + src->nhosts = 0; + src->hosts = NULL; + virStorageAuthDefFree(src->auth); + src->auth = NULL; switch ((virStoragePoolType) pooldef->type) { case VIR_STORAGE_POOL_DIR: @@ -31492,32 +31483,24 @@ virDomainDiskTranslateSourcePool(virDomainDiskDefPtr def) case VIR_STORAGE_POOL_SCSI: case VIR_STORAGE_POOL_ZFS: case VIR_STORAGE_POOL_VSTORAGE: - if (!(def->src->path = virStorageVolGetPath(vol))) + if (!(src->path = virStorageVolGetPath(vol))) return -1; - if (def->startupPolicy && info.type != VIR_STORAGE_VOL_FILE) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("'startupPolicy' is only valid for " - "'file' type volume")); - return -1; - } - - switch (info.type) { case VIR_STORAGE_VOL_FILE: - def->src->srcpool->actualtype = VIR_STORAGE_TYPE_FILE; + src->srcpool->actualtype = VIR_STORAGE_TYPE_FILE; break; case VIR_STORAGE_VOL_DIR: - def->src->srcpool->actualtype = VIR_STORAGE_TYPE_DIR; + src->srcpool->actualtype = VIR_STORAGE_TYPE_DIR; break; case VIR_STORAGE_VOL_BLOCK: - def->src->srcpool->actualtype = VIR_STORAGE_TYPE_BLOCK; + src->srcpool->actualtype = VIR_STORAGE_TYPE_BLOCK; break; case VIR_STORAGE_VOL_PLOOP: - def->src->srcpool->actualtype = VIR_STORAGE_TYPE_FILE; + src->srcpool->actualtype = VIR_STORAGE_TYPE_FILE; break; case VIR_STORAGE_VOL_NETWORK: @@ -31533,39 +31516,25 @@ virDomainDiskTranslateSourcePool(virDomainDiskDefPtr def) break; case VIR_STORAGE_POOL_ISCSI_DIRECT: - if (def->startupPolicy) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("'startupPolicy' is only valid for " - "'file' type volume")); - return -1; - } - - if (virDomainDiskTranslateISCSIDirect(def->src, pooldef) < 0) + if (virDomainDiskTranslateISCSIDirect(src, pooldef) < 0) return -1; break; case VIR_STORAGE_POOL_ISCSI: - if (def->startupPolicy) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("'startupPolicy' is only valid for " - "'file' type volume")); - return -1; - } - - switch (def->src->srcpool->mode) { + switch (src->srcpool->mode) { case VIR_STORAGE_SOURCE_POOL_MODE_DEFAULT: case VIR_STORAGE_SOURCE_POOL_MODE_LAST: - def->src->srcpool->mode = VIR_STORAGE_SOURCE_POOL_MODE_HOST; + src->srcpool->mode = VIR_STORAGE_SOURCE_POOL_MODE_HOST; G_GNUC_FALLTHROUGH; case VIR_STORAGE_SOURCE_POOL_MODE_HOST: - def->src->srcpool->actualtype = VIR_STORAGE_TYPE_BLOCK; - if (!(def->src->path = virStorageVolGetPath(vol))) + src->srcpool->actualtype = VIR_STORAGE_TYPE_BLOCK; + if (!(src->path = virStorageVolGetPath(vol))) return -1; break; case VIR_STORAGE_SOURCE_POOL_MODE_DIRECT: - if (virDomainDiskTranslateISCSIDirect(def->src, pooldef) < 0) + if (virDomainDiskTranslateISCSIDirect(src, pooldef) < 0) return -1; break; } @@ -31587,6 +31556,35 @@ virDomainDiskTranslateSourcePool(virDomainDiskDefPtr def) } +int +virDomainDiskTranslateSourcePool(virDomainDiskDefPtr def) +{ + g_autoptr(virConnect) conn = NULL; + + if (def->src->type != VIR_STORAGE_TYPE_VOLUME) + return 0; + + if (!def->src->srcpool) + return 0; + + if (!(conn = virGetConnectStorage())) + return -1; + + if (virDomainStorageSourceTranslateSourcePool(def->src, conn) < 0) + return -1; + + if (def->startupPolicy != 0 && + virStorageSourceGetActualType(def->src) != VIR_STORAGE_VOL_FILE) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'startupPolicy' is only valid for " + "'file' type volume")); + return -1; + } + + return 0; +} + + /** * virDomainDiskGetDetectZeroesMode: * @discard: disk/image sector discard setting -- 2.24.1

Now that we accept full backing chains on input nothing should prevent users from also using disk type 'VOLUME' for specifying the backing images. Do the translation for the whole backing chain. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/domain_conf.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7bd86d67e5..46062e3969 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -31560,18 +31560,20 @@ int virDomainDiskTranslateSourcePool(virDomainDiskDefPtr def) { g_autoptr(virConnect) conn = NULL; + virStorageSourcePtr n; - if (def->src->type != VIR_STORAGE_TYPE_VOLUME) - return 0; - - if (!def->src->srcpool) - return 0; + for (n = def->src; virStorageSourceIsBacking(n); n = n->backingStore) { + if (n->type != VIR_STORAGE_TYPE_VOLUME || !n-> srcpool) + continue; - if (!(conn = virGetConnectStorage())) - return -1; + if (!conn) { + if (!(conn = virGetConnectStorage())) + return -1; + } - if (virDomainStorageSourceTranslateSourcePool(def->src, conn) < 0) - return -1; + if (virDomainStorageSourceTranslateSourcePool(n, conn) < 0) + return -1; + } if (def->startupPolicy != 0 && virStorageSourceGetActualType(def->src) != VIR_STORAGE_VOL_FILE) { -- 2.24.1