[dm-devel] [PATCH 3/4] libmultipath: use regular array for field widths
Martin Wilck
martin.wilck at suse.com
Fri Oct 21 07:04:55 UTC 2022
On Tue, 2022-10-11 at 16:53 -0500, Benjamin Marzinski wrote:
> We know the size of these arrays, so we can just allocate them on the
> stack. Also, show_path() doesn't use the width, so don't initialize
> it
> in the first place.
>
> Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>
This isn't wrong, but I'm not sure what it actually achieves except a
few less NULL checks (I'm sure you're aware that this doesn't mean
better protection against out-of-memory situations). It comes at the
cost of an ABI change. I understand that the intention is to eliminate
__attribute__((cleanup())). But if we agree we don't want to do that
everywhere, I see no particular reason to do it in this code path.
I'm not totally against it, but I'm not enthusiastic, either.
Regards,
Martin
> ---
> libmultipath/foreign.c | 5 ++--
> libmultipath/libmultipath.version | 4 +--
> libmultipath/print.c | 32 +++++++++++-------------
> libmultipath/print.h | 4 +--
> multipath/main.c | 5 ++--
> multipathd/cli_handlers.c | 41 ++++++++++++-----------------
> --
> 6 files changed, 38 insertions(+), 53 deletions(-)
>
> diff --git a/libmultipath/foreign.c b/libmultipath/foreign.c
> index 8981ff58..4cc2a8e3 100644
> --- a/libmultipath/foreign.c
> +++ b/libmultipath/foreign.c
> @@ -550,10 +550,9 @@ void print_foreign_topology(int verbosity)
> struct strbuf buf = STRBUF_INIT;
> struct foreign *fgn;
> int i;
> - fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp)))
> = NULL;
> + fieldwidth_t width[path_layout_size()];
>
> - if ((width = alloc_path_layout()) == NULL)
> - return;
> + memset(width, 0, sizeof(width));
> rdlock_foreigns();
> if (foreigns == NULL) {
> unlock_foreigns(NULL);
> diff --git a/libmultipath/libmultipath.version
> b/libmultipath/libmultipath.version
> index 8a447f7f..af7c5ed2 100644
> --- a/libmultipath/libmultipath.version
> +++ b/libmultipath/libmultipath.version
> @@ -38,9 +38,7 @@ global:
> add_map_with_path;
> adopt_paths;
> alloc_multipath;
> - alloc_multipath_layout;
> alloc_path;
> - alloc_path_layout;
> alloc_path_with_pathinfo;
> change_foreign;
> check_alias_settings;
> @@ -126,6 +124,7 @@ global:
> libmultipath_exit;
> libmultipath_init;
> load_config;
> + multipath_layout_size;
> need_io_err_check;
> orphan_path;
> parse_prkey_flags;
> @@ -133,6 +132,7 @@ global:
> path_discovery;
> path_get_tpgs;
> pathinfo;
> + path_layout_size;
> path_offline;
> print_all_paths;
> print_foreign_topology;
> diff --git a/libmultipath/print.c b/libmultipath/print.c
> index 97f9a177..87d6a329 100644
> --- a/libmultipath/print.c
> +++ b/libmultipath/print.c
> @@ -805,6 +805,12 @@ static const struct multipath_data mpd[] = {
> {'g', "vpd page data", snprint_multipath_vpd_data},
> };
>
> +
> +int multipath_layout_size(void)
> +{
> + return ARRAY_SIZE(mpd);
> +}
> +
> static const struct path_data pd[] = {
> {'w', "uuid", snprint_path_uuid},
> {'i', "hcil", snprint_hcil},
> @@ -834,6 +840,11 @@ static const struct path_data pd[] = {
> {'L', "LUN hex", snprint_path_lunhex},
> };
>
> +int path_layout_size(void)
> +{
> + return ARRAY_SIZE(pd);
> +}
> +
> static const struct pathgroup_data pgd[] = {
> {'s', "selector", snprint_pg_selector},
> {'p', "pri", snprint_pg_pri},
> @@ -871,10 +882,6 @@ int snprint_wildcards(struct strbuf *buff)
> return get_strbuf_len(buff) - initial_len;
> }
>
> -fieldwidth_t *alloc_path_layout(void) {
> - return calloc(ARRAY_SIZE(pd), sizeof(fieldwidth_t));
> -}
> -
> void get_path_layout(vector pathvec, int header, fieldwidth_t
> *width)
> {
> vector gpvec = vector_convert(NULL, pathvec, struct path,
> @@ -929,11 +936,6 @@ void _get_path_layout (const struct _vector
> *gpvec, enum layout_reset reset,
> }
> }
>
> -fieldwidth_t *alloc_multipath_layout(void) {
> -
> - return calloc(ARRAY_SIZE(mpd), sizeof(fieldwidth_t));
> -}
> -
> void get_multipath_layout (vector mpvec, int header, fieldwidth_t
> *width) {
> vector gmvec = vector_convert(NULL, mpvec, struct multipath,
> dm_multipath_to_gen);
> @@ -1187,12 +1189,11 @@ int _snprint_pathgroup(const struct
> gen_pathgroup *ggp, struct strbuf *line,
> void _print_multipath_topology(const struct gen_multipath *gmp, int
> verbosity)
> {
> struct strbuf buff = STRBUF_INIT;
> - fieldwidth_t *p_width
> __attribute__((cleanup(cleanup_ucharp))) = NULL;
> + fieldwidth_t p_width[ARRAY_SIZE(pd)] = {0};
> const struct gen_pathgroup *gpg;
> const struct _vector *pgvec, *pathvec;
> int j;
>
> - p_width = alloc_path_layout();
> pgvec = gmp->ops->get_pathgroups(gmp);
>
> if (pgvec != NULL) {
> @@ -1236,14 +1237,11 @@ int _snprint_multipath_topology(const struct
> gen_multipath *gmp,
> const struct gen_pathgroup *gpg;
> struct strbuf style = STRBUF_INIT;
> size_t initial_len = get_strbuf_len(buff);
> - fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp)))
> = NULL;
> + fieldwidth_t width[ARRAY_SIZE(mpd)] = {0};
>
> if (verbosity <= 0)
> return 0;
>
> - if ((width = alloc_multipath_layout()) == NULL)
> - return -ENOMEM;
> -
> if (verbosity == 1)
> return _snprint_multipath(gmp, buff, "%n", width);
>
> @@ -2027,7 +2025,7 @@ static void print_all_paths_custo(vector
> pathvec, int banner, const char *fmt)
> int i;
> struct path * pp;
> struct strbuf line = STRBUF_INIT;
> - fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp)))
> = NULL;
> + fieldwidth_t width[ARRAY_SIZE(pd)] = {0};
>
> if (!VECTOR_SIZE(pathvec)) {
> if (banner)
> @@ -2035,8 +2033,6 @@ static void print_all_paths_custo(vector
> pathvec, int banner, const char *fmt)
> return;
> }
>
> - if ((width = alloc_path_layout()) == NULL)
> - return;
> get_path_layout(pathvec, 1, width);
>
> pthread_cleanup_push_cast(reset_strbuf, &line);
> diff --git a/libmultipath/print.h b/libmultipath/print.h
> index 52f5b256..4e50827d 100644
> --- a/libmultipath/print.h
> +++ b/libmultipath/print.h
> @@ -16,11 +16,11 @@ enum layout_reset {
> };
>
> /* fieldwidth_t is defined in generic.h */
> -fieldwidth_t *alloc_path_layout(void);
> +int multipath_layout_size(void);
> +int path_layout_size(void);
> void _get_path_layout (const struct _vector *gpvec, enum
> layout_reset,
> fieldwidth_t *width);
> void get_path_layout (vector pathvec, int header, fieldwidth_t
> *width);
> -fieldwidth_t *alloc_multipath_layout(void);
> void _get_multipath_layout (const struct _vector *gmvec, enum
> layout_reset,
> fieldwidth_t *width);
> void get_multipath_layout (vector mpvec, int header, fieldwidth_t
> *width);
> diff --git a/multipath/main.c b/multipath/main.c
> index 7b69a3ce..f4c85409 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -457,7 +457,7 @@ configure (struct config *conf, enum mpath_cmds
> cmd,
> int di_flag = 0;
> char * refwwid = NULL;
> char * dev = NULL;
> - fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp)))
> = NULL;
> + fieldwidth_t width[path_layout_size()];
>
> /*
> * allocate core vectors to store paths and multipaths
> @@ -544,8 +544,7 @@ configure (struct config *conf, enum mpath_cmds
> cmd,
> if (libmp_verbosity > 2)
> print_all_paths(pathvec, 1);
>
> - if ((width = alloc_path_layout()) == NULL)
> - goto out;
> + memset(width, 0, sizeof(width));
> get_path_layout(pathvec, 0, width);
> foreign_path_layout(width);
>
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index 5b8f647b..ddc807a1 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -38,11 +38,10 @@ show_paths (struct strbuf *reply, struct vectors
> *vecs, char *style, int pretty)
> int i;
> struct path * pp;
> int hdr_len = 0;
> - fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp)))
> = NULL;
> + fieldwidth_t width[path_layout_size()];
>
> + memset(width, 0, sizeof(width));
> if (pretty) {
> - if ((width = alloc_path_layout()) == NULL)
> - return 1;
> get_path_layout(vecs->pathvec, 1, width);
> foreign_path_layout(width);
> }
> @@ -50,10 +49,10 @@ show_paths (struct strbuf *reply, struct vectors
> *vecs, char *style, int pretty)
> return 1;
>
> vector_foreach_slot(vecs->pathvec, pp, i) {
> - if (snprint_path(reply, style, pp, width) < 0)
> + if (snprint_path(reply, style, pp, pretty? width :
> NULL) < 0)
> return 1;
> }
> - if (snprint_foreign_paths(reply, style, width) < 0)
> + if (snprint_foreign_paths(reply, style, pretty? width : NULL)
> < 0)
> return 1;
>
> if (pretty && get_strbuf_len(reply) == (size_t)hdr_len)
> @@ -67,12 +66,7 @@ static int
> show_path (struct strbuf *reply, struct vectors *vecs, struct path
> *pp,
> char *style)
> {
> - fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp)))
> = NULL;
> -
> - if ((width = alloc_path_layout()) == NULL)
> - return 1;
> - get_path_layout(vecs->pathvec, 1, width);
> - if (snprint_path(reply, style, pp, 0) < 0)
> + if (snprint_path(reply, style, pp, NULL) < 0)
> return 1;
> return 0;
> }
> @@ -95,10 +89,9 @@ show_maps_topology (struct strbuf *reply, struct
> vectors * vecs)
> {
> int i;
> struct multipath * mpp;
> - fieldwidth_t *p_width
> __attribute__((cleanup(cleanup_ucharp))) = NULL;
> + fieldwidth_t p_width[path_layout_size()];
>
> - if ((p_width = alloc_path_layout()) == NULL)
> - return 1;
> + memset(p_width, 0, sizeof(p_width));
> get_path_layout(vecs->pathvec, 0, p_width);
> foreign_path_layout(p_width);
>
> @@ -258,10 +251,9 @@ cli_list_map_topology (void *v, struct strbuf
> *reply, void *data)
> struct multipath * mpp;
> struct vectors * vecs = (struct vectors *)data;
> char * param = get_keyparam(v, MAP);
> - fieldwidth_t *p_width
> __attribute__((cleanup(cleanup_ucharp))) = NULL;
> + fieldwidth_t p_width[path_layout_size()];
>
> - if ((p_width = alloc_path_layout()) == NULL)
> - return 1;
> + memset(p_width, 0, sizeof(p_width));
> get_path_layout(vecs->pathvec, 0, p_width);
> param = convert_dev(param, 0);
> mpp = find_mp_by_str(vecs->mpvec, param);
> @@ -357,11 +349,10 @@ show_maps (struct strbuf *reply, struct vectors
> *vecs, char *style,
> int i;
> struct multipath * mpp;
> int hdr_len = 0;
> - fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp)))
> = NULL;
> + fieldwidth_t width[multipath_layout_size()];
>
> + memset(width, 0, sizeof(width));
> if (pretty) {
> - if ((width = alloc_multipath_layout()) == NULL)
> - return 1;
> get_multipath_layout(vecs->mpvec, 1, width);
> foreign_multipath_layout(width);
> }
> @@ -374,10 +365,11 @@ show_maps (struct strbuf *reply, struct vectors
> *vecs, char *style,
> i--;
> continue;
> }
> - if (snprint_multipath(reply, style, mpp, width) < 0)
> + if (snprint_multipath(reply, style, mpp,
> + pretty? width : NULL) < 0)
> return 1;
> }
> - if (snprint_foreign_multipaths(reply, style, width) < 0)
> + if (snprint_foreign_multipaths(reply, style, pretty? width :
> NULL) < 0)
> return 1;
>
> if (pretty && get_strbuf_len(reply) == (size_t)hdr_len)
> @@ -416,10 +408,9 @@ cli_list_map_fmt (void *v, struct strbuf *reply,
> void *data)
> struct vectors * vecs = (struct vectors *)data;
> char * param = get_keyparam(v, MAP);
> char * fmt = get_keyparam(v, FMT);
> - fieldwidth_t *width __attribute__((cleanup(cleanup_ucharp)))
> = NULL;
> + fieldwidth_t width[multipath_layout_size()];
>
> - if ((width = alloc_multipath_layout()) == NULL)
> - return 1;
> + memset(width, 0, sizeof(width));
> get_multipath_layout(vecs->mpvec, 1, width);
> param = convert_dev(param, 0);
> mpp = find_mp_by_str(vecs->mpvec, param);
More information about the dm-devel
mailing list