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@xxxxxxxxxx> > > > --- > > > 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@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/dm-devel
