The magic constant will be used in one more place. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 76e8903dbc..f818fce271 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7471,6 +7471,7 @@ qemuDomainStorageAlias(const char *device, int depth) * * Returns 0 on success and -1 if the chain is too deep. Error is reported. */ +#define QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH 200 int qemuDomainStorageSourceValidateDepth(virStorageSourcePtr src, int add, @@ -7484,7 +7485,7 @@ qemuDomainStorageSourceValidateDepth(virStorageSourcePtr src, nlayers += add; - if (nlayers > 200) { + if (nlayers > QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH) { if (diskdst) virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("backing chains more than 200 layers deep are not " -- 2.29.2

Prevent unbounded chains by limiting the recursion depth of virStorageSourceGetMetadataRecurse to the maximum number of image layers we limit anyways. This removes the last use of virStorageSourceGetUniqueIdentifier which will allow us to delete some crusty old infrastructure which isn't really needed. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 4 ++- src/security/virt-aa-helper.c | 5 +++- src/storage_file/storage_source.c | 44 +++++++++++++------------------ src/storage_file/storage_source.h | 1 + tests/virstoragetest.c | 3 ++- 5 files changed, 28 insertions(+), 29 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f818fce271..cf6d41dcad 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7627,7 +7627,9 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, qemuDomainGetImageIds(cfg, vm, src, disksrc, &uid, &gid); - if (virStorageSourceGetMetadata(src, uid, gid, report_broken) < 0) + if (virStorageSourceGetMetadata(src, uid, gid, + QEMU_DOMAIN_STORAGE_SOURCE_CHAIN_MAX_DEPTH, + report_broken) < 0) return -1; for (n = src->backingStore; virStorageSourceIsBacking(n); n = n->backingStore) { diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index c615305320..7e29e43c2e 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -937,9 +937,12 @@ get_files(vahControl * ctl) continue; /* XXX - if we knew the qemu user:group here we could send it in * so that the open could be re-tried as that user:group. + * + * The maximum depth is limited to 200 layers similarly to the qemu + * implementation. */ if (!disk->src->backingStore) - virStorageSourceGetMetadata(disk->src, -1, -1, false); + virStorageSourceGetMetadata(disk->src, -1, -1, 200, false); /* XXX should handle open errors more careful than just ignoring them. */ diff --git a/src/storage_file/storage_source.c b/src/storage_file/storage_source.c index ffe150a9b0..19b06b02b8 100644 --- a/src/storage_file/storage_source.c +++ b/src/storage_file/storage_source.c @@ -1297,11 +1297,9 @@ virStorageSourceGetMetadataRecurseReadHeader(virStorageSourcePtr src, uid_t uid, gid_t gid, char **buf, - size_t *headerLen, - GHashTable *cycle) + size_t *headerLen) { int ret = -1; - const char *uniqueName; ssize_t len; if (virStorageSourceInitAs(src, uid, gid) < 0) @@ -1312,19 +1310,6 @@ virStorageSourceGetMetadataRecurseReadHeader(virStorageSourcePtr src, goto cleanup; } - if (!(uniqueName = virStorageSourceGetUniqueIdentifier(src))) - goto cleanup; - - if (virHashHasEntry(cycle, uniqueName)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("backing store for %s (%s) is self-referential"), - NULLSTR(src->path), uniqueName); - goto cleanup; - } - - if (virHashAddEntry(cycle, uniqueName, NULL) < 0) - goto cleanup; - if ((len = virStorageSourceRead(src, 0, VIR_STORAGE_MAX_HEADER, buf)) < 0) goto cleanup; @@ -1343,7 +1328,7 @@ virStorageSourceGetMetadataRecurse(virStorageSourcePtr src, virStorageSourcePtr parent, uid_t uid, gid_t gid, bool report_broken, - GHashTable *cycle, + size_t max_depth, unsigned int depth) { virStorageFileFormat orig_format = src->format; @@ -1352,9 +1337,16 @@ virStorageSourceGetMetadataRecurse(virStorageSourcePtr src, g_autofree char *buf = NULL; g_autoptr(virStorageSource) backingStore = NULL; - VIR_DEBUG("path=%s format=%d uid=%u gid=%u", + VIR_DEBUG("path=%s format=%d uid=%u gid=%u depth=%u", NULLSTR(src->path), src->format, - (unsigned int)uid, (unsigned int)gid); + (unsigned int)uid, (unsigned int)gid, depth); + + if (depth > max_depth) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("backing store for %s is self-referential or too deeply nested"), + NULLSTR(src->path)); + return -1; + } if (src->format == VIR_STORAGE_FILE_AUTO_SAFE) src->format = VIR_STORAGE_FILE_AUTO; @@ -1369,7 +1361,7 @@ virStorageSourceGetMetadataRecurse(virStorageSourcePtr src, } if (virStorageSourceGetMetadataRecurseReadHeader(src, parent, uid, gid, - &buf, &headerLen, cycle) < 0) + &buf, &headerLen) < 0) return -1; if (virStorageFileProbeGetMetadata(src, buf, headerLen) < 0) @@ -1396,7 +1388,7 @@ virStorageSourceGetMetadataRecurse(virStorageSourcePtr src, if ((rv = virStorageSourceGetMetadataRecurse(backingStore, parent, uid, gid, report_broken, - cycle, depth + 1)) < 0) { + max_depth, depth + 1)) < 0) { if (!report_broken) return 0; @@ -1427,7 +1419,7 @@ virStorageSourceGetMetadataRecurse(virStorageSourcePtr src, * Extract metadata about the storage volume with the specified * image format. If image format is VIR_STORAGE_FILE_AUTO, it * will probe to automatically identify the format. Recurses through - * the entire chain. + * the chain up to @max_depth layers. * * Open files using UID and GID (or pass -1 for the current user/group). * Treat any backing files without explicit type as raw, unless ALLOW_PROBE. @@ -1445,14 +1437,14 @@ virStorageSourceGetMetadataRecurse(virStorageSourcePtr src, int virStorageSourceGetMetadata(virStorageSourcePtr src, uid_t uid, gid_t gid, + size_t max_depth, bool report_broken) { - g_autoptr(GHashTable) cycle = virHashNew(NULL); virStorageType actualType = virStorageSourceGetActualType(src); - VIR_DEBUG("path=%s format=%d uid=%u gid=%u report_broken=%d", + VIR_DEBUG("path=%s format=%d uid=%u gid=%u max_depth=%zu report_broken=%d", src->path, src->format, (unsigned int)uid, (unsigned int)gid, - report_broken); + max_depth, report_broken); if (src->format <= VIR_STORAGE_FILE_NONE) { if (actualType == VIR_STORAGE_TYPE_DIR) @@ -1462,5 +1454,5 @@ virStorageSourceGetMetadata(virStorageSourcePtr src, } return virStorageSourceGetMetadataRecurse(src, src, uid, gid, - report_broken, cycle, 1); + report_broken, max_depth, 1); } diff --git a/src/storage_file/storage_source.h b/src/storage_file/storage_source.h index 6eb795b301..5e05bde7b1 100644 --- a/src/storage_file/storage_source.h +++ b/src/storage_file/storage_source.h @@ -134,6 +134,7 @@ virStorageSourceSupportsBackingChainTraversal(const virStorageSource *src); int virStorageSourceGetMetadata(virStorageSourcePtr src, uid_t uid, gid_t gid, + size_t max_depth, bool report_broken) ATTRIBUTE_NONNULL(1); diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 84dd813b80..157d577c7d 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -101,7 +101,8 @@ testStorageFileGetMetadata(const char *path, def->path = g_strdup(path); - if (virStorageSourceGetMetadata(def, uid, gid, true) < 0) + /* 20 is picked as an arbitrary depth, since the chains used here don't exceed it */ + if (virStorageSourceGetMetadata(def, uid, gid, 20, true) < 0) return NULL; return g_steal_pointer(&def); -- 2.29.2

