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