| Message ID | 1633550663-25571-7-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: > 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 <bmarzins@redhat.com> 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
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 <bmarzins@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. 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
On Thu, Nov 04, 2021 at 08:34:20PM +0000, 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 <bmarzins@redhat.com> > > 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. > But they don't need to exist, since the multipath code will create the missing directories. > > > + > > +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? It seems valid to set the directory to look in for extra multipath configuration files, and not currently have that directory exist. You may be setting things up for later, in case you happen to need a drop-in config file. -Ben > > 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
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 <bmarzins@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
On Fri, 2021-11-05 at 12:45 -0500, Benjamin Marzinski wrote: > > > > 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. Not impossible, but frankly, that sort of setup would be typical for the mid-2000's. Nowadays people would rather use something like ansible to distribute the configuration. (*) For us as distribution maintainers, these arbitrarily configurable paths are a nightmare. Regards Martin (In my testing and playing-around routine, I actually rely on the fact that it's possible to set a config_dir that doesn't exist. Under normal operation, it doesn't - if I want to experiment, I just move something there. But I think I could easily live with built-time settings). -- 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 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; +} + +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); } 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) /*
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 <bmarzins@redhat.com> --- libmultipath/dict.c | 88 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 78 insertions(+), 10 deletions(-)