The API isn't used any more. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 - src/storage_file/storage_file_backend.h | 4 -- src/storage_file/storage_file_backend_fs.c | 24 ------- .../storage_file_backend_gluster.c | 71 ------------------- src/storage_file/storage_source.c | 37 +--------- 5 files changed, 1 insertion(+), 136 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cb9fe7c80a..62ccda467f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1713,7 +1713,6 @@ virStorageSourceGetMetadata; virStorageSourceGetMetadataFromBuf; virStorageSourceGetMetadataFromFD; virStorageSourceGetRelativeBackingPath; -virStorageSourceGetUniqueIdentifier; virStorageSourceInit; virStorageSourceInitAs; virStorageSourceNewFromBacking; diff --git a/src/storage_file/storage_file_backend.h b/src/storage_file/storage_file_backend.h index ecf5883a55..8ad579a8db 100644 --- a/src/storage_file/storage_file_backend.h +++ b/src/storage_file/storage_file_backend.h @@ -60,9 +60,6 @@ typedef ssize_t size_t len, char **buf); -typedef const char * -(*virStorageFileBackendGetUniqueIdentifier)(virStorageSourcePtr src); - typedef int (*virStorageFileBackendAccess)(virStorageSourcePtr src, int mode); @@ -88,7 +85,6 @@ struct _virStorageFileBackend { virStorageFileBackendInit backendInit; virStorageFileBackendDeinit backendDeinit; virStorageFileBackendRead storageFileRead; - virStorageFileBackendGetUniqueIdentifier storageFileGetUniqueIdentifier; /* The following group of callbacks is expected to set errno * and return -1 on error. No libvirt error shall be reported */ diff --git a/src/storage_file/storage_file_backend_fs.c b/src/storage_file/storage_file_backend_fs.c index 7b114fdeb0..f34ffd5fc8 100644 --- a/src/storage_file/storage_file_backend_fs.c +++ b/src/storage_file/storage_file_backend_fs.c @@ -147,24 +147,6 @@ virStorageFileBackendFileRead(virStorageSourcePtr src, } -static const char * -virStorageFileBackendFileGetUniqueIdentifier(virStorageSourcePtr src) -{ - virStorageDriverDataPtr drv = src->drv; - virStorageFileBackendFsPrivPtr priv = drv->priv; - - if (!priv->canonpath) { - if (!(priv->canonpath = virFileCanonicalizePath(src->path))) { - virReportSystemError(errno, _("can't canonicalize path '%s'"), - src->path); - return NULL; - } - } - - return priv->canonpath; -} - - static int virStorageFileBackendFileAccess(virStorageSourcePtr src, int mode) @@ -197,8 +179,6 @@ virStorageFileBackend virStorageFileBackendFile = { .storageFileRead = virStorageFileBackendFileRead, .storageFileAccess = virStorageFileBackendFileAccess, .storageFileChown = virStorageFileBackendFileChown, - - .storageFileGetUniqueIdentifier = virStorageFileBackendFileGetUniqueIdentifier, }; @@ -212,8 +192,6 @@ virStorageFileBackend virStorageFileBackendBlock = { .storageFileRead = virStorageFileBackendFileRead, .storageFileAccess = virStorageFileBackendFileAccess, .storageFileChown = virStorageFileBackendFileChown, - - .storageFileGetUniqueIdentifier = virStorageFileBackendFileGetUniqueIdentifier, }; @@ -225,8 +203,6 @@ virStorageFileBackend virStorageFileBackendDir = { .storageFileAccess = virStorageFileBackendFileAccess, .storageFileChown = virStorageFileBackendFileChown, - - .storageFileGetUniqueIdentifier = virStorageFileBackendFileGetUniqueIdentifier, }; diff --git a/src/storage_file/storage_file_backend_gluster.c b/src/storage_file/storage_file_backend_gluster.c index 06ba99bfe3..252eb523af 100644 --- a/src/storage_file/storage_file_backend_gluster.c +++ b/src/storage_file/storage_file_backend_gluster.c @@ -255,75 +255,6 @@ virStorageFileBackendGlusterAccess(virStorageSourcePtr src, return glfs_access(priv->vol, src->path, mode); } -static int -virStorageFileBackendGlusterReadlinkCallback(const char *path, - char **linkpath, - void *data) -{ - virStorageFileBackendGlusterPrivPtr priv = data; - size_t bufsiz = 0; - ssize_t ret; - struct stat st; - g_autofree char *buf = NULL; - - *linkpath = NULL; - - if (glfs_stat(priv->vol, path, &st) < 0) { - virReportSystemError(errno, - _("failed to stat gluster path '%s'"), - path); - return -1; - } - - if (!S_ISLNK(st.st_mode)) - return 1; - - realloc: - VIR_EXPAND_N(buf, bufsiz, 256); - - if ((ret = glfs_readlink(priv->vol, path, buf, bufsiz)) < 0) { - virReportSystemError(errno, - _("failed to read link of gluster file '%s'"), - path); - return -1; - } - - if (ret == bufsiz) - goto realloc; - - buf[ret] = '\0'; - - *linkpath = g_steal_pointer(&buf); - - return 0; -} - - -static const char * -virStorageFileBackendGlusterGetUniqueIdentifier(virStorageSourcePtr src) -{ - virStorageDriverDataPtr drv = src->drv; - virStorageFileBackendGlusterPrivPtr priv = drv->priv; - g_autofree char *filePath = NULL; - - if (priv->canonpath) - return priv->canonpath; - - if (!(filePath = virStorageFileCanonicalizePath(src->path, - virStorageFileBackendGlusterReadlinkCallback, - priv))) - return NULL; - - priv->canonpath = g_strdup_printf("gluster://%s:%u/%s/%s", - src->hosts->name, - src->hosts->port, - src->volume, - filePath); - - return priv->canonpath; -} - - static int virStorageFileBackendGlusterChown(const virStorageSource *src, uid_t uid, @@ -349,8 +280,6 @@ virStorageFileBackend virStorageFileBackendGluster = { .storageFileRead = virStorageFileBackendGlusterRead, .storageFileAccess = virStorageFileBackendGlusterAccess, .storageFileChown = virStorageFileBackendGlusterChown, - - .storageFileGetUniqueIdentifier = virStorageFileBackendGlusterGetUniqueIdentifier, }; diff --git a/src/storage_file/storage_source.c b/src/storage_file/storage_source.c index 19b06b02b8..746a3446d8 100644 --- a/src/storage_file/storage_source.c +++ b/src/storage_file/storage_source.c @@ -843,8 +843,7 @@ virStorageSourceSupportsBackingChainTraversal(const virStorageSource *src) if ((rv = virStorageSourceGetBackendForSupportCheck(src, &backend)) < 1) return rv; - return backend->storageFileGetUniqueIdentifier && - backend->storageFileRead && + return backend->storageFileRead && backend->storageFileAccess ? 1 : 0; } @@ -1142,40 +1141,6 @@ virStorageSourceRead(virStorageSourcePtr src, } -/* - * virStorageSourceGetUniqueIdentifier: Get a unique string describing the volume - * - * @src: file structure pointing to the file - * - * Returns a string uniquely describing a single volume (canonical path). - * The string shall not be freed and is valid until the storage file is - * deinitialized. Returns NULL on error and sets a libvirt error code */ -const char * -virStorageSourceGetUniqueIdentifier(virStorageSourcePtr src) -{ - virStorageDriverDataPtr drv = NULL; - - if (!virStorageSourceIsInitialized(src)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("storage file backend not initialized")); - return NULL; - } - - drv = src->drv; - - if (!drv->backend->storageFileGetUniqueIdentifier) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unique storage file identifier not implemented for " - "storage type %s (protocol: %s)'"), - virStorageTypeToString(src->type), - virStorageNetProtocolTypeToString(src->protocol)); - return NULL; - } - - return drv->backend->storageFileGetUniqueIdentifier(src); -} - - /** * virStorageSourceAccess: Check accessibility of a storage file * -- 2.29.2

The private data structure is no longer used. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage_file/storage_file_backend_fs.c | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/src/storage_file/storage_file_backend_fs.c b/src/storage_file/storage_file_backend_fs.c index f34ffd5fc8..8422543057 100644 --- a/src/storage_file/storage_file_backend_fs.c +++ b/src/storage_file/storage_file_backend_fs.c @@ -40,27 +40,12 @@ VIR_LOG_INIT("storage.storage_backend_fs"); -typedef struct _virStorageFileBackendFsPriv virStorageFileBackendFsPriv; -typedef virStorageFileBackendFsPriv *virStorageFileBackendFsPrivPtr; - -struct _virStorageFileBackendFsPriv { - char *canonpath; /* unique file identifier (canonical path) */ -}; - - static void virStorageFileBackendFileDeinit(virStorageSourcePtr src) { - virStorageDriverDataPtr drv = src->drv; - virStorageFileBackendFsPrivPtr priv = drv->priv; - VIR_DEBUG("deinitializing FS storage file %p (%s:%s)", src, virStorageTypeToString(virStorageSourceGetActualType(src)), src->path); - - - VIR_FREE(priv->canonpath); - VIR_FREE(priv); } @@ -68,17 +53,12 @@ static int virStorageFileBackendFileInit(virStorageSourcePtr src) { virStorageDriverDataPtr drv = src->drv; - virStorageFileBackendFsPrivPtr priv = NULL; VIR_DEBUG("initializing FS storage file %p (%s:%s)[%u:%u]", src, virStorageTypeToString(virStorageSourceGetActualType(src)), src->path, (unsigned int)drv->uid, (unsigned int)drv->gid); - priv = g_new0(virStorageFileBackendFsPriv, 1); - - drv->priv = priv; - return 0; } -- 2.29.2

Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage_file/storage_file_backend_gluster.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/storage_file/storage_file_backend_gluster.c b/src/storage_file/storage_file_backend_gluster.c index 252eb523af..0cd4cf9f62 100644 --- a/src/storage_file/storage_file_backend_gluster.c +++ b/src/storage_file/storage_file_backend_gluster.c @@ -41,7 +41,6 @@ typedef virStorageFileBackendGlusterPriv *virStorageFileBackendGlusterPrivPtr; struct _virStorageFileBackendGlusterPriv { glfs_t *vol; - char *canonpath; }; static void @@ -55,7 +54,6 @@ virStorageFileBackendGlusterDeinit(virStorageSourcePtr src) if (priv->vol) glfs_fini(priv->vol); - VIR_FREE(priv->canonpath); VIR_FREE(priv); drv->priv = NULL; -- 2.29.2

