[dm-devel] [PATCH 1/8] libmultipath: cleanup remove_feature

[Benjamin Marzinski]

On Thu, Oct 20, 2022 at 02:58:23PM +0000, Martin Wilck wrote:
> On Fri, 2022-10-07 at 12:35 -0500, Benjamin Marzinski wrote:
> > remove_feature() didn't correctly handle feature strings that used
> > whitespace other than spaces, which the kernel allows. It also didn't
> > check if the feature string to be removed was part of a larger
> > feature
> > token. Finally, it did a lot of unnecessary work. By failing if the
> > feature string to be removed contains leading or trailing whitespace,
> > the function can be significanly simplified.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>
> 
> Reviewed-by: Martin Wilck <mwilck at suse.com>
> 
> This is part of the code that always makes me think  we should clean it
> up more thoroughly. Therefore I've added some remarks below.
> 
> As you've started working on this, perhaps you want to follow up?
> If not, fine with me for now.
> 
> Martin
> 
> > ---
> >  libmultipath/structs.c | 82 +++++++++++++++-------------------------
> > --
> >  1 file changed, 29 insertions(+), 53 deletions(-)
> > 
> > diff --git a/libmultipath/structs.c b/libmultipath/structs.c
> > index 49621cb3..1f9945e0 100644
> > --- a/libmultipath/structs.c
> > +++ b/libmultipath/structs.c
> > @@ -6,6 +6,7 @@
> >  #include <unistd.h>
> >  #include <libdevmapper.h>
> >  #include <libudev.h>
> > +#include <ctype.h>
> >  
> >  #include "checkers.h"
> >  #include "vector.h"
> > @@ -663,7 +664,7 @@ int add_feature(char **f, const char *n)
> >  
> >  int remove_feature(char **f, const char *o)
> >  {
> > -       int c = 0, d, l;
> > +       int c = 0, d;
> >         char *e, *p, *n;
> >         const char *q;
> 
> I see you sticked to the conventions ;-) but the variable naming
> in this function could be improved.

Yeah. I didn't want to while I was chaning things, and then got lazy and
didn't do it in a separate patch, but these names certainly make harder
to follow the logic of the function. I send a patch that cleans this up.
 
> >  
> > @@ -674,33 +675,35 @@ int remove_feature(char **f, const char *o)
> >         if (!o || *o == '\0')
> >                 return 0;
> >  
> > -       /* Check if not present */
> > -       if (!strstr(*f, o))
> > +       d = strlen(o);
> > +       if (isspace(*o) || isspace(*(o + d - 1))) {
> > +               condlog(0, "internal error: feature \"%s\" has
> > leading or trailing spaces", o);
> > +               return 1;
> > +       }
> > +
> > +       /* Check if present and not part of a larger feature token*/
> > +       p = *f + 1; /* the size must be at the start of the features
> > string */
> > +       while ((p = strstr(p, o)) != NULL) {
> > +               if (isspace(*(p - 1)) &&
> > +                   (isspace(*(p + d)) || *(p + d) == '\0'))
> > +                       break;
> > +               p += d;
> > +       }
> > +       if (!p)
> >                 return 0;
> >  
> >         /* Get feature count */
> >         c = strtoul(*f, &e, 10);
> > -       if (*f == e)
> > -               /* parse error */
> > +       if (*f == e || !isspace(*e)) {
> > +               condlog(0, "parse error in feature string \"%s\"",
> > *f);
> >                 return 1;
> > -
> > -       /* Normalize features */
> > -       while (*o == ' ') {
> > -               o++;
> >         }
> > -       /* Just spaces, return */
> > -       if (*o == '\0')
> > -               return 0;
> > -       q = o + strlen(o);
> > -       while (*q == ' ')
> > -               q--;
> > -       d = (int)(q - o);
> >  
> >         /* Update feature count */
> >         c--;
> >         q = o;
> > -       while (q[0] != '\0') {
> > -               if (q[0] == ' ' && q[1] != ' ' && q[1] != '\0')
> > +       while (*q != '\0') {
> > +               if (isspace(*q) && !isspace(*(q + 1)) && *(q + 1) !=
> > '\0')
> >                         c--;
> >                 q++;
> >         }
> > @@ -714,15 +717,8 @@ int remove_feature(char **f, const char *o)
> >                 goto out;
> >         }
> >  
> > -       /* Search feature to be removed */
> > -       e = strstr(*f, o);
> > -       if (!e)
> > -               /* Not found, return */
> > -               return 0;
> > -
> >         /* Update feature count space */
> > -       l = strlen(*f) - d;
> > -       n =  malloc(l + 1);
> > +       n =  malloc(strlen(*f) - d + 1);
> 
> Given that this function never increases the length of the feature
> string, we might as well implement it without the allocating a new
> string. 

A reason to do the malloc is so that we could free the larger chunk of
memory afterwards. But it's not like we're talking a significant
difference, so I'm fine will removing it for simplicity's sake.

-Ben

> >         if (!n)
> >                 return 1;
> >  
> > @@ -732,36 +728,16 @@ int remove_feature(char **f, const char *o)
> >          * Copy existing features up to the feature
> >          * about to be removed
> >          */
> > -       p = strchr(*f, ' ');
> > -       if (!p) {
> > -               /* Internal error, feature string inconsistent */
> > -               free(n);
> > -               return 1;
> > -       }
> > -       while (*p == ' ')
> > -               p++;
> > -       p--;
> > -       if (e != p) {
> > -               do {
> > -                       e--;
> > -                       d++;
> > -               } while (*e == ' ');
> > -               e++; d--;
> > -               strncat(n, p, (size_t)(e - p));
> > -               p += (size_t)(e - p);
> > -       }
> > +       strncat(n, e, (size_t)(p - e));
> >         /* Skip feature to be removed */
> >         p += d;
> > -
> >         /* Copy remaining features */
> > -       if (strlen(p)) {
> > -               while (*p == ' ')
> > -                       p++;
> > -               if (strlen(p)) {
> > -                       p--;
> > -                       strcat(n, p);
> > -               }
> > -       }
> > +       while (isspace(*p))
> > +               p++;
> > +       if (*p != '\0')
> > +               strcat(n, p);
> > +       else
> > +               strchop(n);
> >  
> >  out:
> >         free(*f);
> 
>