On 2/6/21 9:32 AM, Peter Krempa wrote: Is that so? qemuMonitorJSONGetObjectProperty() strdup()-s into ->str. Michal

Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virmacmap.c | 43 ++++++++++++++++--------------------------- 1 file changed, 16 insertions(+), 27 deletions(-) diff --git a/src/util/virmacmap.c b/src/util/virmacmap.c index 0abb6cf580..ec0a67b433 100644 --- a/src/util/virmacmap.c +++ b/src/util/virmacmap.c @@ -133,31 +133,28 @@ virMacMapLoadFile(virMacMapPtr mgr, g_autoptr(virJSONValue) map = NULL; int map_str_len = 0; size_t i; - int ret = -1; if (virFileExists(file) && (map_str_len = virFileReadAll(file, VIR_MAC_MAP_FILE_SIZE_MAX, &map_str)) < 0) - goto cleanup; + return -1; - if (map_str_len == 0) { - ret = 0; - goto cleanup; - } + if (map_str_len == 0) + return 0; if (!(map = virJSONValueFromString(map_str))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid json in file: %s"), file); - goto cleanup; + return -1; } if (!virJSONValueIsArray(map)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Malformed file structure: %s"), file); - goto cleanup; + return -1; } for (i = 0; i < virJSONValueArraySize(map); i++) { @@ -169,13 +166,13 @@ virMacMapLoadFile(virMacMapPtr mgr, if (!(domain = virJSONValueObjectGetString(tmp, "domain"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing domain")); - goto cleanup; + return -1; } if (!(macs = virJSONValueObjectGetArray(tmp, "macs"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing macs")); - goto cleanup; + return -1; } for (j = 0; j < virJSONValueArraySize(macs); j++) { @@ -183,13 +180,11 @@ virMacMapLoadFile(virMacMapPtr mgr, const char *mac = virJSONValueGetString(macJSON); if (virMacMapAddLocked(mgr, domain, mac) < 0) - goto cleanup; + return -1; } } - ret = 0; - cleanup: - return ret; + return 0; } @@ -202,7 +197,6 @@ virMACMapHashDumper(void *payload, g_autoptr(virJSONValue) arr = virJSONValueNewArray(); const char **macs = payload; size_t i; - int ret = -1; for (i = 0; macs[i]; i++) { virJSONValuePtr m = virJSONValueNewString(macs[i]); @@ -210,22 +204,20 @@ virMACMapHashDumper(void *payload, if (!m || virJSONValueArrayAppend(arr, m) < 0) { virJSONValueFree(m); - goto cleanup; + return -1; } } if (virJSONValueObjectAppendString(obj, "domain", name) < 0 || virJSONValueObjectAppend(obj, "macs", arr) < 0) - goto cleanup; + return -1; arr = NULL; if (virJSONValueArrayAppend(data, obj) < 0) - goto cleanup; + return -1; obj = NULL; - ret = 0; - cleanup: - return ret; + return 0; } @@ -234,17 +226,14 @@ virMacMapDumpStrLocked(virMacMapPtr mgr, char **str) { g_autoptr(virJSONValue) arr = virJSONValueNewArray(); - int ret = -1; if (virHashForEachSorted(mgr->macs, virMACMapHashDumper, arr) < 0) - goto cleanup; + return -1; if (!(*str = virJSONValueToString(arr, true))) - goto cleanup; + return -1; - ret = 0; - cleanup: - return ret; + return 0; } -- 2.29.2

'i' is used in both outer and inner loop. Since 'devMountsPath' is now a NULL-terminated list, we can use a GStrv to iterate it; Additionally rewrite the conditional of adding to the 'unlinkPaths' array so that it's more clear what's happening. Fixes: 5c86fbb72d6e90025481db7 Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_namespace.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index 27a78c86e4..03cc6379fe 100644 --- a/src/qemu/qemu_namespace.c +++ b/src/qemu/qemu_namespace.c @@ -1362,14 +1362,20 @@ qemuNamespaceUnlinkPaths(virDomainObjPtr vm, const char *file = paths[i]; if (STRPREFIX(file, QEMU_DEVPREFIX)) { - for (i = 0; i < ndevMountsPath; i++) { - if (STREQ(devMountsPath[i], "/dev")) + GStrv mount; + bool inSubmount = false; + + for (mount = devMountsPath; *mount; mount++) { + if (STREQ(*mount, "/dev")) continue; - if (STRPREFIX(file, devMountsPath[i])) + + if (STRPREFIX(file, *mount)) { + inSubmount = true; break; + } } - if (i == ndevMountsPath && + if (!inSubmount && virStringListAdd(&unlinkPaths, file) < 0) return -1; } -- 2.29.2

On 2/6/21 9:32 AM, Peter Krempa wrote: So, GSList is a singly linked list. This doubles memory usage because for every string we have to also store pointer to next. Is O(n^2) that bad so that we have to sacrifice memory complexity? Michal

On Tue, Feb 09, 2021 at 15:22:40 +0100, Michal Privoznik wrote: Firstly, for any cases where we know the number of elements or at least a sane upper bound before inserting, we still keep using a char **. This leaves us with places that don't know how many elements there will be which insert: 1) a lot of elements -> definitely worth the overhead 2) a few elements -> the overhead is comparatively small In places we might want to care about memory complexity, the code can use a char ** list with a counter variable instead of the GSList to avoid both memory and computational complexity, but not memory fragmentation issues where a (singly) linked list prevents memory copy when enlarging. Or alternatively even use 2 additional size variables, see VIR_RESIZE_N to avoid reallocations for maximum optimization.

'virStringListAdd' calculates the string list length on every invocation so constructing a string list using it results in O(n^2) complexity. Use a GSList which has cheap insertion and iteration and doesn't need failure handling. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_namespace.c | 202 ++++++++++++++++++-------------------- 1 file changed, 98 insertions(+), 104 deletions(-) diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index af08b3e055..e8f205cfdd 100644 --- a/src/qemu/qemu_namespace.c +++ b/src/qemu/qemu_namespace.c @@ -40,6 +40,7 @@ #include "virlog.h" #include "virstring.h" #include "virdevmapper.h" +#include "virglibutil.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -190,7 +191,7 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg, static int qemuDomainPopulateDevices(virQEMUDriverConfigPtr cfg, - char ***paths) + GSList **paths) { const char *const *devices = (const char *const *) cfg->cgroupDeviceACL; size_t i; @@ -199,8 +200,7 @@ qemuDomainPopulateDevices(virQEMUDriverConfigPtr cfg, devices = defaultDeviceACL; for (i = 0; devices[i]; i++) { - if (virStringListAdd(paths, devices[i]) < 0) - return -1; + *paths = g_slist_prepend(*paths, g_strdup(devices[i])); } return 0; @@ -237,7 +237,7 @@ qemuDomainSetupDev(virSecurityManagerPtr mgr, static int qemuDomainSetupDisk(virStorageSourcePtr src, - char ***paths) + GSList **paths) { virStorageSourcePtr next; bool hasNVMe = false; @@ -252,6 +252,7 @@ qemuDomainSetupDisk(virStorageSourcePtr src, return -1; } else { g_auto(GStrv) targetPaths = NULL; + GStrv tmp; if (virStorageSourceIsEmpty(next) || !virStorageSourceIsLocalStorage(next)) { @@ -269,22 +270,19 @@ qemuDomainSetupDisk(virStorageSourcePtr src, return -1; } - if (virStringListMerge(paths, &targetPaths) < 0) - return -1; + for (tmp = targetPaths; *tmp; tmp++) + *paths = g_slist_prepend(*paths, g_steal_pointer(tmp)); } - if (virStringListAdd(paths, tmpPath) < 0) - return -1; + *paths = g_slist_prepend(*paths, g_steal_pointer(&tmpPath)); } /* qemu-pr-helper might require access to /dev/mapper/control. */ - if (src->pr && - virStringListAdd(paths, QEMU_DEVICE_MAPPER_CONTROL_PATH) < 0) - return -1; + if (src->pr) + *paths = g_slist_prepend(*paths, g_strdup(QEMU_DEVICE_MAPPER_CONTROL_PATH)); - if (hasNVMe && - virStringListAdd(paths, QEMU_DEV_VFIO) < 0) - return -1; + if (hasNVMe) + *paths = g_slist_prepend(*paths, g_strdup(QEMU_DEV_VFIO)); return 0; } @@ -292,7 +290,7 @@ qemuDomainSetupDisk(virStorageSourcePtr src, static int qemuDomainSetupAllDisks(virDomainObjPtr vm, - char ***paths) + GSList **paths) { size_t i; @@ -313,20 +311,19 @@ static int qemuDomainSetupHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev, bool hotplug, - char ***paths) + GSList **paths) { g_autofree char *path = NULL; if (qemuDomainGetHostdevPath(hostdev, &path, NULL) < 0) return -1; - if (path && virStringListAdd(paths, path) < 0) - return -1; + if (path) + *paths = g_slist_prepend(*paths, g_steal_pointer(&path)); if (qemuHostdevNeedsVFIO(hostdev) && - (!hotplug || !qemuDomainNeedsVFIO(vm->def)) && - virStringListAdd(paths, QEMU_DEV_VFIO) < 0) - return -1; + (!hotplug || !qemuDomainNeedsVFIO(vm->def))) + *paths = g_slist_prepend(*paths, g_strdup(QEMU_DEV_VFIO)); return 0; } @@ -334,7 +331,7 @@ qemuDomainSetupHostdev(virDomainObjPtr vm, static int qemuDomainSetupAllHostdevs(virDomainObjPtr vm, - char ***paths) + GSList **paths) { size_t i; @@ -353,19 +350,21 @@ qemuDomainSetupAllHostdevs(virDomainObjPtr vm, static int qemuDomainSetupMemory(virDomainMemoryDefPtr mem, - char ***paths) + GSList **paths) { if (mem->model != VIR_DOMAIN_MEMORY_MODEL_NVDIMM && mem->model != VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM) return 0; - return virStringListAdd(paths, mem->nvdimmPath); + *paths = g_slist_prepend(*paths, g_strdup(mem->nvdimmPath)); + + return 0; } static int qemuDomainSetupAllMemories(virDomainObjPtr vm, - char ***paths) + GSList **paths) { size_t i; @@ -385,7 +384,7 @@ qemuDomainSetupChardev(virDomainDefPtr def G_GNUC_UNUSED, virDomainChrDefPtr dev, void *opaque) { - char ***paths = opaque; + GSList **paths = opaque; const char *path = NULL; if (!(path = virDomainChrSourceDefGetPath(dev->source))) @@ -396,13 +395,14 @@ qemuDomainSetupChardev(virDomainDefPtr def G_GNUC_UNUSED, dev->source->data.nix.listen) return 0; - return virStringListAdd(paths, path); + *paths = g_slist_prepend(*paths, g_strdup(path)); + return 0; } static int qemuDomainSetupAllChardevs(virDomainObjPtr vm, - char ***paths) + GSList **paths) { VIR_DEBUG("Setting up chardevs"); @@ -419,12 +419,11 @@ qemuDomainSetupAllChardevs(virDomainObjPtr vm, static int qemuDomainSetupTPM(virDomainTPMDefPtr dev, - char ***paths) + GSList **paths) { switch (dev->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: - if (virStringListAdd(paths, dev->data.passthrough.source.data.file.path) < 0) - return -1; + *paths = g_slist_prepend(*paths, g_strdup(dev->data.passthrough.source.data.file.path)); break; case VIR_DOMAIN_TPM_TYPE_EMULATOR: @@ -439,7 +438,7 @@ qemuDomainSetupTPM(virDomainTPMDefPtr dev, static int qemuDomainSetupAllTPMs(virDomainObjPtr vm, - char ***paths) + GSList **paths) { size_t i; @@ -457,20 +456,21 @@ qemuDomainSetupAllTPMs(virDomainObjPtr vm, static int qemuDomainSetupGraphics(virDomainGraphicsDefPtr gfx, - char ***paths) + GSList **paths) { const char *rendernode = virDomainGraphicsGetRenderNode(gfx); if (!rendernode) return 0; - return virStringListAdd(paths, rendernode); + *paths = g_slist_prepend(*paths, g_strdup(rendernode)); + return 0; } static int qemuDomainSetupAllGraphics(virDomainObjPtr vm, - char ***paths) + GSList **paths) { size_t i; @@ -488,12 +488,14 @@ qemuDomainSetupAllGraphics(virDomainObjPtr vm, static int qemuDomainSetupInput(virDomainInputDefPtr input, - char ***paths) + GSList **paths) { const char *path = virDomainInputDefGetPath(input); - if (path && virStringListAdd(paths, path) < 0) - return -1; + if (!path) + return 0; + + *paths = g_slist_prepend(*paths, g_strdup(path)); return 0; } @@ -501,7 +503,7 @@ qemuDomainSetupInput(virDomainInputDefPtr input, static int qemuDomainSetupAllInputs(virDomainObjPtr vm, - char ***paths) + GSList **paths) { size_t i; @@ -518,12 +520,11 @@ qemuDomainSetupAllInputs(virDomainObjPtr vm, static int qemuDomainSetupRNG(virDomainRNGDefPtr rng, - char ***paths) + GSList **paths) { switch ((virDomainRNGBackend) rng->backend) { case VIR_DOMAIN_RNG_BACKEND_RANDOM: - if (virStringListAdd(paths, rng->source.file) < 0) - return -1; + *paths = g_slist_prepend(*paths, g_strdup(rng->source.file)); break; case VIR_DOMAIN_RNG_BACKEND_EGD: @@ -539,7 +540,7 @@ qemuDomainSetupRNG(virDomainRNGDefPtr rng, static int qemuDomainSetupAllRNGs(virDomainObjPtr vm, - char ***paths) + GSList **paths) { size_t i; @@ -557,7 +558,7 @@ qemuDomainSetupAllRNGs(virDomainObjPtr vm, static int qemuDomainSetupLoader(virDomainObjPtr vm, - char ***paths) + GSList **paths) { virDomainLoaderDefPtr loader = vm->def->os.loader; @@ -566,17 +567,14 @@ qemuDomainSetupLoader(virDomainObjPtr vm, if (loader) { switch ((virDomainLoader) loader->type) { case VIR_DOMAIN_LOADER_TYPE_ROM: - if (virStringListAdd(paths, loader->path) < 0) - return -1; + *paths = g_slist_prepend(*paths, g_strdup(loader->path)); break; case VIR_DOMAIN_LOADER_TYPE_PFLASH: - if (virStringListAdd(paths, loader->path) < 0) - return -1; + *paths = g_slist_prepend(*paths, g_strdup(loader->path)); - if (loader->nvram && - virStringListAdd(paths, loader->nvram) < 0) - return -1; + if (loader->nvram) + *paths = g_slist_prepend(*paths, g_strdup(loader->nvram)); break; case VIR_DOMAIN_LOADER_TYPE_NONE: @@ -592,7 +590,7 @@ qemuDomainSetupLoader(virDomainObjPtr vm, static int qemuDomainSetupLaunchSecurity(virDomainObjPtr vm, - char ***paths) + GSList **paths) { virDomainSEVDefPtr sev = vm->def->sev; @@ -601,8 +599,7 @@ qemuDomainSetupLaunchSecurity(virDomainObjPtr vm, VIR_DEBUG("Setting up launch security"); - if (virStringListAdd(paths, QEMU_DEV_SEV) < 0) - return -1; + *paths = g_slist_prepend(*paths, g_strdup(QEMU_DEV_SEV)); VIR_DEBUG("Set up launch security"); return 0; @@ -611,14 +608,14 @@ qemuDomainSetupLaunchSecurity(virDomainObjPtr vm, static int qemuNamespaceMknodPaths(virDomainObjPtr vm, - const char **paths); + GSList *paths); int qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg, virDomainObjPtr vm) { - g_auto(GStrv) paths = NULL; + g_autoptr(virGSListString) paths = NULL; if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) { VIR_DEBUG("namespaces disabled for domain %s", vm->def->name); @@ -658,7 +655,7 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg, if (qemuDomainSetupLaunchSecurity(vm, &paths) < 0) return -1; - if (qemuNamespaceMknodPaths(vm, (const char **) paths) < 0) + if (qemuNamespaceMknodPaths(vm, paths) < 0) return -1; return 0; @@ -1228,20 +1225,19 @@ qemuNamespacePrepareOneItem(qemuNamespaceMknodDataPtr data, static int qemuNamespaceMknodPaths(virDomainObjPtr vm, - const char **paths) + GSList *paths) { qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverPtr driver = priv->driver; g_autoptr(virQEMUDriverConfig) cfg = NULL; char **devMountsPath = NULL; size_t ndevMountsPath = 0; - size_t npaths = 0; qemuNamespaceMknodData data = { 0 }; size_t i; int ret = -1; + GSList *next; - npaths = virStringListLength(paths); - if (npaths == 0) + if (!paths) return 0; cfg = virQEMUDriverGetConfig(driver); @@ -1253,8 +1249,10 @@ qemuNamespaceMknodPaths(virDomainObjPtr vm, data.driver = driver; data.vm = vm; - for (i = 0; i < npaths; i++) { - if (qemuNamespacePrepareOneItem(&data, cfg, vm, paths[i], + for (next = paths; next; next = next->next) { + const char *path = next->data; + + if (qemuNamespacePrepareOneItem(&data, cfg, vm, path, devMountsPath, ndevMountsPath) < 0) goto cleanup; } @@ -1299,7 +1297,7 @@ qemuNamespaceMknodPaths(virDomainObjPtr vm, static int qemuNamespaceMknodPaths(virDomainObjPtr vm G_GNUC_UNUSED, - const char **paths G_GNUC_UNUSED) + GSList *paths G_GNUC_UNUSED) { virReportSystemError(ENOSYS, "%s", _("Namespaces are not supported on this platform.")); @@ -1314,11 +1312,11 @@ static int qemuNamespaceUnlinkHelper(pid_t pid G_GNUC_UNUSED, void *opaque) { - char **paths = opaque; - size_t i; + g_autoptr(virGSListString) paths = opaque; + GSList *next; - for (i = 0; paths[i]; i++) { - const char *path = paths[i]; + for (next = paths; next; next = next->next) { + const char *path = next->data; VIR_DEBUG("Unlinking %s", path); if (unlink(path) < 0 && errno != ENOENT) { @@ -1328,25 +1326,22 @@ qemuNamespaceUnlinkHelper(pid_t pid G_GNUC_UNUSED, } } - g_strfreev(paths); return 0; } static int qemuNamespaceUnlinkPaths(virDomainObjPtr vm, - const char **paths) + GSList *paths) { qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverPtr driver = priv->driver; g_autoptr(virQEMUDriverConfig) cfg = NULL; - g_auto(GStrv) unlinkPaths = NULL; g_auto(GStrv) devMountsPath = NULL; - size_t npaths; - size_t i; + g_autoptr(virGSListString) unlinkPaths = NULL; + GSList *next; - npaths = virStringListLength(paths); - if (!npaths) + if (!paths) return 0; cfg = virQEMUDriverGetConfig(driver); @@ -1354,10 +1349,10 @@ qemuNamespaceUnlinkPaths(virDomainObjPtr vm, if (qemuDomainGetPreservedMounts(cfg, vm, &devMountsPath, NULL, NULL) < 0) return -1; - for (i = 0; i < npaths; i++) { - const char *file = paths[i]; + for (next = paths; next; next = next->next) { + const char *path = next->data; - if (STRPREFIX(file, QEMU_DEVPREFIX)) { + if (STRPREFIX(path, QEMU_DEVPREFIX)) { GStrv mount; bool inSubmount = false; @@ -1365,15 +1360,14 @@ qemuNamespaceUnlinkPaths(virDomainObjPtr vm, if (STREQ(*mount, "/dev")) continue; - if (STRPREFIX(file, *mount)) { + if (STRPREFIX(path, *mount)) { inSubmount = true; break; } } - if (!inSubmount && - virStringListAdd(&unlinkPaths, file) < 0) - return -1; + if (!inSubmount) + unlinkPaths = g_slist_prepend(unlinkPaths, g_strdup(path)); } } @@ -1391,7 +1385,7 @@ int qemuDomainNamespaceSetupDisk(virDomainObjPtr vm, virStorageSourcePtr src) { - g_auto(GStrv) paths = NULL; + g_autoptr(virGSListString) paths = NULL; if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) return 0; @@ -1399,7 +1393,7 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm, if (qemuDomainSetupDisk(src, &paths) < 0) return -1; - if (qemuNamespaceMknodPaths(vm, (const char **) paths) < 0) + if (qemuNamespaceMknodPaths(vm, paths) < 0) return -1; return 0; @@ -1435,7 +1429,7 @@ int qemuDomainNamespaceSetupHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) { - g_auto(GStrv) paths = NULL; + g_autoptr(virGSListString) paths = NULL; if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) return 0; @@ -1446,7 +1440,7 @@ qemuDomainNamespaceSetupHostdev(virDomainObjPtr vm, &paths) < 0) return -1; - if (qemuNamespaceMknodPaths(vm, (const char **) paths) < 0) + if (qemuNamespaceMknodPaths(vm, paths) < 0) return -1; return 0; @@ -1468,7 +1462,7 @@ int qemuDomainNamespaceTeardownHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) { - g_auto(GStrv) paths = NULL; + g_autoptr(virGSListString) paths = NULL; if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) return 0; @@ -1479,7 +1473,7 @@ qemuDomainNamespaceTeardownHostdev(virDomainObjPtr vm, &paths) < 0) return -1; - if (qemuNamespaceUnlinkPaths(vm, (const char **) paths) < 0) + if (qemuNamespaceUnlinkPaths(vm, paths) < 0) return -1; return 0; @@ -1490,7 +1484,7 @@ int qemuDomainNamespaceSetupMemory(virDomainObjPtr vm, virDomainMemoryDefPtr mem) { - g_auto(GStrv) paths = NULL; + g_autoptr(virGSListString) paths = NULL; if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) return 0; @@ -1498,7 +1492,7 @@ qemuDomainNamespaceSetupMemory(virDomainObjPtr vm, if (qemuDomainSetupMemory(mem, &paths) < 0) return -1; - if (qemuNamespaceMknodPaths(vm, (const char **) paths) < 0) + if (qemuNamespaceMknodPaths(vm, paths) < 0) return -1; return 0; @@ -1509,7 +1503,7 @@ int qemuDomainNamespaceTeardownMemory(virDomainObjPtr vm, virDomainMemoryDefPtr mem) { - g_auto(GStrv) paths = NULL; + g_autoptr(virGSListString) paths = NULL; if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) return 0; @@ -1517,7 +1511,7 @@ qemuDomainNamespaceTeardownMemory(virDomainObjPtr vm, if (qemuDomainSetupMemory(mem, &paths) < 0) return -1; - if (qemuNamespaceUnlinkPaths(vm, (const char **) paths) < 0) + if (qemuNamespaceUnlinkPaths(vm, paths) < 0) return -1; return 0; @@ -1528,7 +1522,7 @@ int qemuDomainNamespaceSetupChardev(virDomainObjPtr vm, virDomainChrDefPtr chr) { - g_auto(GStrv) paths = NULL; + g_autoptr(virGSListString) paths = NULL; if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) return 0; @@ -1536,7 +1530,7 @@ qemuDomainNamespaceSetupChardev(virDomainObjPtr vm, if (qemuDomainSetupChardev(vm->def, chr, &paths) < 0) return -1; - if (qemuNamespaceMknodPaths(vm, (const char **) paths) < 0) + if (qemuNamespaceMknodPaths(vm, paths) < 0) return -1; return 0; @@ -1547,7 +1541,7 @@ int qemuDomainNamespaceTeardownChardev(virDomainObjPtr vm, virDomainChrDefPtr chr) { - g_auto(GStrv) paths = NULL; + g_autoptr(virGSListString) paths = NULL; if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) return 0; @@ -1555,7 +1549,7 @@ qemuDomainNamespaceTeardownChardev(virDomainObjPtr vm, if (qemuDomainSetupChardev(vm->def, chr, &paths) < 0) return -1; - if (qemuNamespaceUnlinkPaths(vm, (const char **) paths) < 0) + if (qemuNamespaceUnlinkPaths(vm, paths) < 0) return -1; return 0; @@ -1566,7 +1560,7 @@ int qemuDomainNamespaceSetupRNG(virDomainObjPtr vm, virDomainRNGDefPtr rng) { - g_auto(GStrv) paths = NULL; + g_autoptr(virGSListString) paths = NULL; if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) return 0; @@ -1574,7 +1568,7 @@ qemuDomainNamespaceSetupRNG(virDomainObjPtr vm, if (qemuDomainSetupRNG(rng, &paths) < 0) return -1; - if (qemuNamespaceMknodPaths(vm, (const char **) paths) < 0) + if (qemuNamespaceMknodPaths(vm, paths) < 0) return -1; return 0; @@ -1585,7 +1579,7 @@ int qemuDomainNamespaceTeardownRNG(virDomainObjPtr vm, virDomainRNGDefPtr rng) { - g_auto(GStrv) paths = NULL; + g_autoptr(virGSListString) paths = NULL; if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) return 0; @@ -1593,7 +1587,7 @@ qemuDomainNamespaceTeardownRNG(virDomainObjPtr vm, if (qemuDomainSetupRNG(rng, &paths) < 0) return -1; - if (qemuNamespaceUnlinkPaths(vm, (const char **) paths) < 0) + if (qemuNamespaceUnlinkPaths(vm, paths) < 0) return -1; return 0; @@ -1604,7 +1598,7 @@ int qemuDomainNamespaceSetupInput(virDomainObjPtr vm, virDomainInputDefPtr input) { - g_auto(GStrv) paths = NULL; + g_autoptr(virGSListString) paths = NULL; if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) return 0; @@ -1612,7 +1606,7 @@ qemuDomainNamespaceSetupInput(virDomainObjPtr vm, if (qemuDomainSetupInput(input, &paths) < 0) return -1; - if (qemuNamespaceMknodPaths(vm, (const char **) paths) < 0) + if (qemuNamespaceMknodPaths(vm, paths) < 0) return -1; return 0; } @@ -1622,7 +1616,7 @@ int qemuDomainNamespaceTeardownInput(virDomainObjPtr vm, virDomainInputDefPtr input) { - g_auto(GStrv) paths = NULL; + g_autoptr(virGSListString) paths = NULL; if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) return 0; @@ -1630,7 +1624,7 @@ qemuDomainNamespaceTeardownInput(virDomainObjPtr vm, if (qemuDomainSetupInput(input, &paths) < 0) return -1; - if (qemuNamespaceUnlinkPaths(vm, (const char **) paths) < 0) + if (qemuNamespaceUnlinkPaths(vm, paths) < 0) return -1; return 0; -- 2.29.2

'virStringListAdd' calculates the string list length on every invocation so constructing a string list using it results in O(n^2) complexity. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virhook.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/util/virhook.c b/src/util/virhook.c index 05d46f259e..e4e1945225 100644 --- a/src/util/virhook.c +++ b/src/util/virhook.c @@ -34,6 +34,7 @@ #include "configmake.h" #include "vircommand.h" #include "virstring.h" +#include "virglibutil.h" #define VIR_FROM_THIS VIR_FROM_HOOK @@ -343,11 +344,11 @@ virHookCall(int driver, struct dirent *entry; g_autofree char *path = NULL; g_autofree char *dir_path = NULL; - g_auto(GStrv) entries = NULL; + g_autoptr(virGSListString) entries = NULL; const char *drvstr; const char *opstr; const char *subopstr; - size_t i, nentries; + GSList *next; if (output) *output = NULL; @@ -433,7 +434,7 @@ virHookCall(int driver, if (!virFileIsExecutable(entry_path)) continue; - virStringListAdd(&entries, entry_path); + entries = g_slist_prepend(entries, g_steal_pointer(&entry_path)); } if (ret < 0) @@ -442,18 +443,18 @@ virHookCall(int driver, if (!entries) return script_ret; - nentries = virStringListLength((const char **)entries); - qsort(entries, nentries, sizeof(*entries), virStringSortCompare); + entries = g_slist_sort(entries, (GCompareFunc) strcmp); - for (i = 0; i < nentries; i++) { + for (next = entries; next; next = next->next) { int entry_ret; const char *entry_input; g_autofree char *entry_output = NULL; + const char *filename = next->data; /* Get input from previous output */ entry_input = (!script_ret && output && !virStringIsEmpty(*output)) ? *output : input; - entry_ret = virRunScript(entries[i], id, opstr, + entry_ret = virRunScript(filename, id, opstr, subopstr, extra, entry_input, (output) ? &entry_output : NULL); if (entry_ret < script_ret) -- 2.29.2

We already know the upper bound of items we might need so we can allocate the array upfront and avoid the quadratic complexity of 'virStringListAdd'. In this instance the returned data is kept only temporarily so a potential unused space due to filtered-out entries doesn't impose a long-term burden on memory. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/cpu_conf.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index f98b0a0eb3..1de4c1e417 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -985,21 +985,21 @@ virCPUDefCheckFeatures(virCPUDefPtr cpu, void *opaque, char ***features) { - g_auto(GStrv) list = NULL; size_t n = 0; size_t i; *features = NULL; + if (cpu->nfeatures == 0) + return 0; + + *features = g_new0(char *, cpu->nfeatures + 1); + for (i = 0; i < cpu->nfeatures; i++) { - if (filter(cpu->features[i].name, cpu->features[i].policy, opaque)) { - if (virStringListAdd(&list, cpu->features[i].name) < 0) - return -1; - n++; - } + if (filter(cpu->features[i].name, cpu->features[i].policy, opaque)) + (*features)[n++] = g_strdup(cpu->features[i].name); } - *features = g_steal_pointer(&list); return n; } -- 2.29.2

Pre-allocate a buffer for the upper limit and shrink it afterwards to avoid use of 'virStringListAdd' in a loop. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virresctrl.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 29df51c652..86b4b9d73b 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -1009,15 +1009,15 @@ virResctrlInfoGetMonitorPrefix(virResctrlInfoPtr resctrl, mon->cache_level = mongrp_info->cache_level; } + mon->features = g_new0(char *, mongrp_info->nfeatures + 1); + for (i = 0; i < mongrp_info->nfeatures; i++) { - if (STRPREFIX(mongrp_info->features[i], prefix)) { - if (virStringListAdd(&mon->features, - mongrp_info->features[i]) < 0) - goto cleanup; - mon->nfeatures++; - } + if (STRPREFIX(mongrp_info->features[i], prefix)) + mon->features[mon->nfeatures++] = g_strdup(mongrp_info->features[i]); } + mon->features = g_renew(char *, mon->features, mon->nfeatures + 1); + ret = 0; /* In case *monitor is pointed to some monitor, clean it. */ -- 2.29.2

The conversion removes the use of virStringListAdd/virStringListRemove which try to add dynamic properties to a string list which is really inefficient. Storing the dbus VMState ids in a GSList is pretty straightforward and the slightly increased complexity of the code will be paid back by removing the string list helpers later. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_dbus.c | 19 ++++++++++++++++--- src/qemu/qemu_dbus.h | 2 +- src/qemu/qemu_domain.c | 4 ++-- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_migration.c | 10 ++++------ src/qemu/qemu_monitor.c | 19 +++++++++++++------ src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_monitor_json.c | 5 ++--- src/qemu/qemu_monitor_json.h | 2 +- src/qemu/qemu_slirp.c | 7 ++----- 11 files changed, 44 insertions(+), 30 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 92036d26c0..ac6bec3389 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9605,7 +9605,7 @@ qemuBuildDBusVMStateCommandLine(virCommandPtr cmd, g_autoptr(virJSONValue) props = NULL; qemuDomainObjPrivatePtr priv = QEMU_DOMAIN_PRIVATE(vm); - if (virStringListLength((const char **)priv->dbusVMStateIds) == 0) + if (!priv->dbusVMStateIds) return 0; if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) { diff --git a/src/qemu/qemu_dbus.c b/src/qemu/qemu_dbus.c index ffcf83e5da..31ede2646f 100644 --- a/src/qemu/qemu_dbus.c +++ b/src/qemu/qemu_dbus.c @@ -287,15 +287,28 @@ qemuDBusStart(virQEMUDriverPtr driver, } -int +void qemuDBusVMStateAdd(virDomainObjPtr vm, const char *id) { - return virStringListAdd(&QEMU_DOMAIN_PRIVATE(vm)->dbusVMStateIds, id); + qemuDomainObjPrivatePtr priv = vm->privateData; + + priv->dbusVMStateIds = g_slist_append(priv->dbusVMStateIds, g_strdup(id)); } void qemuDBusVMStateRemove(virDomainObjPtr vm, const char *id) { - virStringListRemove(&QEMU_DOMAIN_PRIVATE(vm)->dbusVMStateIds, id); + qemuDomainObjPrivatePtr priv = vm->privateData; + GSList *next; + + for (next = priv->dbusVMStateIds; next; next = next->next) { + const char *elem = next->data; + + if (STREQ(id, elem)) { + priv->dbusVMStateIds = g_slist_remove_link(priv->dbusVMStateIds, next); + g_slist_free_full(next, g_free); + break; + } + } } diff --git a/src/qemu/qemu_dbus.h b/src/qemu/qemu_dbus.h index e3ce1330fd..5900b99144 100644 --- a/src/qemu/qemu_dbus.h +++ b/src/qemu/qemu_dbus.h @@ -30,7 +30,7 @@ int qemuDBusStart(virQEMUDriverPtr driver, void qemuDBusStop(virQEMUDriverPtr driver, virDomainObjPtr vm); -int qemuDBusVMStateAdd(virDomainObjPtr vm, const char *id); +void qemuDBusVMStateAdd(virDomainObjPtr vm, const char *id); void qemuDBusVMStateRemove(virDomainObjPtr vm, const char *id); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0f09e321fb..31b1110887 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1785,8 +1785,8 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr priv) priv->dbusDaemonRunning = false; - g_strfreev(priv->dbusVMStateIds); - priv->dbusVMStateIds = NULL; + if (priv->dbusVMStateIds) + g_slist_free_full(g_steal_pointer(&priv->dbusVMStateIds), g_free); priv->dbusVMState = false; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 7453881a31..29a5dd97d8 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -260,7 +260,7 @@ struct _qemuDomainObjPrivate { bool dbusDaemonRunning; /* list of Ids to migrate */ - char **dbusVMStateIds; + GSList *dbusVMStateIds; /* true if -object dbus-vmstate was added */ bool dbusVMState; diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index f44d31c971..1bb4a9608e 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1408,7 +1408,7 @@ qemuMigrationSrcIsAllowed(virQEMUDriverPtr driver, } } - if (virStringListLength((const char **)priv->dbusVMStateIds) > 0 && + if (priv->dbusVMStateIds && !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot migrate this domain without dbus-vmstate support")); @@ -2091,8 +2091,7 @@ qemuMigrationDstRun(virQEMUDriverPtr driver, if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) return -1; - rv = qemuMonitorSetDBusVMStateIdList(priv->mon, - (const char **)priv->dbusVMStateIds); + rv = qemuMonitorSetDBusVMStateIdList(priv->mon, priv->dbusVMStateIds); if (rv < 0) goto exit_monitor; @@ -3602,7 +3601,7 @@ qemuMigrationSetDBusVMState(virQEMUDriverPtr driver, { qemuDomainObjPrivatePtr priv = vm->privateData; - if (virStringListLength((const char **)priv->dbusVMStateIds) > 0) { + if (priv->dbusVMStateIds) { int rv; if (qemuHotplugAttachDBusVMState(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) @@ -3611,8 +3610,7 @@ qemuMigrationSetDBusVMState(virQEMUDriverPtr driver, if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) return -1; - rv = qemuMonitorSetDBusVMStateIdList(priv->mon, - (const char **)priv->dbusVMStateIds); + rv = qemuMonitorSetDBusVMStateIdList(priv->mon, priv->dbusVMStateIds); if (qemuDomainObjExitMonitor(driver, vm) < 0) rv = -1; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 14966d4096..0476d606f5 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2385,21 +2385,28 @@ qemuMonitorSavePhysicalMemory(qemuMonitorPtr mon, int qemuMonitorSetDBusVMStateIdList(qemuMonitorPtr mon, - const char **list) + GSList *list) { g_autofree char *path = NULL; + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + GSList *next; VIR_DEBUG("list=%p", list); - if (virStringListLength(list) == 0) + QEMU_CHECK_MONITOR(mon); + + if (!list) return 0; - path = g_strdup_printf("/objects/%s", - qemuDomainGetDBusVMStateAlias()); + for (next = list; next; next = next->next) + virBufferAsprintf(&buf, "%s,", (const char *) next->data); - QEMU_CHECK_MONITOR(mon); + virBufferTrim(&buf, ","); + + path = g_strdup_printf("/objects/%s", qemuDomainGetDBusVMStateAlias()); - return qemuMonitorJSONSetDBusVMStateIdList(mon, path, list); + return qemuMonitorJSONSetDBusVMStateIdList(mon, path, + virBufferCurrentContent(&buf)); } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index b0068f2a82..32dc96ee82 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -777,7 +777,7 @@ int qemuMonitorSavePhysicalMemory(qemuMonitorPtr mon, const char *path); int qemuMonitorSetDBusVMStateIdList(qemuMonitorPtr mon, - const char **list); + GSList *list); int qemuMonitorSetMigrationSpeed(qemuMonitorPtr mon, unsigned long bandwidth); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index d3b2a2c7a5..72b60daecc 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2359,12 +2359,11 @@ qemuMonitorJSONSetMemoryStatsPeriod(qemuMonitorPtr mon, int qemuMonitorJSONSetDBusVMStateIdList(qemuMonitorPtr mon, const char *vmstatepath, - const char **list) + const char *idstr) { - g_autofree char *str = virStringListJoin(list, ","); qemuMonitorJSONObjectProperty prop = { .type = QEMU_MONITOR_OBJECT_PROPERTY_STRING, - .val.str = str, + .val.str = idstr, }; return qemuMonitorJSONSetObjectProperty(mon, vmstatepath, "id-list", &prop); diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 64af758885..a550dac33c 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -699,7 +699,7 @@ qemuMonitorJSONTransactionBackup(virJSONValuePtr actions, int qemuMonitorJSONSetDBusVMStateIdList(qemuMonitorPtr mon, const char *vmstatepath, - const char **list) + const char *idstr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c index 62a6665679..7f6156fbc1 100644 --- a/src/qemu/qemu_slirp.c +++ b/src/qemu/qemu_slirp.c @@ -320,11 +320,8 @@ qemuSlirpStart(qemuSlirpPtr slirp, virCommandAddArgFormat(cmd, "--dbus-address=%s", dbus_addr); if (qemuSlirpHasFeature(slirp, QEMU_SLIRP_FEATURE_MIGRATE)) { - if (qemuDBusVMStateAdd(vm, id) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Failed to register slirp migration")); - goto error; - } + qemuDBusVMStateAdd(vm, id); + if (incoming) virCommandAddArg(cmd, "--dbus-incoming"); } -- 2.29.2

Since adding and removing is the main use case for the macmap module, convert the code to a more efficient data structure. The refactor also optimizes the loading from file where previously we'd do a hash lookup + list lenght calculation for every entry. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virmacmap.c | 118 +++++++++++++++++++++++------------------- src/util/virmacmap.h | 6 ++- tests/virmacmaptest.c | 21 ++++---- 3 files changed, 81 insertions(+), 64 deletions(-) diff --git a/src/util/virmacmap.c b/src/util/virmacmap.c index ec0a67b433..0d458d7b7b 100644 --- a/src/util/virmacmap.c +++ b/src/util/virmacmap.c @@ -50,21 +50,18 @@ struct virMacMap { static virClassPtr virMacMapClass; -static int -virMacMapHashFree(void *payload, - const char *name G_GNUC_UNUSED, - void *opaque G_GNUC_UNUSED) -{ - g_strfreev(payload); - return 0; -} - - static void virMacMapDispose(void *obj) { virMacMapPtr mgr = obj; - virHashForEach(mgr->macs, virMacMapHashFree, NULL); + GHashTableIter htitr; + void *value; + + g_hash_table_iter_init(&htitr, mgr->macs); + + while (g_hash_table_iter_next(&htitr, NULL, &value)) + g_slist_free_full(value, g_free); + virHashFree(mgr->macs); } @@ -80,48 +77,57 @@ static int virMacMapOnceInit(void) VIR_ONCE_GLOBAL_INIT(virMacMap); -static int +static void virMacMapAddLocked(virMacMapPtr mgr, const char *domain, const char *mac) { - char **macsList = NULL; + GSList *orig_list; + GSList *list; + GSList *next; - if ((macsList = virHashLookup(mgr->macs, domain)) && - virStringListHasString((const char**) macsList, mac)) { - return 0; + list = orig_list = g_hash_table_lookup(mgr->macs, domain); + + for (next = list; next; next = next->next) { + if (STREQ((const char *) next->data, mac)) + return; } - if (virStringListAdd(&macsList, mac) < 0 || - virHashUpdateEntry(mgr->macs, domain, macsList) < 0) - return -1; + list = g_slist_append(list, g_strdup(mac)); - return 0; + if (list != orig_list) + g_hash_table_insert(mgr->macs, g_strdup(domain), list); } -static int +static void virMacMapRemoveLocked(virMacMapPtr mgr, const char *domain, const char *mac) { - char **macsList = NULL; - char **newMacsList = NULL; + GSList *orig_list; + GSList *list; + GSList *next; - if (!(macsList = virHashLookup(mgr->macs, domain))) - return 0; + list = orig_list = g_hash_table_lookup(mgr->macs, domain); - newMacsList = macsList; - virStringListRemove(&newMacsList, mac); - if (!newMacsList) { - virHashSteal(mgr->macs, domain); - } else { - if (macsList != newMacsList && - virHashUpdateEntry(mgr->macs, domain, newMacsList) < 0) - return -1; + if (!orig_list) + return; + + for (next = list; next; next = next->next) { + if (STREQ((const char *) next->data, mac)) { + list = g_slist_remove_link(list, next); + g_slist_free_full(next, g_free); + break; + } } - return 0; + if (list != orig_list) { + if (list) + g_hash_table_insert(mgr->macs, g_strdup(domain), list); + else + g_hash_table_remove(mgr->macs, domain); + } } @@ -162,6 +168,7 @@ virMacMapLoadFile(virMacMapPtr mgr, virJSONValuePtr macs; const char *domain; size_t j; + GSList *vals = NULL; if (!(domain = virJSONValueObjectGetString(tmp, "domain"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -175,13 +182,21 @@ virMacMapLoadFile(virMacMapPtr mgr, return -1; } + if (g_hash_table_contains(mgr->macs, domain)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("duplicate domain '%s'"), domain); + return -1; + + } + for (j = 0; j < virJSONValueArraySize(macs); j++) { virJSONValuePtr macJSON = virJSONValueArrayGet(macs, j); - const char *mac = virJSONValueGetString(macJSON); - if (virMacMapAddLocked(mgr, domain, mac) < 0) - return -1; + vals = g_slist_prepend(vals, g_strdup(virJSONValueGetString(macJSON))); } + + vals = g_slist_reverse(vals); + g_hash_table_insert(mgr->macs, g_strdup(domain), vals); } return 0; @@ -195,11 +210,11 @@ virMACMapHashDumper(void *payload, { g_autoptr(virJSONValue) obj = virJSONValueNewObject(); g_autoptr(virJSONValue) arr = virJSONValueNewArray(); - const char **macs = payload; - size_t i; + GSList *macs = payload; + GSList *next; - for (i = 0; macs[i]; i++) { - virJSONValuePtr m = virJSONValueNewString(macs[i]); + for (next = macs; next; next = next->next) { + virJSONValuePtr m = virJSONValueNewString((const char *) next->data); if (!m || virJSONValueArrayAppend(arr, m) < 0) { @@ -279,8 +294,8 @@ virMacMapNew(const char *file) return NULL; virObjectLock(mgr); - if (!(mgr->macs = virHashNew(NULL))) - goto error; + + mgr->macs = virHashNew(NULL); if (file && virMacMapLoadFile(mgr, file) < 0) @@ -301,12 +316,10 @@ virMacMapAdd(virMacMapPtr mgr, const char *domain, const char *mac) { - int ret; - virObjectLock(mgr); - ret = virMacMapAddLocked(mgr, domain, mac); + virMacMapAddLocked(mgr, domain, mac); virObjectUnlock(mgr); - return ret; + return 0; } @@ -315,20 +328,19 @@ virMacMapRemove(virMacMapPtr mgr, const char *domain, const char *mac) { - int ret; - virObjectLock(mgr); - ret = virMacMapRemoveLocked(mgr, domain, mac); + virMacMapRemoveLocked(mgr, domain, mac); virObjectUnlock(mgr); - return ret; + return 0; } -const char *const * +/* note that the returned pointer may be invalidated by other APIs in this module */ +GSList * virMacMapLookup(virMacMapPtr mgr, const char *domain) { - const char *const *ret; + GSList *ret; virObjectLock(mgr); ret = virHashLookup(mgr->macs, domain); diff --git a/src/util/virmacmap.h b/src/util/virmacmap.h index 4652295033..96e32256e3 100644 --- a/src/util/virmacmap.h +++ b/src/util/virmacmap.h @@ -20,6 +20,8 @@ #pragma once +#include "internal.h" + typedef struct virMacMap virMacMap; typedef virMacMap *virMacMapPtr; @@ -37,8 +39,8 @@ int virMacMapRemove(virMacMapPtr mgr, const char *domain, const char *mac); -const char *const *virMacMapLookup(virMacMapPtr mgr, - const char *domain); +GSList *virMacMapLookup(virMacMapPtr mgr, + const char *domain); int virMacMapWriteFile(virMacMapPtr mgr, const char *filename); diff --git a/tests/virmacmaptest.c b/tests/virmacmaptest.c index 8fd9916b95..77fa2b3e98 100644 --- a/tests/virmacmaptest.c +++ b/tests/virmacmaptest.c @@ -36,7 +36,8 @@ testMACLookup(const void *opaque) { const struct testData *data = opaque; virMacMapPtr mgr = NULL; - const char * const * macs; + GSList *macs; + GSList *next; size_t i, j; char *file = NULL; int ret = -1; @@ -48,26 +49,27 @@ testMACLookup(const void *opaque) macs = virMacMapLookup(mgr, data->domain); - for (i = 0; macs && macs[i]; i++) { + for (next = macs; next; next = next->next) { for (j = 0; data->macs && data->macs[j]; j++) { - if (STREQ(macs[i], data->macs[j])) + if (STREQ((const char *) next->data, data->macs[j])) break; } if (!data->macs || !data->macs[j]) { fprintf(stderr, - "Unexpected %s in the returned list of MACs\n", macs[i]); + "Unexpected %s in the returned list of MACs\n", + (const char *) next->data); goto cleanup; } } for (i = 0; data->macs && data->macs[i]; i++) { - for (j = 0; macs && macs[j]; j++) { - if (STREQ(data->macs[i], macs[j])) + for (next = macs; next; next = next->next) { + if (STREQ(data->macs[i], (const char *) next->data)) break; } - if (!macs || !macs[j]) { + if (!next) { fprintf(stderr, "Expected %s in the returned list of MACs\n", data->macs[i]); goto cleanup; @@ -87,7 +89,7 @@ testMACRemove(const void *opaque) { const struct testData *data = opaque; virMacMapPtr mgr = NULL; - const char * const * macs; + GSList *macs; size_t i; char *file = NULL; int ret = -1; @@ -107,7 +109,8 @@ testMACRemove(const void *opaque) if ((macs = virMacMapLookup(mgr, data->domain))) { fprintf(stderr, - "Not removed all MACs for domain %s: %s\n", data->domain, macs[0]); + "Not removed all MACs for domain %s: %s\n", + data->domain, (const char *) macs->data); goto cleanup; } -- 2.29.2

We are already looping over the arguments to construct the list, so we can add them to fwBuf right away rather than in an extra loop if we move some of the 'fwBuf' parts earlier and merge the two loops. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- tests/virfirewalltest.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c index 9f95520859..e14a34d7d2 100644 --- a/tests/virfirewalltest.c +++ b/tests/virfirewalltest.c @@ -108,12 +108,20 @@ VIR_MOCK_WRAP_RET_ARGS(g_dbus_connection_call_sync, char *item = NULL; bool isAdd = false; bool doError = false; - size_t i; g_variant_get(params, "(&sas)", &type, &iter); nargs = g_variant_iter_n_children(iter); + if (fwBuf) { + if (STREQ(type, "ipv4")) + virBufferAddLit(fwBuf, IPTABLES_PATH); + else if (STREQ(type, "ipv6")) + virBufferAddLit(fwBuf, IP6TABLES_PATH); + else + virBufferAddLit(fwBuf, EBTABLES_PATH); + } + while (g_variant_iter_loop(iter, "s", &item)) { /* Fake failure on the command with this IP addr */ if (STREQ(item, "-A")) { @@ -123,21 +131,10 @@ VIR_MOCK_WRAP_RET_ARGS(g_dbus_connection_call_sync, } virStringListAdd(&args, item); - } - - if (fwBuf) { - if (STREQ(type, "ipv4")) - virBufferAddLit(fwBuf, IPTABLES_PATH); - else if (STREQ(type, "ipv6")) - virBufferAddLit(fwBuf, IP6TABLES_PATH); - else - virBufferAddLit(fwBuf, EBTABLES_PATH); - } - for (i = 0; i < nargs; i++) { if (fwBuf) { virBufferAddLit(fwBuf, " "); - virBufferEscapeShell(fwBuf, args[i]); + virBufferEscapeShell(fwBuf, item); } } -- 2.29.2

The validation code looks whether certain paths are in the 'notRestored' list. For the purpose of lookup it's better to use a hash table rather than a string list. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- tests/qemusecuritymock.c | 19 +++++++++++-------- tests/qemusecuritytest.c | 14 ++++++-------- tests/qemusecuritytest.h | 4 +++- 3 files changed, 20 insertions(+), 17 deletions(-) diff --git a/tests/qemusecuritymock.c b/tests/qemusecuritymock.c index 1fa4e522cc..5daf27ccd7 100644 --- a/tests/qemusecuritymock.c +++ b/tests/qemusecuritymock.c @@ -398,7 +398,7 @@ int virFileUnlock(int fd G_GNUC_UNUSED, typedef struct _checkOwnerData checkOwnerData; struct _checkOwnerData { - const char **paths; + GHashTable *paths; bool chown_fail; bool selinux_fail; }; @@ -413,7 +413,7 @@ checkSELinux(void *payload, char *label = payload; if (STRNEQ(label, DEFAULT_SELINUX_LABEL) && - !virStringListHasString(data->paths, name)) { + !g_hash_table_contains(data->paths, name)) { fprintf(stderr, "Path %s wasn't restored back to its original SELinux label\n", name); @@ -434,7 +434,7 @@ checkOwner(void *payload, if ((owner % 16 != DEFAULT_UID || owner >> 16 != DEFAULT_GID) && - !virStringListHasString(data->paths, name)) { + !g_hash_table_contains(data->paths, name)) { fprintf(stderr, "Path %s wasn't restored back to its original owner\n", name); @@ -473,19 +473,22 @@ printXATTR(void *payload, * can be passed in @paths argument. If a path is not restored * but it's on the list no error is indicated. */ -int checkPaths(const char **paths) +int checkPaths(GHashTable *paths) { int ret = -1; checkOwnerData data = { .paths = paths, .chown_fail = false, .selinux_fail = false }; bool xattr_fail = false; - size_t i; + GHashTableIter htitr; + void *key; virMutexLock(&m); init_hash(); - for (i = 0; paths && paths[i]; i++) { - if (!virHashLookup(chown_paths, paths[i])) { - fprintf(stderr, "Unexpected path restored: %s\n", paths[i]); + g_hash_table_iter_init(&htitr, paths); + + while (g_hash_table_iter_next(&htitr, &key, NULL)) { + if (!virHashLookup(chown_paths, key)) { + fprintf(stderr, "Unexpected path restored: %s\n", (const char *) key); goto cleanup; } } diff --git a/tests/qemusecuritytest.c b/tests/qemusecuritytest.c index 1750018137..74a25f2be1 100644 --- a/tests/qemusecuritytest.c +++ b/tests/qemusecuritytest.c @@ -87,7 +87,7 @@ testDomain(const void *opaque) { const struct testData *data = opaque; g_autoptr(virDomainObj) vm = NULL; - g_auto(GStrv) notRestored = NULL; + g_autoptr(GHashTable) notRestored = virHashNew(NULL); size_t i; int ret = -1; @@ -102,14 +102,12 @@ testDomain(const void *opaque) continue; if (virStorageSourceIsLocalStorage(src) && src->path && - (src->shared || src->readonly) && - virStringListAdd(&notRestored, src->path) < 0) - return -1; + (src->shared || src->readonly)) + g_hash_table_insert(notRestored, g_strdup(src->path), NULL); for (n = src->backingStore; virStorageSourceIsBacking(n); n = n->backingStore) { - if (virStorageSourceIsLocalStorage(n) && n->path && - virStringListAdd(&notRestored, n->path) < 0) - return -1; + if (virStorageSourceIsLocalStorage(n) && n->path) + g_hash_table_insert(notRestored, g_strdup(n->path), NULL); } } @@ -123,7 +121,7 @@ testDomain(const void *opaque) qemuSecurityRestoreAllLabel(data->driver, vm, false); - if (checkPaths((const char **) notRestored) < 0) + if (checkPaths(notRestored) < 0) goto cleanup; ret = 0; diff --git a/tests/qemusecuritytest.h b/tests/qemusecuritytest.h index cc3918ddf5..696cfb4b63 100644 --- a/tests/qemusecuritytest.h +++ b/tests/qemusecuritytest.h @@ -20,6 +20,8 @@ #define ENVVAR "LIBVIRT_QEMU_SECURITY_TEST" -extern int checkPaths(const char **paths); +#include "internal.h" + +extern int checkPaths(GHashTable *paths); extern void freePaths(void); -- 2.29.2

Lookup the string with prefix locally so that we can remove the helper which isn't universal at all. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/vircgroup.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index b5c74e94de..2c418cd84d 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -568,21 +568,21 @@ virCgroupGetValueForBlkDev(const char *str, char **value) { g_autofree char *prefix = NULL; - char **lines = NULL; - int ret = -1; + g_auto(GStrv) lines = NULL; + GStrv tmp; if (!(prefix = virCgroupGetBlockDevString(path))) - goto error; + return -1; if (!(lines = virStringSplit(str, "\n", -1))) - goto error; + return -1; - *value = g_strdup(virStringListGetFirstWithPrefix(lines, prefix)); + for (tmp = lines; *tmp; tmp++) { + if ((*value = g_strdup(STRSKIP(*tmp, prefix)))) + break; + } - ret = 0; - error: - g_strfreev(lines); - return ret; + return 0; } -- 2.29.2

The caller doesn't care about the number of tokens so use the function which doesn't return it. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/vz/vz_sdk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 00891dc16a..9ea0edaf97 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -745,7 +745,7 @@ prlsdkGetFSInfo(PRL_HANDLE prldisk, goto cleanup; } - if (!(matches = virStringSplitCount(uri->path, "/", 0, NULL)) || + if (!(matches = virStringSplit(uri->path, "/", 0)) || !matches[0]) { virReportError(VIR_ERR_INTERNAL_ERROR, _("splitting StorageUrl failed %s"), uri->path); -- 2.29.2

The glib variant doesn't accept NULL list, but there's just one caller where it wasn't checked explicitly, thus there's no need for our own wrapper. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/lxc/lxc_native.c | 4 ++-- src/qemu/qemu_capabilities.c | 8 ++++---- src/qemu/qemu_process.c | 4 ++-- tests/qemumonitorjsontest.c | 6 +++--- tools/virsh-completer.c | 3 ++- 5 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c index 58b9db8f08..e68977dd99 100644 --- a/src/lxc/lxc_native.c +++ b/src/lxc/lxc_native.c @@ -304,7 +304,7 @@ lxcAddFstabLine(virDomainDefPtr def, lxcFstabPtr fstab) type = VIR_DOMAIN_FS_TYPE_BLOCK; /* Do we have ro in options? */ - readonly = virStringListHasString((const char **)options, "ro"); + readonly = g_strv_contains((const char **)options, "ro"); if (lxcAddFSDef(def, type, src, dst, readonly, usage) < 0) goto cleanup; @@ -1108,7 +1108,7 @@ lxcSetCapDrop(virDomainDefPtr def, virConfPtr properties) for (i = 0; i < VIR_DOMAIN_PROCES_CAPS_FEATURE_LAST; i++) { capString = virDomainProcessCapsFeatureTypeToString(i); if (toDrop != NULL && - virStringListHasString((const char **)toDrop, capString)) + g_strv_contains((const char **)toDrop, capString)) def->caps_features[i] = VIR_TRISTATE_SWITCH_OFF; } diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index d41b4a4753..8f60ddeb7d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2185,10 +2185,10 @@ virQEMUCapsCPUDefsToModels(qemuMonitorCPUDefsPtr defs, for (i = 0; i < defs->ncpus; i++) { qemuMonitorCPUDefInfoPtr cpu = defs->cpus + i; - if (modelAllowed && !virStringListHasString(modelAllowed, cpu->name)) + if (modelAllowed && !g_strv_contains(modelAllowed, cpu->name)) continue; - if (modelForbidden && virStringListHasString(modelForbidden, cpu->name)) + if (modelForbidden && g_strv_contains(modelForbidden, cpu->name)) continue; if (virDomainCapsCPUModelsAdd(cpuModels, cpu->name, cpu->usable, @@ -3193,7 +3193,7 @@ virQEMUCapsProbeQMPTPM(virQEMUCapsPtr qemuCaps, for (i = 0; i < G_N_ELEMENTS(virQEMUCapsTPMModelsToCaps); i++) { const char *needle = virDomainTPMModelTypeToString( virQEMUCapsTPMModelsToCaps[i].type); - if (virStringListHasString((const char **)entries, needle)) + if (g_strv_contains((const char **)entries, needle)) virQEMUCapsSet(qemuCaps, virQEMUCapsTPMModelsToCaps[i].caps); } @@ -3207,7 +3207,7 @@ virQEMUCapsProbeQMPTPM(virQEMUCapsPtr qemuCaps, for (i = 0; i < G_N_ELEMENTS(virQEMUCapsTPMTypesToCaps); i++) { const char *needle = virDomainTPMBackendTypeToString( virQEMUCapsTPMTypesToCaps[i].type); - if (virStringListHasString((const char **)entries, needle)) + if (g_strv_contains((const char **)entries, needle)) virQEMUCapsSet(qemuCaps, virQEMUCapsTPMTypesToCaps[i].caps); } } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1fd3230e97..d24ef789ab 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3808,7 +3808,7 @@ qemuProcessUpdateDevices(virQEMUDriverPtr driver, return 0; for (tmp = old; *tmp; tmp++) { - if (!virStringListHasString((const char **) priv->qemuDevices, *tmp) && + if (!g_strv_contains((const char **) priv->qemuDevices, *tmp) && virDomainDefFindDevice(vm->def, *tmp, &dev, false) == 0 && qemuDomainRemoveDevice(driver, vm, &dev)) return -1; @@ -6098,7 +6098,7 @@ qemuProcessDropUnknownCPUFeatures(const char *name, policy != VIR_CPU_FEATURE_FORBID) return true; - if (virStringListHasString(features, name)) + if (g_strv_contains(features, name)) return true; /* Features unknown to QEMU are implicitly disabled, we can just drop them diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 956423f10f..5a5976dbe4 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1057,13 +1057,13 @@ testQemuMonitorJSONGetDeviceAliases(const void *opaque) ret = 0; for (alias = (const char **) aliases; *alias; alias++) { - if (!virStringListHasString(expected, *alias)) { + if (!g_strv_contains(expected, *alias)) { fprintf(stderr, "got unexpected device alias '%s'\n", *alias); ret = -1; } } for (alias = expected; *alias; alias++) { - if (!virStringListHasString((const char **) aliases, *alias)) { + if (!g_strv_contains((const char **) aliases, *alias)) { fprintf(stderr, "missing expected alias '%s'\n", *alias); ret = -1; } @@ -2079,7 +2079,7 @@ testQemuMonitorJSONqemuMonitorJSONGetMigrationCapabilities(const void *opaque) return -1; cap = qemuMigrationCapabilityTypeToString(QEMU_MIGRATION_CAP_XBZRLE); - if (!virStringListHasString((const char **) caps, cap)) { + if (!g_strv_contains((const char **) caps, cap)) { virReportError(VIR_ERR_INTERNAL_ERROR, "Expected capability %s is missing", cap); return -1; diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c index c7aed7c779..fb42358bd0 100644 --- a/tools/virsh-completer.c +++ b/tools/virsh-completer.c @@ -114,7 +114,8 @@ virshCommaStringListComplete(const char *input, ret = g_new0(char *, optionsLen + 1); for (i = 0; i < optionsLen; i++) { - if (virStringListHasString((const char **)inputList, options[i])) + if (inputList && + g_strv_contains((const char **)inputList, options[i])) continue; if (inputCopy) -- 2.29.2

'cells' can be pushed into the loop removing the need for manual cleanup, the check whether 'line' is NULL inside of the loop is always false since the loop checks it right before and 'line' variable is unnecessary. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/storage/storage_backend_sheepdog.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c index c5b7c568dd..178cfbae11 100644 --- a/src/storage/storage_backend_sheepdog.c +++ b/src/storage/storage_backend_sheepdog.c @@ -140,7 +140,6 @@ virStorageBackendSheepdogRefreshAllVol(virStoragePoolObjPtr pool) size_t i; g_autofree char *output = NULL; g_auto(GStrv) lines = NULL; - g_auto(GStrv) cells = NULL; g_autoptr(virCommand) cmd = NULL; cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "list", "-r", NULL); @@ -154,20 +153,15 @@ virStorageBackendSheepdogRefreshAllVol(virStoragePoolObjPtr pool) return -1; for (i = 0; lines[i]; i++) { - const char *line = lines[i]; - if (line == NULL) - break; + g_auto(GStrv) cells = NULL; - cells = virStringSplit(line, " ", 0); + cells = virStringSplit(lines[i], " ", 0); if (cells != NULL && virStringListLength((const char * const *)cells) > 2) { if (virStorageBackendSheepdogAddVolume(pool, cells[1]) < 0) return -1; } - - g_strfreev(cells); - cells = NULL; } return 0; -- 2.29.2

On 2/6/21 9:32 AM, Peter Krempa wrote: Can't we use simple 'const char **'? We can avoid this typecast then. Michal

On 2/6/21 9:32 AM, Peter Krempa wrote: Same here. s/GStrv/const char **/ Michal

The glib implementation doesn't tolerate NULL but in most cases we check before anyways. The rest of the callers adds a NULL check. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/cpu/cpu_arm.c | 2 +- src/qemu/qemu_conf.c | 4 ++-- src/qemu/qemu_firmware.c | 5 ++++- src/qemu/qemu_vhost_user.c | 4 +++- src/util/virprocess.c | 3 ++- src/util/virstring.c | 4 ++-- tests/virconftest.c | 4 ++-- tests/virstringtest.c | 6 +++--- tools/virsh-completer.c | 2 +- tools/virsh-domain.c | 2 +- tools/virsh-host.c | 4 ++-- tools/virt-login-shell-helper.c | 2 +- tools/vsh.c | 2 +- 13 files changed, 25 insertions(+), 19 deletions(-) diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c index 6ba5bf0724..a7aef29fe4 100644 --- a/src/cpu/cpu_arm.c +++ b/src/cpu/cpu_arm.c @@ -637,7 +637,7 @@ virCPUarmDecode(virCPUDefPtr cpu, cpu->vendor = g_strdup(vendor->name); if (cpuData->features) { - cpu->nfeatures = virStringListLength((const char **)cpuData->features); + cpu->nfeatures = g_strv_length(cpuData->features); cpu->features = g_new0(virCPUFeatureDef, cpu->nfeatures); for (i = 0; i < cpu->nfeatures; i++) { diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 35d0b6515c..2bbc75024c 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -648,7 +648,7 @@ virQEMUDriverConfigLoadProcessEntry(virQEMUDriverConfigPtr cfg, } VIR_FREE(cfg->hugetlbfs); - cfg->nhugetlbfs = virStringListLength((const char *const *)hugetlbfs); + cfg->nhugetlbfs = g_strv_length(hugetlbfs); if (hugetlbfs[0]) cfg->hugetlbfs = g_new0(virHugeTLBFS, cfg->nhugetlbfs); @@ -847,7 +847,7 @@ virQEMUDriverConfigLoadNVRAMEntry(virQEMUDriverConfigPtr cfg, return 0; } - cfg->nfirmwares = virStringListLength((const char *const *)nvram); + cfg->nfirmwares = g_strv_length(nvram); cfg->firmwares = g_new0(virFirmwarePtr, cfg->nfirmwares); for (i = 0; nvram[i] != NULL; i++) { diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index e6541d505e..c571fdecf7 100644 --- a/src/qemu/qemu_firmware.c +++ b/src/qemu/qemu_firmware.c @@ -1191,7 +1191,10 @@ qemuFirmwareFetchParsedConfigs(bool privileged, if (qemuFirmwareFetchConfigs(&paths, privileged) < 0) return -1; - npaths = virStringListLength((const char **)paths); + if (!paths) + return 0; + + npaths = g_strv_length(paths); firmwares = g_new0(qemuFirmwarePtr, npaths); diff --git a/src/qemu/qemu_vhost_user.c b/src/qemu/qemu_vhost_user.c index 5ceefbe456..a9f6f3192d 100644 --- a/src/qemu/qemu_vhost_user.c +++ b/src/qemu/qemu_vhost_user.c @@ -250,8 +250,10 @@ qemuVhostUserFetchParsedConfigs(bool privileged, if (qemuVhostUserFetchConfigs(&paths, privileged) < 0) return -1; - npaths = virStringListLength((const char **)paths); + if (!paths) + return 0; + npaths = g_strv_length(paths); vus = g_new0(qemuVhostUserPtr, npaths); for (i = 0; i < npaths; i++) { diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 0523bf9afb..4c27f9d771 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -1013,7 +1013,8 @@ int virProcessGetStartTime(pid_t pid, tokens = virStringSplit(tmp, " ", 0); - if (virStringListLength((const char * const *)tokens) < 20) { + if (!tokens || + g_strv_length(tokens) < 20) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot find start time in %s"), filename); diff --git a/src/util/virstring.c b/src/util/virstring.c index c0393585e2..1fd61650be 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -182,8 +182,8 @@ virStringListMerge(char ***dst, if (!src || !*src) return 0; - dst_len = virStringListLength((const char **) *dst); - src_len = virStringListLength((const char **) *src); + dst_len = g_strv_length(*dst); + src_len = g_strv_length(*src); if (VIR_REALLOC_N(*dst, dst_len + src_len + 1) < 0) return -1; diff --git a/tests/virconftest.c b/tests/virconftest.c index 8269730662..7a759db014 100644 --- a/tests/virconftest.c +++ b/tests/virconftest.c @@ -394,7 +394,7 @@ static int testConfParseStringList(const void *opaque G_GNUC_UNUSED) if (virConfGetValueStringList(conf, "string_list", false, &str) < 0) goto cleanup; - if (virStringListLength((const char *const*)str) != 2) { + if (!str || g_strv_length(str) != 2) { fprintf(stderr, "expected a 2 element list\n"); goto cleanup; } @@ -418,7 +418,7 @@ static int testConfParseStringList(const void *opaque G_GNUC_UNUSED) if (virConfGetValueStringList(conf, "string", true, &str) < 0) goto cleanup; - if (virStringListLength((const char *const*)str) != 1) { + if (!str || g_strv_length(str) != 1) { fprintf(stderr, "expected a 1 element list\n"); goto cleanup; } diff --git a/tests/virstringtest.c b/tests/virstringtest.c index 9eabb5b299..238cb9d79e 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -243,10 +243,10 @@ testStringSearch(const void *opaque) goto cleanup; } - if (virStringListLength((const char * const *)matches) != nmatches) { - fprintf(stderr, "expected %zu matches on %s but got %zd matches\n", + if (g_strv_length(matches) != nmatches) { + fprintf(stderr, "expected %zu matches on %s but got %u matches\n", data->expectNMatches, data->str, - virStringListLength((const char * const *)matches)); + g_strv_length(matches)); goto cleanup; } diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c index fb42358bd0..d3923c4bc7 100644 --- a/tools/virsh-completer.c +++ b/tools/virsh-completer.c @@ -87,7 +87,7 @@ char ** virshCommaStringListComplete(const char *input, const char **options) { - const size_t optionsLen = virStringListLength(options); + const size_t optionsLen = g_strv_length((char **) options); g_autofree char *inputCopy = NULL; g_auto(GStrv) inputList = NULL; g_auto(GStrv) ret = NULL; diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 98a87dd43c..05d79a7843 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -14391,7 +14391,7 @@ cmdSetUserSSHKeys(vshControl *ctl, const vshCmd *cmd) if (!(keys = virStringSplit(buffer, "\n", -1))) goto cleanup; - nkeys = virStringListLength((const char **) keys); + nkeys = g_strv_length(keys); if (nkeys == 0) { vshError(ctl, _("File %s contains no keys"), from); goto cleanup; diff --git a/tools/virsh-host.c b/tools/virsh-host.c index ba524523f5..747d355456 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -1325,7 +1325,7 @@ cmdCPUBaseline(vshControl *ctl, const vshCmd *cmd) return false; result = virConnectBaselineCPU(priv->conn, (const char **)list, - virStringListLength((const char **)list), + g_strv_length(list), flags); if (result) { @@ -1798,7 +1798,7 @@ cmdHypervisorCPUBaseline(vshControl *ctl, result = virConnectBaselineHypervisorCPU(priv->conn, emulator, arch, machine, virttype, (const char **)list, - virStringListLength((const char **)list), + g_strv_length(list), flags); if (result) { diff --git a/tools/virt-login-shell-helper.c b/tools/virt-login-shell-helper.c index 0522896eee..1ac02bed00 100644 --- a/tools/virt-login-shell-helper.c +++ b/tools/virt-login-shell-helper.c @@ -103,7 +103,7 @@ static int virLoginShellGetShellArgv(virConfPtr conf, (*shargv)[0] = g_strdup("/bin/sh"); *shargvlen = 1; } else { - *shargvlen = virStringListLength((const char *const *)*shargv); + *shargvlen = g_strv_length(*shargv); } return 0; } diff --git a/tools/vsh.c b/tools/vsh.c index 202bd564f7..4e76ad348a 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2704,7 +2704,7 @@ vshCompleterFilter(char ***list, if (!list || !*list) return -1; - list_len = virStringListLength((const char **) *list); + list_len = g_strv_length(*list); newList = g_new0(char *, list_len + 1); for (i = 0; i < list_len; i++) { -- 2.29.2

Our implementation was heavily inspired by the glib version so it's a drop-in replacement. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/bhyve/bhyve_command.c | 2 +- src/conf/storage_conf.c | 2 +- src/libxl/libxl_conf.c | 8 ++++---- src/libxl/xen_common.c | 6 +++--- src/libxl/xen_xl.c | 4 ++-- src/lxc/lxc_native.c | 8 ++++---- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_qapi.c | 2 +- src/rpc/virnetsocket.c | 4 ++-- src/storage/storage_backend_sheepdog.c | 4 ++-- src/storage/storage_backend_zfs.c | 6 +++--- src/storage_file/storage_source_backingstore.c | 4 ++-- src/util/vircgroup.c | 6 +++--- src/util/vircgroupv2.c | 2 +- src/util/vircommand.c | 2 +- src/util/virdevmapper.c | 2 +- src/util/virfile.c | 2 +- src/util/virfirewall.c | 2 +- src/util/virfirmware.c | 4 ++-- src/util/virprocess.c | 2 +- src/vz/vz_sdk.c | 2 +- tests/qemuxml2argvtest.c | 2 +- tests/vboxsnapshotxmltest.c | 2 +- tools/virsh-completer.c | 2 +- tools/virsh-domain.c | 12 ++++++------ 25 files changed, 47 insertions(+), 47 deletions(-) diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c index daf313c9c1..a963338654 100644 --- a/src/bhyve/bhyve_command.c +++ b/src/bhyve/bhyve_command.c @@ -806,7 +806,7 @@ virAppendBootloaderArgs(virCommandPtr cmd, virDomainDefPtr def) char **blargs; /* XXX: Handle quoted? */ - blargs = virStringSplit(def->os.bootloaderArgs, " ", 0); + blargs = g_strsplit(def->os.bootloaderArgs, " ", 0); virCommandAddArgSet(cmd, (const char * const *)blargs); g_strfreev(blargs); } diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 3f06fcaebf..0f515c7cbb 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1305,7 +1305,7 @@ virStorageCheckCompat(const char *compat) if (!compat) return 0; - version = virStringSplit(compat, ".", 2); + version = g_strsplit(compat, ".", 2); if (!version || !version[1] || virStrToLong_ui(version[0], NULL, 10, &result) < 0 || virStrToLong_ui(version[1], NULL, 10, &result) < 0) { diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 37a2c2e3cd..f8db118996 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -698,7 +698,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, b_info->bootloader = g_strdup(def->os.bootloader); if (def->os.bootloaderArgs) { if (!(b_info->bootloader_args = - virStringSplit(def->os.bootloaderArgs, " \t\n", 0))) + g_strsplit(def->os.bootloaderArgs, " \t\n", 0))) return -1; } #endif @@ -714,7 +714,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, } if (def->os.bootloaderArgs) { if (!(b_info->u.pv.bootloader_args = - virStringSplit(def->os.bootloaderArgs, " \t\n", 0))) + g_strsplit(def->os.bootloaderArgs, " \t\n", 0))) return -1; } b_info->u.pv.cmdline = g_strdup(def->os.cmdline); @@ -1896,14 +1896,14 @@ libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg, int ret = -1; if (cfg->verInfo->commandline == NULL || - !(cmd_tokens = virStringSplit(cfg->verInfo->commandline, " ", 0))) + !(cmd_tokens = g_strsplit(cfg->verInfo->commandline, " ", 0))) goto physmem; for (i = 0; cmd_tokens[i] != NULL; i++) { if (!STRPREFIX(cmd_tokens[i], "dom0_mem=")) continue; - if (!(mem_tokens = virStringSplit(cmd_tokens[i], ",", 0))) + if (!(mem_tokens = g_strsplit(cmd_tokens[i], ",", 0))) break; for (j = 0; mem_tokens[j] != NULL; j++) { if (STRPREFIX(mem_tokens[j], "max:")) { diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 2d1f5ea5f5..75c65a4135 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -420,7 +420,7 @@ xenParsePCI(char *entry) return NULL; str = nextstr; - if (str && (options = virStringSplit(str, ",", 0))) { + if (str && (options = g_strsplit(str, ",", 0))) { size_t i; for (i = 0; options[i] != NULL; i++) { @@ -1051,7 +1051,7 @@ xenParseVifBridge(virDomainNetDefPtr net, char *bridge) /* 'bridge' string contains a bridge name and one or more vlan trunks */ size_t i; size_t nvlans = 0; - char **vlanstr_list = virStringSplit(bridge, ":", 0); + char **vlanstr_list = g_strsplit(bridge, ":", 0); if (!vlanstr_list) return -1; @@ -1258,7 +1258,7 @@ xenParseVif(char *entry, const char *vif_typename) goto cleanup; } if (ip[0]) { - char **ip_list = virStringSplit(ip, " ", 0); + char **ip_list = g_strsplit(ip, " ", 0); size_t i; if (!ip_list) diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index a8af54a845..a7036ba494 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -268,7 +268,7 @@ xenParseXLCPUID(virConfPtr conf, virDomainDefPtr def) def->cpu->nfeatures_max = 0; } - cpuid_pairs = virStringSplit(cpuid_str, ",", 0); + cpuid_pairs = g_strsplit(cpuid_str, ",", 0); if (!cpuid_pairs) goto cleanup; @@ -285,7 +285,7 @@ xenParseXLCPUID(virConfPtr conf, virDomainDefPtr def) } for (i = 1; cpuid_pairs[i]; i++) { - name_and_value = virStringSplit(cpuid_pairs[i], "=", 2); + name_and_value = g_strsplit(cpuid_pairs[i], "=", 2); if (!name_and_value) goto cleanup; if (!name_and_value[0] || !name_and_value[1]) { diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c index 4229a9ca7a..b903dc91d6 100644 --- a/src/lxc/lxc_native.c +++ b/src/lxc/lxc_native.c @@ -119,7 +119,7 @@ static char ** lxcStringSplit(const char *string) tmp[i] = ' '; } - if (!(parts = virStringSplit(tmp, " ", 0))) + if (!(parts = g_strsplit(tmp, " ", 0))) goto error; /* Append NULL element */ @@ -257,7 +257,7 @@ lxcAddFstabLine(virDomainDefPtr def, lxcFstabPtr fstab) { const char *src = NULL; g_autofree char *dst = NULL; - char **options = virStringSplit(fstab->options, ",", 0); + char **options = g_strsplit(fstab->options, ",", 0); bool readonly; int type = VIR_DOMAIN_FS_TYPE_MOUNT; unsigned long long usage = 0; @@ -567,7 +567,7 @@ lxcNetworkParseDataIPs(const char *name, if (STREQ(name, "ipv6") || STREQ(name, "ipv6.address")) family = AF_INET6; - ipparts = virStringSplit(value->str, "/", 2); + ipparts = g_strsplit(value->str, "/", 2); if (!ipparts || !ipparts[0] || !ipparts[1] || virSocketAddrParse(&ip->address, ipparts[0], family) < 0 || virStrToLong_ui(ipparts[1], NULL, 10, &ip->prefix) < 0) { @@ -1103,7 +1103,7 @@ lxcSetCapDrop(virDomainDefPtr def, virConfPtr properties) size_t i; if (virConfGetValueString(properties, "lxc.cap.drop", &value) > 0) - toDrop = virStringSplit(value, " ", 0); + toDrop = g_strsplit(value, " ", 0); for (i = 0; i < VIR_DOMAIN_PROCES_CAPS_FEATURE_LAST; i++) { capString = virDomainProcessCapsFeatureTypeToString(i); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 80bd27696b..ca4ba8071c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1365,7 +1365,7 @@ qemuGetSchedInfo(unsigned long long *cpuWait, if (virFileReadAll(proc, (1<<16), &data) < 0) goto cleanup; - lines = virStringSplit(data, "\n", 0); + lines = g_strsplit(data, "\n", 0); if (!lines) goto cleanup; diff --git a/src/qemu/qemu_qapi.c b/src/qemu/qemu_qapi.c index b41016f7d8..f7e4ca9324 100644 --- a/src/qemu/qemu_qapi.c +++ b/src/qemu/qemu_qapi.c @@ -440,7 +440,7 @@ virQEMUQAPISchemaPathGet(const char *query, if (entry) *entry = NULL; - if (!(elems = virStringSplit(query, "/", 0))) + if (!(elems = g_strsplit(query, "/", 0))) return -1; if (!*elems) { diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index d0dc59c10c..17aaf86ce3 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -961,7 +961,7 @@ virNetSocketNewConnectLibSSH2(const char *host, virNetSSHSessionSetChannelCommand(sess, command); - if (!(authMethodList = virStringSplit(authMethods, ",", 0))) + if (!(authMethodList = g_strsplit(authMethods, ",", 0))) goto error; for (authMethodNext = authMethodList; *authMethodNext; authMethodNext++) { @@ -1092,7 +1092,7 @@ virNetSocketNewConnectLibssh(const char *host, virNetLibsshSessionSetChannelCommand(sess, command); - if (!(authMethodList = virStringSplit(authMethods, ",", 0))) + if (!(authMethodList = g_strsplit(authMethods, ",", 0))) goto error; for (authMethodNext = authMethodList; *authMethodNext; authMethodNext++) { diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c index 2db2384e67..8c37947308 100644 --- a/src/storage/storage_backend_sheepdog.c +++ b/src/storage/storage_backend_sheepdog.c @@ -148,14 +148,14 @@ virStorageBackendSheepdogRefreshAllVol(virStoragePoolObjPtr pool) if (virCommandRun(cmd, NULL) < 0) return -1; - lines = virStringSplit(output, "\n", 0); + lines = g_strsplit(output, "\n", 0); if (lines == NULL) return -1; for (i = 0; lines[i]; i++) { g_auto(GStrv) cells = NULL; - cells = virStringSplit(lines[i], " ", 0); + cells = g_strsplit(lines[i], " ", 0); if (cells != NULL && cells[0] && cells[1]) { if (virStorageBackendSheepdogAddVolume(pool, cells[1]) < 0) diff --git a/src/storage/storage_backend_zfs.c b/src/storage/storage_backend_zfs.c index 31ffcc6f15..e4ef06722f 100644 --- a/src/storage/storage_backend_zfs.c +++ b/src/storage/storage_backend_zfs.c @@ -195,7 +195,7 @@ virStorageBackendZFSFindVols(virStoragePoolObjPtr pool, if (virCommandRun(cmd, NULL) < 0) return -1; - if (!(lines = virStringSplit(volumes_list, "\n", 0))) + if (!(lines = g_strsplit(volumes_list, "\n", 0))) return -1; for (i = 0; lines[i]; i++) { @@ -230,7 +230,7 @@ virStorageBackendZFSRefreshPool(virStoragePoolObjPtr pool G_GNUC_UNUSED) * * Here we just provide a list of properties we want to see */ - if (!(name_tokens = virStringSplit(def->source.name, "/", 0))) + if (!(name_tokens = g_strsplit(def->source.name, "/", 0))) goto cleanup; cmd = virCommandNewArgList(ZPOOL, @@ -242,7 +242,7 @@ virStorageBackendZFSRefreshPool(virStoragePoolObjPtr pool G_GNUC_UNUSED) if (virCommandRun(cmd, NULL) < 0) goto cleanup; - if (!(lines = virStringSplit(zpool_props, "\n", 0))) + if (!(lines = g_strsplit(zpool_props, "\n", 0))) goto cleanup; for (i = 0; lines[i]; i++) { diff --git a/src/storage_file/storage_source_backingstore.c b/src/storage_file/storage_source_backingstore.c index bbcc720af1..8e1606a1fe 100644 --- a/src/storage_file/storage_source_backingstore.c +++ b/src/storage_file/storage_source_backingstore.c @@ -54,7 +54,7 @@ virStorageSourceParseBackingURI(virStorageSourcePtr src, src->hosts = g_new0(virStorageNetHostDef, 1); src->nhosts = 1; - if (!(scheme = virStringSplit(uri->scheme, "+", 2))) + if (!(scheme = g_strsplit(uri->scheme, "+", 2))) return -1; if (!scheme[0] || @@ -165,7 +165,7 @@ virStorageSourceRBDAddHost(virStorageSourcePtr src, goto error; } - parts = virStringSplit(hostport, "\\:", 0); + parts = g_strsplit(hostport, "\\:", 0); if (!parts) goto error; src->hosts[src->nhosts-1].name = virStringListJoin((const char **)parts, ":"); diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 2c418cd84d..a7e2728dd7 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -574,7 +574,7 @@ virCgroupGetValueForBlkDev(const char *str, if (!(prefix = virCgroupGetBlockDevString(path))) return -1; - if (!(lines = virStringSplit(str, "\n", -1))) + if (!(lines = g_strsplit(str, "\n", -1))) return -1; for (tmp = lines; *tmp; tmp++) { @@ -805,7 +805,7 @@ virCgroupSetPartitionSuffix(const char *path, char **res) size_t i; int ret = -1; - if (!(tokens = virStringSplit(path, "/", 0))) + if (!(tokens = g_strsplit(path, "/", 0))) return ret; for (i = 0; tokens[i] != NULL; i++) { @@ -1140,7 +1140,7 @@ virCgroupEnableMissingControllers(char *path, virCgroupPtr *group) { g_autoptr(virCgroup) parent = NULL; - g_auto(GStrv) tokens = virStringSplit(path, "/", 0); + g_auto(GStrv) tokens = g_strsplit(path, "/", 0); size_t i; if (virCgroupNew("/", controllers, &parent) < 0) diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c index 4a239f067a..c9c3f5b76e 100644 --- a/src/util/vircgroupv2.c +++ b/src/util/vircgroupv2.c @@ -279,7 +279,7 @@ virCgroupV2ParseControllersFile(virCgroupPtr group, virTrimSpaces(contStr, NULL); - contList = virStringSplit(contStr, " ", 20); + contList = g_strsplit(contStr, " ", 20); if (!contList) return -1; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 87a60be201..323f841b98 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -3230,7 +3230,7 @@ virCommandRunRegex(virCommandPtr cmd, goto cleanup; } - if (!(lines = virStringSplit(outbuf, "\n", 0))) + if (!(lines = g_strsplit(outbuf, "\n", 0))) goto cleanup; for (k = 0; lines[k]; k++) { diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c index 15a7eb9fde..d97334bf67 100644 --- a/src/util/virdevmapper.c +++ b/src/util/virdevmapper.c @@ -63,7 +63,7 @@ virDevMapperGetMajor(unsigned int *major) if (virFileReadAll(PROC_DEVICES, BUF_SIZE, &buf) < 0) return -1; - lines = virStringSplit(buf, "\n", 0); + lines = g_strsplit(buf, "\n", 0); if (!lines) return -1; diff --git a/src/util/virfile.c b/src/util/virfile.c index e25cbe797e..5710495bbf 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1711,7 +1711,7 @@ virFindFileInPath(const char *file) * it. return it if found. */ - if (!(paths = virStringSplit(origpath, ":", 0))) + if (!(paths = g_strsplit(origpath, ":", 0))) return NULL; for (pathiter = paths; *pathiter; pathiter++) { diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index 8b69a18ecc..6b04a8011f 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -719,7 +719,7 @@ virFirewallApplyRule(virFirewallPtr firewall, } if (rule->queryCB && output) { - if (!(lines = virStringSplit(output, "\n", -1))) + if (!(lines = g_strsplit(output, "\n", -1))) return -1; VIR_DEBUG("Invoking query %p with '%s'", rule->queryCB, output); diff --git a/src/util/virfirmware.c b/src/util/virfirmware.c index d49bea3a55..c0ffcb4392 100644 --- a/src/util/virfirmware.c +++ b/src/util/virfirmware.c @@ -61,7 +61,7 @@ virFirmwareParse(const char *str, virFirmwarePtr firmware) int ret = -1; char **token; - if (!(token = virStringSplit(str, ":", 0))) + if (!(token = g_strsplit(str, ":", 0))) goto cleanup; if (token[0]) { @@ -98,7 +98,7 @@ virFirmwareParseList(const char *list, char **token; size_t i, j; - if (!(token = virStringSplit(list, ":", 0))) + if (!(token = g_strsplit(list, ":", 0))) goto cleanup; for (i = 0; token[i]; i += 2) { diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 4c27f9d771..41c5678537 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -1011,7 +1011,7 @@ int virProcessGetStartTime(pid_t pid, return -1; } - tokens = virStringSplit(tmp, " ", 0); + tokens = g_strsplit(tmp, " ", 0); if (!tokens || g_strv_length(tokens) < 20) { diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 9ea0edaf97..f49cd983f0 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -745,7 +745,7 @@ prlsdkGetFSInfo(PRL_HANDLE prldisk, goto cleanup; } - if (!(matches = virStringSplit(uri->path, "/", 0)) || + if (!(matches = g_strsplit(uri->path, "/", 0)) || !matches[0]) { virReportError(VIR_ERR_INTERNAL_ERROR, _("splitting StorageUrl failed %s"), uri->path); diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index faa71a7a16..d324921271 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -149,7 +149,7 @@ fakeStorageVolLookupByName(virStoragePoolPtr pool, if (!strchr(name, '+')) goto fallback; - if (!(volinfo = virStringSplit(name, "+", 2))) + if (!(volinfo = g_strsplit(name, "+", 2))) return NULL; if (!volinfo[1]) diff --git a/tests/vboxsnapshotxmltest.c b/tests/vboxsnapshotxmltest.c index d2beb7858d..9298bcb837 100644 --- a/tests/vboxsnapshotxmltest.c +++ b/tests/vboxsnapshotxmltest.c @@ -23,7 +23,7 @@ testFilterXML(char *xml) char **xmlLine; char *ret = NULL; - if (!(xmlLines = virStringSplit(xml, "\n", 0))) { + if (!(xmlLines = g_strsplit(xml, "\n", 0))) { VIR_FREE(xml); goto cleanup; } diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c index d3923c4bc7..57e0c3905c 100644 --- a/tools/virsh-completer.c +++ b/tools/virsh-completer.c @@ -108,7 +108,7 @@ virshCommaStringListComplete(const char *input, g_clear_pointer(&inputCopy, g_free); } - if (inputCopy && !(inputList = virStringSplit(inputCopy, ",", 0))) + if (inputCopy && !(inputList = g_strsplit(inputCopy, ",", 0))) return NULL; ret = g_new0(char *, optionsLen + 1); diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 05d79a7843..1c58c26d93 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -4057,7 +4057,7 @@ cmdStartGetFDs(vshControl *ctl, if (vshCommandOptStringQuiet(ctl, cmd, "pass-fds", &fdopt) <= 0) return 0; - if (!(fdlist = virStringSplit(fdopt, ",", -1))) { + if (!(fdlist = g_strsplit(fdopt, ",", -1))) { vshError(ctl, _("Unable to split FD list '%s'"), fdopt); return -1; } @@ -5929,7 +5929,7 @@ cmdShutdown(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "mode", &mode) < 0) return false; - if (mode && !(modes = virStringSplit(mode, ",", 0))) { + if (mode && !(modes = g_strsplit(mode, ",", 0))) { vshError(ctl, "%s", _("Cannot parse mode string")); return false; } @@ -6013,7 +6013,7 @@ cmdReboot(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "mode", &mode) < 0) return false; - if (mode && !(modes = virStringSplit(mode, ",", 0))) { + if (mode && !(modes = g_strsplit(mode, ",", 0))) { vshError(ctl, "%s", _("Cannot parse mode string")); return false; } @@ -10784,7 +10784,7 @@ doMigrate(void *opaque) if (opt) { char **val = NULL; - val = virStringSplit(opt, ",", 0); + val = g_strsplit(opt, ",", 0); if (virTypedParamsAddStringList(&params, &nparams, @@ -10801,7 +10801,7 @@ doMigrate(void *opaque) if (vshCommandOptStringReq(ctl, cmd, "comp-methods", &opt) < 0) goto out; if (opt) { - char **val = virStringSplit(opt, ",", 0); + char **val = g_strsplit(opt, ",", 0); if (virTypedParamsAddStringList(&params, &nparams, @@ -14388,7 +14388,7 @@ cmdSetUserSSHKeys(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - if (!(keys = virStringSplit(buffer, "\n", -1))) + if (!(keys = g_strsplit(buffer, "\n", -1))) goto cleanup; nkeys = g_strv_length(keys); -- 2.29.2

Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virstring.c | 72 +++----------------------------------------- src/util/virstring.h | 2 +- 2 files changed, 6 insertions(+), 68 deletions(-) diff --git a/src/util/virstring.c b/src/util/virstring.c index fa12b7dcd7..89d9ba4a13 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -43,28 +43,9 @@ VIR_LOG_INIT("util.string"); /** * virStringSplitCount: - * @string: a string to split - * @delim: a string which specifies the places at which to split - * the string. The delimiter is not included in any of the resulting - * strings, unless @max_tokens is reached. - * @max_tokens: the maximum number of pieces to split @string into. - * If this is 0, the string is split completely. - * @tokcount: If provided, the value is set to the count of pieces the string - * was split to excluding the terminating NULL element. * - * Splits a string into a maximum of @max_tokens pieces, using the given - * @delim. If @max_tokens is reached, the remainder of @string is - * appended to the last token. - * - * As a special case, the result of splitting the empty string "" is an empty - * vector, not a vector containing a single string. The reason for this - * special case is that being able to represent an empty vector is typically - * more useful than consistent handling of empty elements. If you do need - * to represent empty elements, you'll need to check for the empty string - * before calling virStringSplit(). - * - * Return value: a newly-allocated NULL-terminated array of strings. Use - * g_strfreev() to free it. + * A wrapper for g_strsplit which provides number of elements of the split + * string. */ char ** virStringSplitCount(const char *string, @@ -72,54 +53,11 @@ virStringSplitCount(const char *string, size_t max_tokens, size_t *tokcount) { - char **tokens = NULL; - size_t ntokens = 0; - size_t maxtokens = 0; - const char *remainder = string; - char *tmp; - size_t i; - - if (max_tokens == 0) - max_tokens = INT_MAX; - - tmp = strstr(remainder, delim); - if (tmp) { - size_t delimlen = strlen(delim); - - while (--max_tokens && tmp) { - size_t len = tmp - remainder; - - if (VIR_RESIZE_N(tokens, maxtokens, ntokens, 1) < 0) - goto error; + GStrv ret = g_strsplit(string, delim, max_tokens); - tokens[ntokens] = g_strndup(remainder, len); - ntokens++; - remainder = tmp + delimlen; - tmp = strstr(remainder, delim); - } - } - if (*string) { - if (VIR_RESIZE_N(tokens, maxtokens, ntokens, 1) < 0) - goto error; - - tokens[ntokens] = g_strdup(remainder); - ntokens++; - } + *tokcount = g_strv_length(ret); - if (VIR_RESIZE_N(tokens, maxtokens, ntokens, 1) < 0) - goto error; - tokens[ntokens++] = NULL; - - if (tokcount) - *tokcount = ntokens - 1; - - return tokens; - - error: - for (i = 0; i < ntokens; i++) - VIR_FREE(tokens[i]); - VIR_FREE(tokens); - return NULL; + return ret; } diff --git a/src/util/virstring.h b/src/util/virstring.h index 18793a34b4..48b20f5c7d 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -26,7 +26,7 @@ char **virStringSplitCount(const char *string, const char *delim, size_t max_tokens, size_t *tokcount) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4); char *virStringListJoin(const char **strings, const char *delim) -- 2.29.2

The glib alternative is now used everywhere. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/libvirt_private.syms | 1 - src/util/virstring.c | 36 ------------------------------------ src/util/virstring.h | 4 ---- tests/virstringtest.c | 36 ------------------------------------ 4 files changed, 77 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7d05d25106..63c2b2bfc3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3241,7 +3241,6 @@ virStringHasSuffix; virStringIsEmpty; virStringIsPrintable; virStringListFreeCount; -virStringListJoin; virStringListMerge; virStringMatch; virStringMatchesNameSuffix; diff --git a/src/util/virstring.c b/src/util/virstring.c index 89d9ba4a13..c98435388a 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -35,12 +35,6 @@ VIR_LOG_INIT("util.string"); -/* - * The following virStringSplit & virStringListJoin methods - * are derived from g_strsplit / g_strjoin in glib2, - * also available under the LGPLv2+ license terms - */ - /** * virStringSplitCount: * @@ -61,36 +55,6 @@ virStringSplitCount(const char *string, } -/** - * virStringListJoin: - * @strings: a NULL-terminated array of strings to join - * @delim: a string to insert between each of the strings - * - * Joins a number of strings together to form one long string, with the - * @delim inserted between each of them. The returned string - * should be freed with VIR_FREE(). - * - * Returns: a newly-allocated string containing all of the strings joined - * together, with @delim between them - */ -char *virStringListJoin(const char **strings, - const char *delim) -{ - char *ret; - g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - while (*strings) { - virBufferAdd(&buf, *strings, -1); - if (*(strings+1)) - virBufferAdd(&buf, delim, -1); - strings++; - } - ret = virBufferContentAndReset(&buf); - if (!ret) - ret = g_strdup(""); - return ret; -} - - /** * virStringListMerge: * @dst: a NULL-terminated array of strings to expand diff --git a/src/util/virstring.h b/src/util/virstring.h index 48b20f5c7d..45aead1838 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -28,10 +28,6 @@ char **virStringSplitCount(const char *string, size_t *tokcount) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4); -char *virStringListJoin(const char **strings, - const char *delim) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); - int virStringListMerge(char ***dst, char ***src); diff --git a/tests/virstringtest.c b/tests/virstringtest.c index 238cb9d79e..3bd9b97db7 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -82,12 +82,6 @@ struct testSplitData { }; -struct testJoinData { - const char *string; - const char *delim; - const char **tokens; -}; - static int testSplit(const void *args) { const struct testSplitData *data = args; @@ -140,29 +134,6 @@ static int testSplit(const void *args) } -static int testJoin(const void *args) -{ - const struct testJoinData *data = args; - char *got; - int ret = -1; - - if (!(got = virStringListJoin(data->tokens, data->delim))) { - VIR_DEBUG("Got no result"); - return -1; - } - if (STRNEQ(got, data->string)) { - fprintf(stderr, "Mismatch '%s' vs '%s'\n", got, data->string); - goto cleanup; - } - - ret = 0; - cleanup: - VIR_FREE(got); - - return ret; -} - - static int testStringSortCompare(const void *opaque G_GNUC_UNUSED) { @@ -606,15 +577,8 @@ mymain(void) .max_tokens = max, \ .tokens = toks, \ }; \ - struct testJoinData joinData = { \ - .string = str, \ - .delim = del, \ - .tokens = toks, \ - }; \ if (virTestRun("Split " #str, testSplit, &splitData) < 0) \ ret = -1; \ - if (virTestRun("Join " #str, testJoin, &joinData) < 0) \ - ret = -1; \ } while (0) VIR_WARNINGS_NO_DECLARATION_AFTER_STATEMENT -- 2.29.2