Remove the last code using the function. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/virstoragetest.c | 100 ----------------------------------------- 1 file changed, 100 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 157d577c7d..23cc72d85c 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -398,62 +398,6 @@ testStorageLookup(const void *args) } -struct testPathCanonicalizeData -{ - const char *path; - const char *expect; -}; - -static const char *testPathCanonicalizeSymlinks[][2] = -{ - {"/path/blah", "/other/path/huzah"}, - {"/path/to/relative/symlink", "../../actual/file"}, - {"/cycle", "/cycle"}, - {"/cycle2/link", "./link"}, -}; - -static int -testPathCanonicalizeReadlink(const char *path, - char **linkpath, - void *data G_GNUC_UNUSED) -{ - size_t i; - - *linkpath = NULL; - - for (i = 0; i < G_N_ELEMENTS(testPathCanonicalizeSymlinks); i++) { - if (STREQ(path, testPathCanonicalizeSymlinks[i][0])) { - *linkpath = g_strdup(testPathCanonicalizeSymlinks[i][1]); - - return 0; - } - } - - return 1; -} - - -static int -testPathCanonicalize(const void *args) -{ - const struct testPathCanonicalizeData *data = args; - g_autofree char *canon = NULL; - - canon = virStorageFileCanonicalizePath(data->path, - testPathCanonicalizeReadlink, - NULL); - - if (STRNEQ_NULLABLE(data->expect, canon)) { - fprintf(stderr, - "path canonicalization of '%s' failed: expected '%s' got '%s'\n", - data->path, NULLSTR(data->expect), NULLSTR(canon)); - - return -1; - } - - return 0; -} - static virStorageSource backingchain[12]; static void @@ -602,7 +546,6 @@ mymain(void) int ret; struct testChainData data; struct testLookupData data2; - struct testPathCanonicalizeData data3; struct testPathRelativeBacking data4; struct testBackingParseData data5; virStorageSourcePtr chain2; /* short for chain->backingStore */ @@ -1086,49 +1029,6 @@ mymain(void) TEST_LOOKUP_TARGET(80, "vda", chain3, "vda[2]", 0, NULL, NULL, NULL); TEST_LOOKUP_TARGET(81, "vda", NULL, "vda[3]", 0, NULL, NULL, NULL); -#define TEST_PATH_CANONICALIZE(id, PATH, EXPECT) \ - do { \ - data3.path = PATH; \ - data3.expect = EXPECT; \ - if (virTestRun("Path canonicalize " #id, \ - testPathCanonicalize, &data3) < 0) \ - ret = -1; \ - } while (0) - - TEST_PATH_CANONICALIZE(1, "/", "/"); - TEST_PATH_CANONICALIZE(2, "/path", "/path"); - TEST_PATH_CANONICALIZE(3, "/path/to/blah", "/path/to/blah"); - TEST_PATH_CANONICALIZE(4, "/path/", "/path"); - TEST_PATH_CANONICALIZE(5, "///////", "/"); - TEST_PATH_CANONICALIZE(6, "//", "//"); - TEST_PATH_CANONICALIZE(7, "", ""); - TEST_PATH_CANONICALIZE(8, ".", "."); - TEST_PATH_CANONICALIZE(9, "../", ".."); - TEST_PATH_CANONICALIZE(10, "../../", "../.."); - TEST_PATH_CANONICALIZE(11, "../../blah", "../../blah"); - TEST_PATH_CANONICALIZE(12, "/./././blah", "/blah"); - TEST_PATH_CANONICALIZE(13, ".././../././../blah", "../../../blah"); - TEST_PATH_CANONICALIZE(14, "/././", "/"); - TEST_PATH_CANONICALIZE(15, "./././", "."); - TEST_PATH_CANONICALIZE(16, "blah/../foo", "foo"); - TEST_PATH_CANONICALIZE(17, "foo/bar/../blah", "foo/blah"); - TEST_PATH_CANONICALIZE(18, "foo/bar/.././blah", "foo/blah"); - TEST_PATH_CANONICALIZE(19, "/path/to/foo/bar/../../../../../../../../baz", "/baz"); - TEST_PATH_CANONICALIZE(20, "path/to/foo/bar/../../../../../../../../baz", "../../../../baz"); - TEST_PATH_CANONICALIZE(21, "path/to/foo/bar", "path/to/foo/bar"); - TEST_PATH_CANONICALIZE(22, "//foo//bar", "//foo/bar"); - TEST_PATH_CANONICALIZE(23, "/bar//foo", "/bar/foo"); - TEST_PATH_CANONICALIZE(24, "//../blah", "//blah"); - - /* test paths with symlinks */ - TEST_PATH_CANONICALIZE(25, "/path/blah", "/other/path/huzah"); - TEST_PATH_CANONICALIZE(26, "/path/to/relative/symlink", "/path/actual/file"); - TEST_PATH_CANONICALIZE(27, "/path/to/relative/symlink/blah", "/path/actual/file/blah"); - TEST_PATH_CANONICALIZE(28, "/path/blah/yippee", "/other/path/huzah/yippee"); - TEST_PATH_CANONICALIZE(29, "/cycle", NULL); - TEST_PATH_CANONICALIZE(30, "/cycle2/link", NULL); - TEST_PATH_CANONICALIZE(31, "///", "/"); - #define TEST_RELATIVE_BACKING(id, TOP, BASE, EXPECT) \ do { \ data4.top = &TOP; \ -- 2.29.2

Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 - src/util/virstoragefile.c | 210 -------------------------------------- src/util/virstoragefile.h | 8 -- 3 files changed, 219 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 62ccda467f..9208db2056 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3231,7 +3231,6 @@ virSocketAddrSetPort; # util/virstoragefile.h -virStorageFileCanonicalizePath; virStorageFileGetNPIVKey; virStorageFileGetSCSIKey; virStorageFileParseBackingStoreStr; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 9df891b57c..e6bc723d1e 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -214,213 +214,3 @@ virStorageFileParseBackingStoreStr(const char *str, *chainIndex = idx; return 0; } - - -static char * -virStorageFileCanonicalizeFormatPath(char **components, - size_t ncomponents, - bool beginSlash, - bool beginDoubleSlash) -{ - g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - size_t i; - char *ret = NULL; - - if (beginSlash) - virBufferAddLit(&buf, "/"); - - if (beginDoubleSlash) - virBufferAddLit(&buf, "/"); - - for (i = 0; i < ncomponents; i++) { - if (i != 0) - virBufferAddLit(&buf, "/"); - - virBufferAdd(&buf, components[i], -1); - } - - /* if the output string is empty just return an empty string */ - if (!(ret = virBufferContentAndReset(&buf))) - ret = g_strdup(""); - - return ret; -} - - -static int -virStorageFileCanonicalizeInjectSymlink(const char *path, - size_t at, - char ***components, - size_t *ncomponents) -{ - char **tmp = NULL; - char **next; - size_t ntmp = 0; - int ret = -1; - - if (!(tmp = virStringSplitCount(path, "/", 0, &ntmp))) - goto cleanup; - - /* prepend */ - for (next = tmp; *next; next++) { - if (VIR_INSERT_ELEMENT(*components, at, *ncomponents, *next) < 0) - goto cleanup; - - at++; - } - - ret = 0; - - cleanup: - virStringListFreeCount(tmp, ntmp); - return ret; -} - - -char * -virStorageFileCanonicalizePath(const char *path, - virStorageFileSimplifyPathReadlinkCallback cb, - void *cbdata) -{ - GHashTable *cycle = NULL; - bool beginSlash = false; - bool beginDoubleSlash = false; - char **components = NULL; - size_t ncomponents = 0; - size_t i = 0; - size_t j = 0; - int rc; - char *ret = NULL; - g_autofree char *linkpath = NULL; - g_autofree char *currentpath = NULL; - - if (path[0] == '/') { - beginSlash = true; - - if (path[1] == '/' && path[2] != '/') - beginDoubleSlash = true; - } - - if (!(cycle = virHashNew(NULL))) - goto cleanup; - - if (!(components = virStringSplitCount(path, "/", 0, &ncomponents))) - goto cleanup; - - j = 0; - while (j < ncomponents) { - /* skip slashes */ - if (STREQ(components[j], "")) { - VIR_FREE(components[j]); - VIR_DELETE_ELEMENT(components, j, ncomponents); - continue; - } - j++; - } - - while (i < ncomponents) { - /* skip '.'s unless it's the last one remaining */ - if (STREQ(components[i], ".") && - (beginSlash || ncomponents > 1)) { - VIR_FREE(components[i]); - VIR_DELETE_ELEMENT(components, i, ncomponents); - continue; - } - - /* resolve changes to parent directory */ - if (STREQ(components[i], "..")) { - if (!beginSlash && - (i == 0 || STREQ(components[i - 1], ".."))) { - i++; - continue; - } - - VIR_FREE(components[i]); - VIR_DELETE_ELEMENT(components, i, ncomponents); - - if (i != 0) { - VIR_FREE(components[i - 1]); - VIR_DELETE_ELEMENT(components, i - 1, ncomponents); - i--; - } - - continue; - } - - /* check if the actual path isn't resulting into a symlink */ - if (!(currentpath = virStorageFileCanonicalizeFormatPath(components, - i + 1, - beginSlash, - beginDoubleSlash))) - goto cleanup; - - if ((rc = cb(currentpath, &linkpath, cbdata)) < 0) - goto cleanup; - - if (rc == 0) { - if (virHashLookup(cycle, currentpath)) { - virReportSystemError(ELOOP, - _("Failed to canonicalize path '%s'"), path); - goto cleanup; - } - - if (virHashAddEntry(cycle, currentpath, (void *) 1) < 0) - goto cleanup; - - if (linkpath[0] == '/') { - /* kill everything from the beginning including the actual component */ - i++; - while (i--) { - VIR_FREE(components[0]); - VIR_DELETE_ELEMENT(components, 0, ncomponents); - } - beginSlash = true; - - if (linkpath[1] == '/' && linkpath[2] != '/') - beginDoubleSlash = true; - else - beginDoubleSlash = false; - - i = 0; - } else { - VIR_FREE(components[i]); - VIR_DELETE_ELEMENT(components, i, ncomponents); - } - - if (virStorageFileCanonicalizeInjectSymlink(linkpath, - i, - &components, - &ncomponents) < 0) - goto cleanup; - - j = 0; - while (j < ncomponents) { - /* skip slashes */ - if (STREQ(components[j], "")) { - VIR_FREE(components[j]); - VIR_DELETE_ELEMENT(components, j, ncomponents); - continue; - } - j++; - } - - VIR_FREE(linkpath); - VIR_FREE(currentpath); - - continue; - } - - VIR_FREE(currentpath); - - i++; - } - - ret = virStorageFileCanonicalizeFormatPath(components, ncomponents, - beginSlash, beginDoubleSlash); - - cleanup: - virHashFree(cycle); - virStringListFreeCount(components, ncomponents); - - return ret; -} diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 6b198858cc..62185e6f4f 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -33,11 +33,3 @@ int virStorageFileGetSCSIKey(const char *path, bool ignoreError); int virStorageFileGetNPIVKey(const char *path, char **key); - -typedef int -(*virStorageFileSimplifyPathReadlinkCallback)(const char *path, - char **link, - void *data); -char *virStorageFileCanonicalizePath(const char *path, - virStorageFileSimplifyPathReadlinkCallback cb, - void *cbdata); -- 2.29.2

