| Message ID | 1665164144-6716-3-git-send-email-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | mutipath: handle nvme:tcp paths better | expand |
On Fri, 2022-10-07 at 12:35 -0500, Benjamin Marzinski wrote: > add_feature() didn't correctly handle feature strings that used > whitespace other than spaces, which the kernel allows. It also didn't > allow adding features with multiple tokens. When it looked to see if > the > feature string to be added already existed, it didn't check if the > match > was part of a larger token. Finally, it did unnecessary work. By > using > asprintf() to create the string, the function can be signifcantly > simplified. > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> Reviewed-by: Martin Wilck <mwilck@suse.com> The same general remarks apply as for 01/08. > --- > libmultipath/structs.c | 49 +++++++++++++++++++++------------------- > -- > 1 file changed, 24 insertions(+), 25 deletions(-) > > diff --git a/libmultipath/structs.c b/libmultipath/structs.c > index 1f9945e0..0f16c071 100644 > --- a/libmultipath/structs.c > +++ b/libmultipath/structs.c > @@ -602,23 +602,33 @@ int add_feature(char **f, const char *n) > { > int c = 0, d, l; > char *e, *t; > + const char *p; Same remark as before about the variable naming. > > if (!f) > return 1; > > /* Nothing to do */ > - if (!n || *n == '0') > + if (!n || *n == '\0') ouch... -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
diff --git a/libmultipath/structs.c b/libmultipath/structs.c index 1f9945e0..0f16c071 100644 --- a/libmultipath/structs.c +++ b/libmultipath/structs.c @@ -602,23 +602,33 @@ int add_feature(char **f, const char *n) { int c = 0, d, l; char *e, *t; + const char *p; if (!f) return 1; /* Nothing to do */ - if (!n || *n == '0') + if (!n || *n == '\0') return 0; - if (strchr(n, ' ') != NULL) { - condlog(0, "internal error: feature \"%s\" contains spaces", n); + l = strlen(n); + if (isspace(*n) || isspace(*(n + l - 1))) { + condlog(0, "internal error: feature \"%s\" has leading or trailing spaces", n); return 1; } + p = n; + d = 1; + while (*p != '\0') { + if (isspace(*p) && !isspace(*(p + 1)) && *(p + 1) != '\0') + d++; + p++; + } + /* default feature is null */ if(!*f) { - l = asprintf(&t, "1 %s", n); + l = asprintf(&t, "%0d %s", d, n); if(l == -1) return 1; @@ -627,35 +637,24 @@ int add_feature(char **f, const char *n) } /* Check if feature is already present */ - if (strstr(*f, n)) - return 0; + e = *f; + while ((e = strstr(e, n)) != NULL) { + if (isspace(*(e - 1)) && + (isspace(*(e + l)) || *(e + l) == '\0')) + return 0; + e += l; + } /* Get feature count */ c = strtoul(*f, &e, 10); - if (*f == e || (*e != ' ' && *e != '\0')) { + if (*f == e || (!isspace(*e) && *e != '\0')) { condlog(0, "parse error in feature string \"%s\"", *f); return 1; } - - /* Add 1 digit and 1 space */ - l = strlen(e) + strlen(n) + 2; - - c++; - /* Check if we need more digits for feature count */ - for (d = c; d >= 10; d /= 10) - l++; - - t = calloc(1, l + 1); - if (!t) + c += d; + if (asprintf(&t, "%0d%s %s", c, e, n) < 0) return 1; - /* e: old feature string with leading space, or "" */ - if (*e == ' ') - while (*(e + 1) == ' ') - e++; - - snprintf(t, l + 1, "%0d%s %s", c, e, n); - free(*f); *f = t;
add_feature() didn't correctly handle feature strings that used whitespace other than spaces, which the kernel allows. It also didn't allow adding features with multiple tokens. When it looked to see if the feature string to be added already existed, it didn't check if the match was part of a larger token. Finally, it did unnecessary work. By using asprintf() to create the string, the function can be signifcantly simplified. Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> --- libmultipath/structs.c | 49 +++++++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 25 deletions(-)