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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

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





[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux