| Message ID | 1633550663-25571-8-git-send-email-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | improving config parsing warnings | expand |
On Wed, 2021-10-06 at 15:04 -0500, Benjamin Marzinski wrote: > Split the code that does the actual value parsing out of set_int(), > into > a helper function, do_set_int(), so that it can be used by other > handlers. These functions no longer set the config value at all, when > they have invalid input. Not sure about that, do_set_int() sets the value to the cap (see below) > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > --- > libmultipath/dict.c | 82 +++++++++++++++++++++++++------------------ > -- > 1 file changed, 46 insertions(+), 36 deletions(-) > > diff --git a/libmultipath/dict.c b/libmultipath/dict.c > index 91333068..e79fcdd7 100644 > --- a/libmultipath/dict.c > +++ b/libmultipath/dict.c > @@ -31,17 +31,12 @@ > #include "strbuf.h" > > static int > -set_int(vector strvec, void *ptr, int min, int max, const char > *file, > - int line_nr) > +do_set_int(vector strvec, void *ptr, int min, int max, const char > *file, > + int line_nr, char *buff) > { > int *int_ptr = (int *)ptr; > - char *buff, *eptr; > + char *eptr; > long res; > - int rc; > - > - buff = set_value(strvec); > - if (!buff) > - return 1; > > res = strtol(buff, &eptr, 10); > if (eptr > buff) > @@ -50,17 +45,30 @@ set_int(vector strvec, void *ptr, int min, int > max, const char *file, > if (*buff == '\0' || *eptr != '\0') { > condlog(1, "%s line %d, invalid value for %s: > \"%s\"", > file, line_nr, (char*)VECTOR_SLOT(strvec, 0), > buff); > - rc = 1; > - } else { > - if (res > max || res < min) { > - res = (res > max) ? max : min; > - condlog(1, "%s line %d, value for %s too %s, > capping at %ld", > + return 1; > + } > + if (res > max || res < min) { > + res = (res > max) ? max : min; > + condlog(1, "%s line %d, value for %s too %s, capping > at %ld", > file, line_nr, (char*)VECTOR_SLOT(strvec, 0), > - (res == max)? "large" : "small", res); > - } > - rc = 0; > - *int_ptr = res; > + (res == max)? "large" : "small", res); > } > + *int_ptr = res; > + return 0; > +} > + > +static int > +set_int(vector strvec, void *ptr, int min, int max, const char > *file, > + int line_nr) > +{ > + char *buff; > + int rc; > + > + buff = set_value(strvec); > + if (!buff) > + return 1; > + > + rc = do_set_int(strvec, ptr, min, max, file, line_nr, buff); > > FREE(buff); > return rc; > @@ -918,6 +926,7 @@ declare_mp_attr_snprint(gid, print_gid) > static int > set_undef_off_zero(vector strvec, void *ptr, const char *file, int > line_nr) > { > + int rc = 0; > char * buff; > int *int_ptr = (int *)ptr; > > @@ -927,10 +936,10 @@ set_undef_off_zero(vector strvec, void *ptr, > const char *file, int line_nr) > > if (strcmp(buff, "off") == 0) > *int_ptr = UOZ_OFF; > - else if (sscanf(buff, "%d", int_ptr) != 1 || > - *int_ptr < UOZ_ZERO) > - *int_ptr = UOZ_UNDEF; > - else if (*int_ptr == 0) > + else > + rc = do_set_int(strvec, int_ptr, 0, INT_MAX, file, > line_nr, > + buff); > + if (rc == 0 && *int_ptr == 0) > *int_ptr = UOZ_ZERO; > > FREE(buff); > @@ -1082,14 +1091,12 @@ max_fds_handler(struct config *conf, vector > strvec, const char *file, > /* Assume safe limit */ > max_fds = 4096; > } > - if (strlen(buff) == 3 && > - !strcmp(buff, "max")) > - conf->max_fds = max_fds; > - else > - conf->max_fds = atoi(buff); > - > - if (conf->max_fds > max_fds) > + if (!strcmp(buff, "max")) { > conf->max_fds = max_fds; > + r = 0; > + } else > + r = do_set_int(strvec, &conf->max_fds, 0, max_fds, > file, > + line_nr, buff); > > FREE(buff); > > @@ -1158,6 +1165,7 @@ declare_mp_snprint(rr_weight, print_rr_weight) > static int > set_pgfailback(vector strvec, void *ptr, const char *file, int > line_nr) > { > + int rc = 0; > int *int_ptr = (int *)ptr; > char * buff; > > @@ -1172,11 +1180,11 @@ set_pgfailback(vector strvec, void *ptr, > const char *file, int line_nr) > else if (strlen(buff) == 10 && !strcmp(buff, "followover")) > *int_ptr = -FAILBACK_FOLLOWOVER; > else > - *int_ptr = atoi(buff); > + rc = do_set_int(strvec, ptr, 0, INT_MAX, file, > line_nr, buff); > > FREE(buff); > > - return 0; > + return rc; > } > > int > @@ -1208,6 +1216,7 @@ declare_mp_snprint(pgfailback, > print_pgfailback) > static int > no_path_retry_helper(vector strvec, void *ptr, const char *file, int > line_nr) > { > + int rc = 0; > int *int_ptr = (int *)ptr; > char * buff; > > @@ -1219,11 +1228,11 @@ no_path_retry_helper(vector strvec, void > *ptr, const char *file, int line_nr) > *int_ptr = NO_PATH_RETRY_FAIL; > else if (!strcmp(buff, "queue")) > *int_ptr = NO_PATH_RETRY_QUEUE; > - else if ((*int_ptr = atoi(buff)) < 1) > - *int_ptr = NO_PATH_RETRY_UNDEF; > + else > + rc = do_set_int(strvec, ptr, 1, INT_MAX, file, > line_nr, buff); This will set no_path_retry to 1 if the input was something like "0 " or a negative value. The previous code would have set NO_PATH_RETRY_UNDEF (== 0). That's a semantic change, as the code checks for NO_PATH_RETRY_UNDEF in various places. Was this intentional? > > FREE(buff); > - return 0; > + return rc; > } > > int > @@ -1365,6 +1374,7 @@ snprint_mp_reservation_key (struct config > *conf, struct strbuf *buff, > static int > set_off_int_undef(vector strvec, void *ptr, const char *file, int > line_nr) > { > + int rc =0; > int *int_ptr = (int *)ptr; > char * buff; > > @@ -1374,11 +1384,11 @@ set_off_int_undef(vector strvec, void *ptr, > const char *file, int line_nr) > > if (!strcmp(buff, "no") || !strcmp(buff, "0")) > *int_ptr = NU_NO; > - else if ((*int_ptr = atoi(buff)) < 1) > - *int_ptr = NU_UNDEF; > + else > + rc = do_set_int(strvec, ptr, 1, INT_MAX, file, > line_nr, buff); Likewise, you'd set 1 here for negative input or "0 ", while previously the result would be NU_UNDEF == 0. Negative values are of course garbage and I'm unsure if trailing spaces can occur at this point in the code, but do_set_int() handles them. Regards, Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Thu, Nov 04, 2021 at 08:54:11PM +0000, Martin Wilck wrote: > On Wed, 2021-10-06 at 15:04 -0500, Benjamin Marzinski wrote: > > Split the code that does the actual value parsing out of set_int(), > > into > > a helper function, do_set_int(), so that it can be used by other > > handlers. These functions no longer set the config value at all, when > > they have invalid input. > > Not sure about that, do_set_int() sets the value to the cap (see below) Sorry for the confustion. That's not what I meant. I meant that if do_set_int() returns failure, we won't override the existing value in the config. > > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > --- > > libmultipath/dict.c | 82 +++++++++++++++++++++++++------------------ > > -- > > 1 file changed, 46 insertions(+), 36 deletions(-) > > > > diff --git a/libmultipath/dict.c b/libmultipath/dict.c > > index 91333068..e79fcdd7 100644 > > --- a/libmultipath/dict.c > > +++ b/libmultipath/dict.c > > @@ -31,17 +31,12 @@ > > #include "strbuf.h" > > > > static int > > -set_int(vector strvec, void *ptr, int min, int max, const char > > *file, > > - int line_nr) > > +do_set_int(vector strvec, void *ptr, int min, int max, const char > > *file, > > + int line_nr, char *buff) > > { > > int *int_ptr = (int *)ptr; > > - char *buff, *eptr; > > + char *eptr; > > long res; > > - int rc; > > - > > - buff = set_value(strvec); > > - if (!buff) > > - return 1; > > > > res = strtol(buff, &eptr, 10); > > if (eptr > buff) > > @@ -50,17 +45,30 @@ set_int(vector strvec, void *ptr, int min, int > > max, const char *file, > > if (*buff == '\0' || *eptr != '\0') { > > condlog(1, "%s line %d, invalid value for %s: > > \"%s\"", > > file, line_nr, (char*)VECTOR_SLOT(strvec, 0), > > buff); > > - rc = 1; > > - } else { > > - if (res > max || res < min) { > > - res = (res > max) ? max : min; > > - condlog(1, "%s line %d, value for %s too %s, > > capping at %ld", > > + return 1; > > + } > > + if (res > max || res < min) { > > + res = (res > max) ? max : min; > > + condlog(1, "%s line %d, value for %s too %s, capping > > at %ld", > > file, line_nr, (char*)VECTOR_SLOT(strvec, 0), > > - (res == max)? "large" : "small", res); > > - } > > - rc = 0; > > - *int_ptr = res; > > + (res == max)? "large" : "small", res); > > } > > + *int_ptr = res; > > + return 0; > > +} > > + > > +static int > > +set_int(vector strvec, void *ptr, int min, int max, const char > > *file, > > + int line_nr) > > +{ > > + char *buff; > > + int rc; > > + > > + buff = set_value(strvec); > > + if (!buff) > > + return 1; > > + > > + rc = do_set_int(strvec, ptr, min, max, file, line_nr, buff); > > > > FREE(buff); > > return rc; > > @@ -918,6 +926,7 @@ declare_mp_attr_snprint(gid, print_gid) > > static int > > set_undef_off_zero(vector strvec, void *ptr, const char *file, int > > line_nr) > > { > > + int rc = 0; > > char * buff; > > int *int_ptr = (int *)ptr; > > > > @@ -927,10 +936,10 @@ set_undef_off_zero(vector strvec, void *ptr, > > const char *file, int line_nr) > > > > if (strcmp(buff, "off") == 0) > > *int_ptr = UOZ_OFF; > > - else if (sscanf(buff, "%d", int_ptr) != 1 || > > - *int_ptr < UOZ_ZERO) > > - *int_ptr = UOZ_UNDEF; > > - else if (*int_ptr == 0) > > + else > > + rc = do_set_int(strvec, int_ptr, 0, INT_MAX, file, > > line_nr, > > + buff); > > + if (rc == 0 && *int_ptr == 0) > > *int_ptr = UOZ_ZERO; > > > > FREE(buff); > > @@ -1082,14 +1091,12 @@ max_fds_handler(struct config *conf, vector > > strvec, const char *file, > > /* Assume safe limit */ > > max_fds = 4096; > > } > > - if (strlen(buff) == 3 && > > - !strcmp(buff, "max")) > > - conf->max_fds = max_fds; > > - else > > - conf->max_fds = atoi(buff); > > - > > - if (conf->max_fds > max_fds) > > + if (!strcmp(buff, "max")) { > > conf->max_fds = max_fds; > > + r = 0; > > + } else > > + r = do_set_int(strvec, &conf->max_fds, 0, max_fds, > > file, > > + line_nr, buff); > > > > FREE(buff); > > > > @@ -1158,6 +1165,7 @@ declare_mp_snprint(rr_weight, print_rr_weight) > > static int > > set_pgfailback(vector strvec, void *ptr, const char *file, int > > line_nr) > > { > > + int rc = 0; > > int *int_ptr = (int *)ptr; > > char * buff; > > > > @@ -1172,11 +1180,11 @@ set_pgfailback(vector strvec, void *ptr, > > const char *file, int line_nr) > > else if (strlen(buff) == 10 && !strcmp(buff, "followover")) > > *int_ptr = -FAILBACK_FOLLOWOVER; > > else > > - *int_ptr = atoi(buff); > > + rc = do_set_int(strvec, ptr, 0, INT_MAX, file, > > line_nr, buff); > > > > FREE(buff); > > > > - return 0; > > + return rc; > > } > > > > int > > @@ -1208,6 +1216,7 @@ declare_mp_snprint(pgfailback, > > print_pgfailback) > > static int > > no_path_retry_helper(vector strvec, void *ptr, const char *file, int > > line_nr) > > { > > + int rc = 0; > > int *int_ptr = (int *)ptr; > > char * buff; > > > > @@ -1219,11 +1228,11 @@ no_path_retry_helper(vector strvec, void > > *ptr, const char *file, int line_nr) > > *int_ptr = NO_PATH_RETRY_FAIL; > > else if (!strcmp(buff, "queue")) > > *int_ptr = NO_PATH_RETRY_QUEUE; > > - else if ((*int_ptr = atoi(buff)) < 1) > > - *int_ptr = NO_PATH_RETRY_UNDEF; > > + else > > + rc = do_set_int(strvec, ptr, 1, INT_MAX, file, > > line_nr, buff); > > This will set no_path_retry to 1 if the input was something like "0 " > or a negative value. The previous code would have set > NO_PATH_RETRY_UNDEF (== 0). That's a semantic change, as the code > checks for NO_PATH_RETRY_UNDEF in various places. Was this intentional? > Not completely. I do think that is makes sense not to change the existing value if the input is invalid. I admit that I didn't think about the fact that "0 " wouldn't be the same as "0". It certainly makes sense to change this so that do_set_int() accepts 0, and then we can handle 0 afterwards. It might also make sense in some cases to simply treat values outside the range as invalid, instead of capping them. Thoughts? > > > > FREE(buff); > > - return 0; > > + return rc; > > } > > > > int > > @@ -1365,6 +1374,7 @@ snprint_mp_reservation_key (struct config > > *conf, struct strbuf *buff, > > static int > > set_off_int_undef(vector strvec, void *ptr, const char *file, int > > line_nr) > > { > > + int rc =0; > > int *int_ptr = (int *)ptr; > > char * buff; > > > > @@ -1374,11 +1384,11 @@ set_off_int_undef(vector strvec, void *ptr, > > const char *file, int line_nr) > > > > if (!strcmp(buff, "no") || !strcmp(buff, "0")) > > *int_ptr = NU_NO; > > - else if ((*int_ptr = atoi(buff)) < 1) > > - *int_ptr = NU_UNDEF; > > + else > > + rc = do_set_int(strvec, ptr, 1, INT_MAX, file, > > line_nr, buff); > > Likewise, you'd set 1 here for negative input or "0 ", while > previously the result would be NU_UNDEF == 0. > > Negative values are of course garbage and I'm unsure if trailing spaces > can occur at this point in the code, but do_set_int() handles them. Same here. -Ben > Regards, > Martin > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Fri, 2021-11-05 at 13:08 -0500, Benjamin Marzinski wrote: > On Thu, Nov 04, 2021 at 08:54:11PM +0000, Martin Wilck wrote: > > On Wed, 2021-10-06 at 15:04 -0500, Benjamin Marzinski wrote: > > > Split the code that does the actual value parsing out of > > > set_int(), > > > into > > > a helper function, do_set_int(), so that it can be used by other > > > handlers. These functions no longer set the config value at all, > > > when > > > they have invalid input. > > > > Not sure about that, do_set_int() sets the value to the cap (see > > below) > > Sorry for the confustion. That's not what I meant. I meant that if > do_set_int() returns failure, we won't override the existing value in > the config. > > > > > > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > > --- > > > libmultipath/dict.c | 82 +++++++++++++++++++++++++-------------- > > > ---- > > > -- > > > 1 file changed, 46 insertions(+), 36 deletions(-) > > > > > > diff --git a/libmultipath/dict.c b/libmultipath/dict.c > > > index 91333068..e79fcdd7 100644 > > > --- a/libmultipath/dict.c > > > +++ b/libmultipath/dict.c > > > @@ -31,17 +31,12 @@ > > > > > > > > > @@ -1219,11 +1228,11 @@ no_path_retry_helper(vector strvec, void > > > *ptr, const char *file, int line_nr) > > > *int_ptr = NO_PATH_RETRY_FAIL; > > > else if (!strcmp(buff, "queue")) > > > *int_ptr = NO_PATH_RETRY_QUEUE; > > > - else if ((*int_ptr = atoi(buff)) < 1) > > > - *int_ptr = NO_PATH_RETRY_UNDEF; > > > + else > > > + rc = do_set_int(strvec, ptr, 1, INT_MAX, file, > > > line_nr, buff); > > > > This will set no_path_retry to 1 if the input was something like > > "0 " > > or a negative value. The previous code would have set > > NO_PATH_RETRY_UNDEF (== 0). That's a semantic change, as the code > > checks for NO_PATH_RETRY_UNDEF in various places. Was this > > intentional? > > > > Not completely. I do think that is makes sense not to change the > existing value if the input is invalid. I admit that I didn't think > about the fact that "0 " wouldn't be the same as "0". It certainly > makes sense to change this so that do_set_int() accepts 0, and then > we > can handle 0 afterwards. > > It might also make sense in some cases to simply treat values outside > the range as invalid, instead of capping them. Thoughts? I don't think it matters much for users. After all, we're talking about corner cases, which are most likely configuration errors or typos. For us and for other future code readers and maintainers, it's important to easily understand what value the code will fall back to if it receives invalid input, without having to search through the code base. In general I agree that not doing anything in this case is a good strategy. I _think_ that it actually comes down to the same thing, as NO_PATH_RETRY_UNDEF is the default setting, which will have been set in _init_config() beforehand. But although I know the code quite well, I wasn't 100% positive if this is always guaranteed. Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Wed, 2021-10-06 at 15:04 -0500, Benjamin Marzinski wrote: > Split the code that does the actual value parsing out of set_int(), > into > a helper function, do_set_int(), so that it can be used by other > handlers. These functions no longer set the config value at all, when > they have invalid input. > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> After discussion on v2 of this patch: Reviewed-by: Martin Wilck <mwilck@suse.com> -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
diff --git a/libmultipath/dict.c b/libmultipath/dict.c index 91333068..e79fcdd7 100644 --- a/libmultipath/dict.c +++ b/libmultipath/dict.c @@ -31,17 +31,12 @@ #include "strbuf.h" static int -set_int(vector strvec, void *ptr, int min, int max, const char *file, - int line_nr) +do_set_int(vector strvec, void *ptr, int min, int max, const char *file, + int line_nr, char *buff) { int *int_ptr = (int *)ptr; - char *buff, *eptr; + char *eptr; long res; - int rc; - - buff = set_value(strvec); - if (!buff) - return 1; res = strtol(buff, &eptr, 10); if (eptr > buff) @@ -50,17 +45,30 @@ set_int(vector strvec, void *ptr, int min, int max, const char *file, if (*buff == '\0' || *eptr != '\0') { condlog(1, "%s line %d, invalid value for %s: \"%s\"", file, line_nr, (char*)VECTOR_SLOT(strvec, 0), buff); - rc = 1; - } else { - if (res > max || res < min) { - res = (res > max) ? max : min; - condlog(1, "%s line %d, value for %s too %s, capping at %ld", + return 1; + } + if (res > max || res < min) { + res = (res > max) ? max : min; + condlog(1, "%s line %d, value for %s too %s, capping at %ld", file, line_nr, (char*)VECTOR_SLOT(strvec, 0), - (res == max)? "large" : "small", res); - } - rc = 0; - *int_ptr = res; + (res == max)? "large" : "small", res); } + *int_ptr = res; + return 0; +} + +static int +set_int(vector strvec, void *ptr, int min, int max, const char *file, + int line_nr) +{ + char *buff; + int rc; + + buff = set_value(strvec); + if (!buff) + return 1; + + rc = do_set_int(strvec, ptr, min, max, file, line_nr, buff); FREE(buff); return rc; @@ -918,6 +926,7 @@ declare_mp_attr_snprint(gid, print_gid) static int set_undef_off_zero(vector strvec, void *ptr, const char *file, int line_nr) { + int rc = 0; char * buff; int *int_ptr = (int *)ptr; @@ -927,10 +936,10 @@ set_undef_off_zero(vector strvec, void *ptr, const char *file, int line_nr) if (strcmp(buff, "off") == 0) *int_ptr = UOZ_OFF; - else if (sscanf(buff, "%d", int_ptr) != 1 || - *int_ptr < UOZ_ZERO) - *int_ptr = UOZ_UNDEF; - else if (*int_ptr == 0) + else + rc = do_set_int(strvec, int_ptr, 0, INT_MAX, file, line_nr, + buff); + if (rc == 0 && *int_ptr == 0) *int_ptr = UOZ_ZERO; FREE(buff); @@ -1082,14 +1091,12 @@ max_fds_handler(struct config *conf, vector strvec, const char *file, /* Assume safe limit */ max_fds = 4096; } - if (strlen(buff) == 3 && - !strcmp(buff, "max")) - conf->max_fds = max_fds; - else - conf->max_fds = atoi(buff); - - if (conf->max_fds > max_fds) + if (!strcmp(buff, "max")) { conf->max_fds = max_fds; + r = 0; + } else + r = do_set_int(strvec, &conf->max_fds, 0, max_fds, file, + line_nr, buff); FREE(buff); @@ -1158,6 +1165,7 @@ declare_mp_snprint(rr_weight, print_rr_weight) static int set_pgfailback(vector strvec, void *ptr, const char *file, int line_nr) { + int rc = 0; int *int_ptr = (int *)ptr; char * buff; @@ -1172,11 +1180,11 @@ set_pgfailback(vector strvec, void *ptr, const char *file, int line_nr) else if (strlen(buff) == 10 && !strcmp(buff, "followover")) *int_ptr = -FAILBACK_FOLLOWOVER; else - *int_ptr = atoi(buff); + rc = do_set_int(strvec, ptr, 0, INT_MAX, file, line_nr, buff); FREE(buff); - return 0; + return rc; } int @@ -1208,6 +1216,7 @@ declare_mp_snprint(pgfailback, print_pgfailback) static int no_path_retry_helper(vector strvec, void *ptr, const char *file, int line_nr) { + int rc = 0; int *int_ptr = (int *)ptr; char * buff; @@ -1219,11 +1228,11 @@ no_path_retry_helper(vector strvec, void *ptr, const char *file, int line_nr) *int_ptr = NO_PATH_RETRY_FAIL; else if (!strcmp(buff, "queue")) *int_ptr = NO_PATH_RETRY_QUEUE; - else if ((*int_ptr = atoi(buff)) < 1) - *int_ptr = NO_PATH_RETRY_UNDEF; + else + rc = do_set_int(strvec, ptr, 1, INT_MAX, file, line_nr, buff); FREE(buff); - return 0; + return rc; } int @@ -1365,6 +1374,7 @@ snprint_mp_reservation_key (struct config *conf, struct strbuf *buff, static int set_off_int_undef(vector strvec, void *ptr, const char *file, int line_nr) { + int rc =0; int *int_ptr = (int *)ptr; char * buff; @@ -1374,11 +1384,11 @@ set_off_int_undef(vector strvec, void *ptr, const char *file, int line_nr) if (!strcmp(buff, "no") || !strcmp(buff, "0")) *int_ptr = NU_NO; - else if ((*int_ptr = atoi(buff)) < 1) - *int_ptr = NU_UNDEF; + else + rc = do_set_int(strvec, ptr, 1, INT_MAX, file, line_nr, buff); FREE(buff); - return 0; + return rc; } int
Split the code that does the actual value parsing out of set_int(), into a helper function, do_set_int(), so that it can be used by other handlers. These functions no longer set the config value at all, when they have invalid input. Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> --- libmultipath/dict.c | 82 +++++++++++++++++++++++++-------------------- 1 file changed, 46 insertions(+), 36 deletions(-)