[PATCH v2] multipath: display the correct configuration when dumpping config

[PATCH v2] multipath: display the correct configuration when dumpping config

[303146950]

From: Kou Wenqi <kouwenqi@kylinos.cn>

"multipath -t" and "multipath -T" might show the wrong
multipathd configuration items "retrigger_tries" and
"force_sync". Make sure they don't.

Signed-off-by: Kou Wenqi <kouwenqi@kylinos.cn>
---
 multipath/main.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/multipath/main.c b/multipath/main.c
index ce702e7f..f4abdad0 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -842,6 +842,8 @@ main (int argc, char *argv[])
 	char *dev = NULL;
 	struct config *conf;
 	bool enable_foreign = false;
+	int retrigger_tries_ori;
+	int force_sync_ori;
 
 	libmultipath_init();
 	if (atexit(dm_lib_exit) || atexit(libmultipath_exit))
@@ -852,7 +854,9 @@ main (int argc, char *argv[])
 	if (atexit(uninit_config))
 		condlog(1, "failed to register cleanup handler for config: %m");
 	conf = get_multipath_config();
+	retrigger_tries_ori = conf->retrigger_tries;
 	conf->retrigger_tries = 0;
+	force_sync_ori = conf->force_sync;
 	conf->force_sync = 1;
 	if (atexit(cleanup_vecs))
 		condlog(1, "failed to register cleanup handler for vecs: %m");
@@ -924,10 +928,14 @@ main (int argc, char *argv[])
 				conf->find_multipaths = FIND_MULTIPATHS_GREEDY;
 			break;
 		case 't':
+			conf->retrigger_tries = retrigger_tries_ori;
+			conf->force_sync = force_sync_ori;
 			r = dump_config(conf, NULL, NULL) ? RTVL_FAIL : RTVL_OK;
 			goto out;
 		case 'T':
 			cmd = CMD_DUMP_CONFIG;
+			conf->retrigger_tries = retrigger_tries_ori;
+			conf->force_sync = force_sync_ori;
 			break;
 		case 'h':
 			usage(argv[0]);
-- 
2.27.0

Re: [PATCH v2] multipath: display the correct configuration when dumpping config

[Benjamin Marzinski]

On Mon, Jun 24, 2024 at 09:45:27AM +0800, 303146950@qq.com wrote:
> From: Kou Wenqi <kouwenqi@kylinos.cn>
> 
> "multipath -t" and "multipath -T" might show the wrong
> multipathd configuration items "retrigger_tries" and
> "force_sync". Make sure they don't.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Kou Wenqi <kouwenqi@kylinos.cn>
> ---
>  multipath/main.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/multipath/main.c b/multipath/main.c
> index ce702e7f..f4abdad0 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -842,6 +842,8 @@ main (int argc, char *argv[])
>  	char *dev = NULL;
>  	struct config *conf;
>  	bool enable_foreign = false;
> +	int retrigger_tries_ori;
> +	int force_sync_ori;
>  
>  	libmultipath_init();
>  	if (atexit(dm_lib_exit) || atexit(libmultipath_exit))
> @@ -852,7 +854,9 @@ main (int argc, char *argv[])
>  	if (atexit(uninit_config))
>  		condlog(1, "failed to register cleanup handler for config: %m");
>  	conf = get_multipath_config();
> +	retrigger_tries_ori = conf->retrigger_tries;
>  	conf->retrigger_tries = 0;
> +	force_sync_ori = conf->force_sync;
>  	conf->force_sync = 1;
>  	if (atexit(cleanup_vecs))
>  		condlog(1, "failed to register cleanup handler for vecs: %m");
> @@ -924,10 +928,14 @@ main (int argc, char *argv[])
>  				conf->find_multipaths = FIND_MULTIPATHS_GREEDY;
>  			break;
>  		case 't':
> +			conf->retrigger_tries = retrigger_tries_ori;
> +			conf->force_sync = force_sync_ori;
>  			r = dump_config(conf, NULL, NULL) ? RTVL_FAIL : RTVL_OK;
>  			goto out;
>  		case 'T':
>  			cmd = CMD_DUMP_CONFIG;
> +			conf->retrigger_tries = retrigger_tries_ori;
> +			conf->force_sync = force_sync_ori;
>  			break;
>  		case 'h':
>  			usage(argv[0]);
> -- 
> 2.27.0
> 

Re: [PATCH v2] multipath: display the correct configuration when dumpping config

[Martin Wilck]

On Mon, 2024-06-24 at 09:45 +0800, 303146950@qq.com wrote:
> From: Kou Wenqi <kouwenqi@kylinos.cn>
> 
> "multipath -t" and "multipath -T" might show the wrong
> multipathd configuration items "retrigger_tries" and
> "force_sync". Make sure they don't.
> 
> Signed-off-by: Kou Wenqi <kouwenqi@kylinos.cn>
> ---
>  multipath/main.c | 8 ++++++++
>  1 file changed, 8 insertions(+)

Thanks for the patch! I'd prefer the following, if you agree.

diff --git a/multipath/main.c b/multipath/main.c
index 4b19d2e..28e3a05 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -853,8 +853,6 @@ main (int argc, char *argv[])
        if (atexit(uninit_config))
                condlog(1, "failed to register cleanup handler for config: %m");
        conf = get_multipath_config();
-       conf->retrigger_tries = 0;
-       conf->force_sync = 1;
        if (atexit(cleanup_vecs))
                condlog(1, "failed to register cleanup handler for vecs: %m");
        if (atexit(cleanup_bindings))
@@ -1001,6 +999,11 @@ main (int argc, char *argv[])
 
        libmp_udev_set_sync_support(1);
 
+       if (cmd != CMD_DUMP_CONFIG) {
+               conf->retrigger_tries = 0;
+               conf->force_sync = 1;
+       }
+
        if ((cmd == CMD_LIST_SHORT || cmd == CMD_LIST_LONG) && enable_foreign)
                conf->enable_foreign = strdup("");
 

[PATCH v3] multipath: display the correct configuration when dumpping config

[303146950]

From: Kou Wenqi <kouwenqi@kylinos.cn>

On Fri, 2024-08-09 at 10:38 +0200, mwilck@suse.com wrote:
> Thanks for the patch! I'd prefer the following, if you agree.
> 
> diff --git a/multipath/main.c b/multipath/main.c
> index 4b19d2e..28e3a05 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -853,8 +853,6 @@ main (int argc, char *argv[])
>         if (atexit(uninit_config))
>                 condlog(1, "failed to register cleanup handler for config: %m");
>         conf = get_multipath_config();
> -       conf->retrigger_tries = 0;
> -       conf->force_sync = 1;
>         if (atexit(cleanup_vecs))
>                 condlog(1, "failed to register cleanup handler for vecs: %m");
>         if (atexit(cleanup_bindings))
> @@ -1001,6 +999,11 @@ main (int argc, char *argv[])
>  
>         libmp_udev_set_sync_support(1);
>  
> +       if (cmd != CMD_DUMP_CONFIG) {
> +               conf->retrigger_tries = 0;
> +               conf->force_sync = 1;
> +       }
> +
>         if ((cmd == CMD_LIST_SHORT || cmd == CMD_LIST_LONG) && enable_foreign)
>                 conf->enable_foreign = strdup("");

Thanks for your review and revised opinion. Here is the new patch.

"multipath -t" and "multipath -T" might show the wrong
multipathd configuration items "retrigger_tries" and
"force_sync". Make sure they don't.

Signed-off-by: Kou Wenqi <kouwenqi@kylinos.cn>
---
 multipath/main.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index ce702e7f..69552896 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -852,8 +852,6 @@ main (int argc, char *argv[])
 	if (atexit(uninit_config))
 		condlog(1, "failed to register cleanup handler for config: %m");
 	conf = get_multipath_config();
-	conf->retrigger_tries = 0;
-	conf->force_sync = 1;
 	if (atexit(cleanup_vecs))
 		condlog(1, "failed to register cleanup handler for vecs: %m");
 	if (atexit(cleanup_bindings))
@@ -1000,6 +998,11 @@ main (int argc, char *argv[])
 
 	libmp_udev_set_sync_support(1);
 
+	if (cmd != CMD_DUMP_CONFIG) {
+		conf->retrigger_tries = 0;
+		conf->force_sync = 1;
+	}
+
 	if ((cmd == CMD_LIST_SHORT || cmd == CMD_LIST_LONG) && enable_foreign)
 		conf->enable_foreign = strdup("");
 
-- 
2.27.0

Re: [PATCH v3] multipath: display the correct configuration when dumpping config

[Martin Wilck]

On Tue, 2024-08-13 at 10:12 +0800, 303146950@qq.com wrote:
> From: Kou Wenqi <kouwenqi@kylinos.cn>
> 
> Thanks for your review and revised opinion. Here is the new patch.
> 
> "multipath -t" and "multipath -T" might show the wrong
> multipathd configuration items "retrigger_tries" and
> "force_sync". Make sure they don't.
> 
> Signed-off-by: Kou Wenqi <kouwenqi@kylinos.cn>
> 

Reviewed-by: Martin Wilck <mwilck@suse.com>