| Message ID | 1636592780-20391-8-git-send-email-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | improving config parsing warnings | expand |
On Wed, 2021-11-10 at 19:06 -0600, Benjamin Marzinski wrote: > Having multipath able to select pathnames for the files and > directories > it needs causes unnecessary maintainer headaches. Mark these as > deprecated, but still support them for now. > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> I would have preferred to start ignoring these options right away (spitting out warnings). After all, this is upstream, we don't have to take care of long-term-support users (distros can keep the old behavior if they want), and experience shows that depreciation warnings are ignored until the deprecated feature is actually removed. But if you prefer doing it this way, fine. Let's agree to remove these options soon, i.e. with the official release after the next one (0.8.9, presumably). > --- > libmultipath/dict.c | 40 +++++++++++++++++++++++++++++------- > -- > multipath/multipath.conf.5 | 5 +++++ > 2 files changed, 36 insertions(+), 9 deletions(-) > > diff --git a/libmultipath/dict.c b/libmultipath/dict.c > index 149d3348..1b4e1106 100644 > --- a/libmultipath/dict.c > +++ b/libmultipath/dict.c > @@ -268,6 +268,15 @@ def_ ## option ## _handler (struct config *conf, > vector strvec, \ > return function (strvec, &conf->option, file, > line_nr); \ > } > > +#define declare_def_warn_handler(option, > function) \ > +static > int \ > +def_ ## option ## _handler (struct config *conf, vector > strvec, \ > + const char *file, int > line_nr) \ > +{ > \ > + condlog(2, "%s line %d, \"" #option "\" is deprecated and > will be disabled in a future release", file, > line_nr); \ > + return function (strvec, &conf->option, file, > line_nr); \ > +} > + > #define declare_def_range_handler(option, minval, > maxval) \ > static > int \ > def_ ## option ## _handler (struct config *conf, vector > strvec, \ > @@ -284,6 +293,17 @@ snprint_def_ ## option (struct config *conf, > struct strbuf *buff, \ > return function(buff, conf- > >option); \ > } > > +#define declare_def_snprint_non_defstr(option, function, > value) \ > +static > int \ > +snprint_def_ ## option (struct config *conf, struct strbuf > *buff, \ > + const void > *data) \ > +{ > \ > + static const char *s = > value; \ > + if (!conf->option || strcmp(conf->option, s) == > 0) \ > + return > 0; \ > + return function(buff, conf- > >option); \ > +} I'd actually print the value here, even if it's empty or equal to the default. That would be helpful in the future too, when these options are removed. This way customers can verify the settings multipathd is using by default. Regards, Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Thu, Nov 11, 2021 at 11:44:44AM +0000, Martin Wilck wrote: > On Wed, 2021-11-10 at 19:06 -0600, Benjamin Marzinski wrote: > > Having multipath able to select pathnames for the files and > > directories > > it needs causes unnecessary maintainer headaches. Mark these as > > deprecated, but still support them for now. > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > I would have preferred to start ignoring these options right away > (spitting out warnings). After all, this is upstream, we don't have to > take care of long-term-support users (distros can keep the old behavior > if they want), and experience shows that depreciation warnings are > ignored until the deprecated feature is actually removed. But if you > prefer doing it this way, fine. > > Let's agree to remove these options soon, i.e. with the official > release after the next one (0.8.9, presumably). Sure. Thanks. > > > --- > > libmultipath/dict.c | 40 +++++++++++++++++++++++++++++------- > > -- > > multipath/multipath.conf.5 | 5 +++++ > > 2 files changed, 36 insertions(+), 9 deletions(-) > > > > diff --git a/libmultipath/dict.c b/libmultipath/dict.c > > index 149d3348..1b4e1106 100644 > > --- a/libmultipath/dict.c > > +++ b/libmultipath/dict.c > > @@ -268,6 +268,15 @@ def_ ## option ## _handler (struct config *conf, > > vector strvec, \ > > return function (strvec, &conf->option, file, > > line_nr); \ > > } > > > > +#define declare_def_warn_handler(option, > > function) \ > > +static > > int \ > > +def_ ## option ## _handler (struct config *conf, vector > > strvec, \ > > + const char *file, int > > line_nr) \ > > +{ > > \ > > + condlog(2, "%s line %d, \"" #option "\" is deprecated and > > will be disabled in a future release", file, > > line_nr); \ > > + return function (strvec, &conf->option, file, > > line_nr); \ > > +} > > + > > #define declare_def_range_handler(option, minval, > > maxval) \ > > static > > int \ > > def_ ## option ## _handler (struct config *conf, vector > > strvec, \ > > @@ -284,6 +293,17 @@ snprint_def_ ## option (struct config *conf, > > struct strbuf *buff, \ > > return function(buff, conf- > > >option); \ > > } > > > > +#define declare_def_snprint_non_defstr(option, function, > > value) \ > > +static > > int \ > > +snprint_def_ ## option (struct config *conf, struct strbuf > > *buff, \ > > + const void > > *data) \ > > +{ > > \ > > + static const char *s = > > value; \ > > + if (!conf->option || strcmp(conf->option, s) == > > 0) \ > > + return > > 0; \ > > + return function(buff, conf- > > >option); \ > > +} > > I'd actually print the value here, even if it's empty or equal to the > default. That would be helpful in the future too, when these options > are removed. This way customers can verify the settings multipathd is > using by default. Sure. -Ben > Regards, > Martin > > -- 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 149d3348..1b4e1106 100644 --- a/libmultipath/dict.c +++ b/libmultipath/dict.c @@ -268,6 +268,15 @@ def_ ## option ## _handler (struct config *conf, vector strvec, \ return function (strvec, &conf->option, file, line_nr); \ } +#define declare_def_warn_handler(option, function) \ +static int \ +def_ ## option ## _handler (struct config *conf, vector strvec, \ + const char *file, int line_nr) \ +{ \ + condlog(2, "%s line %d, \"" #option "\" is deprecated and will be disabled in a future release", file, line_nr); \ + return function (strvec, &conf->option, file, line_nr); \ +} + #define declare_def_range_handler(option, minval, maxval) \ static int \ def_ ## option ## _handler (struct config *conf, vector strvec, \ @@ -284,6 +293,17 @@ snprint_def_ ## option (struct config *conf, struct strbuf *buff, \ return function(buff, conf->option); \ } +#define declare_def_snprint_non_defstr(option, function, value) \ +static int \ +snprint_def_ ## option (struct config *conf, struct strbuf *buff, \ + const void *data) \ +{ \ + static const char *s = value; \ + if (!conf->option || strcmp(conf->option, s) == 0) \ + return 0; \ + return function(buff, conf->option); \ +} + #define declare_def_snprint_defint(option, function, value) \ static int \ snprint_def_ ## option (struct config *conf, struct strbuf *buff, \ @@ -421,8 +441,8 @@ 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_dir) -declare_def_snprint(multipath_dir, print_str) +declare_def_warn_handler(multipath_dir, set_dir) +declare_def_snprint_non_defstr(multipath_dir, print_str, DEFAULT_MULTIPATHDIR) static int def_partition_delim_handler(struct config *conf, vector strvec, const char *file, int line_nr) @@ -654,14 +674,14 @@ 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_path) -declare_def_snprint(bindings_file, print_str) +declare_def_warn_handler(bindings_file, set_path) +declare_def_snprint_non_defstr(bindings_file, print_str, DEFAULT_BINDINGS_FILE) -declare_def_handler(wwids_file, set_path) -declare_def_snprint(wwids_file, print_str) +declare_def_warn_handler(wwids_file, set_path) +declare_def_snprint_non_defstr(wwids_file, print_str, DEFAULT_WWIDS_FILE) -declare_def_handler(prkeys_file, set_path) -declare_def_snprint(prkeys_file, print_str) +declare_def_warn_handler(prkeys_file, set_path) +declare_def_snprint_non_defstr(prkeys_file, print_str, DEFAULT_PRKEYS_FILE) declare_def_handler(retain_hwhandler, set_yes_no_undef) declare_def_snprint_defint(retain_hwhandler, print_yes_no_undef, @@ -760,9 +780,11 @@ 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; + condlog(2, "%s line %d, \"config_dir\" is deprecated and will be disabled in a future release", + file, line_nr); return set_path(strvec, &conf->config_dir, file, line_nr); } -declare_def_snprint(config_dir, print_str) +declare_def_snprint_non_defstr(config_dir, print_str, DEFAULT_CONFIG_DIR) #define declare_def_attr_handler(option, function) \ static int \ diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5 index 42a15ffd..17771e27 100644 --- a/multipath/multipath.conf.5 +++ b/multipath/multipath.conf.5 @@ -178,6 +178,7 @@ The default is: \fBno\fR . .TP .B multipath_dir +This option is deprecated, and will be removed in a future release. Directory where the dynamic shared objects are stored. Defined at compile time, commonly \fI/lib64/multipath/\fR or \fI/lib/multipath/\fR. .RS @@ -742,6 +743,7 @@ The default is: \fB<unset>\fR . .TP .B bindings_file +This option is deprecated, and will be removed in a future release. The full pathname of the binding file to be used when the user_friendly_names option is set. .RS @@ -752,6 +754,7 @@ The default is: \fB/etc/multipath/bindings\fR . .TP .B wwids_file +This option is deprecated, and will be removed in a future release. The full pathname of the WWIDs file, which is used by multipath to keep track of the WWIDs for LUNs it has created multipath devices on in the past. .RS @@ -762,6 +765,7 @@ The default is: \fB/etc/multipath/wwids\fR . .TP .B prkeys_file +This option is deprecated, and will be removed in a future release. The full pathname of the prkeys file, which is used by multipathd to keep track of the persistent reservation key used for a specific WWID, when \fIreservation_key\fR is set to \fBfile\fR. @@ -933,6 +937,7 @@ The default is: \fB<unset>\fR . .TP .B config_dir +This option is deprecated, and will be removed in a future release. If set to anything other than "", multipath will search this directory alphabetically for file ending in ".conf" and it will read configuration information from them, just as if it was in \fI/etc/multipath.conf\fR.
Having multipath able to select pathnames for the files and directories it needs causes unnecessary maintainer headaches. Mark these as deprecated, but still support them for now. Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> --- libmultipath/dict.c | 40 +++++++++++++++++++++++++++++--------- multipath/multipath.conf.5 | 5 +++++ 2 files changed, 36 insertions(+), 9 deletions(-)