On Fri, Oct 21, 2022 at 07:04:55AM +0000, Martin Wilck wrote:
> 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@xxxxxxxxxx>
>
> 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.
That's fine. How about I send a patch to just fix show_path().
-Ben
>
> 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);
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
