[dm-devel] [PATCH 3/9] libmultipath: variable-size parameters in assemble_map()

[Martin Wilck]

On Mi, 2021-07-28 at 10:54 -0500, Benjamin Marzinski wrote:
> 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.
> 
> > -
> >  /*
> >   * 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?

Good catch. I probably wrote this before I wrote the macro.

> 
> > +       struct strbuf __attribute__((cleanup(reset_strbuf))) buff =
> > STRBUF_INIT;
> >         struct pathgroup * pgp;
> >         struct path * pp;
> >  


> > @@ -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"?

No strong reason. I thought it was more obvious this way, and perhaps
less prone to future error. I will change it.

Martin