[PATCH 0/4] Various cleanups
Few patches from old branches. Peter Krempa (4): util: virdevmapper: Sanitize use of macros for buffer size virDevMapperGetTargets: Use a linked list as return type util: virstring: Remove unused 'virStringListMerge' qemuMonitorJSONGetStatus: Refactor cleanup src/libvirt_private.syms | 1 - src/qemu/qemu_cgroup.c | 11 ++++---- src/qemu/qemu_monitor_json.c | 18 +++++-------- src/qemu/qemu_namespace.c | 9 +++---- src/util/virdevmapper.c | 49 +++++++++++++++++------------------- src/util/virdevmapper.h | 2 +- src/util/virstring.c | 35 -------------------------- src/util/virstring.h | 3 --- tests/qemuhotplugmock.c | 10 +++----- 9 files changed, 43 insertions(+), 95 deletions(-) -- 2.31.1
There are two distinct uses of an arbitrary buffers size when querying the device mapper. One is related to loading the /proc/devices file, while the other is used as buffer for ioctls to the devmapper. Split up the macros used here so that it's clear that they are not meant for the same thing. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virdevmapper.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c index 2c4c2df999..301c8f3ba7 100644 --- a/src/util/virdevmapper.c +++ b/src/util/virdevmapper.c @@ -42,12 +42,13 @@ VIR_LOG_INIT("util.virdevmapper"); # define PROC_DEVICES "/proc/devices" +# define PROC_DEVICES_BUF_SIZE (16 * 1024) # define DM_NAME "device-mapper" # define DEV_DM_DIR "/dev/" DM_DIR # define CONTROL_PATH DEV_DM_DIR "/" DM_CONTROL_NODE -# define BUF_SIZE (16 * 1024) -G_STATIC_ASSERT(BUF_SIZE > sizeof(struct dm_ioctl)); +# define VIR_DEVMAPPER_IOCTL_BUF_SIZE_INCREMENT (16 * 1024) +G_STATIC_ASSERT(VIR_DEVMAPPER_IOCTL_BUF_SIZE_INCREMENT > sizeof(struct dm_ioctl)); static int @@ -60,7 +61,7 @@ virDevMapperGetMajor(unsigned int *major) if (!virFileExists(CONTROL_PATH)) return -2; - if (virFileReadAll(PROC_DEVICES, BUF_SIZE, &buf) < 0) + if (virFileReadAll(PROC_DEVICES, PROC_DEVICES_BUF_SIZE, &buf) < 0) return -1; lines = g_strsplit(buf, "\n", 0); @@ -92,7 +93,7 @@ virDevMapperGetMajor(unsigned int *major) static void * virDMIoctl(int controlFD, int cmd, struct dm_ioctl *dm, char **buf) { - size_t bufsize = BUF_SIZE; + size_t bufsize = VIR_DEVMAPPER_IOCTL_BUF_SIZE_INCREMENT; reread: *buf = g_new0(char, bufsize); @@ -113,7 +114,7 @@ virDMIoctl(int controlFD, int cmd, struct dm_ioctl *dm, char **buf) memcpy(dm, *buf, sizeof(struct dm_ioctl)); if (dm->flags & DM_BUFFER_FULL_FLAG) { - bufsize += BUF_SIZE; + bufsize += VIR_DEVMAPPER_IOCTL_BUF_SIZE_INCREMENT; VIR_FREE(*buf); goto reread; } -- 2.31.1
Of the two callers one simply iterates over the returned paths and the second one appends the returned paths to another linked list. Simplify all of this by directly returning a linked list. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_cgroup.c | 11 ++++++----- src/qemu/qemu_namespace.c | 9 +++------ src/util/virdevmapper.c | 38 +++++++++++++++++--------------------- src/util/virdevmapper.h | 2 +- tests/qemuhotplugmock.c | 10 ++++------ 5 files changed, 31 insertions(+), 39 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 6d4a82b3cd..471cbc3b8f 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -38,6 +38,7 @@ #include "virnuma.h" #include "virdevmapper.h" #include "virutil.h" +#include "virglibutil.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -60,8 +61,8 @@ qemuSetupImagePathCgroup(virDomainObj *vm, { qemuDomainObjPrivate *priv = vm->privateData; int perms = VIR_CGROUP_DEVICE_READ; - g_auto(GStrv) targetPaths = NULL; - size_t i; + g_autoptr(virGSListString) targetPaths = NULL; + GSList *n; int rv; if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) @@ -94,10 +95,10 @@ qemuSetupImagePathCgroup(virDomainObj *vm, return -1; } - for (i = 0; targetPaths && targetPaths[i]; i++) { - rv = virCgroupAllowDevicePath(priv->cgroup, targetPaths[i], perms, false); + for (n = targetPaths; n; n = n->next) { + rv = virCgroupAllowDevicePath(priv->cgroup, n->data, perms, false); - virDomainAuditCgroupPath(vm, priv->cgroup, "allow", targetPaths[i], + virDomainAuditCgroupPath(vm, priv->cgroup, "allow", n->data, virCgroupGetDevicePermsString(perms), rv); if (rv < 0) diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index 728d77fc61..f1aaca86b1 100644 --- a/src/qemu/qemu_namespace.c +++ b/src/qemu/qemu_namespace.c @@ -251,8 +251,7 @@ qemuDomainSetupDisk(virStorageSource *src, if (!(tmpPath = virPCIDeviceAddressGetIOMMUGroupDev(&next->nvme->pciAddr))) return -1; } else { - g_auto(GStrv) targetPaths = NULL; - GStrv tmp; + GSList *targetPaths; if (virStorageSourceIsEmpty(next) || !virStorageSourceIsLocalStorage(next)) { @@ -270,10 +269,8 @@ qemuDomainSetupDisk(virStorageSource *src, return -1; } - if (targetPaths) { - for (tmp = targetPaths; *tmp; tmp++) - *paths = g_slist_prepend(*paths, g_steal_pointer(tmp)); - } + if (targetPaths) + *paths = g_slist_concat(g_slist_reverse(targetPaths), *paths); } *paths = g_slist_prepend(*paths, g_steal_pointer(&tmpPath)); diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c index 301c8f3ba7..e42324fd19 100644 --- a/src/util/virdevmapper.c +++ b/src/util/virdevmapper.c @@ -36,6 +36,7 @@ # include "virstring.h" # include "virfile.h" # include "virlog.h" +# include "virglibutil.h" # define VIR_FROM_THIS VIR_FROM_STORAGE @@ -217,18 +218,16 @@ virDMSanitizepath(const char *path) static int virDevMapperGetTargetsImpl(int controlFD, const char *path, - char ***devPaths_ret, + GSList **devPaths, unsigned int ttl) { g_autofree char *sanitizedPath = NULL; g_autofree char *buf = NULL; struct dm_ioctl dm; struct dm_target_deps *deps = NULL; - g_auto(GStrv) devPaths = NULL; size_t i; memset(&dm, 0, sizeof(dm)); - *devPaths_ret = NULL; if (ttl == 0) { errno = ELOOP; @@ -258,24 +257,17 @@ virDevMapperGetTargetsImpl(int controlFD, return -1; } - devPaths = g_new0(char *, deps->count + 1); for (i = 0; i < deps->count; i++) { - devPaths[i] = g_strdup_printf("/dev/block/%u:%u", - major(deps->dev[i]), - minor(deps->dev[i])); - } - - for (i = 0; i < deps->count; i++) { - g_auto(GStrv) tmpPaths = NULL; + char *curpath = g_strdup_printf("/dev/block/%u:%u", + major(deps->dev[i]), + minor(deps->dev[i])); - if (virDevMapperGetTargetsImpl(controlFD, devPaths[i], &tmpPaths, ttl - 1) < 0) - return -1; + *devPaths = g_slist_prepend(*devPaths, curpath); - if (virStringListMerge(&devPaths, &tmpPaths) < 0) + if (virDevMapperGetTargetsImpl(controlFD, curpath, devPaths, ttl - 1) < 0) return -1; } - *devPaths_ret = g_steal_pointer(&devPaths); return 0; } @@ -283,11 +275,10 @@ virDevMapperGetTargetsImpl(int controlFD, /** * virDevMapperGetTargets: * @path: devmapper target - * @devPaths: returned string list of devices + * @devPaths: filled in by a GSList containing the paths * * For given @path figure out its targets, and store them in - * @devPaths array. Note, @devPaths is a string list so it's NULL - * terminated. + * @devPaths. * * If @path is not a devmapper device, @devPaths is set to NULL and * success is returned. @@ -301,10 +292,11 @@ virDevMapperGetTargetsImpl(int controlFD, */ int virDevMapperGetTargets(const char *path, - char ***devPaths) + GSList **devPaths) { VIR_AUTOCLOSE controlFD = -1; const unsigned int ttl = 32; + g_autoptr(virGSListString) paths = NULL; /* Arbitrary limit on recursion level. A devmapper target can * consist of devices or yet another targets. If that's the @@ -321,7 +313,11 @@ virDevMapperGetTargets(const char *path, return -1; } - return virDevMapperGetTargetsImpl(controlFD, path, devPaths, ttl); + if (virDevMapperGetTargetsImpl(controlFD, path, &paths, ttl) < 0) + return -1; + + *devPaths = g_slist_reverse(g_steal_pointer(&paths)); + return 0; } @@ -346,7 +342,7 @@ virIsDevMapperDevice(const char *dev_name) int virDevMapperGetTargets(const char *path G_GNUC_UNUSED, - char ***devPaths G_GNUC_UNUSED) + GSlist **devPaths G_GNUC_UNUSED) { errno = ENOSYS; return -1; diff --git a/src/util/virdevmapper.h b/src/util/virdevmapper.h index 834900692e..4d8d3ccdb8 100644 --- a/src/util/virdevmapper.h +++ b/src/util/virdevmapper.h @@ -24,7 +24,7 @@ int virDevMapperGetTargets(const char *path, - char ***devPaths) G_GNUC_NO_INLINE; + GSList **devPaths) G_GNUC_NO_INLINE; bool virIsDevMapperDevice(const char *dev_name) ATTRIBUTE_NONNULL(1); diff --git a/tests/qemuhotplugmock.c b/tests/qemuhotplugmock.c index d1fc50c5f0..3b5f858044 100644 --- a/tests/qemuhotplugmock.c +++ b/tests/qemuhotplugmock.c @@ -54,16 +54,14 @@ qemuDomainGetUnplugTimeout(virDomainObj *vm) int virDevMapperGetTargets(const char *path, - char ***devPaths) + GSList **devPaths) { *devPaths = NULL; if (STREQ(path, "/dev/mapper/virt")) { - *devPaths = g_new0(char *, 4); - (*devPaths)[0] = g_strdup("/dev/block/8:0"); /* /dev/sda */ - (*devPaths)[1] = g_strdup("/dev/block/8:16"); /* /dev/sdb */ - (*devPaths)[2] = g_strdup("/dev/block/8:32"); /* /dev/sdc */ - (*devPaths)[3] = NULL; + *devPaths = g_slist_prepend(*devPaths, g_strdup("/dev/block/8:32")); /* /dev/sdc */ + *devPaths = g_slist_prepend(*devPaths, g_strdup("/dev/block/8:16")); /* /dev/sdb */ + *devPaths = g_slist_prepend(*devPaths, g_strdup("/dev/block/8:0")); /* /dev/sda */ } return 0; -- 2.31.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 - src/util/virstring.c | 35 ----------------------------------- src/util/virstring.h | 3 --- 3 files changed, 39 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ace35d709f..25ee21463c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3298,7 +3298,6 @@ virStringHasControlChars; virStringHasSuffix; virStringIsEmpty; virStringIsPrintable; -virStringListMerge; virStringMatch; virStringMatchesNameSuffix; virStringParsePort; diff --git a/src/util/virstring.c b/src/util/virstring.c index f416fed3c5..cee56debca 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -35,41 +35,6 @@ VIR_LOG_INIT("util.string"); -/** - * virStringListMerge: - * @dst: a NULL-terminated array of strings to expand - * @src: a NULL-terminated array of strings - * - * Merges @src into @dst. Upon successful return from this - * function, @dst is resized to $(dst + src) elements and @src is - * freed. - * - * Returns 0 on success, -1 otherwise. - */ -int -virStringListMerge(char ***dst, - char ***src) -{ - size_t dst_len, src_len, i; - - if (!src || !*src) - return 0; - - dst_len = g_strv_length(*dst); - src_len = g_strv_length(*src); - - VIR_REALLOC_N(*dst, dst_len + src_len + 1); - - for (i = 0; i <= src_len; i++) - (*dst)[i + dst_len] = (*src)[i]; - - /* Don't call g_strfreev() as it would free strings in - * @src. */ - VIR_FREE(*src); - return 0; -} - - /* 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 ca81889c9b..45f07ddd7a 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -22,9 +22,6 @@ #define VIR_INT64_STR_BUFLEN 21 -int virStringListMerge(char ***dst, - char ***src); - int virStrToLong_i(char const *s, char **end_ptr, int base, -- 2.31.1
Use g_autofree for the JSON values to remove cleanup label and ret variable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 8d3c4031a6..37e9c05d27 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1710,10 +1710,9 @@ qemuMonitorJSONGetStatus(qemuMonitor *mon, bool *running, virDomainPausedReason *reason) { - int ret = -1; const char *status; - virJSONValue *cmd; - virJSONValue *reply = NULL; + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; virJSONValue *data; if (reason) @@ -1723,17 +1722,17 @@ qemuMonitorJSONGetStatus(qemuMonitor *mon, return -1; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) - goto cleanup; + return -1; if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0) - goto cleanup; + return -1; data = virJSONValueObjectGetObject(reply, "return"); if (virJSONValueObjectGetBoolean(data, "running", running) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("query-status reply was missing running state")); - goto cleanup; + return -1; } if ((status = virJSONValueObjectGetString(data, "status"))) { @@ -1743,12 +1742,7 @@ qemuMonitorJSONGetStatus(qemuMonitor *mon, VIR_DEBUG("query-status reply was missing status details"); } - ret = 0; - - cleanup: - virJSONValueFree(cmd); - virJSONValueFree(reply); - return ret; + return 0; } -- 2.31.1
On a Monday in 2021, Peter Krempa wrote:
Few patches from old branches.
Peter Krempa (4): util: virdevmapper: Sanitize use of macros for buffer size virDevMapperGetTargets: Use a linked list as return type util: virstring: Remove unused 'virStringListMerge' qemuMonitorJSONGetStatus: Refactor cleanup
src/libvirt_private.syms | 1 - src/qemu/qemu_cgroup.c | 11 ++++---- src/qemu/qemu_monitor_json.c | 18 +++++-------- src/qemu/qemu_namespace.c | 9 +++---- src/util/virdevmapper.c | 49 +++++++++++++++++------------------- src/util/virdevmapper.h | 2 +- src/util/virstring.c | 35 -------------------------- src/util/virstring.h | 3 --- tests/qemuhotplugmock.c | 10 +++----- 9 files changed, 43 insertions(+), 95 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa