* [dm-devel] [PATCH 0/8] improving config parsing warnings
@ 2021-10-06 20:04 Benjamin Marzinski
2021-10-06 20:04 ` [dm-devel] [PATCH 1/8] libmulitpath: add section name to invalid keyword output Benjamin Marzinski
` (7 more replies)
0 siblings, 8 replies; 29+ messages in thread
From: Benjamin Marzinski @ 2021-10-06 20:04 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
This is a series of patches to make multipath provide better warnings
when parsing multipath.conf. The first three patches fix small issues.
The last five patches make multipath print warning messages with
the file and line number, when it was previously either accepting
invalid values or silently changing them.
Benjamin Marzinski (8):
libmulitpath: add section name to invalid keyword output
libmultipath: use typedef for keyword handler function
libmultipath: print the correct file when parsing fails
libmultipath: pass file and line number to keyword handlers
libmultipath: make set_int take a range for valid values
libmultipath: improve checks for set_str
libmultipath: split set_int to enable reuse
libmultipath: cleanup invalid config handling
libmultipath/dict.c | 481 ++++++++++++++++++++++++++++--------------
libmultipath/parser.c | 31 +--
libmultipath/parser.h | 15 +-
3 files changed, 349 insertions(+), 178 deletions(-)
--
2.17.2
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* [dm-devel] [PATCH 1/8] libmulitpath: add section name to invalid keyword output
2021-10-06 20:04 [dm-devel] [PATCH 0/8] improving config parsing warnings Benjamin Marzinski
@ 2021-10-06 20:04 ` Benjamin Marzinski
2021-11-04 20:55 ` Martin Wilck
2021-10-06 20:04 ` [dm-devel] [PATCH 2/8] libmultipath: use typedef for keyword handler function Benjamin Marzinski
` (6 subsequent siblings)
7 siblings, 1 reply; 29+ messages in thread
From: Benjamin Marzinski @ 2021-10-06 20:04 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
If users forget the closing brace for a section in multipath.conf,
multipath has no way to detect that. When it sees the keyword at the
start of the next section, it will complain that there is an invalid
keyword, because that keyword doesn't belong in previous section (which
was never ended with a closing brace). This can confuse users. To make
this easier to understand, when multipath prints and invalid keyword
message, it now also prints the current section name, which can give
users a hint that they didn't end the previous section.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/parser.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/libmultipath/parser.c b/libmultipath/parser.c
index 8ca91bf2..611054f7 100644
--- a/libmultipath/parser.c
+++ b/libmultipath/parser.c
@@ -504,7 +504,7 @@ validate_config_strvec(vector strvec, const char *file)
static int
process_stream(struct config *conf, FILE *stream, vector keywords,
- const char *file)
+ const char *section, const char *file)
{
int i;
int r = 0, t;
@@ -568,16 +568,22 @@ process_stream(struct config *conf, FILE *stream, vector keywords,
if (keyword->sub) {
kw_level++;
r += process_stream(conf, stream,
- keyword->sub, file);
+ keyword->sub,
+ keyword->string,
+ file);
kw_level--;
}
break;
}
}
- if (i >= VECTOR_SIZE(keywords))
- condlog(1, "%s line %d, invalid keyword: %s",
- file, line_nr, str);
-
+ if (i >= VECTOR_SIZE(keywords)) {
+ if (section)
+ condlog(1, "%s line %d, invalid keyword in the %s section: %s",
+ file, line_nr, section, str);
+ else
+ condlog(1, "%s line %d, invalid keyword: %s",
+ file, line_nr, str);
+ }
free_strvec(strvec);
}
if (kw_level == 1)
@@ -608,7 +614,7 @@ process_file(struct config *conf, const char *file)
/* Stream handling */
line_nr = 0;
- r = process_stream(conf, stream, conf->keywords, file);
+ r = process_stream(conf, stream, conf->keywords, NULL, file);
fclose(stream);
//free_keywords(keywords);
--
2.17.2
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [dm-devel] [PATCH 2/8] libmultipath: use typedef for keyword handler function
2021-10-06 20:04 [dm-devel] [PATCH 0/8] improving config parsing warnings Benjamin Marzinski
2021-10-06 20:04 ` [dm-devel] [PATCH 1/8] libmulitpath: add section name to invalid keyword output Benjamin Marzinski
@ 2021-10-06 20:04 ` Benjamin Marzinski
2021-11-04 20:55 ` Martin Wilck
2021-10-06 20:04 ` [dm-devel] [PATCH 3/8] libmultipath: print the correct file when parsing fails Benjamin Marzinski
` (5 subsequent siblings)
7 siblings, 1 reply; 29+ messages in thread
From: Benjamin Marzinski @ 2021-10-06 20:04 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
Don't keep writing out the function type.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/parser.c | 6 +++---
libmultipath/parser.h | 15 ++++++---------
2 files changed, 9 insertions(+), 12 deletions(-)
diff --git a/libmultipath/parser.c b/libmultipath/parser.c
index 611054f7..ebe1cbd9 100644
--- a/libmultipath/parser.c
+++ b/libmultipath/parser.c
@@ -33,7 +33,7 @@ static int line_nr;
int
keyword_alloc(vector keywords, char *string,
- int (*handler) (struct config *, vector),
+ handler_fn *handler,
print_fn *print,
int unique)
{
@@ -72,7 +72,7 @@ install_sublevel_end(void)
int
_install_keyword(vector keywords, char *string,
- int (*handler) (struct config *, vector),
+ handler_fn *handler,
print_fn *print,
int unique)
{
@@ -558,7 +558,7 @@ process_stream(struct config *conf, FILE *stream, vector keywords,
goto out;
}
if (keyword->handler) {
- t = (*keyword->handler) (conf, strvec);
+ t = keyword->handler(conf, strvec);
r += t;
if (t)
condlog(1, "multipath.conf +%d, parsing failed: %s",
diff --git a/libmultipath/parser.h b/libmultipath/parser.h
index b43d46f8..3452bde1 100644
--- a/libmultipath/parser.h
+++ b/libmultipath/parser.h
@@ -43,10 +43,11 @@ struct strbuf;
/* keyword definition */
typedef int print_fn(struct config *, struct strbuf *, const void *);
+typedef int handler_fn(struct config *, vector);
struct keyword {
char *string;
- int (*handler) (struct config *, vector);
+ handler_fn *handler;
print_fn *print;
vector sub;
int unique;
@@ -62,18 +63,14 @@ struct keyword {
for (i = 0; i < (k)->sub->allocated && ((p) = (k)->sub->slot[i]); i++)
/* Prototypes */
-extern int keyword_alloc(vector keywords, char *string,
- int (*handler) (struct config *, vector),
- print_fn *print,
- int unique);
+extern int keyword_alloc(vector keywords, char *string, handler_fn *handler,
+ print_fn *print, int unique);
#define install_keyword_root(str, h) keyword_alloc(keywords, str, h, NULL, 1)
extern void install_sublevel(void);
extern void install_sublevel_end(void);
-extern int _install_keyword(vector keywords, char *string,
- int (*handler) (struct config *, vector),
- print_fn *print,
- int unique);
+extern int _install_keyword(vector keywords, char *string, handler_fn *handler,
+ print_fn *print, int unique);
#define install_keyword(str, vec, pri) _install_keyword(keywords, str, vec, pri, 1)
#define install_keyword_multi(str, vec, pri) _install_keyword(keywords, str, vec, pri, 0)
extern void dump_keywords(vector keydump, int level);
--
2.17.2
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [dm-devel] [PATCH 3/8] libmultipath: print the correct file when parsing fails
2021-10-06 20:04 [dm-devel] [PATCH 0/8] improving config parsing warnings Benjamin Marzinski
2021-10-06 20:04 ` [dm-devel] [PATCH 1/8] libmulitpath: add section name to invalid keyword output Benjamin Marzinski
2021-10-06 20:04 ` [dm-devel] [PATCH 2/8] libmultipath: use typedef for keyword handler function Benjamin Marzinski
@ 2021-10-06 20:04 ` Benjamin Marzinski
2021-11-04 20:55 ` Martin Wilck
2021-10-06 20:04 ` [dm-devel] [PATCH 4/8] libmultipath: pass file and line number to keyword handlers Benjamin Marzinski
` (4 subsequent siblings)
7 siblings, 1 reply; 29+ messages in thread
From: Benjamin Marzinski @ 2021-10-06 20:04 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
Don't assume that parsing failed on multipath.conf
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/parser.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libmultipath/parser.c b/libmultipath/parser.c
index ebe1cbd9..d5595fb0 100644
--- a/libmultipath/parser.c
+++ b/libmultipath/parser.c
@@ -561,8 +561,8 @@ process_stream(struct config *conf, FILE *stream, vector keywords,
t = keyword->handler(conf, strvec);
r += t;
if (t)
- condlog(1, "multipath.conf +%d, parsing failed: %s",
- line_nr, buf);
+ condlog(1, "%s line %d, parsing failed: %s",
+ file, line_nr, buf);
}
if (keyword->sub) {
--
2.17.2
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [dm-devel] [PATCH 4/8] libmultipath: pass file and line number to keyword handlers
2021-10-06 20:04 [dm-devel] [PATCH 0/8] improving config parsing warnings Benjamin Marzinski
` (2 preceding siblings ...)
2021-10-06 20:04 ` [dm-devel] [PATCH 3/8] libmultipath: print the correct file when parsing fails Benjamin Marzinski
@ 2021-10-06 20:04 ` Benjamin Marzinski
2021-11-04 20:55 ` Martin Wilck
2021-10-06 20:04 ` [dm-devel] [PATCH 5/8] libmultipath: make set_int take a range for valid values Benjamin Marzinski
` (3 subsequent siblings)
7 siblings, 1 reply; 29+ messages in thread
From: Benjamin Marzinski @ 2021-10-06 20:04 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
This will make it possible for the keyword handlers to print more useful
warning messages. It will be used by future patches.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/dict.c | 143 +++++++++++++++++++++++++-----------------
libmultipath/parser.c | 3 +-
libmultipath/parser.h | 2 +-
3 files changed, 90 insertions(+), 58 deletions(-)
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index 7a727389..eb2c44c0 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -29,7 +29,7 @@
#include "strbuf.h"
static int
-set_int(vector strvec, void *ptr)
+set_int(vector strvec, void *ptr, const char *file, int line_nr)
{
int *int_ptr = (int *)ptr;
char *buff, *eptr;
@@ -58,7 +58,7 @@ set_int(vector strvec, void *ptr)
}
static int
-set_uint(vector strvec, void *ptr)
+set_uint(vector strvec, void *ptr, const char *file, int line_nr)
{
unsigned int *uint_ptr = (unsigned int *)ptr;
char *buff, *eptr, *p;
@@ -90,7 +90,7 @@ set_uint(vector strvec, void *ptr)
}
static int
-set_str(vector strvec, void *ptr)
+set_str(vector strvec, void *ptr, const char *file, int line_nr)
{
char **str_ptr = (char **)ptr;
@@ -105,7 +105,7 @@ set_str(vector strvec, void *ptr)
}
static int
-set_yes_no(vector strvec, void *ptr)
+set_yes_no(vector strvec, void *ptr, const char *file, int line_nr)
{
char * buff;
int *int_ptr = (int *)ptr;
@@ -124,7 +124,7 @@ set_yes_no(vector strvec, void *ptr)
}
static int
-set_yes_no_undef(vector strvec, void *ptr)
+set_yes_no_undef(vector strvec, void *ptr, const char *file, int line_nr)
{
char * buff;
int *int_ptr = (int *)ptr;
@@ -187,9 +187,10 @@ static int print_yes_no_undef(struct strbuf *buff, long v)
#define declare_def_handler(option, function) \
static int \
-def_ ## option ## _handler (struct config *conf, vector strvec) \
+def_ ## option ## _handler (struct config *conf, vector strvec, \
+ const char *file, int line_nr) \
{ \
- return function (strvec, &conf->option); \
+ return function (strvec, &conf->option, file, line_nr); \
}
#define declare_def_snprint(option, function) \
@@ -224,12 +225,13 @@ snprint_def_ ## option (struct config *conf, struct strbuf *buff, \
#define declare_hw_handler(option, function) \
static int \
-hw_ ## option ## _handler (struct config *conf, vector strvec) \
+hw_ ## option ## _handler (struct config *conf, vector strvec, \
+ const char *file, int line_nr) \
{ \
struct hwentry * hwe = VECTOR_LAST_SLOT(conf->hwtable); \
if (!hwe) \
return 1; \
- return function (strvec, &hwe->option); \
+ return function (strvec, &hwe->option, file, line_nr); \
}
#define declare_hw_snprint(option, function) \
@@ -243,11 +245,12 @@ snprint_hw_ ## option (struct config *conf, struct strbuf *buff, \
#define declare_ovr_handler(option, function) \
static int \
-ovr_ ## option ## _handler (struct config *conf, vector strvec) \
+ovr_ ## option ## _handler (struct config *conf, vector strvec, \
+ const char *file, int line_nr) \
{ \
if (!conf->overrides) \
return 1; \
- return function (strvec, &conf->overrides->option); \
+ return function (strvec, &conf->overrides->option, file, line_nr); \
}
#define declare_ovr_snprint(option, function) \
@@ -260,12 +263,13 @@ snprint_ovr_ ## option (struct config *conf, struct strbuf *buff, \
#define declare_mp_handler(option, function) \
static int \
-mp_ ## option ## _handler (struct config *conf, vector strvec) \
+mp_ ## option ## _handler (struct config *conf, vector strvec, \
+ const char *file, int line_nr) \
{ \
struct mpentry * mpe = VECTOR_LAST_SLOT(conf->mptable); \
if (!mpe) \
return 1; \
- return function (strvec, &mpe->option); \
+ return function (strvec, &mpe->option, file, line_nr); \
}
#define declare_mp_snprint(option, function) \
@@ -277,9 +281,10 @@ snprint_mp_ ## option (struct config *conf, struct strbuf *buff, \
return function(buff, mpe->option); \
}
-static int checkint_handler(struct config *conf, vector strvec)
+static int checkint_handler(struct config *conf, vector strvec,
+ const char *file, int line_nr)
{
- int rc = set_uint(strvec, &conf->checkint);
+ int rc = set_uint(strvec, &conf->checkint, file, line_nr);
if (rc)
return rc;
@@ -302,9 +307,10 @@ declare_def_snprint(reassign_maps, print_yes_no)
declare_def_handler(multipath_dir, set_str)
declare_def_snprint(multipath_dir, print_str)
-static int def_partition_delim_handler(struct config *conf, vector strvec)
+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);
+ int rc = set_str(strvec, &conf->partition_delim, file, line_nr);
if (rc != 0)
return rc;
@@ -334,13 +340,13 @@ static const char * const find_multipaths_optvals[] = {
};
static int
-def_find_multipaths_handler(struct config *conf, vector strvec)
+def_find_multipaths_handler(struct config *conf, vector strvec,
+ const char *file, int line_nr)
{
char *buff;
int i;
- if (set_yes_no_undef(strvec, &conf->find_multipaths) == 0 &&
- conf->find_multipaths != FIND_MULTIPATHS_UNDEF)
+ if (set_yes_no_undef(strvec, &conf->find_multipaths, file, line_nr) == 0 && conf->find_multipaths != FIND_MULTIPATHS_UNDEF)
return 0;
buff = set_value(strvec);
@@ -396,7 +402,8 @@ static int snprint_uid_attrs(struct config *conf, struct strbuf *buff,
return total;
}
-static int uid_attrs_handler(struct config *conf, vector strvec)
+static int uid_attrs_handler(struct config *conf, vector strvec,
+ const char *file, int line_nr)
{
char *val;
@@ -597,7 +604,8 @@ declare_hw_handler(skip_kpartx, set_yes_no_undef)
declare_hw_snprint(skip_kpartx, print_yes_no_undef)
declare_mp_handler(skip_kpartx, set_yes_no_undef)
declare_mp_snprint(skip_kpartx, print_yes_no_undef)
-static int def_disable_changed_wwids_handler(struct config *conf, vector strvec)
+static int def_disable_changed_wwids_handler(struct config *conf, vector strvec,
+ const char *file, int line_nr)
{
return 0;
}
@@ -629,20 +637,23 @@ declare_def_snprint_defstr(enable_foreign, print_str,
DEFAULT_ENABLE_FOREIGN)
static int
-def_config_dir_handler(struct config *conf, vector strvec)
+def_config_dir_handler(struct config *conf, vector strvec, const char *file,
+ int line_nr)
{
/* this is only valid in the main config file */
if (conf->processed_main_config)
return 0;
- return set_str(strvec, &conf->config_dir);
+ return set_str(strvec, &conf->config_dir, file, line_nr);
}
declare_def_snprint(config_dir, print_str)
#define declare_def_attr_handler(option, function) \
static int \
-def_ ## option ## _handler (struct config *conf, vector strvec) \
+def_ ## option ## _handler (struct config *conf, vector strvec, \
+ const char *file, int line_nr) \
{ \
- return function (strvec, &conf->option, &conf->attribute_flags);\
+ return function (strvec, &conf->option, &conf->attribute_flags, \
+ file, line_nr); \
}
#define declare_def_attr_snprint(option, function) \
@@ -655,12 +666,14 @@ snprint_def_ ## option (struct config *conf, struct strbuf *buff, \
#define declare_mp_attr_handler(option, function) \
static int \
-mp_ ## option ## _handler (struct config *conf, vector strvec) \
+mp_ ## option ## _handler (struct config *conf, vector strvec, \
+ const char *file, int line_nr) \
{ \
struct mpentry * mpe = VECTOR_LAST_SLOT(conf->mptable); \
if (!mpe) \
return 1; \
- return function (strvec, &mpe->option, &mpe->attribute_flags); \
+ return function (strvec, &mpe->option, &mpe->attribute_flags, \
+ file, line_nr); \
}
#define declare_mp_attr_snprint(option, function) \
@@ -673,7 +686,7 @@ snprint_mp_ ## option (struct config *conf, struct strbuf *buff, \
}
static int
-set_mode(vector strvec, void *ptr, int *flags)
+set_mode(vector strvec, void *ptr, int *flags, const char *file, int line_nr)
{
mode_t mode;
mode_t *mode_ptr = (mode_t *)ptr;
@@ -694,7 +707,7 @@ set_mode(vector strvec, void *ptr, int *flags)
}
static int
-set_uid(vector strvec, void *ptr, int *flags)
+set_uid(vector strvec, void *ptr, int *flags, const char *file, int line_nr)
{
uid_t uid;
uid_t *uid_ptr = (uid_t *)ptr;
@@ -719,7 +732,7 @@ set_uid(vector strvec, void *ptr, int *flags)
}
static int
-set_gid(vector strvec, void *ptr, int *flags)
+set_gid(vector strvec, void *ptr, int *flags, const char *file, int line_nr)
{
gid_t gid;
gid_t *gid_ptr = (gid_t *)ptr;
@@ -786,7 +799,7 @@ declare_mp_attr_handler(gid, set_gid)
declare_mp_attr_snprint(gid, print_gid)
static int
-set_undef_off_zero(vector strvec, void *ptr)
+set_undef_off_zero(vector strvec, void *ptr, const char *file, int line_nr)
{
char * buff;
int *int_ptr = (int *)ptr;
@@ -827,7 +840,7 @@ declare_hw_handler(fast_io_fail, set_undef_off_zero)
declare_hw_snprint(fast_io_fail, print_undef_off_zero)
static int
-set_dev_loss(vector strvec, void *ptr)
+set_dev_loss(vector strvec, void *ptr, const char *file, int line_nr)
{
char * buff;
unsigned int *uint_ptr = (unsigned int *)ptr;
@@ -870,7 +883,7 @@ declare_hw_handler(eh_deadline, set_undef_off_zero)
declare_hw_snprint(eh_deadline, print_undef_off_zero)
static int
-set_pgpolicy(vector strvec, void *ptr)
+set_pgpolicy(vector strvec, void *ptr, const char *file, int line_nr)
{
char * buff;
int *int_ptr = (int *)ptr;
@@ -936,7 +949,8 @@ get_sys_max_fds(int *max_fds)
static int
-max_fds_handler(struct config *conf, vector strvec)
+max_fds_handler(struct config *conf, vector strvec, const char *file,
+ int line_nr)
{
char * buff;
int r = 0, max_fds;
@@ -981,7 +995,7 @@ snprint_max_fds (struct config *conf, struct strbuf *buff, const void *data)
}
static int
-set_rr_weight(vector strvec, void *ptr)
+set_rr_weight(vector strvec, void *ptr, const char *file, int line_nr)
{
int *int_ptr = (int *)ptr;
char * buff;
@@ -1025,7 +1039,7 @@ declare_mp_handler(rr_weight, set_rr_weight)
declare_mp_snprint(rr_weight, print_rr_weight)
static int
-set_pgfailback(vector strvec, void *ptr)
+set_pgfailback(vector strvec, void *ptr, const char *file, int line_nr)
{
int *int_ptr = (int *)ptr;
char * buff;
@@ -1075,7 +1089,7 @@ declare_mp_handler(pgfailback, set_pgfailback)
declare_mp_snprint(pgfailback, print_pgfailback)
static int
-no_path_retry_helper(vector strvec, void *ptr)
+no_path_retry_helper(vector strvec, void *ptr, const char *file, int line_nr)
{
int *int_ptr = (int *)ptr;
char * buff;
@@ -1120,7 +1134,8 @@ declare_mp_handler(no_path_retry, no_path_retry_helper)
declare_mp_snprint(no_path_retry, print_no_path_retry)
static int
-def_log_checker_err_handler(struct config *conf, vector strvec)
+def_log_checker_err_handler(struct config *conf, vector strvec,
+ const char *file, int line_nr)
{
char * buff;
@@ -1193,7 +1208,8 @@ print_reservation_key(struct strbuf *buff,
}
static int
-def_reservation_key_handler(struct config *conf, vector strvec)
+def_reservation_key_handler(struct config *conf, vector strvec,
+ const char *file, int line_nr)
{
return set_reservation_key(strvec, &conf->reservation_key,
&conf->sa_flags,
@@ -1209,7 +1225,8 @@ snprint_def_reservation_key (struct config *conf, struct strbuf *buff,
}
static int
-mp_reservation_key_handler(struct config *conf, vector strvec)
+mp_reservation_key_handler(struct config *conf, vector strvec, const char *file,
+ int line_nr)
{
struct mpentry * mpe = VECTOR_LAST_SLOT(conf->mptable);
if (!mpe)
@@ -1229,7 +1246,7 @@ snprint_mp_reservation_key (struct config *conf, struct strbuf *buff,
}
static int
-set_off_int_undef(vector strvec, void *ptr)
+set_off_int_undef(vector strvec, void *ptr, const char *file, int line_nr)
{
int *int_ptr = (int *)ptr;
char * buff;
@@ -1370,7 +1387,8 @@ declare_hw_snprint(recheck_wwid, print_yes_no_undef)
static int
-def_uxsock_timeout_handler(struct config *conf, vector strvec)
+def_uxsock_timeout_handler(struct config *conf, vector strvec, const char *file,
+ int line_nr)
{
unsigned int uxsock_timeout;
char *buff;
@@ -1390,7 +1408,8 @@ def_uxsock_timeout_handler(struct config *conf, vector strvec)
}
static int
-hw_vpd_vendor_handler(struct config *conf, vector strvec)
+hw_vpd_vendor_handler(struct config *conf, vector strvec, const char *file,
+ int line_nr)
{
int i;
char *buff;
@@ -1430,7 +1449,8 @@ snprint_hw_vpd_vendor(struct config *conf, struct strbuf *buff,
* blacklist block handlers
*/
static int
-blacklist_handler(struct config *conf, vector strvec)
+blacklist_handler(struct config *conf, vector strvec, const char*file,
+ int line_nr)
{
if (!conf->blist_devnode)
conf->blist_devnode = vector_alloc();
@@ -1452,7 +1472,8 @@ blacklist_handler(struct config *conf, vector strvec)
}
static int
-blacklist_exceptions_handler(struct config *conf, vector strvec)
+blacklist_exceptions_handler(struct config *conf, vector strvec,
+ const char *file, int line_nr)
{
if (!conf->elist_devnode)
conf->elist_devnode = vector_alloc();
@@ -1475,7 +1496,8 @@ blacklist_exceptions_handler(struct config *conf, vector strvec)
#define declare_ble_handler(option) \
static int \
-ble_ ## option ## _handler (struct config *conf, vector strvec) \
+ble_ ## option ## _handler (struct config *conf, vector strvec, \
+ const char *file, int line_nr) \
{ \
char *buff; \
int rc; \
@@ -1494,7 +1516,8 @@ ble_ ## option ## _handler (struct config *conf, vector strvec) \
#define declare_ble_device_handler(name, option, vend, prod) \
static int \
-ble_ ## option ## _ ## name ## _handler (struct config *conf, vector strvec) \
+ble_ ## option ## _ ## name ## _handler (struct config *conf, vector strvec, \
+ const char *file, int line_nr) \
{ \
char * buff; \
int rc; \
@@ -1536,13 +1559,15 @@ snprint_ble_simple (struct config *conf, struct strbuf *buff, const void *data)
}
static int
-ble_device_handler(struct config *conf, vector strvec)
+ble_device_handler(struct config *conf, vector strvec, const char *file,
+ int line_nr)
{
return alloc_ble_device(conf->blist_device);
}
static int
-ble_except_device_handler(struct config *conf, vector strvec)
+ble_except_device_handler(struct config *conf, vector strvec, const char *file,
+ int line_nr)
{
return alloc_ble_device(conf->elist_device);
}
@@ -1574,7 +1599,8 @@ static int snprint_bled_product(struct config *conf, struct strbuf *buff,
* devices block handlers
*/
static int
-devices_handler(struct config *conf, vector strvec)
+devices_handler(struct config *conf, vector strvec, const char *file,
+ int line_nr)
{
if (!conf->hwtable)
conf->hwtable = vector_alloc();
@@ -1586,7 +1612,8 @@ devices_handler(struct config *conf, vector strvec)
}
static int
-device_handler(struct config *conf, vector strvec)
+device_handler(struct config *conf, vector strvec, const char *file,
+ int line_nr)
{
struct hwentry * hwe;
@@ -1623,7 +1650,8 @@ declare_hw_snprint(hwhandler, print_str)
* overrides handlers
*/
static int
-overrides_handler(struct config *conf, vector strvec)
+overrides_handler(struct config *conf, vector strvec, const char *file,
+ int line_nr)
{
if (!conf->overrides)
conf->overrides = alloc_hwe();
@@ -1640,7 +1668,8 @@ overrides_handler(struct config *conf, vector strvec)
* multipaths block handlers
*/
static int
-multipaths_handler(struct config *conf, vector strvec)
+multipaths_handler(struct config *conf, vector strvec, const char *file,
+ int line_nr)
{
if (!conf->mptable)
conf->mptable = vector_alloc();
@@ -1652,7 +1681,8 @@ multipaths_handler(struct config *conf, vector strvec)
}
static int
-multipath_handler(struct config *conf, vector strvec)
+multipath_handler(struct config *conf, vector strvec, const char *file,
+ int line_nr)
{
struct mpentry * mpe;
@@ -1681,7 +1711,8 @@ declare_mp_snprint(alias, print_str)
*/
static int
-deprecated_handler(struct config *conf, vector strvec)
+deprecated_handler(struct config *conf, vector strvec, const char *file,
+ int line_nr)
{
char * buff;
diff --git a/libmultipath/parser.c b/libmultipath/parser.c
index d5595fb0..68262d0e 100644
--- a/libmultipath/parser.c
+++ b/libmultipath/parser.c
@@ -558,7 +558,8 @@ process_stream(struct config *conf, FILE *stream, vector keywords,
goto out;
}
if (keyword->handler) {
- t = keyword->handler(conf, strvec);
+ t = keyword->handler(conf, strvec, file,
+ line_nr);
r += t;
if (t)
condlog(1, "%s line %d, parsing failed: %s",
diff --git a/libmultipath/parser.h b/libmultipath/parser.h
index 3452bde1..11ea2278 100644
--- a/libmultipath/parser.h
+++ b/libmultipath/parser.h
@@ -43,7 +43,7 @@ struct strbuf;
/* keyword definition */
typedef int print_fn(struct config *, struct strbuf *, const void *);
-typedef int handler_fn(struct config *, vector);
+typedef int handler_fn(struct config *, vector, const char *file, int line_nr);
struct keyword {
char *string;
--
2.17.2
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [dm-devel] [PATCH 5/8] libmultipath: make set_int take a range for valid values
2021-10-06 20:04 [dm-devel] [PATCH 0/8] improving config parsing warnings Benjamin Marzinski
` (3 preceding siblings ...)
2021-10-06 20:04 ` [dm-devel] [PATCH 4/8] libmultipath: pass file and line number to keyword handlers Benjamin Marzinski
@ 2021-10-06 20:04 ` Benjamin Marzinski
2021-11-04 20:34 ` Martin Wilck
2021-10-06 20:04 ` [dm-devel] [PATCH 6/8] libmultipath: improve checks for set_str Benjamin Marzinski
` (2 subsequent siblings)
7 siblings, 1 reply; 29+ messages in thread
From: Benjamin Marzinski @ 2021-10-06 20:04 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
If a value outside of the valid range is passed to set_int, it caps the
value at appropriate limit, and issues a warning.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/dict.c | 121 +++++++++++++++++++++++++++-----------------
1 file changed, 75 insertions(+), 46 deletions(-)
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index eb2c44c0..1758bd26 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -29,7 +29,8 @@
#include "strbuf.h"
static int
-set_int(vector strvec, void *ptr, const char *file, int line_nr)
+set_int(vector strvec, void *ptr, int min, int max, const char *file,
+ int line_nr)
{
int *int_ptr = (int *)ptr;
char *buff, *eptr;
@@ -44,11 +45,17 @@ set_int(vector strvec, void *ptr, const char *file, int line_nr)
if (eptr > buff)
while (isspace(*eptr))
eptr++;
- if (*buff == '\0' || *eptr != '\0' || res > INT_MAX || res < INT_MIN) {
- condlog(1, "%s: invalid value for %s: \"%s\"",
- __func__, (char*)VECTOR_SLOT(strvec, 0), buff);
+ if (*buff == '\0' || *eptr != '\0') {
+ condlog(1, "%s line %d, invalid value for %s: \"%s\"",
+ file, line_nr, (char*)VECTOR_SLOT(strvec, 0), buff);
rc = 1;
} else {
+ if (res > max || res < min) {
+ res = (res > max) ? max : min;
+ condlog(1, "%s line %d, value for %s too %s, capping at %ld",
+ file, line_nr, (char*)VECTOR_SLOT(strvec, 0),
+ (res == max)? "large" : "small", res);
+ }
rc = 0;
*int_ptr = res;
}
@@ -77,8 +84,8 @@ set_uint(vector strvec, void *ptr, const char *file, int line_nr)
while (isspace(*eptr))
eptr++;
if (*buff == '\0' || *eptr != '\0' || !isdigit(*p) || res > UINT_MAX) {
- condlog(1, "%s: invalid value for %s: \"%s\"",
- __func__, (char*)VECTOR_SLOT(strvec, 0), buff);
+ condlog(1, "%s line %d, invalid value for %s: \"%s\"",
+ file, line_nr, (char*)VECTOR_SLOT(strvec, 0), buff);
rc = 1;
} else {
rc = 0;
@@ -193,6 +200,14 @@ def_ ## option ## _handler (struct config *conf, vector strvec, \
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, \
+ const char *file, int line_nr) \
+{ \
+ return set_int(strvec, &conf->option, minval, maxval, file, line_nr); \
+}
+
#define declare_def_snprint(option, function) \
static int \
snprint_def_ ## option (struct config *conf, struct strbuf *buff, \
@@ -234,6 +249,18 @@ hw_ ## option ## _handler (struct config *conf, vector strvec, \
return function (strvec, &hwe->option, file, line_nr); \
}
+#define declare_hw_range_handler(option, minval, maxval) \
+static int \
+hw_ ## option ## _handler (struct config *conf, vector strvec, \
+ const char *file, int line_nr) \
+{ \
+ struct hwentry * hwe = VECTOR_LAST_SLOT(conf->hwtable); \
+ if (!hwe) \
+ return 1; \
+ return set_int(strvec, &hwe->option, minval, maxval, file, line_nr); \
+}
+
+
#define declare_hw_snprint(option, function) \
static int \
snprint_hw_ ## option (struct config *conf, struct strbuf *buff, \
@@ -253,6 +280,17 @@ ovr_ ## option ## _handler (struct config *conf, vector strvec, \
return function (strvec, &conf->overrides->option, file, line_nr); \
}
+#define declare_ovr_range_handler(option, minval, maxval) \
+static int \
+ovr_ ## option ## _handler (struct config *conf, vector strvec, \
+ const char *file, int line_nr) \
+{ \
+ if (!conf->overrides) \
+ return 1; \
+ return set_int(strvec, &conf->overrides->option, minval, maxval, \
+ file, line_nr); \
+}
+
#define declare_ovr_snprint(option, function) \
static int \
snprint_ovr_ ## option (struct config *conf, struct strbuf *buff, \
@@ -272,6 +310,17 @@ mp_ ## option ## _handler (struct config *conf, vector strvec, \
return function (strvec, &mpe->option, file, line_nr); \
}
+#define declare_mp_range_handler(option, minval, maxval) \
+static int \
+mp_ ## option ## _handler (struct config *conf, vector strvec, \
+ const char *file, int line_nr) \
+{ \
+ struct mpentry * mpe = VECTOR_LAST_SLOT(conf->mptable); \
+ if (!mpe) \
+ return 1; \
+ return set_int(strvec, &mpe->option, minval, maxval, file, line_nr); \
+}
+
#define declare_mp_snprint(option, function) \
static int \
snprint_mp_ ## option (struct config *conf, struct strbuf *buff, \
@@ -298,7 +347,7 @@ declare_def_snprint(checkint, print_int)
declare_def_handler(max_checkint, set_uint)
declare_def_snprint(max_checkint, print_int)
-declare_def_handler(verbosity, set_int)
+declare_def_range_handler(verbosity, 0, 4)
declare_def_snprint(verbosity, print_int)
declare_def_handler(reassign_maps, set_yes_no)
@@ -473,22 +522,22 @@ declare_ovr_snprint(checker_name, print_str)
declare_hw_handler(checker_name, set_str)
declare_hw_snprint(checker_name, print_str)
-declare_def_handler(minio, set_int)
+declare_def_range_handler(minio, 0, INT_MAX)
declare_def_snprint_defint(minio, print_int, DEFAULT_MINIO)
-declare_ovr_handler(minio, set_int)
+declare_ovr_range_handler(minio, 0, INT_MAX)
declare_ovr_snprint(minio, print_nonzero)
-declare_hw_handler(minio, set_int)
+declare_hw_range_handler(minio, 0, INT_MAX)
declare_hw_snprint(minio, print_nonzero)
-declare_mp_handler(minio, set_int)
+declare_mp_range_handler(minio, 0, INT_MAX)
declare_mp_snprint(minio, print_nonzero)
-declare_def_handler(minio_rq, set_int)
+declare_def_range_handler(minio_rq, 0, INT_MAX)
declare_def_snprint_defint(minio_rq, print_int, DEFAULT_MINIO_RQ)
-declare_ovr_handler(minio_rq, set_int)
+declare_ovr_range_handler(minio_rq, 0, INT_MAX)
declare_ovr_snprint(minio_rq, print_nonzero)
-declare_hw_handler(minio_rq, set_int)
+declare_hw_range_handler(minio_rq, 0, INT_MAX)
declare_hw_snprint(minio_rq, print_nonzero)
-declare_mp_handler(minio_rq, set_int)
+declare_mp_range_handler(minio_rq, 0, INT_MAX)
declare_mp_snprint(minio_rq, print_nonzero)
declare_def_handler(queue_without_daemon, set_yes_no)
@@ -512,7 +561,7 @@ snprint_def_queue_without_daemon(struct config *conf, struct strbuf *buff,
return append_strbuf_quoted(buff, qwd);
}
-declare_def_handler(checker_timeout, set_int)
+declare_def_range_handler(checker_timeout, 0, INT_MAX)
declare_def_snprint(checker_timeout, print_nonzero)
declare_def_handler(allow_usb_devices, set_yes_no)
@@ -583,13 +632,13 @@ declare_hw_snprint(deferred_remove, print_yes_no_undef)
declare_mp_handler(deferred_remove, set_yes_no_undef)
declare_mp_snprint(deferred_remove, print_yes_no_undef)
-declare_def_handler(retrigger_tries, set_int)
+declare_def_range_handler(retrigger_tries, 0, INT_MAX)
declare_def_snprint(retrigger_tries, print_int)
-declare_def_handler(retrigger_delay, set_int)
+declare_def_range_handler(retrigger_delay, 0, INT_MAX)
declare_def_snprint(retrigger_delay, print_int)
-declare_def_handler(uev_wait_timeout, set_int)
+declare_def_range_handler(uev_wait_timeout, 0, INT_MAX)
declare_def_snprint(uev_wait_timeout, print_int)
declare_def_handler(strict_timing, set_yes_no)
@@ -616,19 +665,19 @@ static int snprint_def_disable_changed_wwids(struct config *conf,
return print_ignored(buff);
}
-declare_def_handler(remove_retries, set_int)
+declare_def_range_handler(remove_retries, 0, INT_MAX)
declare_def_snprint(remove_retries, print_int)
-declare_def_handler(max_sectors_kb, set_int)
+declare_def_range_handler(max_sectors_kb, 0, INT_MAX)
declare_def_snprint(max_sectors_kb, print_nonzero)
-declare_ovr_handler(max_sectors_kb, set_int)
+declare_ovr_range_handler(max_sectors_kb, 0, INT_MAX)
declare_ovr_snprint(max_sectors_kb, print_nonzero)
-declare_hw_handler(max_sectors_kb, set_int)
+declare_hw_range_handler(max_sectors_kb, 0, INT_MAX)
declare_hw_snprint(max_sectors_kb, print_nonzero)
-declare_mp_handler(max_sectors_kb, set_int)
+declare_mp_range_handler(max_sectors_kb, 0, INT_MAX)
declare_mp_snprint(max_sectors_kb, print_nonzero)
-declare_def_handler(find_multipaths_timeout, set_int)
+declare_def_range_handler(find_multipaths_timeout, INT_MIN, INT_MAX)
declare_def_snprint_defint(find_multipaths_timeout, print_int,
DEFAULT_FIND_MULTIPATHS_TIMEOUT)
@@ -1385,27 +1434,7 @@ declare_ovr_snprint(recheck_wwid, print_yes_no_undef)
declare_hw_handler(recheck_wwid, set_yes_no_undef)
declare_hw_snprint(recheck_wwid, print_yes_no_undef)
-
-static int
-def_uxsock_timeout_handler(struct config *conf, vector strvec, const char *file,
- int line_nr)
-{
- unsigned int uxsock_timeout;
- char *buff;
-
- buff = set_value(strvec);
- if (!buff)
- return 1;
-
- if (sscanf(buff, "%u", &uxsock_timeout) == 1 &&
- uxsock_timeout > DEFAULT_REPLY_TIMEOUT)
- conf->uxsock_timeout = uxsock_timeout;
- else
- conf->uxsock_timeout = DEFAULT_REPLY_TIMEOUT;
-
- free(buff);
- return 0;
-}
+declare_def_range_handler(uxsock_timeout, DEFAULT_REPLY_TIMEOUT, INT_MAX)
static int
hw_vpd_vendor_handler(struct config *conf, vector strvec, const char *file,
--
2.17.2
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [dm-devel] [PATCH 6/8] libmultipath: improve checks for set_str
2021-10-06 20:04 [dm-devel] [PATCH 0/8] improving config parsing warnings Benjamin Marzinski
` (4 preceding siblings ...)
2021-10-06 20:04 ` [dm-devel] [PATCH 5/8] libmultipath: make set_int take a range for valid values Benjamin Marzinski
@ 2021-10-06 20:04 ` Benjamin Marzinski
2021-11-04 20:34 ` Martin Wilck
2021-10-06 20:04 ` [dm-devel] [PATCH 7/8] libmultipath: split set_int to enable reuse Benjamin Marzinski
2021-10-06 20:04 ` [dm-devel] [PATCH 8/8] libmultipath: cleanup invalid config handling Benjamin Marzinski
7 siblings, 1 reply; 29+ messages in thread
From: Benjamin Marzinski @ 2021-10-06 20:04 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
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(-)
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)
/*
--
2.17.2
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [dm-devel] [PATCH 7/8] libmultipath: split set_int to enable reuse
2021-10-06 20:04 [dm-devel] [PATCH 0/8] improving config parsing warnings Benjamin Marzinski
` (5 preceding siblings ...)
2021-10-06 20:04 ` [dm-devel] [PATCH 6/8] libmultipath: improve checks for set_str Benjamin Marzinski
@ 2021-10-06 20:04 ` Benjamin Marzinski
2021-11-04 20:54 ` Martin Wilck
2021-11-11 11:56 ` Martin Wilck
2021-10-06 20:04 ` [dm-devel] [PATCH 8/8] libmultipath: cleanup invalid config handling Benjamin Marzinski
7 siblings, 2 replies; 29+ messages in thread
From: Benjamin Marzinski @ 2021-10-06 20:04 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
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.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
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 @@
#include "strbuf.h"
static int
-set_int(vector strvec, void *ptr, int min, int max, const char *file,
- int line_nr)
+do_set_int(vector strvec, void *ptr, int min, int max, const char *file,
+ int line_nr, char *buff)
{
int *int_ptr = (int *)ptr;
- char *buff, *eptr;
+ char *eptr;
long res;
- int rc;
-
- buff = set_value(strvec);
- if (!buff)
- return 1;
res = strtol(buff, &eptr, 10);
if (eptr > buff)
@@ -50,17 +45,30 @@ set_int(vector strvec, void *ptr, int min, int max, const char *file,
if (*buff == '\0' || *eptr != '\0') {
condlog(1, "%s line %d, invalid value for %s: \"%s\"",
file, line_nr, (char*)VECTOR_SLOT(strvec, 0), buff);
- rc = 1;
- } else {
- if (res > max || res < min) {
- res = (res > max) ? max : min;
- condlog(1, "%s line %d, value for %s too %s, capping at %ld",
+ return 1;
+ }
+ if (res > max || res < min) {
+ res = (res > max) ? max : min;
+ condlog(1, "%s line %d, value for %s too %s, capping at %ld",
file, line_nr, (char*)VECTOR_SLOT(strvec, 0),
- (res == max)? "large" : "small", res);
- }
- rc = 0;
- *int_ptr = res;
+ (res == max)? "large" : "small", res);
}
+ *int_ptr = res;
+ return 0;
+}
+
+static int
+set_int(vector strvec, void *ptr, int min, int max, const char *file,
+ int line_nr)
+{
+ char *buff;
+ int rc;
+
+ buff = set_value(strvec);
+ if (!buff)
+ return 1;
+
+ rc = do_set_int(strvec, ptr, min, max, file, line_nr, buff);
FREE(buff);
return rc;
@@ -918,6 +926,7 @@ declare_mp_attr_snprint(gid, print_gid)
static int
set_undef_off_zero(vector strvec, void *ptr, const char *file, int line_nr)
{
+ int rc = 0;
char * buff;
int *int_ptr = (int *)ptr;
@@ -927,10 +936,10 @@ set_undef_off_zero(vector strvec, void *ptr, const char *file, int line_nr)
if (strcmp(buff, "off") == 0)
*int_ptr = UOZ_OFF;
- else if (sscanf(buff, "%d", int_ptr) != 1 ||
- *int_ptr < UOZ_ZERO)
- *int_ptr = UOZ_UNDEF;
- else if (*int_ptr == 0)
+ else
+ rc = do_set_int(strvec, int_ptr, 0, INT_MAX, file, line_nr,
+ buff);
+ if (rc == 0 && *int_ptr == 0)
*int_ptr = UOZ_ZERO;
FREE(buff);
@@ -1082,14 +1091,12 @@ max_fds_handler(struct config *conf, vector strvec, const char *file,
/* Assume safe limit */
max_fds = 4096;
}
- if (strlen(buff) == 3 &&
- !strcmp(buff, "max"))
- conf->max_fds = max_fds;
- else
- conf->max_fds = atoi(buff);
-
- if (conf->max_fds > max_fds)
+ if (!strcmp(buff, "max")) {
conf->max_fds = max_fds;
+ r = 0;
+ } else
+ r = do_set_int(strvec, &conf->max_fds, 0, max_fds, file,
+ line_nr, buff);
FREE(buff);
@@ -1158,6 +1165,7 @@ declare_mp_snprint(rr_weight, print_rr_weight)
static int
set_pgfailback(vector strvec, void *ptr, const char *file, int line_nr)
{
+ int rc = 0;
int *int_ptr = (int *)ptr;
char * buff;
@@ -1172,11 +1180,11 @@ set_pgfailback(vector strvec, void *ptr, const char *file, int line_nr)
else if (strlen(buff) == 10 && !strcmp(buff, "followover"))
*int_ptr = -FAILBACK_FOLLOWOVER;
else
- *int_ptr = atoi(buff);
+ rc = do_set_int(strvec, ptr, 0, INT_MAX, file, line_nr, buff);
FREE(buff);
- return 0;
+ return rc;
}
int
@@ -1208,6 +1216,7 @@ declare_mp_snprint(pgfailback, print_pgfailback)
static int
no_path_retry_helper(vector strvec, void *ptr, const char *file, int line_nr)
{
+ int rc = 0;
int *int_ptr = (int *)ptr;
char * buff;
@@ -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);
FREE(buff);
- return 0;
+ return rc;
}
int
@@ -1365,6 +1374,7 @@ snprint_mp_reservation_key (struct config *conf, struct strbuf *buff,
static int
set_off_int_undef(vector strvec, void *ptr, const char *file, int line_nr)
{
+ int rc =0;
int *int_ptr = (int *)ptr;
char * buff;
@@ -1374,11 +1384,11 @@ set_off_int_undef(vector strvec, void *ptr, const char *file, int line_nr)
if (!strcmp(buff, "no") || !strcmp(buff, "0"))
*int_ptr = NU_NO;
- else if ((*int_ptr = atoi(buff)) < 1)
- *int_ptr = NU_UNDEF;
+ else
+ rc = do_set_int(strvec, ptr, 1, INT_MAX, file, line_nr, buff);
FREE(buff);
- return 0;
+ return rc;
}
int
--
2.17.2
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [dm-devel] [PATCH 8/8] libmultipath: cleanup invalid config handling
2021-10-06 20:04 [dm-devel] [PATCH 0/8] improving config parsing warnings Benjamin Marzinski
` (6 preceding siblings ...)
2021-10-06 20:04 ` [dm-devel] [PATCH 7/8] libmultipath: split set_int to enable reuse Benjamin Marzinski
@ 2021-10-06 20:04 ` Benjamin Marzinski
2021-11-04 21:11 ` Martin Wilck
7 siblings, 1 reply; 29+ messages in thread
From: Benjamin Marzinski @ 2021-10-06 20:04 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
Add error reporting to the remaining config handlers. If the value is
invalid, do not change the existing config option's value. Also print
an error whenever 0 is returned for an invalid value. When the handler
returns 1, config processing already fails with an error message.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/dict.c | 73 +++++++++++++++++++++++++++++++--------------
1 file changed, 51 insertions(+), 22 deletions(-)
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index e79fcdd7..8c3b5f72 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -199,8 +199,11 @@ set_yes_no(vector strvec, void *ptr, const char *file, int line_nr)
if (strcmp(buff, "yes") == 0 || strcmp(buff, "1") == 0)
*int_ptr = YN_YES;
- else
+ else if (strcmp(buff, "no") == 0 || strcmp(buff, "0") == 0)
*int_ptr = YN_NO;
+ else
+ condlog(1, "%s line %d, invalid value for %s: \"%s\"",
+ file, line_nr, (char*)VECTOR_SLOT(strvec, 0), buff);
FREE(buff);
return 0;
@@ -221,7 +224,8 @@ set_yes_no_undef(vector strvec, void *ptr, const char *file, int line_nr)
else if (strcmp(buff, "yes") == 0 || strcmp(buff, "1") == 0)
*int_ptr = YNU_YES;
else
- *int_ptr = YNU_UNDEF;
+ condlog(1, "%s line %d, invalid value for %s: \"%s\"",
+ file, line_nr, (char*)VECTOR_SLOT(strvec, 0), buff);
FREE(buff);
return 0;
@@ -471,9 +475,6 @@ def_find_multipaths_handler(struct config *conf, vector strvec,
char *buff;
int i;
- if (set_yes_no_undef(strvec, &conf->find_multipaths, file, line_nr) == 0 && conf->find_multipaths != FIND_MULTIPATHS_UNDEF)
- return 0;
-
buff = set_value(strvec);
if (!buff)
return 1;
@@ -486,9 +487,14 @@ def_find_multipaths_handler(struct config *conf, vector strvec,
}
}
- if (conf->find_multipaths == YNU_UNDEF) {
- condlog(0, "illegal value for find_multipaths: %s", buff);
- conf->find_multipaths = DEFAULT_FIND_MULTIPATHS;
+ if (i >= __FIND_MULTIPATHS_LAST) {
+ if (strcmp(buff, "no") == 0 || strcmp(buff, "0") == 0)
+ conf->find_multipaths = FIND_MULTIPATHS_OFF;
+ else if (strcmp(buff, "yes") == 0 || strcmp(buff, "1") == 0)
+ conf->find_multipaths = FIND_MULTIPATHS_ON;
+ else
+ condlog(1, "%s line %d, invalid value for find_multipaths: \"%s\"",
+ file, line_nr, buff);
}
FREE(buff);
@@ -537,8 +543,10 @@ static int uid_attrs_handler(struct config *conf, vector strvec,
if (!val)
return 1;
if (parse_uid_attrs(val, conf))
- condlog(1, "error parsing uid_attrs: \"%s\"", val);
- condlog(3, "parsed %d uid_attrs", VECTOR_SIZE(&conf->uid_attrs));
+ condlog(1, "%s line %d,error parsing uid_attrs: \"%s\"", file,
+ line_nr, val);
+ else
+ condlog(4, "parsed %d uid_attrs", VECTOR_SIZE(&conf->uid_attrs));
FREE(val);
return 0;
}
@@ -766,8 +774,11 @@ def_config_dir_handler(struct config *conf, vector strvec, const char *file,
int line_nr)
{
/* this is only valid in the main config file */
- if (conf->processed_main_config)
+ if (conf->processed_main_config) {
+ condlog(1, "%s line %d, config_dir option only valid in /etc/multipath.conf",
+ file, line_nr);
return 0;
+ }
return set_path(strvec, &conf->config_dir, file, line_nr);
}
declare_def_snprint(config_dir, print_str)
@@ -825,7 +836,9 @@ set_mode(vector strvec, void *ptr, int *flags, const char *file, int line_nr)
if (sscanf(buff, "%o", &mode) == 1 && mode <= 0777) {
*flags |= (1 << ATTR_MODE);
*mode_ptr = mode;
- }
+ } else
+ condlog(1, "%s line %d, invalid value for mode: \"%s\"",
+ file, line_nr, buff);
FREE(buff);
return 0;
@@ -850,7 +863,9 @@ set_uid(vector strvec, void *ptr, int *flags, const char *file, int line_nr)
else if (sscanf(buff, "%u", &uid) == 1){
*flags |= (1 << ATTR_UID);
*uid_ptr = uid;
- }
+ } else
+ condlog(1, "%s line %d, invalid value for uid: \"%s\"",
+ file, line_nr, buff);
FREE(buff);
return 0;
@@ -876,7 +891,9 @@ set_gid(vector strvec, void *ptr, int *flags, const char *file, int line_nr)
else if (sscanf(buff, "%u", &gid) == 1){
*flags |= (1 << ATTR_GID);
*gid_ptr = gid;
- }
+ } else
+ condlog(1, "%s line %d, invalid value for gid: \"%s\"",
+ file, line_nr, buff);
FREE(buff);
return 0;
}
@@ -978,7 +995,8 @@ set_dev_loss(vector strvec, void *ptr, const char *file, int line_nr)
if (!strcmp(buff, "infinity"))
*uint_ptr = MAX_DEV_LOSS_TMO;
else if (sscanf(buff, "%u", uint_ptr) != 1)
- *uint_ptr = DEV_LOSS_TMO_UNSET;
+ condlog(1, "%s line %d, invalid value for dev_loss_tmo: \"%s\"",
+ file, line_nr, buff);
FREE(buff);
return 0;
@@ -1012,13 +1030,19 @@ static int
set_pgpolicy(vector strvec, void *ptr, const char *file, int line_nr)
{
char * buff;
+ int policy;
int *int_ptr = (int *)ptr;
buff = set_value(strvec);
if (!buff)
return 1;
- *int_ptr = get_pgpolicy_id(buff);
+ policy = get_pgpolicy_id(buff);
+ if (policy != IOPOLICY_UNDEF)
+ *int_ptr = policy;
+ else
+ condlog(1, "%s line %d, invalid value for path_grouping_policy: \"%s\"",
+ file, line_nr, buff);
FREE(buff);
return 0;
@@ -1131,10 +1155,11 @@ set_rr_weight(vector strvec, void *ptr, const char *file, int line_nr)
if (!strcmp(buff, "priorities"))
*int_ptr = RR_WEIGHT_PRIO;
-
- if (!strcmp(buff, "uniform"))
+ else if (!strcmp(buff, "uniform"))
*int_ptr = RR_WEIGHT_NONE;
-
+ else
+ condlog(1, "%s line %d, invalid value for rr_weight: \"%s\"",
+ file, line_nr, buff);
FREE(buff);
return 0;
@@ -1270,10 +1295,13 @@ def_log_checker_err_handler(struct config *conf, vector strvec,
if (!buff)
return 1;
- if (strlen(buff) == 4 && !strcmp(buff, "once"))
+ if (!strcmp(buff, "once"))
conf->log_checker_err = LOG_CHKR_ERR_ONCE;
- else if (strlen(buff) == 6 && !strcmp(buff, "always"))
+ else if (!strcmp(buff, "always"))
conf->log_checker_err = LOG_CHKR_ERR_ALWAYS;
+ else
+ condlog(1, "%s line %d, invalid value for log_checker_err: \"%s\"",
+ file, line_nr, buff);
free(buff);
return 0;
@@ -1534,7 +1562,8 @@ hw_vpd_vendor_handler(struct config *conf, vector strvec, const char *file,
goto out;
}
}
- hwe->vpd_vendor_id = 0;
+ condlog(1, "%s line %d, invalid value for vpd_vendor: \"%s\"",
+ file, line_nr, buff);
out:
FREE(buff);
return 0;
--
2.17.2
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [dm-devel] [PATCH 6/8] libmultipath: improve checks for set_str
2021-10-06 20:04 ` [dm-devel] [PATCH 6/8] libmultipath: improve checks for set_str Benjamin Marzinski
@ 2021-11-04 20:34 ` Martin Wilck
2021-11-05 6:59 ` Martin Wilck
2021-11-05 17:38 ` Benjamin Marzinski
0 siblings, 2 replies; 29+ messages in thread
From: Martin Wilck @ 2021-11-04 20:34 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [dm-devel] [PATCH 5/8] libmultipath: make set_int take a range for valid values
2021-10-06 20:04 ` [dm-devel] [PATCH 5/8] libmultipath: make set_int take a range for valid values Benjamin Marzinski
@ 2021-11-04 20:34 ` Martin Wilck
2021-11-05 17:34 ` Benjamin Marzinski
0 siblings, 1 reply; 29+ messages in thread
From: Martin Wilck @ 2021-11-04 20:34 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel
On Wed, 2021-10-06 at 15:04 -0500, Benjamin Marzinski wrote:
> If a value outside of the valid range is passed to set_int, it caps
> the
> value at appropriate limit, and issues a warning.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
One nitpick below.
It's a lot of code changes for just two cases where we have nontrivial
values for min and max. I guess for uint the count of nontrivial cases
was zero?
Yet it's an improvement, so I'm going to ack when the nit is fixed.
Martin
> ---
> libmultipath/dict.c | 121 +++++++++++++++++++++++++++---------------
> --
> 1 file changed, 75 insertions(+), 46 deletions(-)
>
> diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> index eb2c44c0..1758bd26 100644
> --- a/libmultipath/dict.c
> +++ b/libmultipath/dict.c
> ...
> @@ -298,7 +347,7 @@ declare_def_snprint(checkint, print_int)
> declare_def_handler(max_checkint, set_uint)
> declare_def_snprint(max_checkint, print_int)
>
> -declare_def_handler(verbosity, set_int)
> +declare_def_range_handler(verbosity, 0, 4)
You should use MAX_VERBOSITY here.
> declare_def_snprint(verbosity, print_int)
>
> declare_def_handler(reassign_maps, set_yes_no)
> @@ -473,22 +522,22 @@ declare_ovr_snprint(checker_name, print_str)
> declare_hw_handler(checker_name, set_str)
> declare_hw_snprint(checker_name, print_str)
>
> -declare_def_handler(minio, set_int)
> +declare_def_range_handler(minio, 0, INT_MAX)
> declare_def_snprint_defint(minio, print_int, DEFAULT_MINIO)
> -declare_ovr_handler(minio, set_int)
> +declare_ovr_range_handler(minio, 0, INT_MAX)
> declare_ovr_snprint(minio, print_nonzero)
> -declare_hw_handler(minio, set_int)
> +declare_hw_range_handler(minio, 0, INT_MAX)
> declare_hw_snprint(minio, print_nonzero)
> -declare_mp_handler(minio, set_int)
> +declare_mp_range_handler(minio, 0, INT_MAX)
> declare_mp_snprint(minio, print_nonzero)
>
> -declare_def_handler(minio_rq, set_int)
> +declare_def_range_handler(minio_rq, 0, INT_MAX)
> declare_def_snprint_defint(minio_rq, print_int, DEFAULT_MINIO_RQ)
> -declare_ovr_handler(minio_rq, set_int)
> +declare_ovr_range_handler(minio_rq, 0, INT_MAX)
> declare_ovr_snprint(minio_rq, print_nonzero)
> -declare_hw_handler(minio_rq, set_int)
> +declare_hw_range_handler(minio_rq, 0, INT_MAX)
> declare_hw_snprint(minio_rq, print_nonzero)
> -declare_mp_handler(minio_rq, set_int)
> +declare_mp_range_handler(minio_rq, 0, INT_MAX)
> declare_mp_snprint(minio_rq, print_nonzero)
>
> declare_def_handler(queue_without_daemon, set_yes_no)
> @@ -512,7 +561,7 @@ snprint_def_queue_without_daemon(struct config
> *conf, struct strbuf *buff,
> return append_strbuf_quoted(buff, qwd);
> }
>
> -declare_def_handler(checker_timeout, set_int)
> +declare_def_range_handler(checker_timeout, 0, INT_MAX)
> declare_def_snprint(checker_timeout, print_nonzero)
>
> declare_def_handler(allow_usb_devices, set_yes_no)
> @@ -583,13 +632,13 @@ declare_hw_snprint(deferred_remove,
> print_yes_no_undef)
> declare_mp_handler(deferred_remove, set_yes_no_undef)
> declare_mp_snprint(deferred_remove, print_yes_no_undef)
>
> -declare_def_handler(retrigger_tries, set_int)
> +declare_def_range_handler(retrigger_tries, 0, INT_MAX)
> declare_def_snprint(retrigger_tries, print_int)
>
> -declare_def_handler(retrigger_delay, set_int)
> +declare_def_range_handler(retrigger_delay, 0, INT_MAX)
> declare_def_snprint(retrigger_delay, print_int)
>
> -declare_def_handler(uev_wait_timeout, set_int)
> +declare_def_range_handler(uev_wait_timeout, 0, INT_MAX)
> declare_def_snprint(uev_wait_timeout, print_int)
>
> declare_def_handler(strict_timing, set_yes_no)
> @@ -616,19 +665,19 @@ static int
> snprint_def_disable_changed_wwids(struct config *conf,
> return print_ignored(buff);
> }
>
> -declare_def_handler(remove_retries, set_int)
> +declare_def_range_handler(remove_retries, 0, INT_MAX)
> declare_def_snprint(remove_retries, print_int)
>
> -declare_def_handler(max_sectors_kb, set_int)
> +declare_def_range_handler(max_sectors_kb, 0, INT_MAX)
> declare_def_snprint(max_sectors_kb, print_nonzero)
> -declare_ovr_handler(max_sectors_kb, set_int)
> +declare_ovr_range_handler(max_sectors_kb, 0, INT_MAX)
> declare_ovr_snprint(max_sectors_kb, print_nonzero)
> -declare_hw_handler(max_sectors_kb, set_int)
> +declare_hw_range_handler(max_sectors_kb, 0, INT_MAX)
> declare_hw_snprint(max_sectors_kb, print_nonzero)
> -declare_mp_handler(max_sectors_kb, set_int)
> +declare_mp_range_handler(max_sectors_kb, 0, INT_MAX)
> declare_mp_snprint(max_sectors_kb, print_nonzero)
>
> -declare_def_handler(find_multipaths_timeout, set_int)
> +declare_def_range_handler(find_multipaths_timeout, INT_MIN, INT_MAX)
> declare_def_snprint_defint(find_multipaths_timeout, print_int,
> DEFAULT_FIND_MULTIPATHS_TIMEOUT)
>
> @@ -1385,27 +1434,7 @@ declare_ovr_snprint(recheck_wwid,
> print_yes_no_undef)
> declare_hw_handler(recheck_wwid, set_yes_no_undef)
> declare_hw_snprint(recheck_wwid, print_yes_no_undef)
>
> -
> -static int
> -def_uxsock_timeout_handler(struct config *conf, vector strvec, const
> char *file,
> - int line_nr)
> -{
> - unsigned int uxsock_timeout;
> - char *buff;
> -
> - buff = set_value(strvec);
> - if (!buff)
> - return 1;
> -
> - if (sscanf(buff, "%u", &uxsock_timeout) == 1 &&
> - uxsock_timeout > DEFAULT_REPLY_TIMEOUT)
> - conf->uxsock_timeout = uxsock_timeout;
> - else
> - conf->uxsock_timeout = DEFAULT_REPLY_TIMEOUT;
> -
> - free(buff);
> - return 0;
> -}
> +declare_def_range_handler(uxsock_timeout, DEFAULT_REPLY_TIMEOUT,
> INT_MAX)
>
> static int
> hw_vpd_vendor_handler(struct config *conf, vector strvec, const char
> *file,
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [dm-devel] [PATCH 7/8] libmultipath: split set_int to enable reuse
2021-10-06 20:04 ` [dm-devel] [PATCH 7/8] libmultipath: split set_int to enable reuse Benjamin Marzinski
@ 2021-11-04 20:54 ` Martin Wilck
2021-11-05 18:08 ` Benjamin Marzinski
2021-11-11 11:56 ` Martin Wilck
1 sibling, 1 reply; 29+ messages in thread
From: Martin Wilck @ 2021-11-04 20:54 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel
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)
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> 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 @@
> #include "strbuf.h"
>
> static int
> -set_int(vector strvec, void *ptr, int min, int max, const char
> *file,
> - int line_nr)
> +do_set_int(vector strvec, void *ptr, int min, int max, const char
> *file,
> + int line_nr, char *buff)
> {
> int *int_ptr = (int *)ptr;
> - char *buff, *eptr;
> + char *eptr;
> long res;
> - int rc;
> -
> - buff = set_value(strvec);
> - if (!buff)
> - return 1;
>
> res = strtol(buff, &eptr, 10);
> if (eptr > buff)
> @@ -50,17 +45,30 @@ set_int(vector strvec, void *ptr, int min, int
> max, const char *file,
> if (*buff == '\0' || *eptr != '\0') {
> condlog(1, "%s line %d, invalid value for %s:
> \"%s\"",
> file, line_nr, (char*)VECTOR_SLOT(strvec, 0),
> buff);
> - rc = 1;
> - } else {
> - if (res > max || res < min) {
> - res = (res > max) ? max : min;
> - condlog(1, "%s line %d, value for %s too %s,
> capping at %ld",
> + return 1;
> + }
> + if (res > max || res < min) {
> + res = (res > max) ? max : min;
> + condlog(1, "%s line %d, value for %s too %s, capping
> at %ld",
> file, line_nr, (char*)VECTOR_SLOT(strvec, 0),
> - (res == max)? "large" : "small", res);
> - }
> - rc = 0;
> - *int_ptr = res;
> + (res == max)? "large" : "small", res);
> }
> + *int_ptr = res;
> + return 0;
> +}
> +
> +static int
> +set_int(vector strvec, void *ptr, int min, int max, const char
> *file,
> + int line_nr)
> +{
> + char *buff;
> + int rc;
> +
> + buff = set_value(strvec);
> + if (!buff)
> + return 1;
> +
> + rc = do_set_int(strvec, ptr, min, max, file, line_nr, buff);
>
> FREE(buff);
> return rc;
> @@ -918,6 +926,7 @@ declare_mp_attr_snprint(gid, print_gid)
> static int
> set_undef_off_zero(vector strvec, void *ptr, const char *file, int
> line_nr)
> {
> + int rc = 0;
> char * buff;
> int *int_ptr = (int *)ptr;
>
> @@ -927,10 +936,10 @@ set_undef_off_zero(vector strvec, void *ptr,
> const char *file, int line_nr)
>
> if (strcmp(buff, "off") == 0)
> *int_ptr = UOZ_OFF;
> - else if (sscanf(buff, "%d", int_ptr) != 1 ||
> - *int_ptr < UOZ_ZERO)
> - *int_ptr = UOZ_UNDEF;
> - else if (*int_ptr == 0)
> + else
> + rc = do_set_int(strvec, int_ptr, 0, INT_MAX, file,
> line_nr,
> + buff);
> + if (rc == 0 && *int_ptr == 0)
> *int_ptr = UOZ_ZERO;
>
> FREE(buff);
> @@ -1082,14 +1091,12 @@ max_fds_handler(struct config *conf, vector
> strvec, const char *file,
> /* Assume safe limit */
> max_fds = 4096;
> }
> - if (strlen(buff) == 3 &&
> - !strcmp(buff, "max"))
> - conf->max_fds = max_fds;
> - else
> - conf->max_fds = atoi(buff);
> -
> - if (conf->max_fds > max_fds)
> + if (!strcmp(buff, "max")) {
> conf->max_fds = max_fds;
> + r = 0;
> + } else
> + r = do_set_int(strvec, &conf->max_fds, 0, max_fds,
> file,
> + line_nr, buff);
>
> FREE(buff);
>
> @@ -1158,6 +1165,7 @@ declare_mp_snprint(rr_weight, print_rr_weight)
> static int
> set_pgfailback(vector strvec, void *ptr, const char *file, int
> line_nr)
> {
> + int rc = 0;
> int *int_ptr = (int *)ptr;
> char * buff;
>
> @@ -1172,11 +1180,11 @@ set_pgfailback(vector strvec, void *ptr,
> const char *file, int line_nr)
> else if (strlen(buff) == 10 && !strcmp(buff, "followover"))
> *int_ptr = -FAILBACK_FOLLOWOVER;
> else
> - *int_ptr = atoi(buff);
> + rc = do_set_int(strvec, ptr, 0, INT_MAX, file,
> line_nr, buff);
>
> FREE(buff);
>
> - return 0;
> + return rc;
> }
>
> int
> @@ -1208,6 +1216,7 @@ declare_mp_snprint(pgfailback,
> print_pgfailback)
> static int
> no_path_retry_helper(vector strvec, void *ptr, const char *file, int
> line_nr)
> {
> + int rc = 0;
> int *int_ptr = (int *)ptr;
> char * buff;
>
> @@ -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?
>
> FREE(buff);
> - return 0;
> + return rc;
> }
>
> int
> @@ -1365,6 +1374,7 @@ snprint_mp_reservation_key (struct config
> *conf, struct strbuf *buff,
> static int
> set_off_int_undef(vector strvec, void *ptr, const char *file, int
> line_nr)
> {
> + int rc =0;
> int *int_ptr = (int *)ptr;
> char * buff;
>
> @@ -1374,11 +1384,11 @@ set_off_int_undef(vector strvec, void *ptr,
> const char *file, int line_nr)
>
> if (!strcmp(buff, "no") || !strcmp(buff, "0"))
> *int_ptr = NU_NO;
> - else if ((*int_ptr = atoi(buff)) < 1)
> - *int_ptr = NU_UNDEF;
> + else
> + rc = do_set_int(strvec, ptr, 1, INT_MAX, file,
> line_nr, buff);
Likewise, you'd set 1 here for negative input or "0 ", while
previously the result would be NU_UNDEF == 0.
Negative values are of course garbage and I'm unsure if trailing spaces
can occur at this point in the code, but do_set_int() handles them.
Regards,
Martin
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [dm-devel] [PATCH 4/8] libmultipath: pass file and line number to keyword handlers
2021-10-06 20:04 ` [dm-devel] [PATCH 4/8] libmultipath: pass file and line number to keyword handlers Benjamin Marzinski
@ 2021-11-04 20:55 ` Martin Wilck
2021-11-05 17:33 ` Benjamin Marzinski
0 siblings, 1 reply; 29+ messages in thread
From: Martin Wilck @ 2021-11-04 20:55 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel
On Wed, 2021-10-06 at 15:04 -0500, Benjamin Marzinski wrote:
> This will make it possible for the keyword handlers to print more
> useful
> warning messages. It will be used by future patches.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Nit: There's one very long line (349).
Apart from that, ack.
> ---
> libmultipath/dict.c | 143 +++++++++++++++++++++++++-----------------
> libmultipath/parser.c | 3 +-
> libmultipath/parser.h | 2 +-
> 3 files changed, 90 insertions(+), 58 deletions(-)
>
> diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> index 7a727389..eb2c44c0 100644
> --- a/libmultipath/dict.c
> +++ b/libmultipath/dict.c
> @@ -29,7 +29,7 @@
> #include "strbuf.h"
>
> static int
> -set_int(vector strvec, void *ptr)
> +set_int(vector strvec, void *ptr, const char *file, int line_nr)
> {
> int *int_ptr = (int *)ptr;
> char *buff, *eptr;
> @@ -58,7 +58,7 @@ set_int(vector strvec, void *ptr)
> }
>
> static int
> -set_uint(vector strvec, void *ptr)
> +set_uint(vector strvec, void *ptr, const char *file, int line_nr)
> {
> unsigned int *uint_ptr = (unsigned int *)ptr;
> char *buff, *eptr, *p;
> @@ -90,7 +90,7 @@ set_uint(vector strvec, void *ptr)
> }
>
> static int
> -set_str(vector strvec, void *ptr)
> +set_str(vector strvec, void *ptr, const char *file, int line_nr)
> {
> char **str_ptr = (char **)ptr;
>
> @@ -105,7 +105,7 @@ set_str(vector strvec, void *ptr)
> }
>
> static int
> -set_yes_no(vector strvec, void *ptr)
> +set_yes_no(vector strvec, void *ptr, const char *file, int line_nr)
> {
> char * buff;
> int *int_ptr = (int *)ptr;
> @@ -124,7 +124,7 @@ set_yes_no(vector strvec, void *ptr)
> }
>
> static int
> -set_yes_no_undef(vector strvec, void *ptr)
> +set_yes_no_undef(vector strvec, void *ptr, const char *file, int
> line_nr)
> {
> char * buff;
> int *int_ptr = (int *)ptr;
> @@ -187,9 +187,10 @@ static int print_yes_no_undef(struct strbuf *buff,
> long v)
>
> #define declare_def_handler(option,
> function) \
> static
> int \
> -def_ ## option ## _handler (struct config *conf, vector
> strvec) \
> +def_ ## option ## _handler (struct config *conf, vector
> strvec, \
> + const char *file, int
> line_nr) \
> {
> \
> - return function (strvec, &conf-
> >option); \
> + return function (strvec, &conf->option, file,
> line_nr); \
> }
>
> #define declare_def_snprint(option,
> function) \
> @@ -224,12 +225,13 @@ snprint_def_ ## option (struct config *conf,
> struct strbuf *buff, \
>
> #define declare_hw_handler(option,
> function) \
> static
> int \
> -hw_ ## option ## _handler (struct config *conf, vector
> strvec) \
> +hw_ ## option ## _handler (struct config *conf, vector
> strvec, \
> + const char *file, int
> line_nr) \
> {
> \
> struct hwentry * hwe = VECTOR_LAST_SLOT(conf-
> >hwtable); \
> if
> (!hwe) \
> return
> 1; \
> - return function (strvec, &hwe-
> >option); \
> + return function (strvec, &hwe->option, file,
> line_nr); \
> }
>
> #define declare_hw_snprint(option,
> function) \
> @@ -243,11 +245,12 @@ snprint_hw_ ## option (struct config *conf,
> struct strbuf *buff, \
>
> #define declare_ovr_handler(option,
> function) \
> static
> int \
> -ovr_ ## option ## _handler (struct config *conf, vector
> strvec) \
> +ovr_ ## option ## _handler (struct config *conf, vector
> strvec, \
> + const char *file, int
> line_nr) \
> {
> \
> if (!conf-
> >overrides) \
> return
> 1; \
> - return function (strvec, &conf->overrides-
> >option); \
> + return function (strvec, &conf->overrides->option, file,
> line_nr); \
> }
>
> #define declare_ovr_snprint(option,
> function) \
> @@ -260,12 +263,13 @@ snprint_ovr_ ## option (struct config *conf,
> struct strbuf *buff, \
>
> #define declare_mp_handler(option,
> function) \
> static
> int \
> -mp_ ## option ## _handler (struct config *conf, vector
> strvec) \
> +mp_ ## option ## _handler (struct config *conf, vector
> strvec, \
> + const char *file, int
> line_nr) \
> {
> \
> struct mpentry * mpe = VECTOR_LAST_SLOT(conf-
> >mptable); \
> if
> (!mpe) \
> return
> 1; \
> - return function (strvec, &mpe-
> >option); \
> + return function (strvec, &mpe->option, file,
> line_nr); \
> }
>
> #define declare_mp_snprint(option,
> function) \
> @@ -277,9 +281,10 @@ snprint_mp_ ## option (struct config *conf, struct
> strbuf *buff, \
> return function(buff, mpe-
> >option); \
> }
>
> -static int checkint_handler(struct config *conf, vector strvec)
> +static int checkint_handler(struct config *conf, vector strvec,
> + const char *file, int line_nr)
> {
> - int rc = set_uint(strvec, &conf->checkint);
> + int rc = set_uint(strvec, &conf->checkint, file, line_nr);
>
> if (rc)
> return rc;
> @@ -302,9 +307,10 @@ declare_def_snprint(reassign_maps, print_yes_no)
> declare_def_handler(multipath_dir, set_str)
> declare_def_snprint(multipath_dir, print_str)
>
> -static int def_partition_delim_handler(struct config *conf, vector
> strvec)
> +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);
> + int rc = set_str(strvec, &conf->partition_delim, file,
> line_nr);
>
> if (rc != 0)
> return rc;
> @@ -334,13 +340,13 @@ static const char * const
> find_multipaths_optvals[] = {
> };
>
> static int
> -def_find_multipaths_handler(struct config *conf, vector strvec)
> +def_find_multipaths_handler(struct config *conf, vector strvec,
> + const char *file, int line_nr)
> {
> char *buff;
> int i;
>
> - if (set_yes_no_undef(strvec, &conf->find_multipaths) == 0 &&
> - conf->find_multipaths != FIND_MULTIPATHS_UNDEF)
> + if (set_yes_no_undef(strvec, &conf->find_multipaths, file,
> line_nr) == 0 && conf->find_multipaths != FIND_MULTIPATHS_UNDEF)
> return 0;
>
> buff = set_value(strvec);
> @@ -396,7 +402,8 @@ static int snprint_uid_attrs(struct config *conf,
> struct strbuf *buff,
> return total;
> }
>
> -static int uid_attrs_handler(struct config *conf, vector strvec)
> +static int uid_attrs_handler(struct config *conf, vector strvec,
> + const char *file, int line_nr)
> {
> char *val;
>
> @@ -597,7 +604,8 @@ declare_hw_handler(skip_kpartx, set_yes_no_undef)
> declare_hw_snprint(skip_kpartx, print_yes_no_undef)
> declare_mp_handler(skip_kpartx, set_yes_no_undef)
> declare_mp_snprint(skip_kpartx, print_yes_no_undef)
> -static int def_disable_changed_wwids_handler(struct config *conf,
> vector strvec)
> +static int def_disable_changed_wwids_handler(struct config *conf,
> vector strvec,
> + const char *file, int
> line_nr)
> {
> return 0;
> }
> @@ -629,20 +637,23 @@ declare_def_snprint_defstr(enable_foreign,
> print_str,
> DEFAULT_ENABLE_FOREIGN)
>
> static int
> -def_config_dir_handler(struct config *conf, vector strvec)
> +def_config_dir_handler(struct config *conf, vector strvec, const char
> *file,
> + int line_nr)
> {
> /* this is only valid in the main config file */
> if (conf->processed_main_config)
> return 0;
> - return set_str(strvec, &conf->config_dir);
> + return set_str(strvec, &conf->config_dir, file, line_nr);
> }
> declare_def_snprint(config_dir, print_str)
>
> #define declare_def_attr_handler(option,
> function) \
> static
> int \
> -def_ ## option ## _handler (struct config *conf, vector
> strvec) \
> +def_ ## option ## _handler (struct config *conf, vector
> strvec, \
> + const char *file, int
> line_nr) \
> {
> \
> - return function (strvec, &conf->option, &conf-
> >attribute_flags);\
> + return function (strvec, &conf->option, &conf->attribute_flags,
> \
> + file,
> line_nr); \
> }
>
> #define declare_def_attr_snprint(option,
> function) \
> @@ -655,12 +666,14 @@ snprint_def_ ## option (struct config *conf,
> struct strbuf *buff, \
>
> #define declare_mp_attr_handler(option,
> function) \
> static
> int \
> -mp_ ## option ## _handler (struct config *conf, vector
> strvec) \
> +mp_ ## option ## _handler (struct config *conf, vector
> strvec, \
> + const char *file, int
> line_nr) \
> {
> \
> struct mpentry * mpe = VECTOR_LAST_SLOT(conf-
> >mptable); \
> if
> (!mpe) \
> return
> 1; \
> - return function (strvec, &mpe->option, &mpe-
> >attribute_flags); \
> + return function (strvec, &mpe->option, &mpe-
> >attribute_flags, \
> + file,
> line_nr); \
> }
>
> #define declare_mp_attr_snprint(option,
> function) \
> @@ -673,7 +686,7 @@ snprint_mp_ ## option (struct config *conf, struct
> strbuf *buff, \
> }
>
> static int
> -set_mode(vector strvec, void *ptr, int *flags)
> +set_mode(vector strvec, void *ptr, int *flags, const char *file, int
> line_nr)
> {
> mode_t mode;
> mode_t *mode_ptr = (mode_t *)ptr;
> @@ -694,7 +707,7 @@ set_mode(vector strvec, void *ptr, int *flags)
> }
>
> static int
> -set_uid(vector strvec, void *ptr, int *flags)
> +set_uid(vector strvec, void *ptr, int *flags, const char *file, int
> line_nr)
> {
> uid_t uid;
> uid_t *uid_ptr = (uid_t *)ptr;
> @@ -719,7 +732,7 @@ set_uid(vector strvec, void *ptr, int *flags)
> }
>
> static int
> -set_gid(vector strvec, void *ptr, int *flags)
> +set_gid(vector strvec, void *ptr, int *flags, const char *file, int
> line_nr)
> {
> gid_t gid;
> gid_t *gid_ptr = (gid_t *)ptr;
> @@ -786,7 +799,7 @@ declare_mp_attr_handler(gid, set_gid)
> declare_mp_attr_snprint(gid, print_gid)
>
> static int
> -set_undef_off_zero(vector strvec, void *ptr)
> +set_undef_off_zero(vector strvec, void *ptr, const char *file, int
> line_nr)
> {
> char * buff;
> int *int_ptr = (int *)ptr;
> @@ -827,7 +840,7 @@ declare_hw_handler(fast_io_fail,
> set_undef_off_zero)
> declare_hw_snprint(fast_io_fail, print_undef_off_zero)
>
> static int
> -set_dev_loss(vector strvec, void *ptr)
> +set_dev_loss(vector strvec, void *ptr, const char *file, int line_nr)
> {
> char * buff;
> unsigned int *uint_ptr = (unsigned int *)ptr;
> @@ -870,7 +883,7 @@ declare_hw_handler(eh_deadline, set_undef_off_zero)
> declare_hw_snprint(eh_deadline, print_undef_off_zero)
>
> static int
> -set_pgpolicy(vector strvec, void *ptr)
> +set_pgpolicy(vector strvec, void *ptr, const char *file, int line_nr)
> {
> char * buff;
> int *int_ptr = (int *)ptr;
> @@ -936,7 +949,8 @@ get_sys_max_fds(int *max_fds)
>
>
> static int
> -max_fds_handler(struct config *conf, vector strvec)
> +max_fds_handler(struct config *conf, vector strvec, const char *file,
> + int line_nr)
> {
> char * buff;
> int r = 0, max_fds;
> @@ -981,7 +995,7 @@ snprint_max_fds (struct config *conf, struct strbuf
> *buff, const void *data)
> }
>
> static int
> -set_rr_weight(vector strvec, void *ptr)
> +set_rr_weight(vector strvec, void *ptr, const char *file, int line_nr)
> {
> int *int_ptr = (int *)ptr;
> char * buff;
> @@ -1025,7 +1039,7 @@ declare_mp_handler(rr_weight, set_rr_weight)
> declare_mp_snprint(rr_weight, print_rr_weight)
>
> static int
> -set_pgfailback(vector strvec, void *ptr)
> +set_pgfailback(vector strvec, void *ptr, const char *file, int
> line_nr)
> {
> int *int_ptr = (int *)ptr;
> char * buff;
> @@ -1075,7 +1089,7 @@ declare_mp_handler(pgfailback, set_pgfailback)
> declare_mp_snprint(pgfailback, print_pgfailback)
>
> static int
> -no_path_retry_helper(vector strvec, void *ptr)
> +no_path_retry_helper(vector strvec, void *ptr, const char *file, int
> line_nr)
> {
> int *int_ptr = (int *)ptr;
> char * buff;
> @@ -1120,7 +1134,8 @@ declare_mp_handler(no_path_retry,
> no_path_retry_helper)
> declare_mp_snprint(no_path_retry, print_no_path_retry)
>
> static int
> -def_log_checker_err_handler(struct config *conf, vector strvec)
> +def_log_checker_err_handler(struct config *conf, vector strvec,
> + const char *file, int line_nr)
> {
> char * buff;
>
> @@ -1193,7 +1208,8 @@ print_reservation_key(struct strbuf *buff,
> }
>
> static int
> -def_reservation_key_handler(struct config *conf, vector strvec)
> +def_reservation_key_handler(struct config *conf, vector strvec,
> + const char *file, int line_nr)
> {
> return set_reservation_key(strvec, &conf->reservation_key,
> &conf->sa_flags,
> @@ -1209,7 +1225,8 @@ snprint_def_reservation_key (struct config *conf,
> struct strbuf *buff,
> }
>
> static int
> -mp_reservation_key_handler(struct config *conf, vector strvec)
> +mp_reservation_key_handler(struct config *conf, vector strvec, const
> char *file,
> + int line_nr)
> {
> struct mpentry * mpe = VECTOR_LAST_SLOT(conf->mptable);
> if (!mpe)
> @@ -1229,7 +1246,7 @@ snprint_mp_reservation_key (struct config *conf,
> struct strbuf *buff,
> }
>
> static int
> -set_off_int_undef(vector strvec, void *ptr)
> +set_off_int_undef(vector strvec, void *ptr, const char *file, int
> line_nr)
> {
> int *int_ptr = (int *)ptr;
> char * buff;
> @@ -1370,7 +1387,8 @@ declare_hw_snprint(recheck_wwid,
> print_yes_no_undef)
>
>
> static int
> -def_uxsock_timeout_handler(struct config *conf, vector strvec)
> +def_uxsock_timeout_handler(struct config *conf, vector strvec, const
> char *file,
> + int line_nr)
> {
> unsigned int uxsock_timeout;
> char *buff;
> @@ -1390,7 +1408,8 @@ def_uxsock_timeout_handler(struct config *conf,
> vector strvec)
> }
>
> static int
> -hw_vpd_vendor_handler(struct config *conf, vector strvec)
> +hw_vpd_vendor_handler(struct config *conf, vector strvec, const char
> *file,
> + int line_nr)
> {
> int i;
> char *buff;
> @@ -1430,7 +1449,8 @@ snprint_hw_vpd_vendor(struct config *conf, struct
> strbuf *buff,
> * blacklist block handlers
> */
> static int
> -blacklist_handler(struct config *conf, vector strvec)
> +blacklist_handler(struct config *conf, vector strvec, const char*file,
> + int line_nr)
> {
> if (!conf->blist_devnode)
> conf->blist_devnode = vector_alloc();
> @@ -1452,7 +1472,8 @@ blacklist_handler(struct config *conf, vector
> strvec)
> }
>
> static int
> -blacklist_exceptions_handler(struct config *conf, vector strvec)
> +blacklist_exceptions_handler(struct config *conf, vector strvec,
> + const char *file, int line_nr)
> {
> if (!conf->elist_devnode)
> conf->elist_devnode = vector_alloc();
> @@ -1475,7 +1496,8 @@ blacklist_exceptions_handler(struct config *conf,
> vector strvec)
>
> #define
> declare_ble_handler(option) \
> static
> int \
> -ble_ ## option ## _handler (struct config *conf, vector
> strvec) \
> +ble_ ## option ## _handler (struct config *conf, vector
> strvec, \
> + const char *file, int
> line_nr) \
> {
> \
> char
> *buff; \
> int
> rc; \
> @@ -1494,7 +1516,8 @@ ble_ ## option ## _handler (struct config *conf,
> vector strvec) \
>
> #define declare_ble_device_handler(name, option, vend,
> prod) \
> static
> int \
> -ble_ ## option ## _ ## name ## _handler (struct config *conf, vector
> strvec) \
> +ble_ ## option ## _ ## name ## _handler (struct config *conf, vector
> strvec, \
> + const char *file, int
> line_nr) \
> {
> \
> char *
> buff; \
> int
> rc; \
> @@ -1536,13 +1559,15 @@ snprint_ble_simple (struct config *conf, struct
> strbuf *buff, const void *data)
> }
>
> static int
> -ble_device_handler(struct config *conf, vector strvec)
> +ble_device_handler(struct config *conf, vector strvec, const char
> *file,
> + int line_nr)
> {
> return alloc_ble_device(conf->blist_device);
> }
>
> static int
> -ble_except_device_handler(struct config *conf, vector strvec)
> +ble_except_device_handler(struct config *conf, vector strvec, const
> char *file,
> + int line_nr)
> {
> return alloc_ble_device(conf->elist_device);
> }
> @@ -1574,7 +1599,8 @@ static int snprint_bled_product(struct config
> *conf, struct strbuf *buff,
> * devices block handlers
> */
> static int
> -devices_handler(struct config *conf, vector strvec)
> +devices_handler(struct config *conf, vector strvec, const char *file,
> + int line_nr)
> {
> if (!conf->hwtable)
> conf->hwtable = vector_alloc();
> @@ -1586,7 +1612,8 @@ devices_handler(struct config *conf, vector
> strvec)
> }
>
> static int
> -device_handler(struct config *conf, vector strvec)
> +device_handler(struct config *conf, vector strvec, const char *file,
> + int line_nr)
> {
> struct hwentry * hwe;
>
> @@ -1623,7 +1650,8 @@ declare_hw_snprint(hwhandler, print_str)
> * overrides handlers
> */
> static int
> -overrides_handler(struct config *conf, vector strvec)
> +overrides_handler(struct config *conf, vector strvec, const char
> *file,
> + int line_nr)
> {
> if (!conf->overrides)
> conf->overrides = alloc_hwe();
> @@ -1640,7 +1668,8 @@ overrides_handler(struct config *conf, vector
> strvec)
> * multipaths block handlers
> */
> static int
> -multipaths_handler(struct config *conf, vector strvec)
> +multipaths_handler(struct config *conf, vector strvec, const char
> *file,
> + int line_nr)
> {
> if (!conf->mptable)
> conf->mptable = vector_alloc();
> @@ -1652,7 +1681,8 @@ multipaths_handler(struct config *conf, vector
> strvec)
> }
>
> static int
> -multipath_handler(struct config *conf, vector strvec)
> +multipath_handler(struct config *conf, vector strvec, const char
> *file,
> + int line_nr)
> {
> struct mpentry * mpe;
>
> @@ -1681,7 +1711,8 @@ declare_mp_snprint(alias, print_str)
> */
>
> static int
> -deprecated_handler(struct config *conf, vector strvec)
> +deprecated_handler(struct config *conf, vector strvec, const char
> *file,
> + int line_nr)
> {
> char * buff;
>
> diff --git a/libmultipath/parser.c b/libmultipath/parser.c
> index d5595fb0..68262d0e 100644
> --- a/libmultipath/parser.c
> +++ b/libmultipath/parser.c
> @@ -558,7 +558,8 @@ process_stream(struct config *conf, FILE *stream,
> vector keywords,
> goto out;
> }
> if (keyword->handler) {
> - t = keyword->handler(conf, strvec);
> + t = keyword->handler(conf, strvec,
> file,
> + line_nr);
> r += t;
> if (t)
> condlog(1, "%s line %d,
> parsing failed: %s",
> diff --git a/libmultipath/parser.h b/libmultipath/parser.h
> index 3452bde1..11ea2278 100644
> --- a/libmultipath/parser.h
> +++ b/libmultipath/parser.h
> @@ -43,7 +43,7 @@ struct strbuf;
>
> /* keyword definition */
> typedef int print_fn(struct config *, struct strbuf *, const void *);
> -typedef int handler_fn(struct config *, vector);
> +typedef int handler_fn(struct config *, vector, const char *file, int
> line_nr);
>
> struct keyword {
> char *string;
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [dm-devel] [PATCH 3/8] libmultipath: print the correct file when parsing fails
2021-10-06 20:04 ` [dm-devel] [PATCH 3/8] libmultipath: print the correct file when parsing fails Benjamin Marzinski
@ 2021-11-04 20:55 ` Martin Wilck
0 siblings, 0 replies; 29+ messages in thread
From: Martin Wilck @ 2021-11-04 20:55 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel
On Wed, 2021-10-06 at 15:04 -0500, Benjamin Marzinski wrote:
> Don't assume that parsing failed on multipath.conf
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
> ---
> libmultipath/parser.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [dm-devel] [PATCH 2/8] libmultipath: use typedef for keyword handler function
2021-10-06 20:04 ` [dm-devel] [PATCH 2/8] libmultipath: use typedef for keyword handler function Benjamin Marzinski
@ 2021-11-04 20:55 ` Martin Wilck
0 siblings, 0 replies; 29+ messages in thread
From: Martin Wilck @ 2021-11-04 20:55 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel
On Wed, 2021-10-06 at 15:04 -0500, Benjamin Marzinski wrote:
> Don't keep writing out the function type.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
> ---
> libmultipath/parser.c | 6 +++---
> libmultipath/parser.h | 15 ++++++---------
> 2 files changed, 9 insertions(+), 12 deletions(-)
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [dm-devel] [PATCH 1/8] libmulitpath: add section name to invalid keyword output
2021-10-06 20:04 ` [dm-devel] [PATCH 1/8] libmulitpath: add section name to invalid keyword output Benjamin Marzinski
@ 2021-11-04 20:55 ` Martin Wilck
2021-11-05 17:28 ` Benjamin Marzinski
0 siblings, 1 reply; 29+ messages in thread
From: Martin Wilck @ 2021-11-04 20:55 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel
On Wed, 2021-10-06 at 15:04 -0500, Benjamin Marzinski wrote:
> If users forget the closing brace for a section in multipath.conf,
> multipath has no way to detect that. When it sees the keyword at the
> start of the next section, it will complain that there is an invalid
> keyword, because that keyword doesn't belong in previous section
> (which
> was never ended with a closing brace). This can confuse users. To
> make
> this easier to understand, when multipath prints and invalid keyword
> message, it now also prints the current section name, which can give
> users a hint that they didn't end the previous section.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Apart from typo in the subject:
Reviewed-by: Martin Wilck <mwilck@suse.com>
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [dm-devel] [PATCH 8/8] libmultipath: cleanup invalid config handling
2021-10-06 20:04 ` [dm-devel] [PATCH 8/8] libmultipath: cleanup invalid config handling Benjamin Marzinski
@ 2021-11-04 21:11 ` Martin Wilck
2021-11-05 20:10 ` Benjamin Marzinski
0 siblings, 1 reply; 29+ messages in thread
From: Martin Wilck @ 2021-11-04 21:11 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel
On Wed, 2021-10-06 at 15:04 -0500, Benjamin Marzinski wrote:
> Add error reporting to the remaining config handlers. If the value is
> invalid, do not change the existing config option's value.
Like for the previous patch, I'm unsure if this is wise. You rely on a
reasonable default being set before the function is called. I suppose
that's the case, but I like seeing the "invalid" value substituted
right there where the validity is checked. That saves us from searching
the code for the default value.
Maybe I overlooked an important rationale for not touching the values
in the case of invalid input, please explain.
Cheers,
Martin
> Also print
> an error whenever 0 is returned for an invalid value. When the
> handler
> returns 1, config processing already fails with an error message.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/dict.c | 73 +++++++++++++++++++++++++++++++------------
> --
> 1 file changed, 51 insertions(+), 22 deletions(-)
>
> diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> index e79fcdd7..8c3b5f72 100644
> --- a/libmultipath/dict.c
> +++ b/libmultipath/dict.c
> @@ -199,8 +199,11 @@ set_yes_no(vector strvec, void *ptr, const char
> *file, int line_nr)
>
> if (strcmp(buff, "yes") == 0 || strcmp(buff, "1") == 0)
> *int_ptr = YN_YES;
> - else
> + else if (strcmp(buff, "no") == 0 || strcmp(buff, "0") == 0)
> *int_ptr = YN_NO;
> + else
> + condlog(1, "%s line %d, invalid value for %s:
> \"%s\"",
> + file, line_nr, (char*)VECTOR_SLOT(strvec, 0),
> buff);
>
> FREE(buff);
> return 0;
> @@ -221,7 +224,8 @@ set_yes_no_undef(vector strvec, void *ptr, const
> char *file, int line_nr)
> else if (strcmp(buff, "yes") == 0 || strcmp(buff, "1") == 0)
> *int_ptr = YNU_YES;
> else
> - *int_ptr = YNU_UNDEF;
> + condlog(1, "%s line %d, invalid value for %s:
> \"%s\"",
> + file, line_nr, (char*)VECTOR_SLOT(strvec, 0),
> buff);
>
> FREE(buff);
> return 0;
> @@ -471,9 +475,6 @@ def_find_multipaths_handler(struct config *conf,
> vector strvec,
> char *buff;
> int i;
>
> - if (set_yes_no_undef(strvec, &conf->find_multipaths, file,
> line_nr) == 0 && conf->find_multipaths != FIND_MULTIPATHS_UNDEF)
> - return 0;
> -
> buff = set_value(strvec);
> if (!buff)
> return 1;
> @@ -486,9 +487,14 @@ def_find_multipaths_handler(struct config *conf,
> vector strvec,
> }
> }
>
> - if (conf->find_multipaths == YNU_UNDEF) {
> - condlog(0, "illegal value for find_multipaths: %s",
> buff);
> - conf->find_multipaths = DEFAULT_FIND_MULTIPATHS;
> + if (i >= __FIND_MULTIPATHS_LAST) {
> + if (strcmp(buff, "no") == 0 || strcmp(buff, "0") ==
> 0)
> + conf->find_multipaths = FIND_MULTIPATHS_OFF;
> + else if (strcmp(buff, "yes") == 0 || strcmp(buff,
> "1") == 0)
> + conf->find_multipaths = FIND_MULTIPATHS_ON;
> + else
> + condlog(1, "%s line %d, invalid value for
> find_multipaths: \"%s\"",
> + file, line_nr, buff);
> }
>
> FREE(buff);
> @@ -537,8 +543,10 @@ static int uid_attrs_handler(struct config
> *conf, vector strvec,
> if (!val)
> return 1;
> if (parse_uid_attrs(val, conf))
> - condlog(1, "error parsing uid_attrs: \"%s\"", val);
> - condlog(3, "parsed %d uid_attrs", VECTOR_SIZE(&conf-
> >uid_attrs));
> + condlog(1, "%s line %d,error parsing uid_attrs:
> \"%s\"", file,
> + line_nr, val);
> + else
> + condlog(4, "parsed %d uid_attrs", VECTOR_SIZE(&conf-
> >uid_attrs));
> FREE(val);
> return 0;
> }
> @@ -766,8 +774,11 @@ def_config_dir_handler(struct config *conf,
> vector strvec, const char *file,
> int line_nr)
> {
> /* this is only valid in the main config file */
> - if (conf->processed_main_config)
> + if (conf->processed_main_config) {
> + condlog(1, "%s line %d, config_dir option only valid
> in /etc/multipath.conf",
> + file, line_nr);
> return 0;
> + }
> return set_path(strvec, &conf->config_dir, file, line_nr);
> }
> declare_def_snprint(config_dir, print_str)
> @@ -825,7 +836,9 @@ set_mode(vector strvec, void *ptr, int *flags,
> const char *file, int line_nr)
> if (sscanf(buff, "%o", &mode) == 1 && mode <= 0777) {
> *flags |= (1 << ATTR_MODE);
> *mode_ptr = mode;
> - }
> + } else
> + condlog(1, "%s line %d, invalid value for mode:
> \"%s\"",
> + file, line_nr, buff);
>
> FREE(buff);
> return 0;
> @@ -850,7 +863,9 @@ set_uid(vector strvec, void *ptr, int *flags,
> const char *file, int line_nr)
> else if (sscanf(buff, "%u", &uid) == 1){
> *flags |= (1 << ATTR_UID);
> *uid_ptr = uid;
> - }
> + } else
> + condlog(1, "%s line %d, invalid value for uid:
> \"%s\"",
> + file, line_nr, buff);
>
> FREE(buff);
> return 0;
> @@ -876,7 +891,9 @@ set_gid(vector strvec, void *ptr, int *flags,
> const char *file, int line_nr)
> else if (sscanf(buff, "%u", &gid) == 1){
> *flags |= (1 << ATTR_GID);
> *gid_ptr = gid;
> - }
> + } else
> + condlog(1, "%s line %d, invalid value for gid:
> \"%s\"",
> + file, line_nr, buff);
> FREE(buff);
> return 0;
> }
> @@ -978,7 +995,8 @@ set_dev_loss(vector strvec, void *ptr, const char
> *file, int line_nr)
> if (!strcmp(buff, "infinity"))
> *uint_ptr = MAX_DEV_LOSS_TMO;
> else if (sscanf(buff, "%u", uint_ptr) != 1)
> - *uint_ptr = DEV_LOSS_TMO_UNSET;
> + condlog(1, "%s line %d, invalid value for
> dev_loss_tmo: \"%s\"",
> + file, line_nr, buff);
>
> FREE(buff);
> return 0;
> @@ -1012,13 +1030,19 @@ static int
> set_pgpolicy(vector strvec, void *ptr, const char *file, int
> line_nr)
> {
> char * buff;
> + int policy;
> int *int_ptr = (int *)ptr;
>
> buff = set_value(strvec);
> if (!buff)
> return 1;
>
> - *int_ptr = get_pgpolicy_id(buff);
> + policy = get_pgpolicy_id(buff);
> + if (policy != IOPOLICY_UNDEF)
> + *int_ptr = policy;
> + else
> + condlog(1, "%s line %d, invalid value for
> path_grouping_policy: \"%s\"",
> + file, line_nr, buff);
> FREE(buff);
>
> return 0;
> @@ -1131,10 +1155,11 @@ set_rr_weight(vector strvec, void *ptr, const
> char *file, int line_nr)
>
> if (!strcmp(buff, "priorities"))
> *int_ptr = RR_WEIGHT_PRIO;
> -
> - if (!strcmp(buff, "uniform"))
> + else if (!strcmp(buff, "uniform"))
> *int_ptr = RR_WEIGHT_NONE;
> -
> + else
> + condlog(1, "%s line %d, invalid value for rr_weight:
> \"%s\"",
> + file, line_nr, buff);
> FREE(buff);
>
> return 0;
> @@ -1270,10 +1295,13 @@ def_log_checker_err_handler(struct config
> *conf, vector strvec,
> if (!buff)
> return 1;
>
> - if (strlen(buff) == 4 && !strcmp(buff, "once"))
> + if (!strcmp(buff, "once"))
> conf->log_checker_err = LOG_CHKR_ERR_ONCE;
> - else if (strlen(buff) == 6 && !strcmp(buff, "always"))
> + else if (!strcmp(buff, "always"))
> conf->log_checker_err = LOG_CHKR_ERR_ALWAYS;
> + else
> + condlog(1, "%s line %d, invalid value for
> log_checker_err: \"%s\"",
> + file, line_nr, buff);
>
We lack a proper DEFAULT for log_checker_err.
> free(buff);
> return 0;
> @@ -1534,7 +1562,8 @@ hw_vpd_vendor_handler(struct config *conf,
> vector strvec, const char *file,
> goto out;
> }
> }
> - hwe->vpd_vendor_id = 0;
> + condlog(1, "%s line %d, invalid value for vpd_vendor:
> \"%s\"",
> + file, line_nr, buff);
> out:
> FREE(buff);
> return 0;
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [dm-devel] [PATCH 6/8] libmultipath: improve checks for set_str
2021-11-04 20:34 ` Martin Wilck
@ 2021-11-05 6:59 ` Martin Wilck
2021-11-05 17:45 ` Benjamin Marzinski
2021-11-05 17:38 ` Benjamin Marzinski
1 sibling, 1 reply; 29+ messages in thread
From: Martin Wilck @ 2021-11-05 6:59 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [dm-devel] [PATCH 1/8] libmulitpath: add section name to invalid keyword output
2021-11-04 20:55 ` Martin Wilck
@ 2021-11-05 17:28 ` Benjamin Marzinski
0 siblings, 0 replies; 29+ messages in thread
From: Benjamin Marzinski @ 2021-11-05 17:28 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel
On Thu, Nov 04, 2021 at 08:55:39PM +0000, Martin Wilck wrote:
> On Wed, 2021-10-06 at 15:04 -0500, Benjamin Marzinski wrote:
> > If users forget the closing brace for a section in multipath.conf,
> > multipath has no way to detect that. When it sees the keyword at the
> > start of the next section, it will complain that there is an invalid
> > keyword, because that keyword doesn't belong in previous section
> > (which
> > was never ended with a closing brace). This can confuse users. To
> > make
> > this easier to understand, when multipath prints and invalid keyword
> > message, it now also prints the current section name, which can give
> > users a hint that they didn't end the previous section.
> >
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
>
> Apart from typo in the subject:
Oops. I'll fix that.
-Ben
>
> Reviewed-by: Martin Wilck <mwilck@suse.com>
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [dm-devel] [PATCH 4/8] libmultipath: pass file and line number to keyword handlers
2021-11-04 20:55 ` Martin Wilck
@ 2021-11-05 17:33 ` Benjamin Marzinski
0 siblings, 0 replies; 29+ messages in thread
From: Benjamin Marzinski @ 2021-11-05 17:33 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel
On Thu, Nov 04, 2021 at 08:55:02PM +0000, Martin Wilck wrote:
> On Wed, 2021-10-06 at 15:04 -0500, Benjamin Marzinski wrote:
> > This will make it possible for the keyword handlers to print more
> > useful
> > warning messages. It will be used by future patches.
> >
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
>
> Nit: There's one very long line (349).
>
> Apart from that, ack.
It gets removed by "libmultipath: cleanup invalid config handling", but
I can fix it in this patch, if you'd like.
-Ben
>
>
>
> > ---
> > libmultipath/dict.c | 143 +++++++++++++++++++++++++-----------------
> > libmultipath/parser.c | 3 +-
> > libmultipath/parser.h | 2 +-
> > 3 files changed, 90 insertions(+), 58 deletions(-)
> >
> > diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> > index 7a727389..eb2c44c0 100644
> > --- a/libmultipath/dict.c
> > +++ b/libmultipath/dict.c
> > @@ -29,7 +29,7 @@
> > #include "strbuf.h"
> >
> > static int
> > -set_int(vector strvec, void *ptr)
> > +set_int(vector strvec, void *ptr, const char *file, int line_nr)
> > {
> > int *int_ptr = (int *)ptr;
> > char *buff, *eptr;
> > @@ -58,7 +58,7 @@ set_int(vector strvec, void *ptr)
> > }
> >
> > static int
> > -set_uint(vector strvec, void *ptr)
> > +set_uint(vector strvec, void *ptr, const char *file, int line_nr)
> > {
> > unsigned int *uint_ptr = (unsigned int *)ptr;
> > char *buff, *eptr, *p;
> > @@ -90,7 +90,7 @@ set_uint(vector strvec, void *ptr)
> > }
> >
> > static int
> > -set_str(vector strvec, void *ptr)
> > +set_str(vector strvec, void *ptr, const char *file, int line_nr)
> > {
> > char **str_ptr = (char **)ptr;
> >
> > @@ -105,7 +105,7 @@ set_str(vector strvec, void *ptr)
> > }
> >
> > static int
> > -set_yes_no(vector strvec, void *ptr)
> > +set_yes_no(vector strvec, void *ptr, const char *file, int line_nr)
> > {
> > char * buff;
> > int *int_ptr = (int *)ptr;
> > @@ -124,7 +124,7 @@ set_yes_no(vector strvec, void *ptr)
> > }
> >
> > static int
> > -set_yes_no_undef(vector strvec, void *ptr)
> > +set_yes_no_undef(vector strvec, void *ptr, const char *file, int
> > line_nr)
> > {
> > char * buff;
> > int *int_ptr = (int *)ptr;
> > @@ -187,9 +187,10 @@ static int print_yes_no_undef(struct strbuf *buff,
> > long v)
> >
> > #define declare_def_handler(option,
> > function) \
> > static
> > int \
> > -def_ ## option ## _handler (struct config *conf, vector
> > strvec) \
> > +def_ ## option ## _handler (struct config *conf, vector
> > strvec, \
> > + const char *file, int
> > line_nr) \
> > {
> > \
> > - return function (strvec, &conf-
> > >option); \
> > + return function (strvec, &conf->option, file,
> > line_nr); \
> > }
> >
> > #define declare_def_snprint(option,
> > function) \
> > @@ -224,12 +225,13 @@ snprint_def_ ## option (struct config *conf,
> > struct strbuf *buff, \
> >
> > #define declare_hw_handler(option,
> > function) \
> > static
> > int \
> > -hw_ ## option ## _handler (struct config *conf, vector
> > strvec) \
> > +hw_ ## option ## _handler (struct config *conf, vector
> > strvec, \
> > + const char *file, int
> > line_nr) \
> > {
> > \
> > struct hwentry * hwe = VECTOR_LAST_SLOT(conf-
> > >hwtable); \
> > if
> > (!hwe) \
> > return
> > 1; \
> > - return function (strvec, &hwe-
> > >option); \
> > + return function (strvec, &hwe->option, file,
> > line_nr); \
> > }
> >
> > #define declare_hw_snprint(option,
> > function) \
> > @@ -243,11 +245,12 @@ snprint_hw_ ## option (struct config *conf,
> > struct strbuf *buff, \
> >
> > #define declare_ovr_handler(option,
> > function) \
> > static
> > int \
> > -ovr_ ## option ## _handler (struct config *conf, vector
> > strvec) \
> > +ovr_ ## option ## _handler (struct config *conf, vector
> > strvec, \
> > + const char *file, int
> > line_nr) \
> > {
> > \
> > if (!conf-
> > >overrides) \
> > return
> > 1; \
> > - return function (strvec, &conf->overrides-
> > >option); \
> > + return function (strvec, &conf->overrides->option, file,
> > line_nr); \
> > }
> >
> > #define declare_ovr_snprint(option,
> > function) \
> > @@ -260,12 +263,13 @@ snprint_ovr_ ## option (struct config *conf,
> > struct strbuf *buff, \
> >
> > #define declare_mp_handler(option,
> > function) \
> > static
> > int \
> > -mp_ ## option ## _handler (struct config *conf, vector
> > strvec) \
> > +mp_ ## option ## _handler (struct config *conf, vector
> > strvec, \
> > + const char *file, int
> > line_nr) \
> > {
> > \
> > struct mpentry * mpe = VECTOR_LAST_SLOT(conf-
> > >mptable); \
> > if
> > (!mpe) \
> > return
> > 1; \
> > - return function (strvec, &mpe-
> > >option); \
> > + return function (strvec, &mpe->option, file,
> > line_nr); \
> > }
> >
> > #define declare_mp_snprint(option,
> > function) \
> > @@ -277,9 +281,10 @@ snprint_mp_ ## option (struct config *conf, struct
> > strbuf *buff, \
> > return function(buff, mpe-
> > >option); \
> > }
> >
> > -static int checkint_handler(struct config *conf, vector strvec)
> > +static int checkint_handler(struct config *conf, vector strvec,
> > + const char *file, int line_nr)
> > {
> > - int rc = set_uint(strvec, &conf->checkint);
> > + int rc = set_uint(strvec, &conf->checkint, file, line_nr);
> >
> > if (rc)
> > return rc;
> > @@ -302,9 +307,10 @@ declare_def_snprint(reassign_maps, print_yes_no)
> > declare_def_handler(multipath_dir, set_str)
> > declare_def_snprint(multipath_dir, print_str)
> >
> > -static int def_partition_delim_handler(struct config *conf, vector
> > strvec)
> > +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);
> > + int rc = set_str(strvec, &conf->partition_delim, file,
> > line_nr);
> >
> > if (rc != 0)
> > return rc;
> > @@ -334,13 +340,13 @@ static const char * const
> > find_multipaths_optvals[] = {
> > };
> >
> > static int
> > -def_find_multipaths_handler(struct config *conf, vector strvec)
> > +def_find_multipaths_handler(struct config *conf, vector strvec,
> > + const char *file, int line_nr)
> > {
> > char *buff;
> > int i;
> >
> > - if (set_yes_no_undef(strvec, &conf->find_multipaths) == 0 &&
> > - conf->find_multipaths != FIND_MULTIPATHS_UNDEF)
> > + if (set_yes_no_undef(strvec, &conf->find_multipaths, file,
> > line_nr) == 0 && conf->find_multipaths != FIND_MULTIPATHS_UNDEF)
> > return 0;
> >
> > buff = set_value(strvec);
> > @@ -396,7 +402,8 @@ static int snprint_uid_attrs(struct config *conf,
> > struct strbuf *buff,
> > return total;
> > }
> >
> > -static int uid_attrs_handler(struct config *conf, vector strvec)
> > +static int uid_attrs_handler(struct config *conf, vector strvec,
> > + const char *file, int line_nr)
> > {
> > char *val;
> >
> > @@ -597,7 +604,8 @@ declare_hw_handler(skip_kpartx, set_yes_no_undef)
> > declare_hw_snprint(skip_kpartx, print_yes_no_undef)
> > declare_mp_handler(skip_kpartx, set_yes_no_undef)
> > declare_mp_snprint(skip_kpartx, print_yes_no_undef)
> > -static int def_disable_changed_wwids_handler(struct config *conf,
> > vector strvec)
> > +static int def_disable_changed_wwids_handler(struct config *conf,
> > vector strvec,
> > + const char *file, int
> > line_nr)
> > {
> > return 0;
> > }
> > @@ -629,20 +637,23 @@ declare_def_snprint_defstr(enable_foreign,
> > print_str,
> > DEFAULT_ENABLE_FOREIGN)
> >
> > static int
> > -def_config_dir_handler(struct config *conf, vector strvec)
> > +def_config_dir_handler(struct config *conf, vector strvec, const char
> > *file,
> > + int line_nr)
> > {
> > /* this is only valid in the main config file */
> > if (conf->processed_main_config)
> > return 0;
> > - return set_str(strvec, &conf->config_dir);
> > + return set_str(strvec, &conf->config_dir, file, line_nr);
> > }
> > declare_def_snprint(config_dir, print_str)
> >
> > #define declare_def_attr_handler(option,
> > function) \
> > static
> > int \
> > -def_ ## option ## _handler (struct config *conf, vector
> > strvec) \
> > +def_ ## option ## _handler (struct config *conf, vector
> > strvec, \
> > + const char *file, int
> > line_nr) \
> > {
> > \
> > - return function (strvec, &conf->option, &conf-
> > >attribute_flags);\
> > + return function (strvec, &conf->option, &conf->attribute_flags,
> > \
> > + file,
> > line_nr); \
> > }
> >
> > #define declare_def_attr_snprint(option,
> > function) \
> > @@ -655,12 +666,14 @@ snprint_def_ ## option (struct config *conf,
> > struct strbuf *buff, \
> >
> > #define declare_mp_attr_handler(option,
> > function) \
> > static
> > int \
> > -mp_ ## option ## _handler (struct config *conf, vector
> > strvec) \
> > +mp_ ## option ## _handler (struct config *conf, vector
> > strvec, \
> > + const char *file, int
> > line_nr) \
> > {
> > \
> > struct mpentry * mpe = VECTOR_LAST_SLOT(conf-
> > >mptable); \
> > if
> > (!mpe) \
> > return
> > 1; \
> > - return function (strvec, &mpe->option, &mpe-
> > >attribute_flags); \
> > + return function (strvec, &mpe->option, &mpe-
> > >attribute_flags, \
> > + file,
> > line_nr); \
> > }
> >
> > #define declare_mp_attr_snprint(option,
> > function) \
> > @@ -673,7 +686,7 @@ snprint_mp_ ## option (struct config *conf, struct
> > strbuf *buff, \
> > }
> >
> > static int
> > -set_mode(vector strvec, void *ptr, int *flags)
> > +set_mode(vector strvec, void *ptr, int *flags, const char *file, int
> > line_nr)
> > {
> > mode_t mode;
> > mode_t *mode_ptr = (mode_t *)ptr;
> > @@ -694,7 +707,7 @@ set_mode(vector strvec, void *ptr, int *flags)
> > }
> >
> > static int
> > -set_uid(vector strvec, void *ptr, int *flags)
> > +set_uid(vector strvec, void *ptr, int *flags, const char *file, int
> > line_nr)
> > {
> > uid_t uid;
> > uid_t *uid_ptr = (uid_t *)ptr;
> > @@ -719,7 +732,7 @@ set_uid(vector strvec, void *ptr, int *flags)
> > }
> >
> > static int
> > -set_gid(vector strvec, void *ptr, int *flags)
> > +set_gid(vector strvec, void *ptr, int *flags, const char *file, int
> > line_nr)
> > {
> > gid_t gid;
> > gid_t *gid_ptr = (gid_t *)ptr;
> > @@ -786,7 +799,7 @@ declare_mp_attr_handler(gid, set_gid)
> > declare_mp_attr_snprint(gid, print_gid)
> >
> > static int
> > -set_undef_off_zero(vector strvec, void *ptr)
> > +set_undef_off_zero(vector strvec, void *ptr, const char *file, int
> > line_nr)
> > {
> > char * buff;
> > int *int_ptr = (int *)ptr;
> > @@ -827,7 +840,7 @@ declare_hw_handler(fast_io_fail,
> > set_undef_off_zero)
> > declare_hw_snprint(fast_io_fail, print_undef_off_zero)
> >
> > static int
> > -set_dev_loss(vector strvec, void *ptr)
> > +set_dev_loss(vector strvec, void *ptr, const char *file, int line_nr)
> > {
> > char * buff;
> > unsigned int *uint_ptr = (unsigned int *)ptr;
> > @@ -870,7 +883,7 @@ declare_hw_handler(eh_deadline, set_undef_off_zero)
> > declare_hw_snprint(eh_deadline, print_undef_off_zero)
> >
> > static int
> > -set_pgpolicy(vector strvec, void *ptr)
> > +set_pgpolicy(vector strvec, void *ptr, const char *file, int line_nr)
> > {
> > char * buff;
> > int *int_ptr = (int *)ptr;
> > @@ -936,7 +949,8 @@ get_sys_max_fds(int *max_fds)
> >
> >
> > static int
> > -max_fds_handler(struct config *conf, vector strvec)
> > +max_fds_handler(struct config *conf, vector strvec, const char *file,
> > + int line_nr)
> > {
> > char * buff;
> > int r = 0, max_fds;
> > @@ -981,7 +995,7 @@ snprint_max_fds (struct config *conf, struct strbuf
> > *buff, const void *data)
> > }
> >
> > static int
> > -set_rr_weight(vector strvec, void *ptr)
> > +set_rr_weight(vector strvec, void *ptr, const char *file, int line_nr)
> > {
> > int *int_ptr = (int *)ptr;
> > char * buff;
> > @@ -1025,7 +1039,7 @@ declare_mp_handler(rr_weight, set_rr_weight)
> > declare_mp_snprint(rr_weight, print_rr_weight)
> >
> > static int
> > -set_pgfailback(vector strvec, void *ptr)
> > +set_pgfailback(vector strvec, void *ptr, const char *file, int
> > line_nr)
> > {
> > int *int_ptr = (int *)ptr;
> > char * buff;
> > @@ -1075,7 +1089,7 @@ declare_mp_handler(pgfailback, set_pgfailback)
> > declare_mp_snprint(pgfailback, print_pgfailback)
> >
> > static int
> > -no_path_retry_helper(vector strvec, void *ptr)
> > +no_path_retry_helper(vector strvec, void *ptr, const char *file, int
> > line_nr)
> > {
> > int *int_ptr = (int *)ptr;
> > char * buff;
> > @@ -1120,7 +1134,8 @@ declare_mp_handler(no_path_retry,
> > no_path_retry_helper)
> > declare_mp_snprint(no_path_retry, print_no_path_retry)
> >
> > static int
> > -def_log_checker_err_handler(struct config *conf, vector strvec)
> > +def_log_checker_err_handler(struct config *conf, vector strvec,
> > + const char *file, int line_nr)
> > {
> > char * buff;
> >
> > @@ -1193,7 +1208,8 @@ print_reservation_key(struct strbuf *buff,
> > }
> >
> > static int
> > -def_reservation_key_handler(struct config *conf, vector strvec)
> > +def_reservation_key_handler(struct config *conf, vector strvec,
> > + const char *file, int line_nr)
> > {
> > return set_reservation_key(strvec, &conf->reservation_key,
> > &conf->sa_flags,
> > @@ -1209,7 +1225,8 @@ snprint_def_reservation_key (struct config *conf,
> > struct strbuf *buff,
> > }
> >
> > static int
> > -mp_reservation_key_handler(struct config *conf, vector strvec)
> > +mp_reservation_key_handler(struct config *conf, vector strvec, const
> > char *file,
> > + int line_nr)
> > {
> > struct mpentry * mpe = VECTOR_LAST_SLOT(conf->mptable);
> > if (!mpe)
> > @@ -1229,7 +1246,7 @@ snprint_mp_reservation_key (struct config *conf,
> > struct strbuf *buff,
> > }
> >
> > static int
> > -set_off_int_undef(vector strvec, void *ptr)
> > +set_off_int_undef(vector strvec, void *ptr, const char *file, int
> > line_nr)
> > {
> > int *int_ptr = (int *)ptr;
> > char * buff;
> > @@ -1370,7 +1387,8 @@ declare_hw_snprint(recheck_wwid,
> > print_yes_no_undef)
> >
> >
> > static int
> > -def_uxsock_timeout_handler(struct config *conf, vector strvec)
> > +def_uxsock_timeout_handler(struct config *conf, vector strvec, const
> > char *file,
> > + int line_nr)
> > {
> > unsigned int uxsock_timeout;
> > char *buff;
> > @@ -1390,7 +1408,8 @@ def_uxsock_timeout_handler(struct config *conf,
> > vector strvec)
> > }
> >
> > static int
> > -hw_vpd_vendor_handler(struct config *conf, vector strvec)
> > +hw_vpd_vendor_handler(struct config *conf, vector strvec, const char
> > *file,
> > + int line_nr)
> > {
> > int i;
> > char *buff;
> > @@ -1430,7 +1449,8 @@ snprint_hw_vpd_vendor(struct config *conf, struct
> > strbuf *buff,
> > * blacklist block handlers
> > */
> > static int
> > -blacklist_handler(struct config *conf, vector strvec)
> > +blacklist_handler(struct config *conf, vector strvec, const char*file,
> > + int line_nr)
> > {
> > if (!conf->blist_devnode)
> > conf->blist_devnode = vector_alloc();
> > @@ -1452,7 +1472,8 @@ blacklist_handler(struct config *conf, vector
> > strvec)
> > }
> >
> > static int
> > -blacklist_exceptions_handler(struct config *conf, vector strvec)
> > +blacklist_exceptions_handler(struct config *conf, vector strvec,
> > + const char *file, int line_nr)
> > {
> > if (!conf->elist_devnode)
> > conf->elist_devnode = vector_alloc();
> > @@ -1475,7 +1496,8 @@ blacklist_exceptions_handler(struct config *conf,
> > vector strvec)
> >
> > #define
> > declare_ble_handler(option) \
> > static
> > int \
> > -ble_ ## option ## _handler (struct config *conf, vector
> > strvec) \
> > +ble_ ## option ## _handler (struct config *conf, vector
> > strvec, \
> > + const char *file, int
> > line_nr) \
> > {
> > \
> > char
> > *buff; \
> > int
> > rc; \
> > @@ -1494,7 +1516,8 @@ ble_ ## option ## _handler (struct config *conf,
> > vector strvec) \
> >
> > #define declare_ble_device_handler(name, option, vend,
> > prod) \
> > static
> > int \
> > -ble_ ## option ## _ ## name ## _handler (struct config *conf, vector
> > strvec) \
> > +ble_ ## option ## _ ## name ## _handler (struct config *conf, vector
> > strvec, \
> > + const char *file, int
> > line_nr) \
> > {
> > \
> > char *
> > buff; \
> > int
> > rc; \
> > @@ -1536,13 +1559,15 @@ snprint_ble_simple (struct config *conf, struct
> > strbuf *buff, const void *data)
> > }
> >
> > static int
> > -ble_device_handler(struct config *conf, vector strvec)
> > +ble_device_handler(struct config *conf, vector strvec, const char
> > *file,
> > + int line_nr)
> > {
> > return alloc_ble_device(conf->blist_device);
> > }
> >
> > static int
> > -ble_except_device_handler(struct config *conf, vector strvec)
> > +ble_except_device_handler(struct config *conf, vector strvec, const
> > char *file,
> > + int line_nr)
> > {
> > return alloc_ble_device(conf->elist_device);
> > }
> > @@ -1574,7 +1599,8 @@ static int snprint_bled_product(struct config
> > *conf, struct strbuf *buff,
> > * devices block handlers
> > */
> > static int
> > -devices_handler(struct config *conf, vector strvec)
> > +devices_handler(struct config *conf, vector strvec, const char *file,
> > + int line_nr)
> > {
> > if (!conf->hwtable)
> > conf->hwtable = vector_alloc();
> > @@ -1586,7 +1612,8 @@ devices_handler(struct config *conf, vector
> > strvec)
> > }
> >
> > static int
> > -device_handler(struct config *conf, vector strvec)
> > +device_handler(struct config *conf, vector strvec, const char *file,
> > + int line_nr)
> > {
> > struct hwentry * hwe;
> >
> > @@ -1623,7 +1650,8 @@ declare_hw_snprint(hwhandler, print_str)
> > * overrides handlers
> > */
> > static int
> > -overrides_handler(struct config *conf, vector strvec)
> > +overrides_handler(struct config *conf, vector strvec, const char
> > *file,
> > + int line_nr)
> > {
> > if (!conf->overrides)
> > conf->overrides = alloc_hwe();
> > @@ -1640,7 +1668,8 @@ overrides_handler(struct config *conf, vector
> > strvec)
> > * multipaths block handlers
> > */
> > static int
> > -multipaths_handler(struct config *conf, vector strvec)
> > +multipaths_handler(struct config *conf, vector strvec, const char
> > *file,
> > + int line_nr)
> > {
> > if (!conf->mptable)
> > conf->mptable = vector_alloc();
> > @@ -1652,7 +1681,8 @@ multipaths_handler(struct config *conf, vector
> > strvec)
> > }
> >
> > static int
> > -multipath_handler(struct config *conf, vector strvec)
> > +multipath_handler(struct config *conf, vector strvec, const char
> > *file,
> > + int line_nr)
> > {
> > struct mpentry * mpe;
> >
> > @@ -1681,7 +1711,8 @@ declare_mp_snprint(alias, print_str)
> > */
> >
> > static int
> > -deprecated_handler(struct config *conf, vector strvec)
> > +deprecated_handler(struct config *conf, vector strvec, const char
> > *file,
> > + int line_nr)
> > {
> > char * buff;
> >
> > diff --git a/libmultipath/parser.c b/libmultipath/parser.c
> > index d5595fb0..68262d0e 100644
> > --- a/libmultipath/parser.c
> > +++ b/libmultipath/parser.c
> > @@ -558,7 +558,8 @@ process_stream(struct config *conf, FILE *stream,
> > vector keywords,
> > goto out;
> > }
> > if (keyword->handler) {
> > - t = keyword->handler(conf, strvec);
> > + t = keyword->handler(conf, strvec,
> > file,
> > + line_nr);
> > r += t;
> > if (t)
> > condlog(1, "%s line %d,
> > parsing failed: %s",
> > diff --git a/libmultipath/parser.h b/libmultipath/parser.h
> > index 3452bde1..11ea2278 100644
> > --- a/libmultipath/parser.h
> > +++ b/libmultipath/parser.h
> > @@ -43,7 +43,7 @@ struct strbuf;
> >
> > /* keyword definition */
> > typedef int print_fn(struct config *, struct strbuf *, const void *);
> > -typedef int handler_fn(struct config *, vector);
> > +typedef int handler_fn(struct config *, vector, const char *file, int
> > line_nr);
> >
> > struct keyword {
> > char *string;
>
>
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [dm-devel] [PATCH 5/8] libmultipath: make set_int take a range for valid values
2021-11-04 20:34 ` Martin Wilck
@ 2021-11-05 17:34 ` Benjamin Marzinski
0 siblings, 0 replies; 29+ messages in thread
From: Benjamin Marzinski @ 2021-11-05 17:34 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel
On Thu, Nov 04, 2021 at 08:34:33PM +0000, Martin Wilck wrote:
> On Wed, 2021-10-06 at 15:04 -0500, Benjamin Marzinski wrote:
> > If a value outside of the valid range is passed to set_int, it caps
> > the
> > value at appropriate limit, and issues a warning.
> >
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
>
> One nitpick below.
>
> It's a lot of code changes for just two cases where we have nontrivial
> values for min and max. I guess for uint the count of nontrivial cases
> was zero?
Yeah. all the uint cases accepted the entire UINT range. As you've
seen, I end up using the int range checking for more functions later.
> Yet it's an improvement, so I'm going to ack when the nit is fixed.
>
> Martin
>
>
>
> > ---
> > libmultipath/dict.c | 121 +++++++++++++++++++++++++++---------------
> > --
> > 1 file changed, 75 insertions(+), 46 deletions(-)
> >
> > diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> > index eb2c44c0..1758bd26 100644
> > --- a/libmultipath/dict.c
> > +++ b/libmultipath/dict.c
> > ...
> > @@ -298,7 +347,7 @@ declare_def_snprint(checkint, print_int)
> > declare_def_handler(max_checkint, set_uint)
> > declare_def_snprint(max_checkint, print_int)
> >
> > -declare_def_handler(verbosity, set_int)
> > +declare_def_range_handler(verbosity, 0, 4)
>
> You should use MAX_VERBOSITY here.
Sure.
-Ben
>
> > declare_def_snprint(verbosity, print_int)
> >
> > declare_def_handler(reassign_maps, set_yes_no)
> > @@ -473,22 +522,22 @@ declare_ovr_snprint(checker_name, print_str)
> > declare_hw_handler(checker_name, set_str)
> > declare_hw_snprint(checker_name, print_str)
> >
> > -declare_def_handler(minio, set_int)
> > +declare_def_range_handler(minio, 0, INT_MAX)
> > declare_def_snprint_defint(minio, print_int, DEFAULT_MINIO)
> > -declare_ovr_handler(minio, set_int)
> > +declare_ovr_range_handler(minio, 0, INT_MAX)
> > declare_ovr_snprint(minio, print_nonzero)
> > -declare_hw_handler(minio, set_int)
> > +declare_hw_range_handler(minio, 0, INT_MAX)
> > declare_hw_snprint(minio, print_nonzero)
> > -declare_mp_handler(minio, set_int)
> > +declare_mp_range_handler(minio, 0, INT_MAX)
> > declare_mp_snprint(minio, print_nonzero)
> >
> > -declare_def_handler(minio_rq, set_int)
> > +declare_def_range_handler(minio_rq, 0, INT_MAX)
> > declare_def_snprint_defint(minio_rq, print_int, DEFAULT_MINIO_RQ)
> > -declare_ovr_handler(minio_rq, set_int)
> > +declare_ovr_range_handler(minio_rq, 0, INT_MAX)
> > declare_ovr_snprint(minio_rq, print_nonzero)
> > -declare_hw_handler(minio_rq, set_int)
> > +declare_hw_range_handler(minio_rq, 0, INT_MAX)
> > declare_hw_snprint(minio_rq, print_nonzero)
> > -declare_mp_handler(minio_rq, set_int)
> > +declare_mp_range_handler(minio_rq, 0, INT_MAX)
> > declare_mp_snprint(minio_rq, print_nonzero)
> >
> > declare_def_handler(queue_without_daemon, set_yes_no)
> > @@ -512,7 +561,7 @@ snprint_def_queue_without_daemon(struct config
> > *conf, struct strbuf *buff,
> > return append_strbuf_quoted(buff, qwd);
> > }
> >
> > -declare_def_handler(checker_timeout, set_int)
> > +declare_def_range_handler(checker_timeout, 0, INT_MAX)
> > declare_def_snprint(checker_timeout, print_nonzero)
> >
> > declare_def_handler(allow_usb_devices, set_yes_no)
> > @@ -583,13 +632,13 @@ declare_hw_snprint(deferred_remove,
> > print_yes_no_undef)
> > declare_mp_handler(deferred_remove, set_yes_no_undef)
> > declare_mp_snprint(deferred_remove, print_yes_no_undef)
> >
> > -declare_def_handler(retrigger_tries, set_int)
> > +declare_def_range_handler(retrigger_tries, 0, INT_MAX)
> > declare_def_snprint(retrigger_tries, print_int)
> >
> > -declare_def_handler(retrigger_delay, set_int)
> > +declare_def_range_handler(retrigger_delay, 0, INT_MAX)
> > declare_def_snprint(retrigger_delay, print_int)
> >
> > -declare_def_handler(uev_wait_timeout, set_int)
> > +declare_def_range_handler(uev_wait_timeout, 0, INT_MAX)
> > declare_def_snprint(uev_wait_timeout, print_int)
> >
> > declare_def_handler(strict_timing, set_yes_no)
> > @@ -616,19 +665,19 @@ static int
> > snprint_def_disable_changed_wwids(struct config *conf,
> > return print_ignored(buff);
> > }
> >
> > -declare_def_handler(remove_retries, set_int)
> > +declare_def_range_handler(remove_retries, 0, INT_MAX)
> > declare_def_snprint(remove_retries, print_int)
> >
> > -declare_def_handler(max_sectors_kb, set_int)
> > +declare_def_range_handler(max_sectors_kb, 0, INT_MAX)
> > declare_def_snprint(max_sectors_kb, print_nonzero)
> > -declare_ovr_handler(max_sectors_kb, set_int)
> > +declare_ovr_range_handler(max_sectors_kb, 0, INT_MAX)
> > declare_ovr_snprint(max_sectors_kb, print_nonzero)
> > -declare_hw_handler(max_sectors_kb, set_int)
> > +declare_hw_range_handler(max_sectors_kb, 0, INT_MAX)
> > declare_hw_snprint(max_sectors_kb, print_nonzero)
> > -declare_mp_handler(max_sectors_kb, set_int)
> > +declare_mp_range_handler(max_sectors_kb, 0, INT_MAX)
> > declare_mp_snprint(max_sectors_kb, print_nonzero)
> >
> > -declare_def_handler(find_multipaths_timeout, set_int)
> > +declare_def_range_handler(find_multipaths_timeout, INT_MIN, INT_MAX)
> > declare_def_snprint_defint(find_multipaths_timeout, print_int,
> > DEFAULT_FIND_MULTIPATHS_TIMEOUT)
> >
> > @@ -1385,27 +1434,7 @@ declare_ovr_snprint(recheck_wwid,
> > print_yes_no_undef)
> > declare_hw_handler(recheck_wwid, set_yes_no_undef)
> > declare_hw_snprint(recheck_wwid, print_yes_no_undef)
> >
> > -
> > -static int
> > -def_uxsock_timeout_handler(struct config *conf, vector strvec, const
> > char *file,
> > - int line_nr)
> > -{
> > - unsigned int uxsock_timeout;
> > - char *buff;
> > -
> > - buff = set_value(strvec);
> > - if (!buff)
> > - return 1;
> > -
> > - if (sscanf(buff, "%u", &uxsock_timeout) == 1 &&
> > - uxsock_timeout > DEFAULT_REPLY_TIMEOUT)
> > - conf->uxsock_timeout = uxsock_timeout;
> > - else
> > - conf->uxsock_timeout = DEFAULT_REPLY_TIMEOUT;
> > -
> > - free(buff);
> > - return 0;
> > -}
> > +declare_def_range_handler(uxsock_timeout, DEFAULT_REPLY_TIMEOUT,
> > INT_MAX)
> >
> > static int
> > hw_vpd_vendor_handler(struct config *conf, vector strvec, const char
> > *file,
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [dm-devel] [PATCH 6/8] libmultipath: improve checks for set_str
2021-11-04 20:34 ` Martin Wilck
2021-11-05 6:59 ` Martin Wilck
@ 2021-11-05 17:38 ` Benjamin Marzinski
1 sibling, 0 replies; 29+ messages in thread
From: Benjamin Marzinski @ 2021-11-05 17:38 UTC (permalink / raw)
To: Martin Wilck; +Cc: 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [dm-devel] [PATCH 6/8] libmultipath: improve checks for set_str
2021-11-05 6:59 ` Martin Wilck
@ 2021-11-05 17:45 ` Benjamin Marzinski
2021-11-05 20:13 ` Martin Wilck
0 siblings, 1 reply; 29+ messages in thread
From: Benjamin Marzinski @ 2021-11-05 17:45 UTC (permalink / raw)
To: Martin Wilck; +Cc: 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [dm-devel] [PATCH 7/8] libmultipath: split set_int to enable reuse
2021-11-04 20:54 ` Martin Wilck
@ 2021-11-05 18:08 ` Benjamin Marzinski
2021-11-05 19:56 ` Martin Wilck
0 siblings, 1 reply; 29+ messages in thread
From: Benjamin Marzinski @ 2021-11-05 18:08 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel
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@redhat.com>
> > ---
> > 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 @@
> > #include "strbuf.h"
> >
> > static int
> > -set_int(vector strvec, void *ptr, int min, int max, const char
> > *file,
> > - int line_nr)
> > +do_set_int(vector strvec, void *ptr, int min, int max, const char
> > *file,
> > + int line_nr, char *buff)
> > {
> > int *int_ptr = (int *)ptr;
> > - char *buff, *eptr;
> > + char *eptr;
> > long res;
> > - int rc;
> > -
> > - buff = set_value(strvec);
> > - if (!buff)
> > - return 1;
> >
> > res = strtol(buff, &eptr, 10);
> > if (eptr > buff)
> > @@ -50,17 +45,30 @@ set_int(vector strvec, void *ptr, int min, int
> > max, const char *file,
> > if (*buff == '\0' || *eptr != '\0') {
> > condlog(1, "%s line %d, invalid value for %s:
> > \"%s\"",
> > file, line_nr, (char*)VECTOR_SLOT(strvec, 0),
> > buff);
> > - rc = 1;
> > - } else {
> > - if (res > max || res < min) {
> > - res = (res > max) ? max : min;
> > - condlog(1, "%s line %d, value for %s too %s,
> > capping at %ld",
> > + return 1;
> > + }
> > + if (res > max || res < min) {
> > + res = (res > max) ? max : min;
> > + condlog(1, "%s line %d, value for %s too %s, capping
> > at %ld",
> > file, line_nr, (char*)VECTOR_SLOT(strvec, 0),
> > - (res == max)? "large" : "small", res);
> > - }
> > - rc = 0;
> > - *int_ptr = res;
> > + (res == max)? "large" : "small", res);
> > }
> > + *int_ptr = res;
> > + return 0;
> > +}
> > +
> > +static int
> > +set_int(vector strvec, void *ptr, int min, int max, const char
> > *file,
> > + int line_nr)
> > +{
> > + char *buff;
> > + int rc;
> > +
> > + buff = set_value(strvec);
> > + if (!buff)
> > + return 1;
> > +
> > + rc = do_set_int(strvec, ptr, min, max, file, line_nr, buff);
> >
> > FREE(buff);
> > return rc;
> > @@ -918,6 +926,7 @@ declare_mp_attr_snprint(gid, print_gid)
> > static int
> > set_undef_off_zero(vector strvec, void *ptr, const char *file, int
> > line_nr)
> > {
> > + int rc = 0;
> > char * buff;
> > int *int_ptr = (int *)ptr;
> >
> > @@ -927,10 +936,10 @@ set_undef_off_zero(vector strvec, void *ptr,
> > const char *file, int line_nr)
> >
> > if (strcmp(buff, "off") == 0)
> > *int_ptr = UOZ_OFF;
> > - else if (sscanf(buff, "%d", int_ptr) != 1 ||
> > - *int_ptr < UOZ_ZERO)
> > - *int_ptr = UOZ_UNDEF;
> > - else if (*int_ptr == 0)
> > + else
> > + rc = do_set_int(strvec, int_ptr, 0, INT_MAX, file,
> > line_nr,
> > + buff);
> > + if (rc == 0 && *int_ptr == 0)
> > *int_ptr = UOZ_ZERO;
> >
> > FREE(buff);
> > @@ -1082,14 +1091,12 @@ max_fds_handler(struct config *conf, vector
> > strvec, const char *file,
> > /* Assume safe limit */
> > max_fds = 4096;
> > }
> > - if (strlen(buff) == 3 &&
> > - !strcmp(buff, "max"))
> > - conf->max_fds = max_fds;
> > - else
> > - conf->max_fds = atoi(buff);
> > -
> > - if (conf->max_fds > max_fds)
> > + if (!strcmp(buff, "max")) {
> > conf->max_fds = max_fds;
> > + r = 0;
> > + } else
> > + r = do_set_int(strvec, &conf->max_fds, 0, max_fds,
> > file,
> > + line_nr, buff);
> >
> > FREE(buff);
> >
> > @@ -1158,6 +1165,7 @@ declare_mp_snprint(rr_weight, print_rr_weight)
> > static int
> > set_pgfailback(vector strvec, void *ptr, const char *file, int
> > line_nr)
> > {
> > + int rc = 0;
> > int *int_ptr = (int *)ptr;
> > char * buff;
> >
> > @@ -1172,11 +1180,11 @@ set_pgfailback(vector strvec, void *ptr,
> > const char *file, int line_nr)
> > else if (strlen(buff) == 10 && !strcmp(buff, "followover"))
> > *int_ptr = -FAILBACK_FOLLOWOVER;
> > else
> > - *int_ptr = atoi(buff);
> > + rc = do_set_int(strvec, ptr, 0, INT_MAX, file,
> > line_nr, buff);
> >
> > FREE(buff);
> >
> > - return 0;
> > + return rc;
> > }
> >
> > int
> > @@ -1208,6 +1216,7 @@ declare_mp_snprint(pgfailback,
> > print_pgfailback)
> > static int
> > no_path_retry_helper(vector strvec, void *ptr, const char *file, int
> > line_nr)
> > {
> > + int rc = 0;
> > int *int_ptr = (int *)ptr;
> > char * buff;
> >
> > @@ -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?
> >
> > FREE(buff);
> > - return 0;
> > + return rc;
> > }
> >
> > int
> > @@ -1365,6 +1374,7 @@ snprint_mp_reservation_key (struct config
> > *conf, struct strbuf *buff,
> > static int
> > set_off_int_undef(vector strvec, void *ptr, const char *file, int
> > line_nr)
> > {
> > + int rc =0;
> > int *int_ptr = (int *)ptr;
> > char * buff;
> >
> > @@ -1374,11 +1384,11 @@ set_off_int_undef(vector strvec, void *ptr,
> > const char *file, int line_nr)
> >
> > if (!strcmp(buff, "no") || !strcmp(buff, "0"))
> > *int_ptr = NU_NO;
> > - else if ((*int_ptr = atoi(buff)) < 1)
> > - *int_ptr = NU_UNDEF;
> > + else
> > + rc = do_set_int(strvec, ptr, 1, INT_MAX, file,
> > line_nr, buff);
>
> Likewise, you'd set 1 here for negative input or "0 ", while
> previously the result would be NU_UNDEF == 0.
>
> Negative values are of course garbage and I'm unsure if trailing spaces
> can occur at this point in the code, but do_set_int() handles them.
Same here.
-Ben
> Regards,
> Martin
>
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [dm-devel] [PATCH 7/8] libmultipath: split set_int to enable reuse
2021-11-05 18:08 ` Benjamin Marzinski
@ 2021-11-05 19:56 ` Martin Wilck
0 siblings, 0 replies; 29+ messages in thread
From: Martin Wilck @ 2021-11-05 19:56 UTC (permalink / raw)
To: bmarzins; +Cc: dm-devel
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@redhat.com>
> > > ---
> > > 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@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [dm-devel] [PATCH 8/8] libmultipath: cleanup invalid config handling
2021-11-04 21:11 ` Martin Wilck
@ 2021-11-05 20:10 ` Benjamin Marzinski
2021-11-05 20:16 ` Martin Wilck
0 siblings, 1 reply; 29+ messages in thread
From: Benjamin Marzinski @ 2021-11-05 20:10 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel
On Thu, Nov 04, 2021 at 09:11:34PM +0000, Martin Wilck wrote:
> On Wed, 2021-10-06 at 15:04 -0500, Benjamin Marzinski wrote:
> > Add error reporting to the remaining config handlers. If the value is
> > invalid, do not change the existing config option's value.
>
> Like for the previous patch, I'm unsure if this is wise. You rely on a
> reasonable default being set before the function is called. I suppose
> that's the case, but I like seeing the "invalid" value substituted
> right there where the validity is checked. That saves us from searching
> the code for the default value.
>
> Maybe I overlooked an important rationale for not touching the values
> in the case of invalid input, please explain.
Since these handlers are only called if people put the corresponding
option in the config files, we had better have sensible defaults if
they're not called (or if they don't set anything).
I admit that I should take a look for cases were we cap an out-of-range
value, to see if it would make more sense to treat it as an invalid
value instead. Also, instead of accepting strings that are simply a
number, we should convert the string, and the check the actual number.
But I don't see any harm in simply ignoring the invalid values. It's no
different than if the user didn't put the invalid line into
multipath.conf
Not setting the values on garbage input makes the handlers more general.
If you have two options that work the same except that they have
different defaults, then by not explicitly setting the value to the
default when you have invalid input, one handler can be used for both
options. set_yes_no() is a good example. Without my patch, it always
set the value to something, even if the input was garbage. But the
default value it set was "no". That had nothing to do with the default
value of the options that were using it. You could do extra work to make
sure that it would correctly use the option's default value, but you get
the same outcome, with simpler code, just by not changing the default if
you have a garbage value.
Also, many of the handlers never set the value on invalid input. I'm just
making that consistent across all of the handlers.
-Ben
> Cheers,
> Martin
>
> > Also print
> > an error whenever 0 is returned for an invalid value. When the
> > handler
> > returns 1, config processing already fails with an error message.
> >
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> > libmultipath/dict.c | 73 +++++++++++++++++++++++++++++++------------
> > --
> > 1 file changed, 51 insertions(+), 22 deletions(-)
> >
> > diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> > index e79fcdd7..8c3b5f72 100644
> > --- a/libmultipath/dict.c
> > +++ b/libmultipath/dict.c
> > @@ -199,8 +199,11 @@ set_yes_no(vector strvec, void *ptr, const char
> > *file, int line_nr)
> >
> > if (strcmp(buff, "yes") == 0 || strcmp(buff, "1") == 0)
> > *int_ptr = YN_YES;
> > - else
> > + else if (strcmp(buff, "no") == 0 || strcmp(buff, "0") == 0)
> > *int_ptr = YN_NO;
> > + else
> > + condlog(1, "%s line %d, invalid value for %s:
> > \"%s\"",
> > + file, line_nr, (char*)VECTOR_SLOT(strvec, 0),
> > buff);
> >
> > FREE(buff);
> > return 0;
> > @@ -221,7 +224,8 @@ set_yes_no_undef(vector strvec, void *ptr, const
> > char *file, int line_nr)
> > else if (strcmp(buff, "yes") == 0 || strcmp(buff, "1") == 0)
> > *int_ptr = YNU_YES;
> > else
> > - *int_ptr = YNU_UNDEF;
> > + condlog(1, "%s line %d, invalid value for %s:
> > \"%s\"",
> > + file, line_nr, (char*)VECTOR_SLOT(strvec, 0),
> > buff);
> >
> > FREE(buff);
> > return 0;
> > @@ -471,9 +475,6 @@ def_find_multipaths_handler(struct config *conf,
> > vector strvec,
> > char *buff;
> > int i;
> >
> > - if (set_yes_no_undef(strvec, &conf->find_multipaths, file,
> > line_nr) == 0 && conf->find_multipaths != FIND_MULTIPATHS_UNDEF)
> > - return 0;
> > -
> > buff = set_value(strvec);
> > if (!buff)
> > return 1;
> > @@ -486,9 +487,14 @@ def_find_multipaths_handler(struct config *conf,
> > vector strvec,
> > }
> > }
> >
> > - if (conf->find_multipaths == YNU_UNDEF) {
> > - condlog(0, "illegal value for find_multipaths: %s",
> > buff);
> > - conf->find_multipaths = DEFAULT_FIND_MULTIPATHS;
> > + if (i >= __FIND_MULTIPATHS_LAST) {
> > + if (strcmp(buff, "no") == 0 || strcmp(buff, "0") ==
> > 0)
> > + conf->find_multipaths = FIND_MULTIPATHS_OFF;
> > + else if (strcmp(buff, "yes") == 0 || strcmp(buff,
> > "1") == 0)
> > + conf->find_multipaths = FIND_MULTIPATHS_ON;
> > + else
> > + condlog(1, "%s line %d, invalid value for
> > find_multipaths: \"%s\"",
> > + file, line_nr, buff);
> > }
> >
> > FREE(buff);
> > @@ -537,8 +543,10 @@ static int uid_attrs_handler(struct config
> > *conf, vector strvec,
> > if (!val)
> > return 1;
> > if (parse_uid_attrs(val, conf))
> > - condlog(1, "error parsing uid_attrs: \"%s\"", val);
> > - condlog(3, "parsed %d uid_attrs", VECTOR_SIZE(&conf-
> > >uid_attrs));
> > + condlog(1, "%s line %d,error parsing uid_attrs:
> > \"%s\"", file,
> > + line_nr, val);
> > + else
> > + condlog(4, "parsed %d uid_attrs", VECTOR_SIZE(&conf-
> > >uid_attrs));
> > FREE(val);
> > return 0;
> > }
> > @@ -766,8 +774,11 @@ def_config_dir_handler(struct config *conf,
> > vector strvec, const char *file,
> > int line_nr)
> > {
> > /* this is only valid in the main config file */
> > - if (conf->processed_main_config)
> > + if (conf->processed_main_config) {
> > + condlog(1, "%s line %d, config_dir option only valid
> > in /etc/multipath.conf",
> > + file, line_nr);
> > return 0;
> > + }
> > return set_path(strvec, &conf->config_dir, file, line_nr);
> > }
> > declare_def_snprint(config_dir, print_str)
> > @@ -825,7 +836,9 @@ set_mode(vector strvec, void *ptr, int *flags,
> > const char *file, int line_nr)
> > if (sscanf(buff, "%o", &mode) == 1 && mode <= 0777) {
> > *flags |= (1 << ATTR_MODE);
> > *mode_ptr = mode;
> > - }
> > + } else
> > + condlog(1, "%s line %d, invalid value for mode:
> > \"%s\"",
> > + file, line_nr, buff);
> >
> > FREE(buff);
> > return 0;
> > @@ -850,7 +863,9 @@ set_uid(vector strvec, void *ptr, int *flags,
> > const char *file, int line_nr)
> > else if (sscanf(buff, "%u", &uid) == 1){
> > *flags |= (1 << ATTR_UID);
> > *uid_ptr = uid;
> > - }
> > + } else
> > + condlog(1, "%s line %d, invalid value for uid:
> > \"%s\"",
> > + file, line_nr, buff);
> >
> > FREE(buff);
> > return 0;
> > @@ -876,7 +891,9 @@ set_gid(vector strvec, void *ptr, int *flags,
> > const char *file, int line_nr)
> > else if (sscanf(buff, "%u", &gid) == 1){
> > *flags |= (1 << ATTR_GID);
> > *gid_ptr = gid;
> > - }
> > + } else
> > + condlog(1, "%s line %d, invalid value for gid:
> > \"%s\"",
> > + file, line_nr, buff);
> > FREE(buff);
> > return 0;
> > }
> > @@ -978,7 +995,8 @@ set_dev_loss(vector strvec, void *ptr, const char
> > *file, int line_nr)
> > if (!strcmp(buff, "infinity"))
> > *uint_ptr = MAX_DEV_LOSS_TMO;
> > else if (sscanf(buff, "%u", uint_ptr) != 1)
> > - *uint_ptr = DEV_LOSS_TMO_UNSET;
> > + condlog(1, "%s line %d, invalid value for
> > dev_loss_tmo: \"%s\"",
> > + file, line_nr, buff);
> >
> > FREE(buff);
> > return 0;
> > @@ -1012,13 +1030,19 @@ static int
> > set_pgpolicy(vector strvec, void *ptr, const char *file, int
> > line_nr)
> > {
> > char * buff;
> > + int policy;
> > int *int_ptr = (int *)ptr;
> >
> > buff = set_value(strvec);
> > if (!buff)
> > return 1;
> >
> > - *int_ptr = get_pgpolicy_id(buff);
> > + policy = get_pgpolicy_id(buff);
> > + if (policy != IOPOLICY_UNDEF)
> > + *int_ptr = policy;
> > + else
> > + condlog(1, "%s line %d, invalid value for
> > path_grouping_policy: \"%s\"",
> > + file, line_nr, buff);
> > FREE(buff);
> >
> > return 0;
> > @@ -1131,10 +1155,11 @@ set_rr_weight(vector strvec, void *ptr, const
> > char *file, int line_nr)
> >
> > if (!strcmp(buff, "priorities"))
> > *int_ptr = RR_WEIGHT_PRIO;
> > -
> > - if (!strcmp(buff, "uniform"))
> > + else if (!strcmp(buff, "uniform"))
> > *int_ptr = RR_WEIGHT_NONE;
> > -
> > + else
> > + condlog(1, "%s line %d, invalid value for rr_weight:
> > \"%s\"",
> > + file, line_nr, buff);
> > FREE(buff);
> >
> > return 0;
> > @@ -1270,10 +1295,13 @@ def_log_checker_err_handler(struct config
> > *conf, vector strvec,
> > if (!buff)
> > return 1;
> >
> > - if (strlen(buff) == 4 && !strcmp(buff, "once"))
> > + if (!strcmp(buff, "once"))
> > conf->log_checker_err = LOG_CHKR_ERR_ONCE;
> > - else if (strlen(buff) == 6 && !strcmp(buff, "always"))
> > + else if (!strcmp(buff, "always"))
> > conf->log_checker_err = LOG_CHKR_ERR_ALWAYS;
> > + else
> > + condlog(1, "%s line %d, invalid value for
> > log_checker_err: \"%s\"",
> > + file, line_nr, buff);
> >
>
> We lack a proper DEFAULT for log_checker_err.
>
True. Is it really the only one? I thought that there were a number of
options for which we just relied on the fact that conf is zeroed out
when it's created, so the 0 value (in this case LOG_CHKR_ERR_ALWAYS) is
the implicit default.
> > free(buff);
> > return 0;
> > @@ -1534,7 +1562,8 @@ hw_vpd_vendor_handler(struct config *conf,
> > vector strvec, const char *file,
> > goto out;
> > }
> > }
> > - hwe->vpd_vendor_id = 0;
> > + condlog(1, "%s line %d, invalid value for vpd_vendor:
> > \"%s\"",
> > + file, line_nr, buff);
> > out:
> > FREE(buff);
> > return 0;
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [dm-devel] [PATCH 6/8] libmultipath: improve checks for set_str
2021-11-05 17:45 ` Benjamin Marzinski
@ 2021-11-05 20:13 ` Martin Wilck
0 siblings, 0 replies; 29+ messages in thread
From: Martin Wilck @ 2021-11-05 20:13 UTC (permalink / raw)
To: bmarzins; +Cc: 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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [dm-devel] [PATCH 8/8] libmultipath: cleanup invalid config handling
2021-11-05 20:10 ` Benjamin Marzinski
@ 2021-11-05 20:16 ` Martin Wilck
0 siblings, 0 replies; 29+ messages in thread
From: Martin Wilck @ 2021-11-05 20:16 UTC (permalink / raw)
To: bmarzins; +Cc: dm-devel
On Fri, 2021-11-05 at 15:10 -0500, Benjamin Marzinski wrote:
> On Thu, Nov 04, 2021 at 09:11:34PM +0000, Martin Wilck wrote:
> > On Wed, 2021-10-06 at 15:04 -0500, Benjamin Marzinski wrote:
> > > Add error reporting to the remaining config handlers. If the
> > > value is
> > > invalid, do not change the existing config option's value.
> >
> > Like for the previous patch, I'm unsure if this is wise. You rely
> > on a
> > reasonable default being set before the function is called. I
> > suppose
> > that's the case, but I like seeing the "invalid" value substituted
> > right there where the validity is checked. That saves us from
> > searching
> > the code for the default value.
> >
> > Maybe I overlooked an important rationale for not touching the
> > values
> > in the case of invalid input, please explain.
>
> Since these handlers are only called if people put the corresponding
> option in the config files, we had better have sensible defaults if
> they're not called (or if they don't set anything).
>
> I admit that I should take a look for cases were we cap an out-of-
> range
> value, to see if it would make more sense to treat it as an invalid
> value instead. Also, instead of accepting strings that are simply a
> number, we should convert the string, and the check the actual
> number.
> But I don't see any harm in simply ignoring the invalid values. It's
> no
> different than if the user didn't put the invalid line into
> multipath.conf
>
> Not setting the values on garbage input makes the handlers more
> general.
> If you have two options that work the same except that they have
> different defaults, then by not explicitly setting the value to the
> default when you have invalid input, one handler can be used for both
> options. set_yes_no() is a good example. Without my patch, it always
> set the value to something, even if the input was garbage. But the
> default value it set was "no". That had nothing to do with the
> default
> value of the options that were using it. You could do extra work to
> make
> sure that it would correctly use the option's default value, but you
> get
> the same outcome, with simpler code, just by not changing the default
> if
> you have a garbage value.
>
> Also, many of the handlers never set the value on invalid input. I'm
> just
> making that consistent across all of the handlers.
OK, you've convinced me.
Thanks
Martin
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [dm-devel] [PATCH 7/8] libmultipath: split set_int to enable reuse
2021-10-06 20:04 ` [dm-devel] [PATCH 7/8] libmultipath: split set_int to enable reuse Benjamin Marzinski
2021-11-04 20:54 ` Martin Wilck
@ 2021-11-11 11:56 ` Martin Wilck
1 sibling, 0 replies; 29+ messages in thread
From: Martin Wilck @ 2021-11-11 11:56 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel
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.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
After discussion on v2 of this patch:
Reviewed-by: Martin Wilck <mwilck@suse.com>
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2021-11-11 11:57 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-06 20:04 [dm-devel] [PATCH 0/8] improving config parsing warnings Benjamin Marzinski
2021-10-06 20:04 ` [dm-devel] [PATCH 1/8] libmulitpath: add section name to invalid keyword output Benjamin Marzinski
2021-11-04 20:55 ` Martin Wilck
2021-11-05 17:28 ` Benjamin Marzinski
2021-10-06 20:04 ` [dm-devel] [PATCH 2/8] libmultipath: use typedef for keyword handler function Benjamin Marzinski
2021-11-04 20:55 ` Martin Wilck
2021-10-06 20:04 ` [dm-devel] [PATCH 3/8] libmultipath: print the correct file when parsing fails Benjamin Marzinski
2021-11-04 20:55 ` Martin Wilck
2021-10-06 20:04 ` [dm-devel] [PATCH 4/8] libmultipath: pass file and line number to keyword handlers Benjamin Marzinski
2021-11-04 20:55 ` Martin Wilck
2021-11-05 17:33 ` Benjamin Marzinski
2021-10-06 20:04 ` [dm-devel] [PATCH 5/8] libmultipath: make set_int take a range for valid values Benjamin Marzinski
2021-11-04 20:34 ` Martin Wilck
2021-11-05 17:34 ` Benjamin Marzinski
2021-10-06 20:04 ` [dm-devel] [PATCH 6/8] libmultipath: improve checks for set_str Benjamin Marzinski
2021-11-04 20:34 ` Martin Wilck
2021-11-05 6:59 ` Martin Wilck
2021-11-05 17:45 ` Benjamin Marzinski
2021-11-05 20:13 ` Martin Wilck
2021-11-05 17:38 ` Benjamin Marzinski
2021-10-06 20:04 ` [dm-devel] [PATCH 7/8] libmultipath: split set_int to enable reuse Benjamin Marzinski
2021-11-04 20:54 ` Martin Wilck
2021-11-05 18:08 ` Benjamin Marzinski
2021-11-05 19:56 ` Martin Wilck
2021-11-11 11:56 ` Martin Wilck
2021-10-06 20:04 ` [dm-devel] [PATCH 8/8] libmultipath: cleanup invalid config handling Benjamin Marzinski
2021-11-04 21:11 ` Martin Wilck
2021-11-05 20:10 ` Benjamin Marzinski
2021-11-05 20:16 ` Martin Wilck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.