8:04 a.m.
Peter Krempa (4):
conf: storage: Introduce virStoragePoolSourceListFree
virStorageBackendISCSIDirectFindPoolSources: Use allocated
virStoragePoolSourceList
virISCSIDirectUpdateTargets: Rework to simplify cleanup and return
GStrv
virStorageBackendISCSIDirectFindPoolSources: Rework cleanup
src/conf/storage_conf.c | 16 ++++
src/conf/storage_conf.h | 5 ++
src/libvirt_private.syms | 1 +
src/storage/storage_backend_iscsi_direct.c | 87 +++++++++-------------
4 files changed, 56 insertions(+), 53 deletions(-)
--
2.31.1
8:04 a.m.
New subject: [PATCH 1/4] conf: storage: Introduce virStoragePoolSourceListFree
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/storage_conf.c | 16 ++++++++++++++++
src/conf/storage_conf.h | 5 +++++
src/libvirt_private.syms | 1 +
3 files changed, 22 insertions(+)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 0ecdb0969a..2aa9a3d8f9 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1803,3 +1803,19 @@ virStoragePoolSourceListFormat(virStoragePoolSourceList *def)
return virBufferContentAndReset(&buf);
}
+
+
+void
+virStoragePoolSourceListFree(virStoragePoolSourceList *list)
+{
+ size_t i;
+
+ if (!list)
+ return;
+
+ for (i = 0; i < list->nsources; i++)
+ virStoragePoolSourceClear(&list->sources[i]);
+
+ g_free(list->sources);
+ g_free(list);
+}
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
index 345026aa37..76efaac531 100644
--- a/src/conf/storage_conf.h
+++ b/src/conf/storage_conf.h
@@ -266,6 +266,11 @@ struct _virStoragePoolSourceList {
virStoragePoolSource *sources;
};
+void
+virStoragePoolSourceListFree(virStoragePoolSourceList *list);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStoragePoolSourceList, virStoragePoolSourceListFree);
+
+
virStoragePoolDef *
virStoragePoolDefParseXML(xmlXPathContextPtr ctxt);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 2efa787664..68e4b6aab8 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1048,6 +1048,7 @@ virStoragePoolSourceClear;
virStoragePoolSourceDeviceClear;
virStoragePoolSourceFree;
virStoragePoolSourceListFormat;
+virStoragePoolSourceListFree;
virStoragePoolSourceListNewSource;
virStoragePoolTypeFromString;
virStoragePoolTypeToString;
--
2.31.1
8:04 a.m.
New subject: [PATCH 2/4] virStorageBackendISCSIDirectFindPoolSources: Use allocated virStoragePoolSourceList
Using an allocated version together with copying the
host/initiator/device portions into it allows us to switch to automatic
clearing rather than open-coding it.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/storage/storage_backend_iscsi_direct.c | 42 ++++++++++------------
1 file changed, 19 insertions(+), 23 deletions(-)
diff --git a/src/storage/storage_backend_iscsi_direct.c
b/src/storage/storage_backend_iscsi_direct.c
index e4a14c3fd6..263db835ae 100644
--- a/src/storage/storage_backend_iscsi_direct.c
+++ b/src/storage/storage_backend_iscsi_direct.c
@@ -495,23 +495,21 @@ virStorageBackendISCSIDirectFindPoolSources(const char *srcSpec,
char **targets = NULL;
char *ret = NULL;
size_t i;
- virStoragePoolSourceList list = {
- .type = VIR_STORAGE_POOL_ISCSI_DIRECT,
- .nsources = 0,
- .sources = NULL
- };
+ g_autoptr(virStoragePoolSourceList) list = g_new0(virStoragePoolSourceList, 1);
g_autofree char *portal = NULL;
g_autoptr(virStoragePoolSource) source = NULL;
virCheckFlags(0, NULL);
+ list->type = VIR_STORAGE_POOL_ISCSI_DIRECT;
+
if (!srcSpec) {
virReportError(VIR_ERR_INVALID_ARG, "%s",
_("hostname must be specified for iscsi sources"));
return NULL;
}
- if (!(source = virStoragePoolDefParseSourceString(srcSpec, list.type)))
+ if (!(source = virStoragePoolDefParseSourceString(srcSpec, list->type)))
return NULL;
if (source->nhost != 1) {
@@ -532,30 +530,28 @@ virStorageBackendISCSIDirectFindPoolSources(const char *srcSpec,
if (virISCSIDirectScanTargets(source->initiator.iqn, portal, &ntargets,
&targets) < 0)
goto cleanup;
- list.sources = g_new0(virStoragePoolSource, ntargets);
+ list->sources = g_new0(virStoragePoolSource, ntargets);
for (i = 0; i < ntargets; i++) {
- list.sources[i].devices = g_new0(virStoragePoolSourceDevice, 1);
- list.sources[i].hosts = g_new0(virStoragePoolSourceHost, 1);
- list.sources[i].nhost = 1;
- list.sources[i].hosts[0] = source->hosts[0];
- list.sources[i].initiator = source->initiator;
- list.sources[i].ndevice = 1;
- list.sources[i].devices[0].path = targets[i];
- list.nsources++;
+ list->sources[i].hosts = g_new0(virStoragePoolSourceHost, 1);
+ list->sources[i].nhost = 1;
+ list->sources[i].hosts[0].name = g_strdup(source->hosts[0].name);
+ list->sources[i].hosts[0].port = source->hosts[0].port;
+
+ virStorageSourceInitiatorCopy(&list->sources[i].initiator,
+ &source->initiator);
+
+ list->sources[i].devices = g_new0(virStoragePoolSourceDevice, 1);
+ list->sources[i].ndevice = 1;
+ list->sources[i].devices[0].path = g_strdup(targets[i]);
+
+ list->nsources++;
}
- if (!(ret = virStoragePoolSourceListFormat(&list)))
+ if (!(ret = virStoragePoolSourceListFormat(list)))
goto cleanup;
cleanup:
- if (list.sources) {
- for (i = 0; i < ntargets; i++) {
- VIR_FREE(list.sources[i].hosts);
- VIR_FREE(list.sources[i].devices);
- }
- VIR_FREE(list.sources);
- }
for (i = 0; i < ntargets; i++)
VIR_FREE(targets[i]);
VIR_FREE(targets);
--
2.31.1
8:04 a.m.
New subject: [PATCH 3/4] virISCSIDirectUpdateTargets: Rework to simplify cleanup and return GStrv
Count the elements in advance rather than using VIR_APPEND_ELEMENT and
ensure that there's a NULL terminator for the string list so it's GStrv
compatible.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/storage/storage_backend_iscsi_direct.c | 29 ++++++++--------------
1 file changed, 11 insertions(+), 18 deletions(-)
diff --git a/src/storage/storage_backend_iscsi_direct.c
b/src/storage/storage_backend_iscsi_direct.c
index 263db835ae..2073a6df38 100644
--- a/src/storage/storage_backend_iscsi_direct.c
+++ b/src/storage/storage_backend_iscsi_direct.c
@@ -420,37 +420,30 @@ virISCSIDirectUpdateTargets(struct iscsi_context *iscsi,
size_t *ntargets,
char ***targets)
{
- int ret = -1;
struct iscsi_discovery_address *addr;
struct iscsi_discovery_address *tmp_addr;
- size_t tmp_ntargets = 0;
- char **tmp_targets = NULL;
+
+ *ntargets = 0;
if (!(addr = iscsi_discovery_sync(iscsi))) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to discover session: %s"),
iscsi_get_error(iscsi));
- return ret;
+ return -1;
}
- for (tmp_addr = addr; tmp_addr; tmp_addr = tmp_addr->next) {
- g_autofree char *target = NULL;
-
- target = g_strdup(tmp_addr->target_name);
+ for (tmp_addr = addr; tmp_addr; tmp_addr = tmp_addr->next)
+ (*ntargets)++;
- if (VIR_APPEND_ELEMENT(tmp_targets, tmp_ntargets, target) < 0)
- goto cleanup;
- }
+ *targets = g_new0(char *, ntargets + 1);
+ *ntargets = 0;
- *targets = g_steal_pointer(&tmp_targets);
- *ntargets = tmp_ntargets;
- tmp_ntargets = 0;
+ for (tmp_addr = addr; tmp_addr; tmp_addr = tmp_addr->next)
+ *targets[(*ntargets)++] = g_strdup(tmp_addr->target_name);
- ret = 0;
- cleanup:
iscsi_free_discovery_data(iscsi, addr);
- virStringListFreeCount(tmp_targets, tmp_ntargets);
- return ret;
+
+ return 0;
}
static int
--
2.31.1
8:34 a.m.
New subject: [PATCH 3/4] virISCSIDirectUpdateTargets: Rework to simplify cleanup and return GStrv
On 6/18/21 3:04 PM, Peter Krempa wrote:
Count the elements in advance rather than using VIR_APPEND_ELEMENT
and
ensure that there's a NULL terminator for the string list so it's GStrv
compatible.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/storage/storage_backend_iscsi_direct.c | 29 ++++++++--------------
1 file changed, 11 insertions(+), 18 deletions(-)
diff --git a/src/storage/storage_backend_iscsi_direct.c
b/src/storage/storage_backend_iscsi_direct.c
index 263db835ae..2073a6df38 100644
--- a/src/storage/storage_backend_iscsi_direct.c
+++ b/src/storage/storage_backend_iscsi_direct.c
@@ -420,37 +420,30 @@ virISCSIDirectUpdateTargets(struct iscsi_context *iscsi,
size_t *ntargets,
char ***targets)
{
- int ret = -1;
struct iscsi_discovery_address *addr;
struct iscsi_discovery_address *tmp_addr;
- size_t tmp_ntargets = 0;
- char **tmp_targets = NULL;
+
+ *ntargets = 0;
if (!(addr = iscsi_discovery_sync(iscsi))) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to discover session: %s"),
iscsi_get_error(iscsi));
- return ret;
+ return -1;
}
- for (tmp_addr = addr; tmp_addr; tmp_addr = tmp_addr->next) {
- g_autofree char *target = NULL;
-
- target = g_strdup(tmp_addr->target_name);
+ for (tmp_addr = addr; tmp_addr; tmp_addr = tmp_addr->next)
+ (*ntargets)++;
- if (VIR_APPEND_ELEMENT(tmp_targets, tmp_ntargets, target) < 0)
- goto cleanup;
- }
+ *targets = g_new0(char *, ntargets + 1);
I'm afraid ntargets + 1 will be too big on many systems. Please use:
*ntargets + 1
+ *ntargets = 0;
- *targets = g_steal_pointer(&tmp_targets);
- *ntargets = tmp_ntargets;
- tmp_ntargets = 0;
+ for (tmp_addr = addr; tmp_addr; tmp_addr = tmp_addr->next)
+ *targets[(*ntargets)++] = g_strdup(tmp_addr->target_name);
Re-using *ntargets makes this a bit harder to follow, consider using 'i' and only
filling *ntargets once.
Jano
- ret = 0;
- cleanup:
iscsi_free_discovery_data(iscsi, addr);
- virStringListFreeCount(tmp_targets, tmp_ntargets);
- return ret;
+
+ return 0;
}
static int
8:04 a.m.
New subject: [PATCH 4/4] virStorageBackendISCSIDirectFindPoolSources: Rework cleanup
virISCSIDirectScanTargets now returns a GStrv, so we can use automatic
cleanup for it and get rid of the cleanup section.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/storage/storage_backend_iscsi_direct.c | 20 ++++++--------------
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/src/storage/storage_backend_iscsi_direct.c
b/src/storage/storage_backend_iscsi_direct.c
index 2073a6df38..b108b12f7b 100644
--- a/src/storage/storage_backend_iscsi_direct.c
+++ b/src/storage/storage_backend_iscsi_direct.c
@@ -485,8 +485,7 @@ virStorageBackendISCSIDirectFindPoolSources(const char *srcSpec,
unsigned int flags)
{
size_t ntargets = 0;
- char **targets = NULL;
- char *ret = NULL;
+ g_auto(GStrv) targets = NULL;
size_t i;
g_autoptr(virStoragePoolSourceList) list = g_new0(virStoragePoolSourceList, 1);
g_autofree char *portal = NULL;
@@ -508,20 +507,20 @@ virStorageBackendISCSIDirectFindPoolSources(const char *srcSpec,
if (source->nhost != 1) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Expected exactly 1 host for the storage pool"));
- goto cleanup;
+ return NULL;
}
if (!source->initiator.iqn) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("missing initiator IQN"));
- goto cleanup;
+ return NULL;
}
if (!(portal = virStorageBackendISCSIDirectPortal(source)))
- goto cleanup;
+ return NULL;
if (virISCSIDirectScanTargets(source->initiator.iqn, portal, &ntargets,
&targets) < 0)
- goto cleanup;
+ return NULL;
list->sources = g_new0(virStoragePoolSource, ntargets);
@@ -541,14 +540,7 @@ virStorageBackendISCSIDirectFindPoolSources(const char *srcSpec,
list->nsources++;
}
- if (!(ret = virStoragePoolSourceListFormat(list)))
- goto cleanup;
-
- cleanup:
- for (i = 0; i < ntargets; i++)
- VIR_FREE(targets[i]);
- VIR_FREE(targets);
- return ret;
+ return virStoragePoolSourceListFormat(list);
}
static struct iscsi_context *
--
2.31.1
8:35 a.m.
On 6/18/21 3:04 PM, Peter Krempa wrote:
Peter Krempa (4):
conf: storage: Introduce virStoragePoolSourceListFree
virStorageBackendISCSIDirectFindPoolSources: Use allocated
virStoragePoolSourceList
virISCSIDirectUpdateTargets: Rework to simplify cleanup and return
GStrv
virStorageBackendISCSIDirectFindPoolSources: Rework cleanup
src/conf/storage_conf.c | 16 ++++
src/conf/storage_conf.h | 5 ++
src/libvirt_private.syms | 1 +
src/storage/storage_backend_iscsi_direct.c | 87 +++++++++-------------
4 files changed, 56 insertions(+), 53 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
1254
days inactive
1254
days old
6 comments
2 participants
participants (2)
-
Jano Tomko -
Peter Krempa