Re: [dm-devel] [PATCH 3/4] libmultipath: use regular array for field widths

[Benjamin Marzinski <bmarzins@redhat.com>]

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@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.

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@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel