[PATCH 00/40] Replace various string helpers (and fixes for surrounding code)
This series mainly focuses on removal of virStringListAdd which tries to promote the use of a string list without counter variable as a dynamic array. This means that every operation counts the number of elements and when used in a loop resutls in O(n^2) algorithms. To discourage it's future buggy use remove the helpers completely. The end of the series then replaces some libvirt helpers for string lists by the glib equivalents. Patches: 1-3,18,24-25,28: cleanups 4-6: fix buggy iteration and memory handling in qemuNamespaceUnlinkPaths 7: helpers for easy use of GSList + char * 8-17,19-20: Don't use virStringListAdd inside of loops and prepare for removal 21: Remove virStringListAdd and virStringListRemove 22-23: Open-code and remove virStringListGetFirstWithPrefix 26-27: Replace virStringListHasString by g_strv_contains 29-34: Preparation and removal of virStringListLength (mostly inefficient iteration) 35-37,40: Replace/reimplement virStringSplit(Count) by g_strsplit 38-39: Replace virStringListJoin by g_strjoinv Peter Krempa (40): qemuMonitorJSONObjectProperty: Make 'str' member const util: virmacmap: Use g_autofree for virJSONValue util: macmap: Remove unused cleanup labels and 'ret' variables qemuDomainGetPreservedMounts: Refactor to return NULL-terminated string lists qemuNamespaceUnlinkPaths: Fix wrong use of iterator variable qemuNamespaceUnlinkPaths: Fix inconsistent cleanup handling util: Add helpers for auto-freeing GSList filled with strings qemu: namespace: Don't use 'virStringListAdd' inside loops virHookCall: Don't use 'virStringListAdd' to construct list in loop qemuInteropFetchConfigs: Don't use 'virStringListAdd' to construct list virCPUDefCheckFeatures: Don't use 'virStringListAdd' to construct list x86ModelParseFeatures: Don't construct list using 'virStringListAdd' virResctrlInfoGetMonitorPrefix: Don't use 'virStringListAdd' to construct list virResctrlMonitorGetStats: Don't use 'virStringListAdd' qemu: Convert 'priv->dbusVMStateIds' to a GSList util: macmap: Convert to use GSList for storing macs instead of string lists xenParseXLNamespaceData: Pre-calculate the length of array virfirewalltest: Shuffle the code around to remove a loop virfirewalltest: Avoid use of 'virStringListAdd' qemusecuritytest: Store 'notRestored' files in a hash table util: virstring: Remove virStringListAdd and virStringListRemove virCgroupGetValueForBlkDev: Rewrite lookup of returned string virStringListGetFirstWithPrefix: Remove unused helper vz: Replace virStringSplitCount(, , , NULL) with virStringSplit qemuProcessUpdateDevices: Refactor cleanup and memory handling Replace virStringListHasString by g_strv_contains util: virstring: Remove virStringListHasString virStorageBackendSheepdogAddVolume: Clean up memory handling qemufirmwaretest: Base iteration on string lists qemuvhostusertest: Base iteration on string lists Replace virStringListLength where actual lenght is not needed virPolkitCheckAuth: Avoid virStringListLength in loop condition Replace virStringListLength by g_strv_length util: virstring: Remove virStringListLength Replace virStringSplit with g_strsplit util: virstring: Remove virStringSplit virStringSplitCount: Reimplement using g_strsplit and g_strv_length Replace virStringListJoin by g_strjoinv util: virstring: Remove virStringListJoin virstringtest: Remove testing of virStringSplitCount src/bhyve/bhyve_command.c | 2 +- src/bhyve/bhyve_parse_command.c | 4 +- src/conf/cpu_conf.c | 14 +- src/conf/storage_conf.c | 2 +- src/cpu/cpu_arm.c | 2 +- src/cpu/cpu_x86.c | 8 +- src/libvirt_private.syms | 11 +- src/libxl/libxl_conf.c | 8 +- src/libxl/xen_common.c | 8 +- src/libxl/xen_xl.c | 49 +-- src/lxc/lxc_native.c | 14 +- src/qemu/qemu_capabilities.c | 8 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_conf.c | 4 +- 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_driver.c | 4 +- src/qemu/qemu_firmware.c | 5 +- src/qemu/qemu_interop_config.c | 13 +- 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 | 4 +- src/qemu/qemu_namespace.c | 283 +++++++++--------- src/qemu/qemu_process.c | 37 +-- src/qemu/qemu_qapi.c | 2 +- src/qemu/qemu_slirp.c | 7 +- src/qemu/qemu_vhost_user.c | 4 +- src/rpc/virnetsocket.c | 4 +- src/storage/storage_backend_sheepdog.c | 15 +- src/storage/storage_backend_zfs.c | 6 +- .../storage_source_backingstore.c | 8 +- src/util/meson.build | 1 + src/util/vircgroup.c | 26 +- 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/virglibutil.c | 27 ++ src/util/virglibutil.h | 28 ++ src/util/virhook.c | 15 +- src/util/virmacmap.c | 175 ++++++----- src/util/virmacmap.h | 6 +- src/util/virpolkit.c | 13 +- src/util/virprocess.c | 5 +- src/util/virresctrl.c | 17 +- src/util/virstring.c | 226 +------------- src/util/virstring.h | 24 +- src/vz/vz_sdk.c | 2 +- tests/qemufirmwaretest.c | 24 +- tests/qemumonitorjsontest.c | 6 +- tests/qemusecuritymock.c | 19 +- tests/qemusecuritytest.c | 14 +- tests/qemusecuritytest.h | 4 +- tests/qemuvhostusertest.c | 24 +- tests/qemuxml2argvtest.c | 2 +- tests/vboxsnapshotxmltest.c | 2 +- tests/virconftest.c | 4 +- tests/virfirewalltest.c | 30 +- tests/virmacmaptest.c | 21 +- tests/virstringtest.c | 211 +------------ tools/virsh-completer.c | 7 +- tools/virsh-domain.c | 14 +- tools/virsh-host.c | 4 +- tools/virt-login-shell-helper.c | 2 +- tools/vsh.c | 2 +- 71 files changed, 595 insertions(+), 965 deletions(-) create mode 100644 src/util/virglibutil.c create mode 100644 src/util/virglibutil.h -- 2.29.2
The code only reads it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index ba1531fee8..64af758885 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -477,7 +477,7 @@ struct _qemuMonitorJSONObjectProperty { unsigned int ui; unsigned long long ul; double d; - char *str; + const char *str; } val; }; -- 2.29.2
On 2/6/21 9:32 AM, Peter Krempa wrote:
The code only reads it.
Is that so? qemuMonitorJSONGetObjectProperty() strdup()-s into ->str.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index ba1531fee8..64af758885 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -477,7 +477,7 @@ struct _qemuMonitorJSONObjectProperty { unsigned int ui; unsigned long long ul; double d; - char *str; + const char *str; } val; };
Michal
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virmacmap.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/src/util/virmacmap.c b/src/util/virmacmap.c index e68742de10..0abb6cf580 100644 --- a/src/util/virmacmap.c +++ b/src/util/virmacmap.c @@ -130,7 +130,7 @@ virMacMapLoadFile(virMacMapPtr mgr, const char *file) { g_autofree char *map_str = NULL; - virJSONValuePtr map = NULL; + g_autoptr(virJSONValue) map = NULL; int map_str_len = 0; size_t i; int ret = -1; @@ -189,7 +189,6 @@ virMacMapLoadFile(virMacMapPtr mgr, ret = 0; cleanup: - virJSONValueFree(map); return ret; } @@ -199,14 +198,12 @@ virMACMapHashDumper(void *payload, const char *name, void *data) { - virJSONValuePtr obj = virJSONValueNewObject(); - virJSONValuePtr arr = NULL; + g_autoptr(virJSONValue) obj = virJSONValueNewObject(); + g_autoptr(virJSONValue) arr = virJSONValueNewArray(); const char **macs = payload; size_t i; int ret = -1; - arr = virJSONValueNewArray(); - for (i = 0; macs[i]; i++) { virJSONValuePtr m = virJSONValueNewString(macs[i]); @@ -228,8 +225,6 @@ virMACMapHashDumper(void *payload, ret = 0; cleanup: - virJSONValueFree(obj); - virJSONValueFree(arr); return ret; } @@ -238,11 +233,9 @@ static int virMacMapDumpStrLocked(virMacMapPtr mgr, char **str) { - virJSONValuePtr arr; + g_autoptr(virJSONValue) arr = virJSONValueNewArray(); int ret = -1; - arr = virJSONValueNewArray(); - if (virHashForEachSorted(mgr->macs, virMACMapHashDumper, arr) < 0) goto cleanup; @@ -251,7 +244,6 @@ virMacMapDumpStrLocked(virMacMapPtr mgr, ret = 0; cleanup: - virJSONValueFree(arr); return ret; } -- 2.29.2
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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
Refactor the handling of internals so that NULL-terminated lists are always returned. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_namespace.c | 56 +++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index 35fe177bef..27a78c86e4 100644 --- a/src/qemu/qemu_namespace.c +++ b/src/qemu/qemu_namespace.c @@ -108,7 +108,7 @@ qemuDomainGetPreservedMountPath(virQEMUDriverConfigPtr cfg, * a) save all FSs mounted under /dev to @devPath * b) generate backup path for all the entries in a) * - * Any of the return pointers can be NULL. + * Any of the return pointers can be NULL. Both arrays are NULL-terminated. * * Returns 0 on success, -1 otherwise (with error reported) */ @@ -119,18 +119,20 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg, char ***devSavePath, size_t *ndevPath) { - char **paths = NULL, **mounts = NULL; - size_t i, j, nmounts; + g_auto(GStrv) mounts = NULL; + size_t nmounts = 0; + g_auto(GStrv) paths = NULL; + g_auto(GStrv) savePaths = NULL; + size_t i; - if (virFileGetMountSubtree(QEMU_PROC_MOUNTS, "/dev", - &mounts, &nmounts) < 0) - goto error; + if (ndevPath) + *ndevPath = 0; - if (!nmounts) { - if (ndevPath) - *ndevPath = 0; + if (virFileGetMountSubtree(QEMU_PROC_MOUNTS, "/dev", &mounts, &nmounts) < 0) + return -1; + + if (nmounts == 0) return 0; - } /* There can be nested mount points. For instance * /dev/shm/blah can be a mount point and /dev/shm too. It @@ -142,7 +144,8 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg, * just the first element. Think about it. */ for (i = 1; i < nmounts; i++) { - j = i + 1; + size_t j = i + 1; + while (j < nmounts) { char *c = STRSKIP(mounts[j], mounts[i]); @@ -155,32 +158,33 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg, } } - paths = g_new0(char *, nmounts); + /* mounts may not be NULL-terminated at this point, but we convert it into + * 'paths' which is NULL-terminated */ - for (i = 0; i < nmounts; i++) { - if (!(paths[i] = qemuDomainGetPreservedMountPath(cfg, vm, mounts[i]))) - goto error; + paths = g_new0(char *, nmounts + 1); + + for (i = 0; i < nmounts; i++) + paths[i] = g_steal_pointer(&mounts[i]); + + if (devSavePath) { + savePaths = g_new0(char *, nmounts + 1); + + for (i = 0; i < nmounts; i++) { + if (!(savePaths[i] = qemuDomainGetPreservedMountPath(cfg, vm, paths[i]))) + return -1; + } } if (devPath) - *devPath = mounts; - else - virStringListFreeCount(mounts, nmounts); + *devPath = g_steal_pointer(&paths); if (devSavePath) - *devSavePath = paths; - else - virStringListFreeCount(paths, nmounts); + *devSavePath = g_steal_pointer(&savePaths); if (ndevPath) *ndevPath = nmounts; return 0; - - error: - virStringListFreeCount(mounts, nmounts); - virStringListFreeCount(paths, nmounts); - return -1; } -- 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@redhat.com> --- 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
Some code paths return -1 directly while others jump to 'cleanup' which cleans the list of mounts. Since qemuDomainGetPreservedMounts now returns a NULL-terminated list, convert devMountsPath to g_auto(GStrv) and remove the cleanup altoghether; Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_namespace.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index 03cc6379fe..af08b3e055 100644 --- a/src/qemu/qemu_namespace.c +++ b/src/qemu/qemu_namespace.c @@ -1341,11 +1341,9 @@ qemuNamespaceUnlinkPaths(virDomainObjPtr vm, virQEMUDriverPtr driver = priv->driver; g_autoptr(virQEMUDriverConfig) cfg = NULL; g_auto(GStrv) unlinkPaths = NULL; - char **devMountsPath = NULL; - size_t ndevMountsPath = 0; + g_auto(GStrv) devMountsPath = NULL; size_t npaths; size_t i; - int ret = -1; npaths = virStringListLength(paths); if (!npaths) @@ -1353,10 +1351,8 @@ qemuNamespaceUnlinkPaths(virDomainObjPtr vm, cfg = virQEMUDriverGetConfig(driver); - if (qemuDomainGetPreservedMounts(cfg, vm, - &devMountsPath, NULL, - &ndevMountsPath) < 0) - goto cleanup; + if (qemuDomainGetPreservedMounts(cfg, vm, &devMountsPath, NULL, NULL) < 0) + return -1; for (i = 0; i < npaths; i++) { const char *file = paths[i]; @@ -1387,10 +1383,7 @@ qemuNamespaceUnlinkPaths(virDomainObjPtr vm, unlinkPaths) < 0) return -1; - ret = 0; - cleanup: - virStringListFreeCount(devMountsPath, ndevMountsPath); - return ret; + return 0; } -- 2.29.2
On a Saturday in 2021, Peter Krempa wrote:
Some code paths return -1 directly while others jump to 'cleanup' which cleans the list of mounts. Since qemuDomainGetPreservedMounts now returns a NULL-terminated list, convert devMountsPath to g_auto(GStrv) and remove the cleanup altoghether;
Suspicious semicolon. Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_namespace.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-)
glib's 'g_autoslist()' doesn't support lists of 'char *' strings. Add a type alias 'virGSListString' so that we can register an 'autoptr' function for it for simple usage of GSList with strings. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 4 ++++ src/util/meson.build | 1 + src/util/virglibutil.c | 27 +++++++++++++++++++++++++++ src/util/virglibutil.h | 28 ++++++++++++++++++++++++++++ 4 files changed, 60 insertions(+) create mode 100644 src/util/virglibutil.c create mode 100644 src/util/virglibutil.h diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0636b0d8c9..c2270208e4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2304,6 +2304,10 @@ virGICVersionTypeFromString; virGICVersionTypeToString; +# util/virglibutil.h +virGSListStringFree; + + # util/virhash.h virHashAddEntry; virHashAtomicNew; diff --git a/src/util/meson.build b/src/util/meson.build index e89d32c33d..0080825bd0 100644 --- a/src/util/meson.build +++ b/src/util/meson.build @@ -35,6 +35,7 @@ util_sources = [ 'virgdbus.c', 'virgettext.c', 'virgic.c', + 'virglibutil.c', 'virhash.c', 'virhashcode.c', 'virhook.c', diff --git a/src/util/virglibutil.c b/src/util/virglibutil.c new file mode 100644 index 0000000000..8d93e27fc9 --- /dev/null +++ b/src/util/virglibutil.c @@ -0,0 +1,27 @@ +/* + * virglibutil.c: Utilities helping with glib usage + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#include "virglibutil.h" + +void +virGSListStringFree(GSList *l) +{ + g_slist_free_full(l, g_free); +} diff --git a/src/util/virglibutil.h b/src/util/virglibutil.h new file mode 100644 index 0000000000..2bff69f22f --- /dev/null +++ b/src/util/virglibutil.h @@ -0,0 +1,28 @@ +/* + * virglibutil.h: Utilities helping with glib usage + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#pragma once + +#include "internal.h" + +void +virGSListStringFree(GSList *l); + +typedef GSList virGSListString; + +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virGSListString, virGSListStringFree); -- 2.29.2
On 2/6/21 9:32 AM, Peter Krempa wrote:
glib's 'g_autoslist()' doesn't support lists of 'char *' strings. Add a type alias 'virGSListString' so that we can register an 'autoptr' function for it for simple usage of GSList with strings.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 4 ++++ src/util/meson.build | 1 + src/util/virglibutil.c | 27 +++++++++++++++++++++++++++ src/util/virglibutil.h | 28 ++++++++++++++++++++++++++++ 4 files changed, 60 insertions(+) create mode 100644 src/util/virglibutil.c create mode 100644 src/util/virglibutil.h
diff --git a/src/util/virglibutil.h b/src/util/virglibutil.h new file mode 100644 index 0000000000..2bff69f22f --- /dev/null +++ b/src/util/virglibutil.h @@ -0,0 +1,28 @@ +/* + * virglibutil.h: Utilities helping with glib usage + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#pragma once + +#include "internal.h" + +void +virGSListStringFree(GSList *l); + +typedef GSList virGSListString;
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 a Tuesday in 2021, Michal Privoznik wrote:
On 2/6/21 9:32 AM, Peter Krempa wrote:
glib's 'g_autoslist()' doesn't support lists of 'char *' strings. Add a type alias 'virGSListString' so that we can register an 'autoptr' function for it for simple usage of GSList with strings.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 4 ++++ src/util/meson.build | 1 + src/util/virglibutil.c | 27 +++++++++++++++++++++++++++ src/util/virglibutil.h | 28 ++++++++++++++++++++++++++++ 4 files changed, 60 insertions(+) create mode 100644 src/util/virglibutil.c create mode 100644 src/util/virglibutil.h
diff --git a/src/util/virglibutil.h b/src/util/virglibutil.h new file mode 100644 index 0000000000..2bff69f22f --- /dev/null +++ b/src/util/virglibutil.h @@ -0,0 +1,28 @@ +/* + * virglibutil.h: Utilities helping with glib usage + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#pragma once + +#include "internal.h" + +void +virGSListStringFree(GSList *l); + +typedef GSList virGSListString;
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?
Yes. Jano
On Tue, Feb 09, 2021 at 15:22:40 +0100, Michal Privoznik wrote:
On 2/6/21 9:32 AM, Peter Krempa wrote:
glib's 'g_autoslist()' doesn't support lists of 'char *' strings. Add a type alias 'virGSListString' so that we can register an 'autoptr' function for it for simple usage of GSList with strings.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 4 ++++ src/util/meson.build | 1 + src/util/virglibutil.c | 27 +++++++++++++++++++++++++++ src/util/virglibutil.h | 28 ++++++++++++++++++++++++++++ 4 files changed, 60 insertions(+) create mode 100644 src/util/virglibutil.c create mode 100644 src/util/virglibutil.h
diff --git a/src/util/virglibutil.h b/src/util/virglibutil.h new file mode 100644 index 0000000000..2bff69f22f --- /dev/null +++ b/src/util/virglibutil.h @@ -0,0 +1,28 @@ +/* + * virglibutil.h: Utilities helping with glib usage + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#pragma once + +#include "internal.h" + +void +virGSListStringFree(GSList *l); + +typedef GSList virGSListString;
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?
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.
On 2/9/21 3:48 PM, Peter Krempa wrote:
On Tue, Feb 09, 2021 at 15:22:40 +0100, Michal Privoznik wrote:
On 2/6/21 9:32 AM, Peter Krempa wrote:
glib's 'g_autoslist()' doesn't support lists of 'char *' strings. Add a type alias 'virGSListString' so that we can register an 'autoptr' function for it for simple usage of GSList with strings.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 4 ++++ src/util/meson.build | 1 + src/util/virglibutil.c | 27 +++++++++++++++++++++++++++ src/util/virglibutil.h | 28 ++++++++++++++++++++++++++++ 4 files changed, 60 insertions(+) create mode 100644 src/util/virglibutil.c create mode 100644 src/util/virglibutil.h
diff --git a/src/util/virglibutil.h b/src/util/virglibutil.h new file mode 100644 index 0000000000..2bff69f22f --- /dev/null +++ b/src/util/virglibutil.h @@ -0,0 +1,28 @@ +/* + * virglibutil.h: Utilities helping with glib usage + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#pragma once + +#include "internal.h" + +void +virGSListStringFree(GSList *l); + +typedef GSList virGSListString;
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?
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
Yeah, this is my assumption (based in my experience with small machines), that we usually have very few items on string lists (thinking of namespace code, cgroup code and likes).
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.
Yeah. I guess I'm more fan of arrays then lists because arrays are more cache friendly than linked lists.
Or alternatively even use 2 additional size variables, see VIR_RESIZE_N to avoid reallocations for maximum optimization.
Hopefully we don't care about performance that much. <rant>I am also not fan of glib. That may be another reason that I prefer our own implementation. It suits our needs better.</rant> Michal
'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@redhat.com> --- 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
On 2/6/21 9:32 AM, Peter Krempa wrote:
'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@redhat.com> --- 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));
It's not that simple. virStringListMerge() turned into NOP when @targetPaths was NULL (which is very easy to achive - just start a domain with a disk that's not a devmapper device). This code dereferences NULL directly. I think this is the only place that suffers from this so you can get away with if (targetPaths) check. Other places will demonstrate themselves :-) Michal
'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@redhat.com> --- 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
'virHashGetItems' already returns the number of entries which will be considered for addition to the list so we can allocate it to the upper bound upfront rather than growing it in a loop. This avoids the quadratic complexity of 'virStringListAdd'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_interop_config.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_interop_config.c b/src/qemu/qemu_interop_config.c index bcaddda446..9733df7194 100644 --- a/src/qemu/qemu_interop_config.c +++ b/src/qemu/qemu_interop_config.c @@ -94,7 +94,9 @@ qemuInteropFetchConfigs(const char *name, g_autofree char *sysLocation = virFileBuildPath(QEMU_SYSTEM_LOCATION, name, NULL); g_autofree char *etcLocation = virFileBuildPath(QEMU_ETC_LOCATION, name, NULL); g_autofree virHashKeyValuePairPtr pairs = NULL; + size_t npairs; virHashKeyValuePairPtr tmp = NULL; + size_t nconfigs = 0; *configs = NULL; @@ -132,11 +134,13 @@ qemuInteropFetchConfigs(const char *name, * where each filename (as key) has the highest priority full pathname * associated with it. */ - if (virHashSize(files) == 0) + if (!(pairs = virHashGetItems(files, &npairs, true))) + return -1; + + if (npairs == 0) return 0; - if (!(pairs = virHashGetItems(files, NULL, true))) - return -1; + *configs = g_new0(char *, npairs + 1); for (tmp = pairs; tmp->key; tmp++) { const char *path = tmp->value; @@ -158,8 +162,7 @@ qemuInteropFetchConfigs(const char *name, continue; } - if (virStringListAdd(configs, path) < 0) - return -1; + (*configs)[nconfigs++] = g_strdup(path); } return 0; -- 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@redhat.com> --- 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 the list to the upper bound and fill it gradually. Since the data is kept long-term and the list won't be populated much shrink it to the actual size after parsing. While using 'virStringListAdd' here wouldn't be as expensive as this function is used just once, the removal will allow to remove 'virStringListAdd' altogether to discourage the antipattern it promotes. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/cpu/cpu_x86.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 2422e258ec..a64a971540 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -1604,11 +1604,14 @@ x86ModelParseFeatures(virCPUx86ModelPtr model, { g_autofree xmlNodePtr *nodes = NULL; size_t i; + size_t nremoved = 0; int n; if ((n = virXPathNodeSet("./feature", ctxt, &nodes)) <= 0) return n; + model->removedFeatures = g_new0(char *, n + 1); + for (i = 0; i < n; i++) { g_autofree char *ftname = NULL; g_autofree char *removed = NULL; @@ -1640,8 +1643,7 @@ x86ModelParseFeatures(virCPUx86ModelPtr model, } if (rem == VIR_TRISTATE_BOOL_YES) { - if (virStringListAdd(&model->removedFeatures, ftname) < 0) - return -1; + model->removedFeatures[nremoved++] = g_strdup(ftname); continue; } } @@ -1650,6 +1652,8 @@ x86ModelParseFeatures(virCPUx86ModelPtr model, return -1; } + model->removedFeatures = g_renew(char *, model->removedFeatures, nremoved + 1); + return 0; } -- 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@redhat.com> --- 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 iner loop copies the 'resources' array multiple times using 'virStringListAdd' which has O(n^2) complexity. Pre-calculate the length so we can allocate the array upfront and just copy the strings in the loop. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virresctrl.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 86b4b9d73b..ab35bccfc5 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2662,6 +2662,7 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor, char *filepath = NULL; struct dirent *ent = NULL; virResctrlMonitorStatsPtr stat = NULL; + size_t nresources = g_strv_length((char **) resources); if (!monitor) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -2705,6 +2706,7 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor, continue; stat = g_new0(virResctrlMonitorStats, 1); + stat->features = g_new0(char *, nresources + 1); /* The node ID number should be here, parsing it. */ if (virStrToLong_uip(node_id, NULL, 0, &stat->id) < 0) @@ -2724,8 +2726,7 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor, if (VIR_APPEND_ELEMENT(stat->vals, stat->nvals, val) < 0) goto cleanup; - if (virStringListAdd(&stat->features, resources[i]) < 0) - goto cleanup; + stat->features[i] = g_strdup(resources[i]); } if (VIR_APPEND_ELEMENT(*stats, *nstats, stat) < 0) -- 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@redhat.com> --- 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
On 2/6/21 9:32 AM, Peter Krempa wrote:
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@redhat.com> --- 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; + } + }
It's rather sad that there is no better way to do this. I mean one would expect that removing an item from a singly linked list based on value not address of that value is pretty common. Michal
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@redhat.com> --- 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
Precalculate the lenght to avoid use of 'virStringListAdd' in a loop. The code is also simplified by using APIs which don't return errors. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/xen_xl.c | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index 941832ce4e..a8af54a845 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -1150,33 +1150,36 @@ static int xenParseXLNamespaceData(virConfPtr conf, virDomainDefPtr def) { virConfValuePtr list = virConfGetValue(conf, "device_model_args"); - g_auto(GStrv) args = NULL; - size_t nargs; + virConfValuePtr next; + size_t nargs = 0; libxlDomainXmlNsDefPtr nsdata = NULL; + size_t n = 0; - if (list && list->type == VIR_CONF_LIST) { - list = list->list; - while (list) { - if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) { - list = list->next; - continue; - } + if (!list || list->type != VIR_CONF_LIST) + return 0; - virStringListAdd(&args, list->str); - list = list->next; - } + list = list->list; + + for (next = list; next; next = next->next) { + if (next->type != VIR_CONF_STRING || !next->str) + continue; + + nargs++; } - if (!args) + if (nargs == 0) return 0; - nargs = g_strv_length(args); - if (nargs > 0) { - nsdata = g_new0(libxlDomainXmlNsDef, 1); + nsdata = g_new0(libxlDomainXmlNsDef, 1); + def->namespaceData = nsdata; + nsdata->args = g_new0(char *, nargs + 1); + nsdata->num_args = nargs; + + for (next = list; next; next = next->next) { + if (next->type != VIR_CONF_STRING || !next->str) + continue; - nsdata->args = g_steal_pointer(&args); - nsdata->num_args = nargs; - def->namespaceData = nsdata; + nsdata->args[n++] = g_strdup(next->str); } return 0; -- 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@redhat.com> --- 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
To allow later removal of 'virStringListAdd' add an arbitrary upper limit on the number of args we care about and don't store more than that until necessary later. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/virfirewalltest.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c index e14a34d7d2..8bd73311fd 100644 --- a/tests/virfirewalltest.c +++ b/tests/virfirewalltest.c @@ -102,6 +102,7 @@ VIR_MOCK_WRAP_RET_ARGS(g_dbus_connection_call_sync, } else if (STREQ(bus_name, VIR_FIREWALL_FIREWALLD_SERVICE) && STREQ(method_name, "passthrough")) { g_autoptr(GVariantIter) iter = NULL; + static const size_t maxargs = 5; g_auto(GStrv) args = NULL; size_t nargs = 0; char *type = NULL; @@ -111,7 +112,7 @@ VIR_MOCK_WRAP_RET_ARGS(g_dbus_connection_call_sync, g_variant_get(params, "(&sas)", &type, &iter); - nargs = g_variant_iter_n_children(iter); + args = g_new0(char *, maxargs); if (fwBuf) { if (STREQ(type, "ipv4")) @@ -130,7 +131,9 @@ VIR_MOCK_WRAP_RET_ARGS(g_dbus_connection_call_sync, doError = true; } - virStringListAdd(&args, item); + if (nargs < maxargs) + args[nargs] = g_strdup(item); + nargs++; if (fwBuf) { virBufferAddLit(fwBuf, " "); -- 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@redhat.com> --- 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(¬Restored, 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(¬Restored, 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
virStringListAdd hides the fact that a O(n) count of elements is performed every time it's called which makes it inefficient. Stop supporting such semantics and remove the helpers. Users have a choice of using GSList or an array with a counter variable rather than repeated lookups. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 2 -- src/util/virstring.c | 61 ---------------------------------- src/util/virstring.h | 5 --- tests/virstringtest.c | 72 ---------------------------------------- 4 files changed, 140 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c2270208e4..86380d0853 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3240,14 +3240,12 @@ virStringHasControlChars; virStringHasSuffix; virStringIsEmpty; virStringIsPrintable; -virStringListAdd; virStringListFreeCount; virStringListGetFirstWithPrefix; virStringListHasString; virStringListJoin; virStringListLength; virStringListMerge; -virStringListRemove; virStringMatch; virStringMatchesNameSuffix; virStringParsePort; diff --git a/src/util/virstring.c b/src/util/virstring.c index 521f2de3f6..bf861d4847 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -162,67 +162,6 @@ char *virStringListJoin(const char **strings, } -/** - * virStringListAdd: - * @strings: a NULL-terminated array of strings - * @item: string to add - * - * Appends @item into string list @strings. If *@strings is not - * allocated yet new string list is created. - * - * Returns: 0 on success, - * -1 otherwise - */ -int -virStringListAdd(char ***strings, - const char *item) -{ - size_t i = virStringListLength((const char **) *strings); - - if (VIR_EXPAND_N(*strings, i, 2) < 0) - return -1; - - (*strings)[i - 2] = g_strdup(item); - - return 0; -} - - -/** - * virStringListRemove: - * @strings: a NULL-terminated array of strings - * @item: string to remove - * - * Remove every occurrence of @item in list of @strings. - */ -void -virStringListRemove(char ***strings, - const char *item) -{ - size_t r, w = 0; - - if (!strings || !*strings) - return; - - for (r = 0; (*strings)[r]; r++) { - if (STREQ((*strings)[r], item)) { - VIR_FREE((*strings)[r]); - continue; - } - if (r != w) - (*strings)[w] = (*strings)[r]; - w++; - } - - if (w == 0) { - VIR_FREE(*strings); - } else { - (*strings)[w] = NULL; - ignore_value(VIR_REALLOC_N(*strings, w + 1)); - } -} - - /** * virStringListMerge: * @dst: a NULL-terminated array of strings to expand diff --git a/src/util/virstring.h b/src/util/virstring.h index cfd24f0b74..00c669b43b 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -37,11 +37,6 @@ char *virStringListJoin(const char **strings, const char *delim) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -int virStringListAdd(char ***strings, - const char *item); -void virStringListRemove(char ***strings, - const char *item); - int virStringListMerge(char ***dst, char ***src); diff --git a/tests/virstringtest.c b/tests/virstringtest.c index 7305190691..9eabb5b299 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -163,74 +163,6 @@ static int testJoin(const void *args) } -static int testAdd(const void *args) -{ - const struct testJoinData *data = args; - char **list = NULL; - char *got = NULL; - int ret = -1; - size_t i; - - for (i = 0; data->tokens[i]; i++) { - if (virStringListAdd(&list, data->tokens[i]) < 0) - goto cleanup; - } - - if (!list) - list = g_new0(char *, 1); - - if (!(got = virStringListJoin((const char **)list, data->delim))) { - VIR_DEBUG("Got no result"); - goto cleanup; - } - - if (STRNEQ(got, data->string)) { - fprintf(stderr, "Mismatch '%s' vs '%s'\n", got, data->string); - goto cleanup; - } - - ret = 0; - cleanup: - g_strfreev(list); - VIR_FREE(got); - return ret; -} - - -static int testRemove(const void *args) -{ - const struct testSplitData *data = args; - char **list = NULL; - size_t ntokens; - size_t i; - int ret = -1; - - if (!(list = virStringSplitCount(data->string, data->delim, - data->max_tokens, &ntokens))) { - VIR_DEBUG("Got no tokens at all"); - return -1; - } - - for (i = 0; data->tokens[i]; i++) { - virStringListRemove(&list, data->tokens[i]); - if (virStringListHasString((const char **) list, data->tokens[i])) { - fprintf(stderr, "Not removed %s", data->tokens[i]); - goto cleanup; - } - } - - if (list && list[0]) { - fprintf(stderr, "Not removed all tokens: %s", list[0]); - goto cleanup; - } - - ret = 0; - cleanup: - g_strfreev(list); - return ret; -} - - static int testStringSortCompare(const void *opaque G_GNUC_UNUSED) { @@ -683,10 +615,6 @@ mymain(void) ret = -1; \ if (virTestRun("Join " #str, testJoin, &joinData) < 0) \ ret = -1; \ - if (virTestRun("Add " #str, testAdd, &joinData) < 0) \ - ret = -1; \ - if (virTestRun("Remove " #str, testRemove, &splitData) < 0) \ - ret = -1; \ } while (0) VIR_WARNINGS_NO_DECLARATION_AFTER_STATEMENT -- 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@redhat.com> --- 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
This is a uncommon and trivial operation, so having an utility function for it is pointless. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 - src/util/virstring.c | 18 ------------------ src/util/virstring.h | 3 --- 3 files changed, 22 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 86380d0853..d215665ee0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3241,7 +3241,6 @@ virStringHasSuffix; virStringIsEmpty; virStringIsPrintable; virStringListFreeCount; -virStringListGetFirstWithPrefix; virStringListHasString; virStringListJoin; virStringListLength; diff --git a/src/util/virstring.c b/src/util/virstring.c index bf861d4847..516b898223 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -238,24 +238,6 @@ virStringListHasString(const char **strings, return false; } -char * -virStringListGetFirstWithPrefix(char **strings, - const char *prefix) -{ - size_t i = 0; - - if (!strings) - return NULL; - - while (strings[i]) { - if (STRPREFIX(strings[i], prefix)) - return strings[i] + strlen(prefix); - i++; - } - - return NULL; -} - /* Like strtol, but produce an "int" result, and check more carefully. Return 0 upon success; return -1 to indicate failure. When END_PTR is NULL, the byte after the final valid digit must be NUL. diff --git a/src/util/virstring.h b/src/util/virstring.h index 00c669b43b..4705b9c141 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -45,9 +45,6 @@ void virStringListFreeCount(char **strings, bool virStringListHasString(const char **strings, const char *needle); -char *virStringListGetFirstWithPrefix(char **strings, - const char *prefix) - ATTRIBUTE_NONNULL(2); int virStrToLong_i(char const *s, char **end_ptr, -- 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@redhat.com> --- 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
Use automatic memory freeing and remove the 'cleanup' label. Also make it a bit more obvious that nothing happens if the 'old' list wasn't present. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 30cfa4d485..1fd3230e97 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3798,32 +3798,23 @@ qemuProcessUpdateDevices(virQEMUDriverPtr driver, { qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDeviceDef dev; - const char **qemuDevices; - char **old; - char **tmp; - int ret = -1; + g_auto(GStrv) old = g_steal_pointer(&priv->qemuDevices); + GStrv tmp; - old = priv->qemuDevices; - priv->qemuDevices = NULL; if (qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) - goto cleanup; + return -1; - qemuDevices = (const char **)priv->qemuDevices; - if ((tmp = old)) { - while (*tmp) { - if (!virStringListHasString(qemuDevices, *tmp) && - virDomainDefFindDevice(vm->def, *tmp, &dev, false) == 0 && - qemuDomainRemoveDevice(driver, vm, &dev) < 0) { - goto cleanup; - } - tmp++; - } + if (!old) + return 0; + + for (tmp = old; *tmp; tmp++) { + if (!virStringListHasString((const char **) priv->qemuDevices, *tmp) && + virDomainDefFindDevice(vm->def, *tmp, &dev, false) == 0 && + qemuDomainRemoveDevice(driver, vm, &dev)) + return -1; } - ret = 0; - cleanup: - g_strfreev(old); - return ret; + return 0; } static int -- 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@redhat.com> --- 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
All callers were converted to the glib alternative. Providing our own just to have NULL tolerance doesn't make sense. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 - src/util/virstring.c | 17 ----------------- src/util/virstring.h | 3 --- 3 files changed, 21 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d215665ee0..ab8cf62552 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3241,7 +3241,6 @@ virStringHasSuffix; virStringIsEmpty; virStringIsPrintable; virStringListFreeCount; -virStringListHasString; virStringListJoin; virStringListLength; virStringListMerge; diff --git a/src/util/virstring.c b/src/util/virstring.c index 516b898223..c0393585e2 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -221,23 +221,6 @@ virStringListFreeCount(char **strings, } -bool -virStringListHasString(const char **strings, - const char *needle) -{ - size_t i = 0; - - if (!strings) - return false; - - while (strings[i]) { - if (STREQ(strings[i++], needle)) - return true; - } - - return false; -} - /* Like strtol, but produce an "int" result, and check more carefully. Return 0 upon success; return -1 to indicate failure. When END_PTR is NULL, the byte after the final valid digit must be NUL. diff --git a/src/util/virstring.h b/src/util/virstring.h index 4705b9c141..1a15812307 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -43,9 +43,6 @@ int virStringListMerge(char ***dst, void virStringListFreeCount(char **strings, size_t count); -bool virStringListHasString(const char **strings, - const char *needle); - int virStrToLong_i(char const *s, char **end_ptr, int base, -- 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@redhat.com> --- 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
Remove the need to calculate list lengths. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemufirmwaretest.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/qemufirmwaretest.c b/tests/qemufirmwaretest.c index ced9d53260..b59b002c3b 100644 --- a/tests/qemufirmwaretest.c +++ b/tests/qemufirmwaretest.c @@ -57,16 +57,16 @@ testFWPrecedence(const void *opaque G_GNUC_UNUSED) { g_autofree char *fakehome = NULL; g_auto(GStrv) fwList = NULL; - size_t nfwList; - size_t i; const char *expected[] = { PREFIX "/share/qemu/firmware/40-bios.json", SYSCONFDIR "/qemu/firmware/40-ovmf-sb-keys.json", PREFIX "/share/qemu/firmware/50-ovmf-sb-keys.json", PREFIX "/share/qemu/firmware/61-ovmf.json", PREFIX "/share/qemu/firmware/70-aavmf.json", + NULL }; - const size_t nexpected = G_N_ELEMENTS(expected); + GStrv e; + GStrv f; fakehome = g_strdup(abs_srcdir "/qemufirmwaredata/home/user/.config"); @@ -80,18 +80,18 @@ testFWPrecedence(const void *opaque G_GNUC_UNUSED) return -1; } - nfwList = virStringListLength((const char **)fwList); - - for (i = 0; i < MAX(nfwList, nexpected); i++) { - const char *e = i < nexpected ? expected[i] : NULL; - const char *f = i < nfwList ? fwList[i] : NULL; - - if (STRNEQ_NULLABLE(e, f)) { + for (e = (char **) expected, f = fwList; *f || *e;) { + if (STRNEQ_NULLABLE(*f, *e)) { fprintf(stderr, - "Unexpected path (i=%zu). Expected %s got %s \n", - i, NULLSTR(e), NULLSTR(f)); + "Unexpected path. Expected %s got %s \n", + NULLSTR(*e), NULLSTR(*f)); return -1; } + + if (*f) + f++; + if (*e) + e++; } return 0; -- 2.29.2
On 2/6/21 9:32 AM, Peter Krempa wrote:
Remove the need to calculate list lengths.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemufirmwaretest.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/tests/qemufirmwaretest.c b/tests/qemufirmwaretest.c index ced9d53260..b59b002c3b 100644 --- a/tests/qemufirmwaretest.c +++ b/tests/qemufirmwaretest.c @@ -57,16 +57,16 @@ testFWPrecedence(const void *opaque G_GNUC_UNUSED) { g_autofree char *fakehome = NULL; g_auto(GStrv) fwList = NULL; - size_t nfwList; - size_t i; const char *expected[] = { PREFIX "/share/qemu/firmware/40-bios.json", SYSCONFDIR "/qemu/firmware/40-ovmf-sb-keys.json", PREFIX "/share/qemu/firmware/50-ovmf-sb-keys.json", PREFIX "/share/qemu/firmware/61-ovmf.json", PREFIX "/share/qemu/firmware/70-aavmf.json", + NULL }; - const size_t nexpected = G_N_ELEMENTS(expected); + GStrv e;
Can't we use simple 'const char **'?
+ GStrv f;
fakehome = g_strdup(abs_srcdir "/qemufirmwaredata/home/user/.config");
@@ -80,18 +80,18 @@ testFWPrecedence(const void *opaque G_GNUC_UNUSED) return -1; }
- nfwList = virStringListLength((const char **)fwList); - - for (i = 0; i < MAX(nfwList, nexpected); i++) { - const char *e = i < nexpected ? expected[i] : NULL; - const char *f = i < nfwList ? fwList[i] : NULL; - - if (STRNEQ_NULLABLE(e, f)) { + for (e = (char **) expected, f = fwList; *f || *e;) {
We can avoid this typecast then.
+ if (STRNEQ_NULLABLE(*f, *e)) { fprintf(stderr, - "Unexpected path (i=%zu). Expected %s got %s \n", - i, NULLSTR(e), NULLSTR(f)); + "Unexpected path. Expected %s got %s \n", + NULLSTR(*e), NULLSTR(*f)); return -1; } + + if (*f) + f++; + if (*e) + e++; }
return 0;
Michal
Remove the need to calculate list lengths. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuvhostusertest.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/qemuvhostusertest.c b/tests/qemuvhostusertest.c index 17e4e83b99..a34b60c7dd 100644 --- a/tests/qemuvhostusertest.c +++ b/tests/qemuvhostusertest.c @@ -57,14 +57,14 @@ testVUPrecedence(const void *opaque G_GNUC_UNUSED) { g_autofree char *fakehome = NULL; g_auto(GStrv) vuList = NULL; - size_t nvuList; - size_t i; const char *expected[] = { PREFIX "/share/qemu/vhost-user/30-gpu.json", SYSCONFDIR "/qemu/vhost-user/40-gpu.json", PREFIX "/share/qemu/vhost-user/60-gpu.json", + NULL }; - const size_t nexpected = G_N_ELEMENTS(expected); + GStrv e; + GStrv f; fakehome = g_strdup(abs_srcdir "/qemuvhostuserdata/home/user/.config"); @@ -78,18 +78,18 @@ testVUPrecedence(const void *opaque G_GNUC_UNUSED) return -1; } - nvuList = virStringListLength((const char **)vuList); - - for (i = 0; i < MAX(nvuList, nexpected); i++) { - const char *e = i < nexpected ? expected[i] : NULL; - const char *f = i < nvuList ? vuList[i] : NULL; - - if (STRNEQ_NULLABLE(e, f)) { + for (e = (char **) expected, f = vuList; *f || *e;) { + if (STRNEQ_NULLABLE(*f, *e)) { fprintf(stderr, - "Unexpected path (i=%zu). Expected %s got %s \n", - i, NULLSTR(e), NULLSTR(f)); + "Unexpected path. Expected %s got %s \n", + NULLSTR(*e), NULLSTR(*f)); return -1; } + + if (*f) + f++; + if (*e) + e++; } return 0; -- 2.29.2
On 2/6/21 9:32 AM, Peter Krempa wrote:
Remove the need to calculate list lengths.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemuvhostusertest.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/tests/qemuvhostusertest.c b/tests/qemuvhostusertest.c index 17e4e83b99..a34b60c7dd 100644 --- a/tests/qemuvhostusertest.c +++ b/tests/qemuvhostusertest.c @@ -57,14 +57,14 @@ testVUPrecedence(const void *opaque G_GNUC_UNUSED) { g_autofree char *fakehome = NULL; g_auto(GStrv) vuList = NULL; - size_t nvuList; - size_t i; const char *expected[] = { PREFIX "/share/qemu/vhost-user/30-gpu.json", SYSCONFDIR "/qemu/vhost-user/40-gpu.json", PREFIX "/share/qemu/vhost-user/60-gpu.json", + NULL }; - const size_t nexpected = G_N_ELEMENTS(expected); + GStrv e;
Same here. s/GStrv/const char **/
+ GStrv f;
Michal
Some callers don't need to know the actual lenght of the list but only care whether the required element is present or the list is non-empty. Don't calculate the list length in those cases. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/lxc/lxc_native.c | 2 +- src/qemu/qemu_driver.c | 2 +- src/storage/storage_backend_sheepdog.c | 3 +-- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c index e68977dd99..4229a9ca7a 100644 --- a/src/lxc/lxc_native.c +++ b/src/lxc/lxc_native.c @@ -568,7 +568,7 @@ lxcNetworkParseDataIPs(const char *name, family = AF_INET6; ipparts = virStringSplit(value->str, "/", 2); - if (virStringListLength((const char * const *)ipparts) != 2 || + if (!ipparts || !ipparts[0] || !ipparts[1] || virSocketAddrParse(&ip->address, ipparts[0], family) < 0 || virStrToLong_ui(ipparts[1], NULL, 10, &ip->prefix) < 0) { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c34af6b7d1..80bd27696b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17737,7 +17737,7 @@ qemuDomainGetResctrlMonData(virQEMUDriverPtr driver, return -1; } - if (virStringListLength((const char * const *)features) == 0) + if (!features || !*features) return 0; for (i = 0; i < dom->def->nresctrls; i++) { diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c index 178cfbae11..2db2384e67 100644 --- a/src/storage/storage_backend_sheepdog.c +++ b/src/storage/storage_backend_sheepdog.c @@ -157,8 +157,7 @@ virStorageBackendSheepdogRefreshAllVol(virStoragePoolObjPtr pool) cells = virStringSplit(lines[i], " ", 0); - if (cells != NULL && - virStringListLength((const char * const *)cells) > 2) { + if (cells != NULL && cells[0] && cells[1]) { if (virStorageBackendSheepdogAddVolume(pool, cells[1]) < 0) return -1; } -- 2.29.2
On a Saturday in 2021, Peter Krempa wrote:
Some callers don't need to know the actual lenght of the list but only
s/lenght/length/ here and in the commit summary Jano
care whether the required element is present or the list is non-empty. Don't calculate the list length in those cases.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/lxc/lxc_native.c | 2 +- src/qemu/qemu_driver.c | 2 +- src/storage/storage_backend_sheepdog.c | 3 +-- 3 files changed, 3 insertions(+), 4 deletions(-)
Don't re-calculate the string list length on every iteration. Convert the loop to NULL-terminated iteration. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virpolkit.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c index 1d6be443f7..c657c45681 100644 --- a/src/util/virpolkit.c +++ b/src/util/virpolkit.c @@ -75,7 +75,7 @@ int virPolkitCheckAuth(const char *actionid, gboolean is_authorized; gboolean is_challenge; bool is_dismissed = false; - size_t i; + const char **next; if (!(sysbus = virGDBusGetSystemBus())) return -1; @@ -90,8 +90,15 @@ int virPolkitCheckAuth(const char *actionid, gprocess = g_variant_builder_end(&builder); g_variant_builder_init(&builder, G_VARIANT_TYPE("a{ss}")); - for (i = 0; i < virStringListLength(details); i += 2) - g_variant_builder_add(&builder, "{ss}", details[i], details[i + 1]); + + if (details) { + for (next = details; *next; next++) { + const char *detail1 = *(next++); + const char *detail2 = *next; + g_variant_builder_add(&builder, "{ss}", detail1, detail2); + } + } + gdetails = g_variant_builder_end(&builder); message = g_variant_new("((s@a{sv})s@a{ss}us)", -- 2.29.2
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@redhat.com> --- 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
glib provides g_strv_length. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 - src/util/virstring.c | 11 ----------- src/util/virstring.h | 2 -- 3 files changed, 14 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ab8cf62552..535776166d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3242,7 +3242,6 @@ virStringIsEmpty; virStringIsPrintable; virStringListFreeCount; virStringListJoin; -virStringListLength; virStringListMerge; virStringMatch; virStringMatchesNameSuffix; diff --git a/src/util/virstring.c b/src/util/virstring.c index 1fd61650be..c7276fa7c4 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -747,17 +747,6 @@ virStringIsEmpty(const char *str) } -size_t virStringListLength(const char * const *strings) -{ - size_t i = 0; - - while (strings && strings[i]) - i++; - - return i; -} - - /** * virStringSortCompare: * diff --git a/src/util/virstring.h b/src/util/virstring.h index 1a15812307..db6f2d4795 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -112,8 +112,6 @@ int virStrcpy(char *dest, const char *src, size_t destbytes) G_GNUC_WARN_UNUSED_RESULT; #define virStrcpyStatic(dest, src) virStrcpy((dest), (src), sizeof(dest)) -size_t virStringListLength(const char * const *strings); - int virStringSortCompare(const void *a, const void *b); int virStringSortRevCompare(const void *a, const void *b); int virStringToUpper(char **dst, const char *src); -- 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@redhat.com> --- 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(¶ms, &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(¶ms, &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
Callers were replaced by g_strsplit. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 - src/util/virstring.c | 9 --------- src/util/virstring.h | 5 ----- 3 files changed, 15 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 535776166d..7d05d25106 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3251,7 +3251,6 @@ virStringReplace; virStringSearch; virStringSortCompare; virStringSortRevCompare; -virStringSplit; virStringSplitCount; virStringStripControlChars; virStringStripIPv6Brackets; diff --git a/src/util/virstring.c b/src/util/virstring.c index c7276fa7c4..fa12b7dcd7 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -123,15 +123,6 @@ virStringSplitCount(const char *string, } -char ** -virStringSplit(const char *string, - const char *delim, - size_t max_tokens) -{ - return virStringSplitCount(string, delim, max_tokens, NULL); -} - - /** * virStringListJoin: * @strings: a NULL-terminated array of strings to join diff --git a/src/util/virstring.h b/src/util/virstring.h index db6f2d4795..18793a34b4 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -28,11 +28,6 @@ char **virStringSplitCount(const char *string, size_t *tokcount) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -char **virStringSplit(const char *string, - const char *delim, - size_t max_tokens) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); - char *virStringListJoin(const char **strings, const char *delim) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -- 2.29.2
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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
Our implementation was inspired by glib anyways. The difference is only the order of arguments. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/bhyve/bhyve_parse_command.c | 4 ++-- src/libxl/xen_common.c | 2 +- src/libxl/xen_xl.c | 4 +--- src/qemu/qemu_process.c | 2 +- src/storage_file/storage_source_backingstore.c | 4 +--- src/util/vircgroup.c | 2 +- 6 files changed, 7 insertions(+), 11 deletions(-) diff --git a/src/bhyve/bhyve_parse_command.c b/src/bhyve/bhyve_parse_command.c index 4f1d384da1..2762b7e921 100644 --- a/src/bhyve/bhyve_parse_command.c +++ b/src/bhyve/bhyve_parse_command.c @@ -923,7 +923,7 @@ bhyveParseBhyveLoadCommandLine(virDomainDefPtr def, /* Set os.bootloader since virDomainDefFormatInternal will only format * the bootloader arguments if os->bootloader is set. */ def->os.bootloader = g_strdup(argv[0]); - def->os.bootloaderArgs = virStringListJoin((const char**) &argv[1], " "); + def->os.bootloaderArgs = g_strjoinv(" ", &argv[1]); } if (def->name == NULL) { @@ -950,7 +950,7 @@ bhyveParseCustomLoaderCommandLine(virDomainDefPtr def, return -1; def->os.bootloader = g_strdup(argv[0]); - def->os.bootloaderArgs = virStringListJoin((const char**) &argv[1], " "); + def->os.bootloaderArgs = g_strjoinv(" ", &argv[1]); return 0; } diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 75c65a4135..f852032d8a 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -1659,7 +1659,7 @@ xenMakeIPList(virNetDevIPInfoPtr guestIP) if (!address_array[i]) goto cleanup; } - ret = virStringListJoin((const char**)address_array, " "); + ret = g_strjoinv(" ", address_array); cleanup: g_strfreev(address_array); diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index a7036ba494..3d44dd1c59 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -1421,9 +1421,7 @@ xenFormatXLCPUID(virConfPtr conf, virDomainDefPtr def) cpuid_pairs[j] = NULL; if (j > 1) { - cpuid_string = virStringListJoin((const char **)cpuid_pairs, ","); - if (!cpuid_string) - goto cleanup; + cpuid_string = g_strjoinv(",", cpuid_pairs); if (xenConfigSetString(conf, "cpuid", cpuid_string) < 0) goto cleanup; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d24ef789ab..13231ed0de 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5489,7 +5489,7 @@ qemuProcessStartValidate(virQEMUDriverPtr driver, if (n > 0) { g_autofree char *str = NULL; - str = virStringListJoin((const char **)features, ", "); + str = g_strjoinv(", ", features); virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Some features cannot be reliably used " "with this QEMU: %s"), str); diff --git a/src/storage_file/storage_source_backingstore.c b/src/storage_file/storage_source_backingstore.c index 8e1606a1fe..5e2db3d390 100644 --- a/src/storage_file/storage_source_backingstore.c +++ b/src/storage_file/storage_source_backingstore.c @@ -168,9 +168,7 @@ virStorageSourceRBDAddHost(virStorageSourcePtr src, parts = g_strsplit(hostport, "\\:", 0); if (!parts) goto error; - src->hosts[src->nhosts-1].name = virStringListJoin((const char **)parts, ":"); - if (!src->hosts[src->nhosts-1].name) - goto error; + src->hosts[src->nhosts-1].name = g_strjoinv(":", parts); src->hosts[src->nhosts-1].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; src->hosts[src->nhosts-1].socket = NULL; diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index a7e2728dd7..abaf1c98f7 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -831,7 +831,7 @@ virCgroupSetPartitionSuffix(const char *path, char **res) goto cleanup; } - if (!(*res = virStringListJoin((const char **)tokens, "/"))) + if (!(*res = g_strjoinv("/", tokens))) goto cleanup; ret = 0; -- 2.29.2
The glib alternative is now used everywhere. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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
The function is a wrapper on top of glibs g_strsplit, so is covered by glibs testing. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/virstringtest.c | 97 ------------------------------------------- 1 file changed, 97 deletions(-) diff --git a/tests/virstringtest.c b/tests/virstringtest.c index 3bd9b97db7..82e8e2106f 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -74,65 +74,6 @@ static int testStreq(const void *args) return 0; } -struct testSplitData { - const char *string; - const char *delim; - size_t max_tokens; - const char **tokens; -}; - - -static int testSplit(const void *args) -{ - const struct testSplitData *data = args; - char **got; - size_t ntokens; - size_t exptokens = 0; - char **tmp1; - const char **tmp2; - int ret = -1; - - if (!(got = virStringSplitCount(data->string, data->delim, - data->max_tokens, &ntokens))) { - VIR_DEBUG("Got no tokens at all"); - return -1; - } - - tmp1 = got; - tmp2 = data->tokens; - while (*tmp1 && *tmp2) { - if (STRNEQ(*tmp1, *tmp2)) { - fprintf(stderr, "Mismatch '%s' vs '%s'\n", *tmp1, *tmp2); - goto cleanup; - } - tmp1++; - tmp2++; - exptokens++; - } - if (*tmp1) { - fprintf(stderr, "Too many pieces returned\n"); - goto cleanup; - } - if (*tmp2) { - fprintf(stderr, "Too few pieces returned\n"); - goto cleanup; - } - - if (ntokens != exptokens) { - fprintf(stderr, - "Returned token count (%zu) doesn't match " - "expected count (%zu)", - ntokens, exptokens); - goto cleanup; - } - - ret = 0; - cleanup: - g_strfreev(got); - - return ret; -} - static int testStringSortCompare(const void *opaque G_GNUC_UNUSED) @@ -569,44 +510,6 @@ mymain(void) TEST_STREQ("", ""); TEST_STREQ("hello", "hello"); -#define TEST_SPLIT(str, del, max, toks) \ - do { \ - struct testSplitData splitData = { \ - .string = str, \ - .delim = del, \ - .max_tokens = max, \ - .tokens = toks, \ - }; \ - if (virTestRun("Split " #str, testSplit, &splitData) < 0) \ - ret = -1; \ - } while (0) - - VIR_WARNINGS_NO_DECLARATION_AFTER_STATEMENT - const char *tokens1[] = { NULL }; - TEST_SPLIT("", " ", 0, tokens1); - - const char *tokens2[] = { "", "", NULL }; - TEST_SPLIT(" ", " ", 0, tokens2); - - const char *tokens3[] = { "", "", "", NULL }; - TEST_SPLIT(" ", " ", 0, tokens3); - - const char *tokens4[] = { "The", "quick", "brown", "fox", NULL }; - TEST_SPLIT("The quick brown fox", " ", 0, tokens4); - - const char *tokens5[] = { "The quick ", " fox", NULL }; - TEST_SPLIT("The quick brown fox", "brown", 0, tokens5); - - const char *tokens6[] = { "", "The", "quick", "brown", "fox", NULL }; - TEST_SPLIT(" The quick brown fox", " ", 0, tokens6); - - const char *tokens7[] = { "The", "quick", "brown", "fox", "", NULL }; - TEST_SPLIT("The quick brown fox ", " ", 0, tokens7); - - const char *tokens8[] = { "gluster", "rdma", NULL }; - TEST_SPLIT("gluster+rdma", "+", 2, tokens8); - VIR_WARNINGS_RESET - if (virTestRun("virStringSortCompare", testStringSortCompare, NULL) < 0) ret = -1; -- 2.29.2
On 2/6/21 9:32 AM, Peter Krempa wrote:
This series mainly focuses on removal of virStringListAdd which tries to promote the use of a string list without counter variable as a dynamic array. This means that every operation counts the number of elements and when used in a loop resutls in O(n^2) algorithms.
To discourage it's future buggy use remove the helpers completely.
The end of the series then replaces some libvirt helpers for string lists by the glib equivalents.
Patches: 1-3,18,24-25,28: cleanups 4-6: fix buggy iteration and memory handling in qemuNamespaceUnlinkPaths 7: helpers for easy use of GSList + char * 8-17,19-20: Don't use virStringListAdd inside of loops and prepare for removal 21: Remove virStringListAdd and virStringListRemove 22-23: Open-code and remove virStringListGetFirstWithPrefix 26-27: Replace virStringListHasString by g_strv_contains 29-34: Preparation and removal of virStringListLength (mostly inefficient iteration) 35-37,40: Replace/reimplement virStringSplit(Count) by g_strsplit 38-39: Replace virStringListJoin by g_strjoinv
Peter Krempa (40): qemuMonitorJSONObjectProperty: Make 'str' member const util: virmacmap: Use g_autofree for virJSONValue util: macmap: Remove unused cleanup labels and 'ret' variables qemuDomainGetPreservedMounts: Refactor to return NULL-terminated string lists qemuNamespaceUnlinkPaths: Fix wrong use of iterator variable qemuNamespaceUnlinkPaths: Fix inconsistent cleanup handling util: Add helpers for auto-freeing GSList filled with strings qemu: namespace: Don't use 'virStringListAdd' inside loops virHookCall: Don't use 'virStringListAdd' to construct list in loop qemuInteropFetchConfigs: Don't use 'virStringListAdd' to construct list virCPUDefCheckFeatures: Don't use 'virStringListAdd' to construct list x86ModelParseFeatures: Don't construct list using 'virStringListAdd' virResctrlInfoGetMonitorPrefix: Don't use 'virStringListAdd' to construct list virResctrlMonitorGetStats: Don't use 'virStringListAdd' qemu: Convert 'priv->dbusVMStateIds' to a GSList util: macmap: Convert to use GSList for storing macs instead of string lists xenParseXLNamespaceData: Pre-calculate the length of array virfirewalltest: Shuffle the code around to remove a loop virfirewalltest: Avoid use of 'virStringListAdd' qemusecuritytest: Store 'notRestored' files in a hash table util: virstring: Remove virStringListAdd and virStringListRemove virCgroupGetValueForBlkDev: Rewrite lookup of returned string virStringListGetFirstWithPrefix: Remove unused helper vz: Replace virStringSplitCount(, , , NULL) with virStringSplit qemuProcessUpdateDevices: Refactor cleanup and memory handling Replace virStringListHasString by g_strv_contains util: virstring: Remove virStringListHasString virStorageBackendSheepdogAddVolume: Clean up memory handling qemufirmwaretest: Base iteration on string lists qemuvhostusertest: Base iteration on string lists Replace virStringListLength where actual lenght is not needed virPolkitCheckAuth: Avoid virStringListLength in loop condition Replace virStringListLength by g_strv_length util: virstring: Remove virStringListLength Replace virStringSplit with g_strsplit util: virstring: Remove virStringSplit virStringSplitCount: Reimplement using g_strsplit and g_strv_length Replace virStringListJoin by g_strjoinv util: virstring: Remove virStringListJoin virstringtest: Remove testing of virStringSplitCount
src/bhyve/bhyve_command.c | 2 +- src/bhyve/bhyve_parse_command.c | 4 +- src/conf/cpu_conf.c | 14 +- src/conf/storage_conf.c | 2 +- src/cpu/cpu_arm.c | 2 +- src/cpu/cpu_x86.c | 8 +- src/libvirt_private.syms | 11 +- src/libxl/libxl_conf.c | 8 +- src/libxl/xen_common.c | 8 +- src/libxl/xen_xl.c | 49 +-- src/lxc/lxc_native.c | 14 +- src/qemu/qemu_capabilities.c | 8 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_conf.c | 4 +- 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_driver.c | 4 +- src/qemu/qemu_firmware.c | 5 +- src/qemu/qemu_interop_config.c | 13 +- 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 | 4 +- src/qemu/qemu_namespace.c | 283 +++++++++--------- src/qemu/qemu_process.c | 37 +-- src/qemu/qemu_qapi.c | 2 +- src/qemu/qemu_slirp.c | 7 +- src/qemu/qemu_vhost_user.c | 4 +- src/rpc/virnetsocket.c | 4 +- src/storage/storage_backend_sheepdog.c | 15 +- src/storage/storage_backend_zfs.c | 6 +- .../storage_source_backingstore.c | 8 +- src/util/meson.build | 1 + src/util/vircgroup.c | 26 +- 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/virglibutil.c | 27 ++ src/util/virglibutil.h | 28 ++ src/util/virhook.c | 15 +- src/util/virmacmap.c | 175 ++++++----- src/util/virmacmap.h | 6 +- src/util/virpolkit.c | 13 +- src/util/virprocess.c | 5 +- src/util/virresctrl.c | 17 +- src/util/virstring.c | 226 +------------- src/util/virstring.h | 24 +- src/vz/vz_sdk.c | 2 +- tests/qemufirmwaretest.c | 24 +- tests/qemumonitorjsontest.c | 6 +- tests/qemusecuritymock.c | 19 +- tests/qemusecuritytest.c | 14 +- tests/qemusecuritytest.h | 4 +- tests/qemuvhostusertest.c | 24 +- tests/qemuxml2argvtest.c | 2 +- tests/vboxsnapshotxmltest.c | 2 +- tests/virconftest.c | 4 +- tests/virfirewalltest.c | 30 +- tests/virmacmaptest.c | 21 +- tests/virstringtest.c | 211 +------------ tools/virsh-completer.c | 7 +- tools/virsh-domain.c | 14 +- tools/virsh-host.c | 4 +- tools/virt-login-shell-helper.c | 2 +- tools/vsh.c | 2 +- 71 files changed, 595 insertions(+), 965 deletions(-) create mode 100644 src/util/virglibutil.c create mode 100644 src/util/virglibutil.h
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> except for the first patch. Michal
participants (3)
-
Ján Tomko -
Michal Privoznik -
Peter Krempa