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@xxxxxxxxxx>
>
> Reviewed-by: Martin Wilck <mwilck@xxxxxxxx>
>
> 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);
>
>
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