Count the elements directly using g_strv_length. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d050a519c6..2ab476dfdb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -32080,7 +32080,6 @@ virDomainDiskAddISCSIPoolSourceHost(virStorageSourcePtr src, virStoragePoolDefPtr pooldef) { g_auto(GStrv) tokens = NULL; - size_t ntokens; /* Only support one host */ if (pooldef->source.nhost != 1) { @@ -32101,10 +32100,10 @@ virDomainDiskAddISCSIPoolSourceHost(virStorageSourcePtr src, src->hosts[0].port = 3260; /* iscsi volume has name like "unit:0:0:1" */ - if (!(tokens = virStringSplitCount(src->srcpool->volume, ":", 0, &ntokens))) + if (!(tokens = g_strsplit(src->srcpool->volume, ":", 0))) return -1; - if (ntokens != 4) { + if (g_strv_length(tokens) != 4) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected iscsi volume name '%s'"), src->srcpool->volume); -- 2.29.2

The presence of the second element can be checked by looking at it directly. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virjson.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/util/virjson.c b/src/util/virjson.c index 82081db870..f376490288 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -2029,7 +2029,6 @@ virJSONValueObjectDeflattenWorker(const char *key, g_autoptr(virJSONValue) newval = NULL; virJSONValuePtr existobj; g_auto(GStrv) tokens = NULL; - size_t ntokens = 0; /* non-nested keys only need to be copied */ if (!strchr(key, '.')) { @@ -2054,10 +2053,10 @@ virJSONValueObjectDeflattenWorker(const char *key, return 0; } - if (!(tokens = virStringSplitCount(key, ".", 2, &ntokens))) + if (!(tokens = g_strsplit(key, ".", 2))) return -1; - if (ntokens != 2) { + if (!tokens[0] || !tokens[1]) { virReportError(VIR_ERR_INVALID_ARG, _("invalid nested value key '%s'"), key); return -1; -- 2.29.2

The presence of the second element can be checked by looking at it directly. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstoragefile.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index e6bc723d1e..e3ec64486e 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -192,17 +192,16 @@ virStorageFileParseBackingStoreStr(const char *str, char **target, unsigned int *chainIndex) { - size_t nstrings; unsigned int idx = 0; char *suffix; g_auto(GStrv) strings = NULL; *chainIndex = 0; - if (!(strings = virStringSplitCount(str, "[", 2, &nstrings))) + if (!(strings = g_strsplit(str, "[", 2))) return -1; - if (nstrings == 2) { + if (strings[0] && strings[1]) { if (virStrToLong_uip(strings[1], &suffix, 10, &idx) < 0 || STRNEQ(suffix, "]")) return -1; -- 2.29.2

Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virlog.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index 934c96915b..4782f6f27d 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1459,7 +1459,7 @@ virLogOutputPtr virLogParseOutput(const char *src) { virLogOutputPtr ret = NULL; - char **tokens = NULL; + g_auto(GStrv) tokens = NULL; char *abspath = NULL; size_t count = 0; virLogPriority prio; @@ -1526,7 +1526,6 @@ virLogParseOutput(const char *src) } cleanup: - g_strfreev(tokens); return ret; } @@ -1561,7 +1560,7 @@ virLogParseFilter(const char *src) virLogFilterPtr ret = NULL; size_t count = 0; virLogPriority prio; - char **tokens = NULL; + g_auto(GStrv) tokens = NULL; char *match = NULL; VIR_DEBUG("filter=%s", src); @@ -1601,7 +1600,6 @@ virLogParseFilter(const char *src) goto cleanup; cleanup: - g_strfreev(tokens); return ret; } @@ -1623,7 +1621,7 @@ virLogParseOutputs(const char *src, virLogOutputPtr **outputs) int at = -1; size_t noutputs = 0; size_t i, count; - char **strings = NULL; + g_auto(GStrv) strings = NULL; virLogOutputPtr output = NULL; virLogOutputPtr *list = NULL; @@ -1660,7 +1658,6 @@ virLogParseOutputs(const char *src, virLogOutputPtr **outputs) ret = noutputs; *outputs = g_steal_pointer(&list); cleanup: - g_strfreev(strings); return ret; } @@ -1683,7 +1680,7 @@ virLogParseFilters(const char *src, virLogFilterPtr **filters) int ret = -1; size_t nfilters = 0; size_t i, count; - char **strings = NULL; + g_auto(GStrv) strings = NULL; virLogFilterPtr filter = NULL; virLogFilterPtr *list = NULL; @@ -1709,7 +1706,6 @@ virLogParseFilters(const char *src, virLogFilterPtr **filters) ret = nfilters; *filters = g_steal_pointer(&list); cleanup: - g_strfreev(strings); return ret; } -- 2.29.2

Previous refactors left empty cleanup labels. Remove them. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virlog.c | 47 +++++++++++++++++------------------------------ 1 file changed, 17 insertions(+), 30 deletions(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index 4782f6f27d..004792db01 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1473,7 +1473,7 @@ virLogParseOutput(const char *src) if (!(tokens = virStringSplitCount(src, ":", 0, &count)) || count < 2) { virReportError(VIR_ERR_INVALID_ARG, _("Malformed format for output '%s'"), src); - goto cleanup; + return NULL; } if (virStrToLong_uip(tokens[0], NULL, 10, &prio) < 0 || @@ -1481,14 +1481,14 @@ virLogParseOutput(const char *src) virReportError(VIR_ERR_INVALID_ARG, _("Invalid priority '%s' for output '%s'"), tokens[0], src); - goto cleanup; + return NULL; } if ((dest = virLogDestinationTypeFromString(tokens[1])) < 0) { virReportError(VIR_ERR_INVALID_ARG, _("Invalid destination '%s' for output '%s'"), tokens[1], src); - goto cleanup; + return NULL; } if (((dest == VIR_LOG_TO_STDERR || @@ -1498,7 +1498,7 @@ virLogParseOutput(const char *src) virReportError(VIR_ERR_INVALID_ARG, _("Output '%s' does not meet the format requirements " "for destination type '%s'"), src, tokens[1]); - goto cleanup; + return NULL; } switch ((virLogDestination) dest) { @@ -1512,7 +1512,7 @@ virLogParseOutput(const char *src) break; case VIR_LOG_TO_FILE: if (virFileAbsPath(tokens[2], &abspath) < 0) - goto cleanup; + return NULL; ret = virLogNewOutputToFile(prio, abspath); VIR_FREE(abspath); break; @@ -1525,7 +1525,6 @@ virLogParseOutput(const char *src) break; } - cleanup: return ret; } @@ -1557,7 +1556,6 @@ virLogParseOutput(const char *src) virLogFilterPtr virLogParseFilter(const char *src) { - virLogFilterPtr ret = NULL; size_t count = 0; virLogPriority prio; g_auto(GStrv) tokens = NULL; @@ -1569,7 +1567,7 @@ virLogParseFilter(const char *src) if (!(tokens = virStringSplitCount(src, ":", 0, &count)) || count != 2) { virReportError(VIR_ERR_INVALID_ARG, _("Malformed format for filter '%s'"), src); - goto cleanup; + return NULL; } if (virStrToLong_uip(tokens[0], NULL, 10, &prio) < 0 || @@ -1577,7 +1575,7 @@ virLogParseFilter(const char *src) virReportError(VIR_ERR_INVALID_ARG, _("Invalid priority '%s' for filter '%s'"), tokens[0], src); - goto cleanup; + return NULL; } match = tokens[1]; @@ -1592,15 +1590,10 @@ virLogParseFilter(const char *src) if (!*match) { virReportError(VIR_ERR_INVALID_ARG, _("Invalid match string '%s'"), tokens[1]); - - goto cleanup; + return NULL; } - if (!(ret = virLogFilterNew(match, prio))) - goto cleanup; - - cleanup: - return ret; + return virLogFilterNew(match, prio); } /** @@ -1617,7 +1610,6 @@ virLogParseFilter(const char *src) int virLogParseOutputs(const char *src, virLogOutputPtr **outputs) { - int ret = -1; int at = -1; size_t noutputs = 0; size_t i, count; @@ -1628,7 +1620,7 @@ virLogParseOutputs(const char *src, virLogOutputPtr **outputs) VIR_DEBUG("outputs=%s", src); if (!(strings = virStringSplitCount(src, " ", 0, &count))) - goto cleanup; + return -1; for (i = 0; i < count; i++) { /* virStringSplit may return empty strings */ @@ -1636,7 +1628,7 @@ virLogParseOutputs(const char *src, virLogOutputPtr **outputs) continue; if (!(output = virLogParseOutput(strings[i]))) - goto cleanup; + return -1; /* let's check if a duplicate output does not already exist in which * case we need to replace it with its last occurrence, however, rather @@ -1647,7 +1639,7 @@ virLogParseOutputs(const char *src, virLogOutputPtr **outputs) at = virLogFindOutput(list, noutputs, output->dest, output->name); if (VIR_APPEND_ELEMENT(list, noutputs, output) < 0) { virLogOutputFree(output); - goto cleanup; + return -1; } if (at >= 0) { virLogOutputFree(list[at]); @@ -1655,10 +1647,8 @@ virLogParseOutputs(const char *src, virLogOutputPtr **outputs) } } - ret = noutputs; *outputs = g_steal_pointer(&list); - cleanup: - return ret; + return noutputs; } /** @@ -1677,7 +1667,6 @@ virLogParseOutputs(const char *src, virLogOutputPtr **outputs) int virLogParseFilters(const char *src, virLogFilterPtr **filters) { - int ret = -1; size_t nfilters = 0; size_t i, count; g_auto(GStrv) strings = NULL; @@ -1687,7 +1676,7 @@ virLogParseFilters(const char *src, virLogFilterPtr **filters) VIR_DEBUG("filters=%s", src); if (!(strings = virStringSplitCount(src, " ", 0, &count))) - goto cleanup; + return -1; for (i = 0; i < count; i++) { /* virStringSplit may return empty strings */ @@ -1695,18 +1684,16 @@ virLogParseFilters(const char *src, virLogFilterPtr **filters) continue; if (!(filter = virLogParseFilter(strings[i]))) - goto cleanup; + return -1; if (VIR_APPEND_ELEMENT(list, nfilters, filter)) { virLogFilterFree(filter); - goto cleanup; + return -1; } } - ret = nfilters; *filters = g_steal_pointer(&list); - cleanup: - return ret; + return nfilters; } /** -- 2.29.2

Rewrite the code to remove the need to calculate the string list count. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virlog.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index 004792db01..56059f62f1 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1612,22 +1612,22 @@ virLogParseOutputs(const char *src, virLogOutputPtr **outputs) { int at = -1; size_t noutputs = 0; - size_t i, count; g_auto(GStrv) strings = NULL; + GStrv next; virLogOutputPtr output = NULL; virLogOutputPtr *list = NULL; VIR_DEBUG("outputs=%s", src); - if (!(strings = virStringSplitCount(src, " ", 0, &count))) + if (!(strings = g_strsplit(src, " ", 0))) return -1; - for (i = 0; i < count; i++) { + for (next = strings; *next; next++) { /* virStringSplit may return empty strings */ - if (STREQ(strings[i], "")) + if (STREQ(*next, "")) continue; - if (!(output = virLogParseOutput(strings[i]))) + if (!(output = virLogParseOutput(*next))) return -1; /* let's check if a duplicate output does not already exist in which -- 2.29.2

Rewrite the code to remove the need to calculate the string list count. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virlog.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index 56059f62f1..57eb0de538 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1668,22 +1668,22 @@ int virLogParseFilters(const char *src, virLogFilterPtr **filters) { size_t nfilters = 0; - size_t i, count; g_auto(GStrv) strings = NULL; + GStrv next; virLogFilterPtr filter = NULL; virLogFilterPtr *list = NULL; VIR_DEBUG("filters=%s", src); - if (!(strings = virStringSplitCount(src, " ", 0, &count))) + if (!(strings = g_strsplit(src, " ", 0))) return -1; - for (i = 0; i < count; i++) { + for (next = strings; *next; next++) { /* virStringSplit may return empty strings */ - if (STREQ(strings[i], "")) + if (STREQ(*next, "")) continue; - if (!(filter = virLogParseFilter(strings[i]))) + if (!(filter = virLogParseFilter(*next))) return -1; if (VIR_APPEND_ELEMENT(list, nfilters, filter)) { -- 2.29.2

We don't really need the count. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virlog.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index 57eb0de538..8a4cfbfdc2 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1556,7 +1556,6 @@ virLogParseOutput(const char *src) virLogFilterPtr virLogParseFilter(const char *src) { - size_t count = 0; virLogPriority prio; g_auto(GStrv) tokens = NULL; char *match = NULL; @@ -1564,7 +1563,8 @@ virLogParseFilter(const char *src) VIR_DEBUG("filter=%s", src); /* split our format prio:match_str to tokens and parse them individually */ - if (!(tokens = virStringSplitCount(src, ":", 0, &count)) || count != 2) { + if (!(tokens = g_strsplit(src, ":", 0)) || + !tokens[0] || !tokens[1]) { virReportError(VIR_ERR_INVALID_ARG, _("Malformed format for filter '%s'"), src); return NULL; -- 2.29.2

Unfortunately here we do need the count of elements. Use g_strv_length to calculate it so that virStringSplitCount can be removed later. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virlog.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index 8a4cfbfdc2..3fb27fcb25 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1470,7 +1470,8 @@ virLogParseOutput(const char *src) /* split our format prio:destination:additional_data to tokens and parse * them individually */ - if (!(tokens = virStringSplitCount(src, ":", 0, &count)) || count < 2) { + if (!(tokens = g_strsplit(src, ":", 0)) || + (count = g_strv_length(tokens))< 2) { virReportError(VIR_ERR_INVALID_ARG, _("Malformed format for output '%s'"), src); return NULL; -- 2.29.2

Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index a778421b66..86de4255fa 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9342,24 +9342,21 @@ virshParseEventStr(const char *event, int *nparams, int *maxparams) { - char **tok = NULL; - size_t i, ntok; - int ret = -1; + g_auto(GStrv) tok = NULL; + GStrv next; - if (!(tok = virStringSplitCount(event, ",", 0, &ntok))) + if (!(tok = g_strsplit(event, ",", 0))) return -1; - for (i = 0; i < ntok; i++) { - if ((*tok[i] != '\0') && - virTypedParamsAddBoolean(params, nparams, - maxparams, tok[i], state) < 0) - goto cleanup; + for (next = tok; *next; next++) { + if (*next[0] == '\0') + continue; + + if (virTypedParamsAddBoolean(params, nparams, maxparams, *next, state) < 0) + return -1; } - ret = 0; - cleanup: - g_strfreev(tok); - return ret; + return 0; } static void -- 2.29.2

Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/bhyve/bhyve_parse_command.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/bhyve/bhyve_parse_command.c b/src/bhyve/bhyve_parse_command.c index 70f5ac42a0..d86d37b697 100644 --- a/src/bhyve/bhyve_parse_command.c +++ b/src/bhyve/bhyve_parse_command.c @@ -558,10 +558,8 @@ bhyveParsePCIFbuf(virDomainDefPtr def, virDomainVideoDefPtr video = NULL; virDomainGraphicsDefPtr graphics = NULL; - char **params = NULL; - char *param = NULL, *separator = NULL; - size_t nparams = 0; - size_t i = 0; + g_auto(GStrv) **params = NULL; + GStrv next; if (!(video = virDomainVideoDefNew(xmlopt))) goto cleanup; @@ -579,11 +577,11 @@ bhyveParsePCIFbuf(virDomainDefPtr def, if (!config) goto error; - if (!(params = virStringSplitCount(config, ",", 0, &nparams))) + if (!(params = g_strsplit(config, ",", 0))) goto error; - for (i = 0; i < nparams; i++) { - param = params[i]; + for (next = params; *next; next++) { + char *param = *next; if (!video->driver) video->driver = g_new0(virDomainVideoDriverDef, 1); @@ -649,13 +647,11 @@ bhyveParsePCIFbuf(virDomainDefPtr def, if (VIR_APPEND_ELEMENT(def->graphics, def->ngraphics, graphics) < 0) goto error; - g_strfreev(params); return 0; error: virDomainVideoDefFree(video); virDomainGraphicsDefFree(graphics); - g_strfreev(params); return -1; } -- 2.29.2

We don't need the count of elements to iterate the list. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virt-host-validate-common.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index fc43b2ddc8..7e9f5a667c 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -201,9 +201,8 @@ virBitmapPtr virHostValidateGetCPUFlags(void) do { char line[1024]; char *start; - char **tokens; - size_t ntokens; - size_t i; + g_auto(GStrv) tokens = NULL; + GStrv next; if (!fgets(line, sizeof(line), fp)) break; @@ -228,20 +227,18 @@ virBitmapPtr virHostValidateGetCPUFlags(void) /* Split the line using " " as a delimiter. The first token * will always be ":", but that's okay */ - if (!(tokens = virStringSplitCount(start, " ", 0, &ntokens))) + if (!(tokens = g_strsplit(start, " ", 0))) continue; /* Go through all flags and check whether one of those we * might want to check for later on is present; if that's * the case, set the relevant bit in the bitmap */ - for (i = 0; i < ntokens; i++) { + for (next = tokens; *next; next++) { int value; - if ((value = virHostValidateCPUFlagTypeFromString(tokens[i])) >= 0) + if ((value = virHostValidateCPUFlagTypeFromString(*next)) >= 0) ignore_value(virBitmapSetBit(flags, value)); } - - virStringListFreeCount(tokens, ntokens); } while (1); VIR_FORCE_FCLOSE(fp); -- 2.29.2

Use g_auto for the string list and remove 'ret' and 'cleanup:'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 86de4255fa..37be298809 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -848,7 +848,7 @@ static const vshCmdOptDef opts_attach_interface[] = { *tok[index] != '\0' && \ virStrToLong_ullp(tok[index], NULL, 10, &rate->name) < 0) { \ vshError(ctl, _("field '%s' is malformed"), #name); \ - goto cleanup; \ + return -1; \ } \ } while (0) @@ -857,16 +857,15 @@ virshParseRateStr(vshControl *ctl, const char *rateStr, virNetDevBandwidthRatePtr rate) { - char **tok = NULL; + g_auto(GStrv) tok = NULL; size_t ntok; - int ret = -1; if (!(tok = virStringSplitCount(rateStr, ",", 0, &ntok))) return -1; if (ntok > 4) { vshError(ctl, _("Rate string '%s' has too many fields"), rateStr); - goto cleanup; + return -1; } VIRSH_PARSE_RATE_FIELD(0, average); @@ -874,10 +873,7 @@ virshParseRateStr(vshControl *ctl, VIRSH_PARSE_RATE_FIELD(2, burst); VIRSH_PARSE_RATE_FIELD(3, floor); - ret = 0; - cleanup: - g_strfreev(tok); - return ret; + return 0; } #undef VIRSH_PARSE_RATE_FIELD -- 2.29.2

Count the elements after splitting the string. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 37be298809..532169d8d7 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -860,10 +860,10 @@ virshParseRateStr(vshControl *ctl, g_auto(GStrv) tok = NULL; size_t ntok; - if (!(tok = virStringSplitCount(rateStr, ",", 0, &ntok))) + if (!(tok = g_strsplit(rateStr, ",", 0))) return -1; - if (ntok > 4) { + if ((ntok = g_strv_length(tok)) > 4) { vshError(ctl, _("Rate string '%s' has too many fields"), rateStr); return -1; } -- 2.29.2

Count the elements after splitting the string. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage_file/storage_source_backingstore.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/storage_file/storage_source_backingstore.c b/src/storage_file/storage_source_backingstore.c index bac5a043e5..9a67d64fd6 100644 --- a/src/storage_file/storage_source_backingstore.c +++ b/src/storage_file/storage_source_backingstore.c @@ -483,7 +483,6 @@ virStorageSourceParseBackingJSONUriCookies(virStorageSourcePtr src, { const char *cookiestr; g_auto(GStrv) cookies = NULL; - size_t ncookies = 0; size_t i; if (!virJSONValueObjectHasKey(json, "cookie")) @@ -496,13 +495,13 @@ virStorageSourceParseBackingJSONUriCookies(virStorageSourcePtr src, return -1; } - if (!(cookies = virStringSplitCount(cookiestr, ";", 0, &ncookies))) + if (!(cookies = g_strsplit(cookiestr, ";", 0))) return -1; - src->cookies = g_new0(virStorageNetCookieDefPtr, ncookies); - src->ncookies = ncookies; + src->ncookies = g_strv_length(cookies); + src->cookies = g_new0(virStorageNetCookieDefPtr, src->ncookies); - for (i = 0; i < ncookies; i++) { + for (i = 0; i < src->ncookies; i++) { char *cookiename = cookies[i]; char *cookievalue; -- 2.29.2

Use str(r)chr to find the correct bit rather than fully splitting the string. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage/storage_backend_zfs.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/storage/storage_backend_zfs.c b/src/storage/storage_backend_zfs.c index e4ef06722f..cab1fd1637 100644 --- a/src/storage/storage_backend_zfs.c +++ b/src/storage/storage_backend_zfs.c @@ -102,7 +102,7 @@ virStorageBackendZFSParseVol(virStoragePoolObjPtr pool, virStorageVolDefPtr volume = NULL; virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); g_auto(GStrv) tokens = NULL; - g_auto(GStrv) name_tokens = NULL; + char *tmp; if (!(tokens = virStringSplitCount(volume_string, "\t", 0, &count))) return -1; @@ -110,10 +110,9 @@ virStorageBackendZFSParseVol(virStoragePoolObjPtr pool, if (count != 3) goto cleanup; - if (!(name_tokens = virStringSplitCount(tokens[0], "/", 0, &count))) - goto cleanup; - - vol_name = name_tokens[count-1]; + vol_name = tokens[0]; + if ((tmp = strrchr(vol_name, '/'))) + vol_name = tmp + 1; if (vol == NULL) volume = virStorageVolDefFindByName(pool, vol_name); @@ -218,7 +217,8 @@ virStorageBackendZFSRefreshPool(virStoragePoolObjPtr pool G_GNUC_UNUSED) g_autoptr(virCommand) cmd = NULL; g_auto(GStrv) lines = NULL; g_auto(GStrv) tokens = NULL; - g_auto(GStrv) name_tokens = NULL; + g_autofree char *name = g_strdup(def->source.name); + char *tmp; /** * $ zpool get -Hp health,size,free,allocated test @@ -230,13 +230,13 @@ virStorageBackendZFSRefreshPool(virStoragePoolObjPtr pool G_GNUC_UNUSED) * * Here we just provide a list of properties we want to see */ - if (!(name_tokens = g_strsplit(def->source.name, "/", 0))) - goto cleanup; + if ((tmp = strchr(name, '/'))) + *tmp = '\0'; cmd = virCommandNewArgList(ZPOOL, "get", "-Hp", "health,size,free,allocated", - name_tokens[0], + name, NULL); virCommandSetOutputBuffer(cmd, &zpool_props); if (virCommandRun(cmd, NULL) < 0) -- 2.29.2

Declare it in the loop that actually uses it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage/storage_backend_zfs.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/storage/storage_backend_zfs.c b/src/storage/storage_backend_zfs.c index cab1fd1637..6fed25cf4d 100644 --- a/src/storage/storage_backend_zfs.c +++ b/src/storage/storage_backend_zfs.c @@ -216,7 +216,6 @@ virStorageBackendZFSRefreshPool(virStoragePoolObjPtr pool G_GNUC_UNUSED) size_t i; g_autoptr(virCommand) cmd = NULL; g_auto(GStrv) lines = NULL; - g_auto(GStrv) tokens = NULL; g_autofree char *name = g_strdup(def->source.name); char *tmp; @@ -246,13 +245,13 @@ virStorageBackendZFSRefreshPool(virStoragePoolObjPtr pool G_GNUC_UNUSED) goto cleanup; for (i = 0; lines[i]; i++) { + g_auto(GStrv) tokens = NULL; size_t count; char *prop_name; if (STREQ(lines[i], "")) continue; - g_strfreev(tokens); if (!(tokens = virStringSplitCount(lines[i], "\t", 0, &count))) goto cleanup; -- 2.29.2

Both instances just check the length once. Replicate that faithfully. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage/storage_backend_zfs.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/storage/storage_backend_zfs.c b/src/storage/storage_backend_zfs.c index 6fed25cf4d..c36deb9dac 100644 --- a/src/storage/storage_backend_zfs.c +++ b/src/storage/storage_backend_zfs.c @@ -96,7 +96,6 @@ virStorageBackendZFSParseVol(virStoragePoolObjPtr pool, const char *volume_string) { int ret = -1; - size_t count; char *vol_name; bool is_new_vol = false; virStorageVolDefPtr volume = NULL; @@ -104,10 +103,10 @@ virStorageBackendZFSParseVol(virStoragePoolObjPtr pool, g_auto(GStrv) tokens = NULL; char *tmp; - if (!(tokens = virStringSplitCount(volume_string, "\t", 0, &count))) + if (!(tokens = g_strsplit(volume_string, "\t", 0))) return -1; - if (count != 3) + if (g_strv_length(tokens) != 3) goto cleanup; vol_name = tokens[0]; @@ -246,16 +245,15 @@ virStorageBackendZFSRefreshPool(virStoragePoolObjPtr pool G_GNUC_UNUSED) for (i = 0; lines[i]; i++) { g_auto(GStrv) tokens = NULL; - size_t count; char *prop_name; if (STREQ(lines[i], "")) continue; - if (!(tokens = virStringSplitCount(lines[i], "\t", 0, &count))) + if (!(tokens = g_strsplit(lines[i], "\t", 0))) goto cleanup; - if (count != 4) + if (g_strv_length(tokens) != 4) continue; prop_name = tokens[1]; -- 2.29.2

Move variables into the loop which uses them and use automatic freeing for temporarily allocated variables. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virresctrl.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 8458db96bc..fbc8e9c766 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -526,18 +526,19 @@ static int virResctrlGetCacheInfo(virResctrlInfoPtr resctrl, DIR *dirp) { - char *endptr = NULL; - char *tmp_str = NULL; - int ret = -1; int rv = -1; - int type = 0; + int ret = -1; struct dirent *ent = NULL; - unsigned int level = 0; - virBitmapPtr tmp_map = NULL; - virResctrlInfoPerLevelPtr i_level = NULL; - virResctrlInfoPerTypePtr i_type = NULL; while ((rv = virDirRead(dirp, &ent, SYSFS_RESCTRL_PATH "/info")) > 0) { + g_autofree char *cbm_mask_str = NULL; + g_autoptr(virBitmap) cbm_mask_map = NULL; + char *endptr = NULL; + int type = 0; + unsigned int level = 0; + virResctrlInfoPerLevelPtr i_level = NULL; + g_autofree virResctrlInfoPerTypePtr i_type = NULL; + VIR_DEBUG("Parsing info type '%s'", ent->d_name); if (ent->d_name[0] != 'L') continue; @@ -570,7 +571,7 @@ virResctrlGetCacheInfo(virResctrlInfoPtr resctrl, goto cleanup; } - rv = virFileReadValueString(&tmp_str, + rv = virFileReadValueString(&cbm_mask_str, SYSFS_RESCTRL_PATH "/info/%s/cbm_mask", ent->d_name); @@ -585,19 +586,15 @@ virResctrlGetCacheInfo(virResctrlInfoPtr resctrl, if (rv < 0) goto cleanup; - virStringTrimOptionalNewline(tmp_str); + virStringTrimOptionalNewline(cbm_mask_str); - tmp_map = virBitmapNewString(tmp_str); - VIR_FREE(tmp_str); - if (!tmp_map) { + if (!(cbm_mask_map = virBitmapNewString(cbm_mask_str))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot parse cbm_mask from resctrl cache info")); goto cleanup; } - i_type->bits = virBitmapCountBits(tmp_map); - virBitmapFree(tmp_map); - tmp_map = NULL; + i_type->bits = virBitmapCountBits(cbm_mask_map); rv = virFileReadValueUint(&i_type->min_cbm_bits, SYSFS_RESCTRL_PATH "/info/%s/min_cbm_bits", @@ -635,7 +632,6 @@ virResctrlGetCacheInfo(virResctrlInfoPtr resctrl, ret = 0; cleanup: - VIR_FREE(i_type); return ret; } -- 2.29.2

Move variables into the loop which uses them and use automatic freeing for temporarily allocated variables. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virresctrl.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index fbc8e9c766..5e7c391524 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -1811,27 +1811,26 @@ static virResctrlAllocPtr virResctrlAllocNewFromInfo(virResctrlInfoPtr info) { size_t i = 0; - size_t j = 0; - size_t k = 0; virResctrlAllocPtr ret = virResctrlAllocNew(); - virBitmapPtr mask = NULL; if (!ret) return NULL; for (i = 0; i < info->nlevels; i++) { virResctrlInfoPerLevelPtr i_level = info->levels[i]; + size_t j = 0; if (!i_level) continue; for (j = 0; j < VIR_CACHE_TYPE_LAST; j++) { virResctrlInfoPerTypePtr i_type = i_level->types[j]; + g_autoptr(virBitmap) mask = NULL; + size_t k = 0; if (!i_type) continue; - virBitmapFree(mask); mask = virBitmapNew(i_type->bits); virBitmapSetAll(mask); @@ -1856,7 +1855,6 @@ virResctrlAllocNewFromInfo(virResctrlInfoPtr info) } cleanup: - virBitmapFree(mask); return ret; error: virObjectUnref(ret); -- 2.29.2

Remove 'cleanup' and 'error' labels by switching 'ret' to automatic pointer and stealing it in the return statement. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virresctrl.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 5e7c391524..c66cf4b087 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -1811,7 +1811,7 @@ static virResctrlAllocPtr virResctrlAllocNewFromInfo(virResctrlInfoPtr info) { size_t i = 0; - virResctrlAllocPtr ret = virResctrlAllocNew(); + g_autoptr(virResctrlAlloc) ret = virResctrlAllocNew(); if (!ret) return NULL; @@ -1836,7 +1836,7 @@ virResctrlAllocNewFromInfo(virResctrlInfoPtr info) for (k = 0; k <= i_type->max_cache_id; k++) { if (virResctrlAllocUpdateMask(ret, i, j, k, mask) < 0) - goto error; + return NULL; } } } @@ -1854,12 +1854,7 @@ virResctrlAllocNewFromInfo(virResctrlInfoPtr info) } } - cleanup: - return ret; - error: - virObjectUnref(ret); - ret = NULL; - goto cleanup; + return g_steal_pointer(&ret); } /* -- 2.29.2

Refactor the handling of variables so that the cleanup section can be sanitized. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virresctrl.c | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index c66cf4b087..53c202f99f 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -1874,8 +1874,8 @@ virResctrlAllocNewFromInfo(virResctrlInfoPtr info) virResctrlAllocPtr virResctrlAllocGetUnused(virResctrlInfoPtr resctrl) { - virResctrlAllocPtr ret = NULL; - virResctrlAllocPtr alloc = NULL; + g_autoptr(virResctrlAlloc) ret = NULL; + g_autoptr(virResctrlAlloc) alloc_default = NULL; struct dirent *ent = NULL; g_autoptr(DIR) dirp = NULL; int rv = -1; @@ -1890,17 +1890,18 @@ virResctrlAllocGetUnused(virResctrlInfoPtr resctrl) if (!ret) return NULL; - alloc = virResctrlAllocGetDefault(resctrl); - if (!alloc) - goto error; + alloc_default = virResctrlAllocGetDefault(resctrl); + if (!alloc_default) + return NULL; - virResctrlAllocSubtract(ret, alloc); - virObjectUnref(alloc); + virResctrlAllocSubtract(ret, alloc_default); if (virDirOpen(&dirp, SYSFS_RESCTRL_PATH) < 0) - goto error; + return NULL; while ((rv = virDirRead(dirp, &ent, SYSFS_RESCTRL_PATH)) > 0) { + g_autoptr(virResctrlAlloc) alloc = NULL; + if (STREQ(ent->d_name, "info")) continue; @@ -1912,24 +1913,15 @@ virResctrlAllocGetUnused(virResctrlInfoPtr resctrl) virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not read schemata file for group %s"), ent->d_name); - goto error; + return NULL; } virResctrlAllocSubtract(ret, alloc); - virObjectUnref(alloc); - alloc = NULL; } if (rv < 0) - goto error; - - cleanup: - virObjectUnref(alloc); - return ret; + return NULL; - error: - virObjectUnref(ret); - ret = NULL; - goto cleanup; + return g_steal_pointer(&ret); } -- 2.29.2

Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virresctrl.c | 53 ++++++++++++++----------------------------- 1 file changed, 17 insertions(+), 36 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 53c202f99f..6d8e669852 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -641,7 +641,7 @@ virResctrlGetMemoryBandwidthInfo(virResctrlInfoPtr resctrl) { int ret = -1; int rv = -1; - virResctrlInfoMemBWPtr i_membw = NULL; + g_autofree virResctrlInfoMemBWPtr i_membw = NULL; /* query memory bandwidth allocation info */ i_membw = g_new0(virResctrlInfoMemBW, 1); @@ -684,7 +684,6 @@ virResctrlGetMemoryBandwidthInfo(virResctrlInfoPtr resctrl) resctrl->membw_info = g_steal_pointer(&i_membw); ret = 0; cleanup: - VIR_FREE(i_membw); return ret; } @@ -705,10 +704,10 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl) { int ret = -1; int rv = -1; - char *featurestr = NULL; - char **features = NULL; + g_autofree char *featurestr = NULL; + g_auto(GStrv) features = NULL; size_t nfeatures = 0; - virResctrlInfoMongrpPtr info_monitor = NULL; + g_autofree virResctrlInfoMongrpPtr info_monitor = NULL; info_monitor = g_new0(virResctrlInfoMongrp, 1); @@ -767,9 +766,6 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl) ret = 0; cleanup: - VIR_FREE(featurestr); - g_strfreev(features); - VIR_FREE(info_monitor); return ret; } @@ -1480,7 +1476,7 @@ virResctrlAllocParseMemoryBandwidthLine(virResctrlInfoPtr resctrl, virResctrlAllocPtr alloc, char *line) { - char **mbs = NULL; + g_auto(GStrv) mbs = NULL; char *tmp = NULL; size_t nmbs = 0; size_t i; @@ -1517,7 +1513,6 @@ virResctrlAllocParseMemoryBandwidthLine(virResctrlInfoPtr resctrl, ret = 0; cleanup: - g_strfreev(mbs); return ret; } @@ -1595,7 +1590,7 @@ virResctrlAllocParseProcessCache(virResctrlInfoPtr resctrl, { char *tmp = strchr(cache, '='); unsigned int cache_id = 0; - virBitmapPtr mask = NULL; + g_autoptr(virBitmap) mask = NULL; int ret = -1; if (!tmp) @@ -1632,7 +1627,6 @@ virResctrlAllocParseProcessCache(virResctrlInfoPtr resctrl, ret = 0; cleanup: - virBitmapFree(mask); return ret; } @@ -1642,7 +1636,7 @@ virResctrlAllocParseCacheLine(virResctrlInfoPtr resctrl, virResctrlAllocPtr alloc, char *line) { - char **caches = NULL; + g_auto(GStrv) caches = NULL; char *tmp = NULL; unsigned int level = 0; int type = -1; @@ -1691,7 +1685,6 @@ virResctrlAllocParseCacheLine(virResctrlInfoPtr resctrl, ret = 0; cleanup: - g_strfreev(caches); return ret; } @@ -1701,7 +1694,7 @@ virResctrlAllocParse(virResctrlInfoPtr resctrl, virResctrlAllocPtr alloc, const char *schemata) { - char **lines = NULL; + g_auto(GStrv) lines = NULL; size_t nlines = 0; size_t i = 0; int ret = -1; @@ -1717,7 +1710,6 @@ virResctrlAllocParse(virResctrlInfoPtr resctrl, ret = 0; cleanup: - g_strfreev(lines); return ret; } @@ -1945,7 +1937,7 @@ virResctrlAllocFindUnused(virResctrlAllocPtr alloc, { unsigned long long *size = alloc->levels[level]->types[type]->sizes[cache]; virBitmapPtr a_mask = NULL; - virBitmapPtr f_mask = NULL; + g_autoptr(virBitmap) f_mask = NULL; unsigned long long need_bits; size_t i = 0; ssize_t pos = -1; @@ -2049,7 +2041,6 @@ virResctrlAllocFindUnused(virResctrlAllocPtr alloc, ret = 0; cleanup: - virBitmapFree(a_mask); return ret; } @@ -2185,8 +2176,8 @@ virResctrlAllocAssign(virResctrlInfoPtr resctrl, { int ret = -1; unsigned int level = 0; - virResctrlAllocPtr alloc_free = NULL; - virResctrlAllocPtr alloc_default = NULL; + g_autoptr(virResctrlAlloc) alloc_free = NULL; + g_autoptr(virResctrlAlloc) alloc_default = NULL; alloc_free = virResctrlAllocGetUnused(resctrl); if (!alloc_free) @@ -2251,8 +2242,6 @@ virResctrlAllocAssign(virResctrlInfoPtr resctrl, ret = 0; cleanup: - virObjectUnref(alloc_free); - virObjectUnref(alloc_default); return ret; } @@ -2328,8 +2317,8 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl, virResctrlAllocPtr alloc, const char *machinename) { - char *schemata_path = NULL; - char *alloc_str = NULL; + g_autofree char *schemata_path = NULL; + g_autofree char *alloc_str = NULL; int ret = -1; int lockfd = -1; @@ -2377,8 +2366,6 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl, ret = 0; cleanup: virResctrlUnlock(lockfd); - VIR_FREE(alloc_str); - VIR_FREE(schemata_path); return ret; } @@ -2387,8 +2374,8 @@ static int virResctrlAddPID(const char *path, pid_t pid) { - char *tasks = NULL; - char *pidstr = NULL; + g_autofree char *tasks = NULL; + g_autofree char *pidstr = NULL; int ret = 0; if (!path) { @@ -2410,8 +2397,6 @@ virResctrlAddPID(const char *path, ret = 0; cleanup: - VIR_FREE(tasks); - VIR_FREE(pidstr); return ret; } @@ -2631,8 +2616,7 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor, size_t i = 0; unsigned long long val = 0; g_autoptr(DIR) dirp = NULL; - char *datapath = NULL; - char *filepath = NULL; + g_autofree char *datapath = NULL; struct dirent *ent = NULL; virResctrlMonitorStatsPtr stat = NULL; size_t nresources = g_strv_length((char **) resources); @@ -2650,10 +2634,9 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor, *nstats = 0; while (virDirRead(dirp, &ent, datapath) > 0) { + g_autofree char *filepath = NULL; char *node_id = NULL; - VIR_FREE(filepath); - /* Looking for directory that contains resource utilization * information file. The directory name is arranged in format * "mon_<node_name>_<node_id>". For example, "mon_L3_00" and @@ -2712,8 +2695,6 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor, ret = 0; cleanup: - VIR_FREE(datapath); - VIR_FREE(filepath); virResctrlMonitorStatsFree(stat); return ret; } -- 2.29.2

Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virresctrl.c | 111 +++++++++++++++--------------------------- 1 file changed, 39 insertions(+), 72 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 6d8e669852..73b0061937 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -527,7 +527,6 @@ virResctrlGetCacheInfo(virResctrlInfoPtr resctrl, DIR *dirp) { int rv = -1; - int ret = -1; struct dirent *ent = NULL; while ((rv = virDirRead(dirp, &ent, SYSFS_RESCTRL_PATH "/info")) > 0) { @@ -568,7 +567,7 @@ virResctrlGetCacheInfo(virResctrlInfoPtr resctrl, ent->d_name); } else if (rv < 0) { /* Other failures are fatal, so just quit */ - goto cleanup; + return -1; } rv = virFileReadValueString(&cbm_mask_str, @@ -584,14 +583,14 @@ virResctrlGetCacheInfo(virResctrlInfoPtr resctrl, _("Cannot get cbm_mask from resctrl cache info")); } if (rv < 0) - goto cleanup; + return -1; virStringTrimOptionalNewline(cbm_mask_str); if (!(cbm_mask_map = virBitmapNewString(cbm_mask_str))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot parse cbm_mask from resctrl cache info")); - goto cleanup; + return -1; } i_type->bits = virBitmapCountBits(cbm_mask_map); @@ -603,7 +602,7 @@ virResctrlGetCacheInfo(virResctrlInfoPtr resctrl, virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot get min_cbm_bits from resctrl cache info")); if (rv < 0) - goto cleanup; + return -1; if (resctrl->nlevels <= level) VIR_EXPAND_N(resctrl->levels, resctrl->nlevels, @@ -624,22 +623,19 @@ virResctrlGetCacheInfo(virResctrlInfoPtr resctrl, virReportError(VIR_ERR_INTERNAL_ERROR, _("Duplicate cache type in resctrl for level %u"), level); - goto cleanup; + return -1; } i_level->types[type] = g_steal_pointer(&i_type); } - ret = 0; - cleanup: - return ret; + return 0; } static int virResctrlGetMemoryBandwidthInfo(virResctrlInfoPtr resctrl) { - int ret = -1; int rv = -1; g_autofree virResctrlInfoMemBWPtr i_membw = NULL; @@ -652,11 +648,10 @@ virResctrlGetMemoryBandwidthInfo(virResctrlInfoPtr resctrl) * probably memory bandwidth allocation unsupported */ VIR_INFO("The path '" SYSFS_RESCTRL_PATH "/info/MB/bandwidth_gran'" "does not exist"); - ret = 0; - goto cleanup; + return 0; } else if (rv < 0) { /* Other failures are fatal, so just quit */ - goto cleanup; + return -1; } rv = virFileReadValueUint(&i_membw->min_bandwidth, @@ -669,7 +664,7 @@ virResctrlGetMemoryBandwidthInfo(virResctrlInfoPtr resctrl) _("Cannot get min bandwidth from resctrl memory info")); } if (rv < 0) - goto cleanup; + return -1; rv = virFileReadValueUint(&i_membw->max_allocation, SYSFS_RESCTRL_PATH "/info/MB/num_closids"); @@ -679,12 +674,10 @@ virResctrlGetMemoryBandwidthInfo(virResctrlInfoPtr resctrl) _("Cannot get max allocation from resctrl memory info")); } if (rv < 0) - goto cleanup; + return -1; resctrl->membw_info = g_steal_pointer(&i_membw); - ret = 0; - cleanup: - return ret; + return 0; } @@ -702,7 +695,6 @@ virResctrlGetMemoryBandwidthInfo(virResctrlInfoPtr resctrl) static int virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl) { - int ret = -1; int rv = -1; g_autofree char *featurestr = NULL; g_auto(GStrv) features = NULL; @@ -721,11 +713,10 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl) * monitor unsupported */ VIR_INFO("The file '" SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids' " "does not exist"); - ret = 0; - goto cleanup; + return 0; } else if (rv < 0) { /* Other failures are fatal, so just quit */ - goto cleanup; + return -1; } rv = virFileReadValueUint(&info_monitor->cache_reuse_threshold, @@ -737,7 +728,7 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl) VIR_DEBUG("File '" SYSFS_RESCTRL_PATH "/info/L3_MON/max_threshold_occupancy' does not exist"); } else if (rv < 0) { - goto cleanup; + return -1; } rv = virFileReadValueString(&featurestr, @@ -747,14 +738,14 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot get mon_features from resctrl")); if (rv < 0) - goto cleanup; + return -1; if (!*featurestr) { /* If no feature found in "/info/L3_MON/mon_features", * some error happens */ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Got empty feature list from resctrl")); - goto cleanup; + return -1; } features = virStringSplitCount(featurestr, "\n", 0, &nfeatures); @@ -764,9 +755,7 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl) info_monitor->features = g_steal_pointer(&features); resctrl->monitor_info = g_steal_pointer(&info_monitor); - ret = 0; - cleanup: - return ret; + return 0; } @@ -1480,7 +1469,6 @@ virResctrlAllocParseMemoryBandwidthLine(virResctrlInfoPtr resctrl, char *tmp = NULL; size_t nmbs = 0; size_t i; - int ret = -1; /* For no reason there can be spaces */ virSkipSpaces((const char **) &line); @@ -1508,12 +1496,10 @@ virResctrlAllocParseMemoryBandwidthLine(virResctrlInfoPtr resctrl, mbs = virStringSplitCount(tmp, ";", 0, &nmbs); for (i = 0; i < nmbs; i++) { if (virResctrlAllocParseProcessMemoryBandwidth(resctrl, alloc, mbs[i]) < 0) - goto cleanup; + return -1; } - ret = 0; - cleanup: - return ret; + return 0; } @@ -1591,7 +1577,6 @@ virResctrlAllocParseProcessCache(virResctrlInfoPtr resctrl, char *tmp = strchr(cache, '='); unsigned int cache_id = 0; g_autoptr(virBitmap) mask = NULL; - int ret = -1; if (!tmp) return 0; @@ -1617,17 +1602,15 @@ virResctrlAllocParseProcessCache(virResctrlInfoPtr resctrl, _("Missing or inconsistent resctrl info for " "level '%u' type '%s'"), level, virCacheTypeToString(type)); - goto cleanup; + return -1; } virBitmapShrink(mask, resctrl->levels[level]->types[type]->bits); if (virResctrlAllocUpdateMask(alloc, level, type, cache_id, mask) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - return ret; + return 0; } @@ -1642,7 +1625,6 @@ virResctrlAllocParseCacheLine(virResctrlInfoPtr resctrl, int type = -1; size_t ncaches = 0; size_t i = 0; - int ret = -1; /* For no reason there can be spaces */ virSkipSpaces((const char **) &line); @@ -1680,12 +1662,10 @@ virResctrlAllocParseCacheLine(virResctrlInfoPtr resctrl, for (i = 0; i < ncaches; i++) { if (virResctrlAllocParseProcessCache(resctrl, alloc, level, type, caches[i]) < 0) - goto cleanup; + return -1; } - ret = 0; - cleanup: - return ret; + return 0; } @@ -1697,20 +1677,16 @@ virResctrlAllocParse(virResctrlInfoPtr resctrl, g_auto(GStrv) lines = NULL; size_t nlines = 0; size_t i = 0; - int ret = -1; lines = virStringSplitCount(schemata, "\n", 0, &nlines); for (i = 0; i < nlines; i++) { if (virResctrlAllocParseCacheLine(resctrl, alloc, lines[i]) < 0) - goto cleanup; + return -1; if (virResctrlAllocParseMemoryBandwidthLine(resctrl, alloc, lines[i]) < 0) - goto cleanup; - + return -1; } - ret = 0; - cleanup: - return ret; + return 0; } @@ -1943,7 +1919,6 @@ virResctrlAllocFindUnused(virResctrlAllocPtr alloc, ssize_t pos = -1; ssize_t last_bits = 0; ssize_t last_pos = -1; - int ret = -1; if (!size) return 0; @@ -2037,11 +2012,9 @@ virResctrlAllocFindUnused(virResctrlAllocPtr alloc, ignore_value(virBitmapSetBit(a_mask, i)); if (virResctrlAllocUpdateMask(alloc, level, type, cache, a_mask) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - return ret; + return 0; } @@ -2174,7 +2147,6 @@ static int virResctrlAllocAssign(virResctrlInfoPtr resctrl, virResctrlAllocPtr alloc) { - int ret = -1; unsigned int level = 0; g_autoptr(virResctrlAlloc) alloc_free = NULL; g_autoptr(virResctrlAlloc) alloc_default = NULL; @@ -2185,16 +2157,16 @@ virResctrlAllocAssign(virResctrlInfoPtr resctrl, alloc_default = virResctrlAllocGetDefault(resctrl); if (!alloc_default) - goto cleanup; + return -1; if (virResctrlAllocMemoryBandwidth(resctrl, alloc) < 0) - goto cleanup; + return -1; if (virResctrlAllocCopyMasks(alloc, alloc_default) < 0) - goto cleanup; + return -1; if (virResctrlAllocCopyMemBW(alloc, alloc_default) < 0) - goto cleanup; + return -1; for (level = 0; level < alloc->nlevels; level++) { virResctrlAllocPerLevelPtr a_level = alloc->levels[level]; @@ -2211,7 +2183,7 @@ virResctrlAllocAssign(virResctrlInfoPtr resctrl, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Cache level %d does not support tuning"), level); - goto cleanup; + return -1; } for (type = 0; type < VIR_CACHE_TYPE_LAST; type++) { @@ -2227,7 +2199,7 @@ virResctrlAllocAssign(virResctrlInfoPtr resctrl, _("Cache level %d does not support tuning for " "scope type '%s'"), level, virCacheTypeToString(type)); - goto cleanup; + return -1; } for (cache = 0; cache < a_type->nsizes; cache++) { @@ -2235,14 +2207,12 @@ virResctrlAllocAssign(virResctrlInfoPtr resctrl, virResctrlInfoPerTypePtr i_type = i_level->types[type]; if (virResctrlAllocFindUnused(alloc, i_type, f_type, level, type, cache) < 0) - goto cleanup; + return -1; } } } - ret = 0; - cleanup: - return ret; + return 0; } @@ -2376,7 +2346,6 @@ virResctrlAddPID(const char *path, { g_autofree char *tasks = NULL; g_autofree char *pidstr = NULL; - int ret = 0; if (!path) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -2392,12 +2361,10 @@ virResctrlAddPID(const char *path, virReportSystemError(errno, _("Cannot write pid in tasks file '%s'"), tasks); - goto cleanup; + return -1; } - ret = 0; - cleanup: - return ret; + return 0; } -- 2.29.2

In 3 of 4 instances the code didn't even need the count of the elements. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virresctrl.c | 38 ++++++++++++++++---------------------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 73b0061937..7891bc7411 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -697,8 +697,6 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl) { int rv = -1; g_autofree char *featurestr = NULL; - g_auto(GStrv) features = NULL; - size_t nfeatures = 0; g_autofree virResctrlInfoMongrpPtr info_monitor = NULL; info_monitor = g_new0(virResctrlInfoMongrp, 1); @@ -748,11 +746,10 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl) return -1; } - features = virStringSplitCount(featurestr, "\n", 0, &nfeatures); - VIR_DEBUG("Resctrl supported %zd monitoring features", nfeatures); + info_monitor->features = g_strsplit(featurestr, "\n", 0); + info_monitor->nfeatures = g_strv_length(info_monitor->features); + VIR_DEBUG("Resctrl supported %zd monitoring features", info_monitor->nfeatures); - info_monitor->nfeatures = nfeatures; - info_monitor->features = g_steal_pointer(&features); resctrl->monitor_info = g_steal_pointer(&info_monitor); return 0; @@ -1466,9 +1463,8 @@ virResctrlAllocParseMemoryBandwidthLine(virResctrlInfoPtr resctrl, char *line) { g_auto(GStrv) mbs = NULL; + GStrv next; char *tmp = NULL; - size_t nmbs = 0; - size_t i; /* For no reason there can be spaces */ virSkipSpaces((const char **) &line); @@ -1493,9 +1489,9 @@ virResctrlAllocParseMemoryBandwidthLine(virResctrlInfoPtr resctrl, return 0; tmp++; - mbs = virStringSplitCount(tmp, ";", 0, &nmbs); - for (i = 0; i < nmbs; i++) { - if (virResctrlAllocParseProcessMemoryBandwidth(resctrl, alloc, mbs[i]) < 0) + mbs = g_strsplit(tmp, ";", 0); + for (next = mbs; *next; next++) { + if (virResctrlAllocParseProcessMemoryBandwidth(resctrl, alloc, *next) < 0) return -1; } @@ -1620,11 +1616,10 @@ virResctrlAllocParseCacheLine(virResctrlInfoPtr resctrl, char *line) { g_auto(GStrv) caches = NULL; + GStrv next; char *tmp = NULL; unsigned int level = 0; int type = -1; - size_t ncaches = 0; - size_t i = 0; /* For no reason there can be spaces */ virSkipSpaces((const char **) &line); @@ -1656,12 +1651,12 @@ virResctrlAllocParseCacheLine(virResctrlInfoPtr resctrl, return -1; } - caches = virStringSplitCount(tmp, ";", 0, &ncaches); + caches = g_strsplit(tmp, ";", 0); if (!caches) return 0; - for (i = 0; i < ncaches; i++) { - if (virResctrlAllocParseProcessCache(resctrl, alloc, level, type, caches[i]) < 0) + for (next = caches; *next; next++) { + if (virResctrlAllocParseProcessCache(resctrl, alloc, level, type, *next) < 0) return -1; } @@ -1675,14 +1670,13 @@ virResctrlAllocParse(virResctrlInfoPtr resctrl, const char *schemata) { g_auto(GStrv) lines = NULL; - size_t nlines = 0; - size_t i = 0; + GStrv next; - lines = virStringSplitCount(schemata, "\n", 0, &nlines); - for (i = 0; i < nlines; i++) { - if (virResctrlAllocParseCacheLine(resctrl, alloc, lines[i]) < 0) + lines = g_strsplit(schemata, "\n", 0); + for (next = lines; *next; next++) { + if (virResctrlAllocParseCacheLine(resctrl, alloc, *next) < 0) return -1; - if (virResctrlAllocParseMemoryBandwidthLine(resctrl, alloc, lines[i]) < 0) + if (virResctrlAllocParseMemoryBandwidthLine(resctrl, alloc, *next) < 0) return -1; } -- 2.29.2

Count the number of elements in place just for the check. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/xen_common.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 835f48ec42..49a816add2 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -372,7 +372,6 @@ xenParsePCI(char *entry) virDomainHostdevDefPtr hostdev = NULL; g_auto(GStrv) tokens = NULL; g_auto(GStrv) options = NULL; - size_t ntokens = 0; size_t nexttoken = 0; char *str; char *nextstr; @@ -383,11 +382,11 @@ xenParsePCI(char *entry) virTristateBool filtered = VIR_TRISTATE_BOOL_ABSENT; /* pci=['00:1b.0','0000:00:13.0,permissive=1'] */ - if (!(tokens = virStringSplitCount(entry, ":", 3, &ntokens))) + if (!(tokens = g_strsplit(entry, ":", 3))) return NULL; /* domain */ - if (ntokens == 3) { + if (g_strv_length(tokens) == 3) { if (virStrToLong_i(tokens[nexttoken], NULL, 16, &domain) < 0) return NULL; nexttoken++; -- 2.29.2

Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/xen_xl.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index 65cd26bb21..e8a75979d4 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -523,9 +523,11 @@ xenParseXLVnuma(virConfPtr conf, tmp = g_strdup(vtoken); g_strfreev(token); - if (!(token = virStringSplitCount(tmp, ",", 0, &ndistances))) + if (!(token = g_strsplit(tmp, ",", 0))) goto cleanup; + ndistances = g_strv_length(token); + if (ndistances != nr_nodes) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("vnuma pnode %d configured '%s' (count %zu) doesn't fit the number of specified vnodes %zu"), -- 2.29.2

Use g_strsplit instead of virStringSplitCount and automatically free the temporary string list. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/openvz/openvz_conf.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 836885af18..cf29ce30ad 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -123,26 +123,21 @@ openvzParseBarrierLimit(const char* value, unsigned long long *barrier, unsigned long long *limit) { - char **tmp = NULL; - size_t ntmp = 0; - int ret = -1; + g_auto(GStrv) tmp = NULL; - if (!(tmp = virStringSplitCount(value, ":", 0, &ntmp))) - goto error; + if (!(tmp = g_strsplit(value, ":", 0))) + return -1; - if (ntmp != 2) - goto error; + if (g_strv_length(tmp) != 2) + return -1; if (barrier && virStrToLong_ull(tmp[0], NULL, 10, barrier) < 0) - goto error; + return -1; if (limit && virStrToLong_ull(tmp[1], NULL, 10, limit) < 0) - goto error; + return -1; - ret = 0; - error: - virStringListFreeCount(tmp, ntmp); - return ret; + return 0; } -- 2.29.2

While the code invokes the string list length calculation twice, it happens only on error path, which by itself should never happen. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virsystemd.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index cf22edaa0a..718d24dfc5 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -754,19 +754,18 @@ virSystemdActivationInitFromNames(virSystemdActivationPtr act, { g_auto(GStrv) fdnamelistptr = NULL; char **fdnamelist; - size_t nfdnames; size_t i; int nextfd = STDERR_FILENO + 1; VIR_DEBUG("FD names %s", fdnames); - if (!(fdnamelistptr = virStringSplitCount(fdnames, ":", 0, &nfdnames))) + if (!(fdnamelistptr = g_strsplit(fdnames, ":", 0))) goto error; - if (nfdnames != nfds) { + if (g_strv_length(fdnamelistptr) != nfds) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Expecting %d FD names but got %zu"), - nfds, nfdnames); + _("Expecting %d FD names but got %u"), + nfds, g_strv_length(fdnamelistptr)); goto error; } -- 2.29.2

Remove the last usage of virStringSplitCount Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/vmx/vmx.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 0b12b5dd7d..7832fd143e 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -1574,18 +1574,17 @@ virVMXParseConfig(virVMXContext *ctx, if (sched_cpu_affinity != NULL && STRCASENEQ(sched_cpu_affinity, "all")) { g_auto(GStrv) afflist = NULL; char **aff; - size_t naffs; def->cpumask = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN); - if (!(afflist = virStringSplitCount(sched_cpu_affinity, ",", 0, &naffs))) + if (!(afflist = g_strsplit(sched_cpu_affinity, ",", 0))) goto cleanup; - if (naffs < numvcpus) { + if (g_strv_length(afflist) < numvcpus) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Expecting VMX entry 'sched.cpu.affinity' to contain " "at least as many values as 'numvcpus' (%lld) but " - "found only %zu value(s)"), numvcpus, naffs); + "found only %u value(s)"), numvcpus, g_strv_length(afflist)); goto cleanup; } -- 2.29.2

Callers which need the count of elements now count it in place. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 - src/util/virstring.c | 20 -------------------- src/util/virstring.h | 6 ------ 3 files changed, 27 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9208db2056..68cdb92453 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3260,7 +3260,6 @@ virStringReplace; virStringSearch; virStringSortCompare; virStringSortRevCompare; -virStringSplitCount; virStringStripControlChars; virStringStripIPv6Brackets; virStringStripSuffix; diff --git a/src/util/virstring.c b/src/util/virstring.c index 7749eb2db5..a2c07e5c17 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -35,26 +35,6 @@ VIR_LOG_INIT("util.string"); -/** - * virStringSplitCount: - * - * A wrapper for g_strsplit which provides number of elements of the split - * string. - */ -char ** -virStringSplitCount(const char *string, - const char *delim, - size_t max_tokens, - size_t *tokcount) -{ - GStrv ret = g_strsplit(string, delim, max_tokens); - - *tokcount = g_strv_length(ret); - - 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 e688495574..7cc1d8c55f 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -22,12 +22,6 @@ #define VIR_INT64_STR_BUFLEN 21 -char **virStringSplitCount(const char *string, - const char *delim, - size_t max_tokens, - size_t *tokcount) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4); - int virStringListMerge(char ***dst, char ***src); -- 2.29.2

It's way more useful to run valgrind against the rest of the code than this test to see whether virStringListFreeCount works. Remove the test. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/virstringtest.c | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/tests/virstringtest.c b/tests/virstringtest.c index 82e8e2106f..83b883524d 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -392,24 +392,6 @@ testStringToDouble(const void *opaque) return ret; } -/* The point of this test is to check whether all members of the array are - * freed. The test has to be checked using valgrind. */ -static int -testVirStringListFreeCount(const void *opaque G_GNUC_UNUSED) -{ - char **list; - - list = g_new0(char *, 4); - - list[0] = g_strdup("test1"); - list[2] = g_strdup("test2"); - list[3] = g_strdup("test3"); - - virStringListFreeCount(list, 4); - - return 0; -} - struct testStripData { const char *string; @@ -718,11 +700,6 @@ mymain(void) NULL, 3.141592653589793238462643383279502884197169399375105); - /* test virStringListFreeCount */ - if (virTestRun("virStringListFreeCount", testVirStringListFreeCount, - NULL) < 0) - ret = -1; - #define TEST_STRIP_IPV6_BRACKETS(str, res) \ do { \ struct testStripData stripData = { \ -- 2.29.2