[dm-devel] [PATCH 3/9] libmultipath: variable-size parameters in assemble_map()
Benjamin Marzinski
bmarzins at redhat.com
Wed Jul 28 15:54:20 UTC 2021
On Thu, Jul 15, 2021 at 12:52:17PM +0200, mwilck at suse.com wrote:
> From: Martin Wilck <mwilck at suse.com>
>
> Instead of using fixed PARAMS_SIZE-sized arrays for parameters, use
> dynamically allocated memory.
>
> The library version needs to be bumped, because setup_map() argument
> list has changed.
>
Looks good. Only minor nits below.
> Signed-off-by: Martin Wilck <mwilck at suse.com>
> ---
> libmultipath/configure.c | 18 ++++++------
> libmultipath/configure.h | 3 +-
> libmultipath/dmparser.c | 47 ++++++++++---------------------
> libmultipath/dmparser.h | 2 +-
> libmultipath/libmultipath.version | 5 +++-
> libmultipath/structs.h | 1 -
> libmultipath/util.c | 5 ++++
> libmultipath/util.h | 1 +
> multipathd/cli_handlers.c | 4 +--
> multipathd/main.c | 20 +++++++------
> 10 files changed, 50 insertions(+), 56 deletions(-)
>
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index a6ae335..1227864 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -292,8 +292,7 @@ static int wait_for_pending_paths(struct multipath *mpp,
> return n_pending;
> }
>
> -int setup_map(struct multipath *mpp, char *params, int params_size,
> - struct vectors *vecs)
> +int setup_map(struct multipath *mpp, char **params, struct vectors *vecs)
> {
> struct pathgroup * pgp;
> struct config *conf;
> @@ -462,7 +461,7 @@ int setup_map(struct multipath *mpp, char *params, int params_size,
> * transform the mp->pg vector of vectors of paths
> * into a mp->params strings to feed the device-mapper
> */
> - if (assemble_map(mpp, params, params_size)) {
> + if (assemble_map(mpp, params)) {
> condlog(0, "%s: problem assembing map", mpp->alias);
> return 1;
> }
> @@ -811,7 +810,7 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
> remove_feature(&mpp_feat, "retain_attached_hw_handler");
> remove_feature(&cmpp_feat, "queue_if_no_path");
> remove_feature(&cmpp_feat, "retain_attached_hw_handler");
> - if (strncmp(mpp_feat, cmpp_feat, PARAMS_SIZE)) {
> + if (strcmp(mpp_feat, cmpp_feat)) {
> select_reload_action(mpp, "features change");
> FREE(cmpp_feat);
> FREE(mpp_feat);
> @@ -1128,14 +1127,14 @@ int coalesce_paths (struct vectors *vecs, vector mpvec, char *refwwid,
> int ret = CP_FAIL;
> int k, i, r;
> int is_daemon = (cmd == CMD_NONE) ? 1 : 0;
> - char params[PARAMS_SIZE];
> + char *params __attribute__((cleanup(cleanup_charp))) = NULL;
> struct multipath * mpp;
> - struct path * pp1;
> + struct path * pp1 = NULL;
> struct path * pp2;
> vector curmp = vecs->mpvec;
> vector pathvec = vecs->pathvec;
> vector newmp;
> - struct config *conf;
> + struct config *conf = NULL;
> int allow_queueing;
> struct bitfield *size_mismatch_seen;
>
> @@ -1247,8 +1246,7 @@ int coalesce_paths (struct vectors *vecs, vector mpvec, char *refwwid,
> }
> verify_paths(mpp);
>
> - params[0] = '\0';
> - if (setup_map(mpp, params, PARAMS_SIZE, vecs)) {
> + if (setup_map(mpp, ¶ms, vecs)) {
> remove_map(mpp, vecs->pathvec, vecs->mpvec, KEEP_VEC);
> continue;
> }
> @@ -1260,6 +1258,8 @@ int coalesce_paths (struct vectors *vecs, vector mpvec, char *refwwid,
> force_reload == FORCE_RELOAD_YES ? 1 : 0);
>
> r = domap(mpp, params, is_daemon);
> + free(params);
> + params = NULL;
>
> if (r == DOMAP_FAIL || r == DOMAP_RETRY) {
> condlog(3, "%s: domap (%u) failure "
> diff --git a/libmultipath/configure.h b/libmultipath/configure.h
> index 70cf77a..92a5aba 100644
> --- a/libmultipath/configure.h
> +++ b/libmultipath/configure.h
> @@ -47,8 +47,7 @@ enum {
>
> struct vectors;
>
> -int setup_map (struct multipath * mpp, char * params, int params_size,
> - struct vectors *vecs );
> +int setup_map (struct multipath * mpp, char **params, struct vectors *vecs);
> void select_action (struct multipath *mpp, const struct _vector *curmp,
> int force_reload);
> int domap (struct multipath * mpp, char * params, int is_daemon);
> diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
> index b306c46..fb09e0a 100644
> --- a/libmultipath/dmparser.c
> +++ b/libmultipath/dmparser.c
> @@ -15,6 +15,7 @@
> #include "util.h"
> #include "debug.h"
> #include "dmparser.h"
> +#include "strbuf.h"
>
> #define WORD_SIZE 64
>
> @@ -41,40 +42,21 @@ merge_words(char **dst, const char *word)
> return 0;
> }
>
> -#define APPEND(p, end, args...) \
> -({ \
> - int ret; \
> - \
> - ret = snprintf(p, end - p, ##args); \
> - if (ret < 0) { \
> - condlog(0, "%s: conversion error", mp->alias); \
> - goto err; \
> - } \
> - p += ret; \
> - if (p >= end) { \
> - condlog(0, "%s: params too small", mp->alias); \
> - goto err; \
> - } \
> -})
> -
> /*
> * Transforms the path group vector into a proper device map string
> */
> -int
> -assemble_map (struct multipath * mp, char * params, int len)
> +int assemble_map(struct multipath *mp, char **params)
> {
> + static const char no_path_retry[] = "queue_if_no_path";
> + static const char retain_hwhandler[] = "retain_attached_hw_handler";
> int i, j;
> int minio;
> int nr_priority_groups, initial_pg_nr;
> - char * p;
> - const char *const end = params + len;
> - char no_path_retry[] = "queue_if_no_path";
> - char retain_hwhandler[] = "retain_attached_hw_handler";
Why not use STRBUF_ON_STACK() here?
> + struct strbuf __attribute__((cleanup(reset_strbuf))) buff = STRBUF_INIT;
> struct pathgroup * pgp;
> struct path * pp;
>
> minio = mp->minio;
> - p = params;
>
> nr_priority_groups = VECTOR_SIZE(mp->pg);
> initial_pg_nr = (nr_priority_groups ? mp->bestpg : 0);
> @@ -87,14 +69,15 @@ assemble_map (struct multipath * mp, char * params, int len)
> get_linux_version_code() < KERNEL_VERSION(4, 3, 0))
> add_feature(&mp->features, retain_hwhandler);
>
> - /* mp->features must not be NULL */
> - APPEND(p, end, "%s %s %i %i", mp->features, mp->hwhandler,
> - nr_priority_groups, initial_pg_nr);
> + if (print_strbuf(&buff, "%s %s %i %i", mp->features, mp->hwhandler,
> + nr_priority_groups, initial_pg_nr) < 0)
> + goto err;
>
> vector_foreach_slot (mp->pg, pgp, i) {
> pgp = VECTOR_SLOT(mp->pg, i);
> - APPEND(p, end, " %s %i 1", mp->selector,
> - VECTOR_SIZE(pgp->paths));
> + if (print_strbuf(&buff, " %s %i 1", mp->selector,
> + VECTOR_SIZE(pgp->paths)) < 0)
> + goto err;
>
> vector_foreach_slot (pgp->paths, pp, j) {
> int tmp_minio = minio;
> @@ -106,19 +89,19 @@ assemble_map (struct multipath * mp, char * params, int len)
> condlog(0, "dev_t not set for '%s'", pp->dev);
> goto err;
> }
> - APPEND(p, end, " %s %d", pp->dev_t, tmp_minio);
> + if (print_strbuf(&buff, " %s %d", pp->dev_t, tmp_minio) < 0)
> + goto err;
> }
> }
>
> - condlog(4, "%s: assembled map [%s]", mp->alias, params);
> + *params = steal_strbuf_str(&buff);
> + condlog(4, "%s: assembled map [%s]", mp->alias, *params);
> return 0;
>
> err:
> return 1;
> }
>
> -#undef APPEND
> -
> /*
> * Caution callers: If this function encounters yet unkown path devices, it
> * adds them uninitialized to the mpp.
> diff --git a/libmultipath/dmparser.h b/libmultipath/dmparser.h
> index 212fee5..666ae74 100644
> --- a/libmultipath/dmparser.h
> +++ b/libmultipath/dmparser.h
> @@ -1,3 +1,3 @@
> -int assemble_map (struct multipath *, char *, int);
> +int assemble_map (struct multipath *, char **);
> int disassemble_map (const struct _vector *, const char *, struct multipath *);
> int disassemble_status (const char *, struct multipath *);
> diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
> index 7567837..6dd52c2 100644
> --- a/libmultipath/libmultipath.version
> +++ b/libmultipath/libmultipath.version
> @@ -31,7 +31,7 @@
> * The new version inherits the previous ones.
> */
>
> -LIBMULTIPATH_6.0.0 {
> +LIBMULTIPATH_7.0.0 {
> global:
> /* symbols referenced by multipath and multipathd */
> add_foreign;
> @@ -267,6 +267,9 @@ global:
> /* added in 4.5.0 */
> get_vpd_sgio;
> trigger_partitions_udev_change;
> +
> + /* added in 7.0.0 */
> + cleanup_charp;
> local:
> *;
> };
> diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> index c52bcee..399540e 100644
> --- a/libmultipath/structs.h
> +++ b/libmultipath/structs.h
> @@ -13,7 +13,6 @@
> #define SERIAL_SIZE 128
> #define NODE_NAME_SIZE 224
> #define PATH_STR_SIZE 16
> -#define PARAMS_SIZE 4096
> #define FILE_NAME_SIZE 256
> #define CALLOUT_MAX_SIZE 256
> #define BLK_DEV_SIZE 33
> diff --git a/libmultipath/util.c b/libmultipath/util.c
> index 0e37f3f..e2fafe8 100644
> --- a/libmultipath/util.c
> +++ b/libmultipath/util.c
> @@ -455,3 +455,8 @@ int should_exit(void)
> {
> return 0;
> }
> +
> +void cleanup_charp(char **p)
> +{
> + free(*p);
> +}
> diff --git a/libmultipath/util.h b/libmultipath/util.h
> index e9b48f9..89027f8 100644
> --- a/libmultipath/util.h
> +++ b/libmultipath/util.h
> @@ -123,4 +123,5 @@ static inline void clear_bit_in_bitfield(unsigned int bit, struct bitfield *bf)
> ___p; \
> })
>
> +void cleanup_charp(char **p);
> #endif /* _UTIL_H */
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index d70e1db..bce40b1 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -972,12 +972,12 @@ cli_reload(void *v, char **reply, int *len, void *data)
> int resize_map(struct multipath *mpp, unsigned long long size,
> struct vectors * vecs)
> {
> - char params[PARAMS_SIZE] = {0};
> + char *params __attribute__((cleanup(cleanup_charp))) = NULL;
> unsigned long long orig_size = mpp->size;
>
> mpp->size = size;
> update_mpp_paths(mpp, vecs->pathvec);
> - if (setup_map(mpp, params, PARAMS_SIZE, vecs) != 0) {
> + if (setup_map(mpp, ¶ms, vecs) != 0) {
> condlog(0, "%s: failed to setup map for resize : %s",
> mpp->alias, strerror(errno));
> mpp->size = orig_size;
> diff --git a/multipathd/main.c b/multipathd/main.c
> index bdd629e..f760643 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -489,7 +489,7 @@ static int
> update_map (struct multipath *mpp, struct vectors *vecs, int new_map)
> {
> int retries = 3;
> - char params[PARAMS_SIZE] = {0};
> + char *params __attribute__((cleanup(cleanup_charp))) = NULL;
>
> retry:
> condlog(4, "%s: updating new map", mpp->alias);
> @@ -502,13 +502,15 @@ retry:
> verify_paths(mpp);
> mpp->action = ACT_RELOAD;
>
> - if (setup_map(mpp, params, PARAMS_SIZE, vecs)) {
> + if (setup_map(mpp, ¶ms, vecs)) {
> condlog(0, "%s: failed to setup new map in update", mpp->alias);
> retries = -1;
> goto fail;
> }
> if (domap(mpp, params, 1) == DOMAP_FAIL && retries-- > 0) {
> condlog(0, "%s: map_udate sleep", mpp->alias);
> + free(params);
> + params = NULL;
> sleep(1);
> goto retry;
> }
> @@ -1028,7 +1030,7 @@ int
> ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
> {
> struct multipath * mpp;
> - char params[PARAMS_SIZE] = {0};
> + char *params __attribute((cleanup(cleanup_charp))) = NULL;
> int retries = 3;
> int start_waiter = 0;
> int ret;
> @@ -1104,7 +1106,9 @@ rescan:
> /*
> * push the map to the device-mapper
> */
> - if (setup_map(mpp, params, PARAMS_SIZE, vecs)) {
Is there a reason to free params here, instead of just doing it before
the "goto rescan"?
-Ben
> + free(params);
> + params = NULL;
> + if (setup_map(mpp, ¶ms, vecs)) {
> condlog(0, "%s: failed to setup map for addition of new "
> "path %s", mpp->alias, pp->dev);
> goto fail_map;
> @@ -1189,7 +1193,7 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
> {
> struct multipath * mpp;
> int i, retval = REMOVE_PATH_SUCCESS;
> - char params[PARAMS_SIZE] = {0};
> + char *params __attribute__((cleanup(cleanup_charp))) = NULL;
>
> /*
> * avoid referring to the map of an orphaned path
> @@ -1250,7 +1254,7 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
> */
> }
>
> - if (setup_map(mpp, params, PARAMS_SIZE, vecs)) {
> + if (setup_map(mpp, ¶ms, vecs)) {
> condlog(0, "%s: failed to setup map for"
> " removal of path %s", mpp->alias, pp->dev);
> goto fail;
> @@ -1940,7 +1944,7 @@ int update_prio(struct path *pp, int refresh_all)
> static int reload_map(struct vectors *vecs, struct multipath *mpp, int refresh,
> int is_daemon)
> {
> - char params[PARAMS_SIZE] = {0};
> + char *params __attribute__((cleanup(cleanup_charp))) = NULL;
> struct path *pp;
> int i, r;
>
> @@ -1958,7 +1962,7 @@ static int reload_map(struct vectors *vecs, struct multipath *mpp, int refresh,
> }
> }
> }
> - if (setup_map(mpp, params, PARAMS_SIZE, vecs)) {
> + if (setup_map(mpp, ¶ms, vecs)) {
> condlog(0, "%s: failed to setup map", mpp->alias);
> return 1;
> }
> --
> 2.32.0
More information about the dm-devel
mailing list