On Fri, Nov 05, 2021 at 06:59:11AM +0000, Martin Wilck wrote: > Hi Ben, > > On Thu, 2021-11-04 at 21:34 +0100, Martin Wilck wrote: > > On Wed, 2021-10-06 at 15:04 -0500, Benjamin Marzinski wrote: > > > multipath always requires absolute pathnames, so make sure all file > > > and > > > directory names start with a slash. Also check that the > > > directories > > > exist. Finally, some strings, like the alias, will be used in > > > paths. > > > These must not contain the slash character '/', since it is a > > > forbidden > > > character in file/directory names. This patch adds seperate > > > handlers > > > for > > > these three cases. If a config line is invalid, these handlers > > > retain > > > the existing config string, if any. > > > > > > Signed-off-by: Benjamin Marzinski <bmarz...@redhat.com> > > I've changed my mind on this one. The options for directories and paths > should be turned into buildtime options instead. If we do that, we > don't need this sort of checks any more, except the "noslash" part.
That seems reasonable. I do want to ask around a little bit to see if I can find anyone who is actually setting the directories. The only one I really worry about is "config_dir". I worry that people might do something like stick that on shared storage, to make it possible to change the multipath config on a bunch of machines in one place. -Ben > > Regards, > Martin > > > > > Mostly OK, see remarks below. I'm a bit wary that when we start this, > > we might need to do other checks as well. For example, as multipathd > > is > > running as root, we may want to check that the paths to files it > > writes > > to (bindings_file etc.) don't contain symlinks and have proper > > permissions... But that'd be another patch. > > > > Regards, > > Martin > > > > > > > --- > > > libmultipath/dict.c | 88 +++++++++++++++++++++++++++++++++++++++-- > > > -- > > > -- > > > 1 file changed, 78 insertions(+), 10 deletions(-) > > > > > > diff --git a/libmultipath/dict.c b/libmultipath/dict.c > > > index 1758bd26..91333068 100644 > > > --- a/libmultipath/dict.c > > > +++ b/libmultipath/dict.c > > > @@ -5,6 +5,8 @@ > > > * Copyright (c) 2005 Kiyoshi Ueda, NEC > > > */ > > > #include <sys/types.h> > > > +#include <sys/stat.h> > > > +#include <unistd.h> > > > #include <pwd.h> > > > #include <string.h> > > > #include "checkers.h" > > > @@ -111,6 +113,72 @@ set_str(vector strvec, void *ptr, const char > > > *file, int line_nr) > > > return 0; > > > } > > > > > > +static int > > > +set_dir(vector strvec, void *ptr, const char *file, int line_nr) > > > +{ > > > + char **str_ptr = (char **)ptr; > > > + char *old_str = *str_ptr; > > > + struct stat sb; > > > + > > > + *str_ptr = set_value(strvec); > > > + if (!*str_ptr) { > > > + free(old_str); > > > + return 1; > > > + } > > > + if ((*str_ptr)[0] != '/'){ > > > + condlog(1, "%s line %d, %s is not an absolute > > > directory path. Ignoring", file, line_nr, *str_ptr); > > > + *str_ptr = old_str; > > > + } else { > > > + if (stat(*str_ptr, &sb) == 0 && > > > S_ISDIR(sb.st_mode)) > > > + free(old_str); > > > + else { > > > + condlog(1, "%s line %d, %s is not an > > > existing > > > directory. Ignoring", file, line_nr, *str_ptr); > > > + *str_ptr = old_str; > > > + } > > > + } > > > + return 0; > > > +} > > > + > > > +static int > > > +set_path(vector strvec, void *ptr, const char *file, int line_nr) > > > +{ > > > + char **str_ptr = (char **)ptr; > > > + char *old_str = *str_ptr; > > > + > > > + *str_ptr = set_value(strvec); > > > + if (!*str_ptr) { > > > + free(old_str); > > > + return 1; > > > + } > > > + if ((*str_ptr)[0] != '/'){ > > > + condlog(1, "%s line %d, %s is not an absolute path. > > > Ignoring", > > > + file, line_nr, *str_ptr); > > > + *str_ptr = old_str; > > > + } else > > > + free(old_str); > > > + return 0; > > > +} > > > > Once you go down this route, you might as well test that the dirname > > of > > the path is an existing directory. > > > > > > > > > + > > > +static int > > > +set_str_noslash(vector strvec, void *ptr, const char *file, int > > > line_nr) > > > +{ > > > + char **str_ptr = (char **)ptr; > > > + char *old_str = *str_ptr; > > > + > > > + *str_ptr = set_value(strvec); > > > + if (!*str_ptr) { > > > + free(old_str); > > > + return 1; > > > + } > > > + if (strchr(*str_ptr, '/')) { > > > + condlog(1, "%s line %d, %s cannot contain a slash. > > > Ignoring", > > > + file, line_nr, *str_ptr); > > > + *str_ptr = old_str; > > > + } else > > > + free(old_str); > > > + return 0; > > > +} > > > + > > > static int > > > set_yes_no(vector strvec, void *ptr, const char *file, int > > > line_nr) > > > { > > > @@ -353,13 +421,13 @@ declare_def_snprint(verbosity, print_int) > > > declare_def_handler(reassign_maps, set_yes_no) > > > declare_def_snprint(reassign_maps, print_yes_no) > > > > > > -declare_def_handler(multipath_dir, set_str) > > > +declare_def_handler(multipath_dir, set_dir) > > > declare_def_snprint(multipath_dir, print_str) > > > > > > static int def_partition_delim_handler(struct config *conf, vector > > > strvec, > > > const char *file, int > > > line_nr) > > > { > > > - int rc = set_str(strvec, &conf->partition_delim, file, > > > line_nr); > > > + int rc = set_str_noslash(strvec, &conf->partition_delim, > > > file, line_nr); > > > > > > if (rc != 0) > > > return rc; > > > @@ -490,11 +558,11 @@ declare_hw_snprint(prio_name, print_str) > > > declare_mp_handler(prio_name, set_str) > > > declare_mp_snprint(prio_name, print_str) > > > > > > -declare_def_handler(alias_prefix, set_str) > > > +declare_def_handler(alias_prefix, set_str_noslash) > > > declare_def_snprint_defstr(alias_prefix, print_str, > > > DEFAULT_ALIAS_PREFIX) > > > -declare_ovr_handler(alias_prefix, set_str) > > > +declare_ovr_handler(alias_prefix, set_str_noslash) > > > declare_ovr_snprint(alias_prefix, print_str) > > > -declare_hw_handler(alias_prefix, set_str) > > > +declare_hw_handler(alias_prefix, set_str_noslash) > > > declare_hw_snprint(alias_prefix, print_str) > > > > > > declare_def_handler(prio_args, set_str) > > > @@ -586,13 +654,13 @@ declare_hw_snprint(user_friendly_names, > > > print_yes_no_undef) > > > declare_mp_handler(user_friendly_names, set_yes_no_undef) > > > declare_mp_snprint(user_friendly_names, print_yes_no_undef) > > > > > > -declare_def_handler(bindings_file, set_str) > > > +declare_def_handler(bindings_file, set_path) > > > declare_def_snprint(bindings_file, print_str) > > > > > > -declare_def_handler(wwids_file, set_str) > > > +declare_def_handler(wwids_file, set_path) > > > declare_def_snprint(wwids_file, print_str) > > > > > > -declare_def_handler(prkeys_file, set_str) > > > +declare_def_handler(prkeys_file, set_path) > > > declare_def_snprint(prkeys_file, print_str) > > > > > > declare_def_handler(retain_hwhandler, set_yes_no_undef) > > > @@ -692,7 +760,7 @@ def_config_dir_handler(struct config *conf, > > > vector strvec, const char *file, > > > /* this is only valid in the main config file */ > > > if (conf->processed_main_config) > > > return 0; > > > - return set_str(strvec, &conf->config_dir, file, line_nr); > > > + return set_path(strvec, &conf->config_dir, file, line_nr); > > > } > > > > Why not set_dir() here? > > > > > declare_def_snprint(config_dir, print_str) > > > > > > @@ -1732,7 +1800,7 @@ multipath_handler(struct config *conf, vector > > > strvec, const char *file, > > > declare_mp_handler(wwid, set_str) > > > declare_mp_snprint(wwid, print_str) > > > > > > -declare_mp_handler(alias, set_str) > > > +declare_mp_handler(alias, set_str_noslash) > > > declare_mp_snprint(alias, print_str) > > > > > > /* > > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel