[PATCH] multipath-tools: Fix compiler warnings when built without systemd.

[PATCH] multipath-tools: Fix compiler warnings when built without systemd.

[Marius Bakke]

---
 libmultipath/config.c |  2 +-
 multipathd/main.c     | 19 +++++++++++++------
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index b4d87689..a28dc4f2 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -696,7 +696,7 @@ process_config_dir(struct config *conf, char *dir)
 	pthread_cleanup_pop(1);
 }
 
-static void set_max_checkint_from_watchdog(struct config *conf)
+static void set_max_checkint_from_watchdog(__attribute__((unused)) struct config *conf)
 {
 #ifdef USE_SYSTEMD
 	char *envp = getenv("WATCHDOG_USEC");
diff --git a/multipathd/main.c b/multipathd/main.c
index 8baf9abe..8d3eace6 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -176,6 +176,7 @@ daemon_status(void)
 /*
  * I love you too, systemd ...
  */
+#ifdef USE_SYSTEMD
 static const char *
 sd_notify_status(enum daemon_status state)
 {
@@ -195,7 +196,6 @@ sd_notify_status(enum daemon_status state)
 	return NULL;
 }
 
-#ifdef USE_SYSTEMD
 static void do_sd_notify(enum daemon_status old_state,
 			 enum daemon_status new_state)
 {
@@ -247,7 +247,9 @@ enum daemon_status wait_for_state_change_if(enum daemon_status oldstate,
 static void __post_config_state(enum daemon_status state)
 {
 	if (state != running_state && running_state != DAEMON_SHUTDOWN) {
-		enum daemon_status old_state = running_state;
+		/* save state for sd_notify */
+		enum daemon_status
+			__attribute__((unused)) old_state = running_state;
 
 		running_state = state;
 		pthread_cond_broadcast(&config_cond);
@@ -272,7 +274,9 @@ int set_config_state(enum daemon_status state)
 	pthread_cleanup_push(config_cleanup, NULL);
 	pthread_mutex_lock(&config_lock);
 	if (running_state != state) {
-		enum daemon_status old_state = running_state;
+		/* save state for sd_notify */
+		enum daemon_status
+			__attribute__((unused)) old_state = running_state;
 
 		if (running_state == DAEMON_SHUTDOWN)
 			rc = EINVAL;
@@ -2280,7 +2284,6 @@ checkerloop (void *ap)
 	struct timespec last_time;
 	struct config *conf;
 	int foreign_tick = 0;
-	bool use_watchdog;
 
 	pthread_cleanup_push(rcu_unregister, NULL);
 	rcu_register_thread();
@@ -2292,11 +2295,15 @@ checkerloop (void *ap)
 	get_monotonic_time(&last_time);
 	last_time.tv_sec -= 1;
 
-	/* use_watchdog is set from process environment and never changes */
 	conf = get_multipath_config();
-	use_watchdog = conf->use_watchdog;
 	put_multipath_config(conf);
 
+#ifdef USE_SYSTEMD
+	/* use_watchdog is set from process environment and never changes */
+	bool use_watchdog;
+	use_watchdog = conf->use_watchdog;
+#endif
+
 	while (1) {
 		struct timespec diff_time, start_time, end_time;
 		int num_paths = 0, strict_timing, rc = 0;
-- 
2.26.2

Re: [PATCH] multipath-tools: Fix compiler warnings when built without systemd.

[Xose Vazquez Perez]

On 5/16/20 7:55 PM, Marius Bakke wrote:

You should send it, at least, to the project maintainer.

> ---
>   libmultipath/config.c |  2 +-
>   multipathd/main.c     | 19 +++++++++++++------
>   2 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index b4d87689..a28dc4f2 100644
> --- a/libmultipath/config.c
> +++ b/libmultipath/config.c
> @@ -696,7 +696,7 @@ process_config_dir(struct config *conf, char *dir)
>   	pthread_cleanup_pop(1);
>   }
>   
> -static void set_max_checkint_from_watchdog(struct config *conf)
> +static void set_max_checkint_from_watchdog(__attribute__((unused)) struct config *conf)
>   {
>   #ifdef USE_SYSTEMD
>   	char *envp = getenv("WATCHDOG_USEC");
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 8baf9abe..8d3eace6 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -176,6 +176,7 @@ daemon_status(void)
>   /*
>    * I love you too, systemd ...
>    */
> +#ifdef USE_SYSTEMD
>   static const char *
>   sd_notify_status(enum daemon_status state)
>   {
> @@ -195,7 +196,6 @@ sd_notify_status(enum daemon_status state)
>   	return NULL;
>   }
>   
> -#ifdef USE_SYSTEMD
>   static void do_sd_notify(enum daemon_status old_state,
>   			 enum daemon_status new_state)
>   {
> @@ -247,7 +247,9 @@ enum daemon_status wait_for_state_change_if(enum daemon_status oldstate,
>   static void __post_config_state(enum daemon_status state)
>   {
>   	if (state != running_state && running_state != DAEMON_SHUTDOWN) {
> -		enum daemon_status old_state = running_state;
> +		/* save state for sd_notify */
> +		enum daemon_status
> +			__attribute__((unused)) old_state = running_state;
>   
>   		running_state = state;
>   		pthread_cond_broadcast(&config_cond);
> @@ -272,7 +274,9 @@ int set_config_state(enum daemon_status state)
>   	pthread_cleanup_push(config_cleanup, NULL);
>   	pthread_mutex_lock(&config_lock);
>   	if (running_state != state) {
> -		enum daemon_status old_state = running_state;
> +		/* save state for sd_notify */
> +		enum daemon_status
> +			__attribute__((unused)) old_state = running_state;
>   
>   		if (running_state == DAEMON_SHUTDOWN)
>   			rc = EINVAL;
> @@ -2280,7 +2284,6 @@ checkerloop (void *ap)
>   	struct timespec last_time;
>   	struct config *conf;
>   	int foreign_tick = 0;
> -	bool use_watchdog;
>   
>   	pthread_cleanup_push(rcu_unregister, NULL);
>   	rcu_register_thread();
> @@ -2292,11 +2295,15 @@ checkerloop (void *ap)
>   	get_monotonic_time(&last_time);
>   	last_time.tv_sec -= 1;
>   
> -	/* use_watchdog is set from process environment and never changes */
>   	conf = get_multipath_config();
> -	use_watchdog = conf->use_watchdog;
>   	put_multipath_config(conf);
>   
> +#ifdef USE_SYSTEMD
> +	/* use_watchdog is set from process environment and never changes */
> +	bool use_watchdog;
> +	use_watchdog = conf->use_watchdog;
> +#endif
> +
>   	while (1) {
>   		struct timespec diff_time, start_time, end_time;
>   		int num_paths = 0, strict_timing, rc = 0;
> 

Re: [PATCH] multipath-tools: Fix compiler warnings when built without systemd.

[Martin Wilck]

Hello Marius,

On Sat, 2020-05-16 at 19:55 +0200, Marius Bakke wrote:
> ---
>  libmultipath/config.c |  2 +-
>  multipathd/main.c     | 19 +++++++++++++------
>  2 files changed, 14 insertions(+), 7 deletions(-)
> 

thank you for the patch, and sorry for the late reply.
Thanks to Xose, too, for making me aware of it.

This patch needs some improvements, see remarks below.

In general, please note that the development and in particular the
testing of multipath-tools has been done almost exclusively on systems
using systemd for several years now. The compilation issues you
encountered may only be the tip of the iceberg. In particular, the udev
rules shipped with multipath-tools rely on systemd for proper device
setup.

> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index b4d87689..a28dc4f2 100644
> --- a/libmultipath/config.c
> +++ b/libmultipath/config.c
> @@ -696,7 +696,7 @@ process_config_dir(struct config *conf, char
> *dir)
>  	pthread_cleanup_pop(1);
>  }
>  
> -static void set_max_checkint_from_watchdog(struct config *conf)
> +static void set_max_checkint_from_watchdog(__attribute__((unused))
> struct config *conf)

Rather than adding the attribute, please put the whole function in an
#if conditional, and the call in load_config(), too. We don't need this
without systemd.

>  {
>  #ifdef USE_SYSTEMD
>  	char *envp = getenv("WATCHDOG_USEC");
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 8baf9abe..8d3eace6 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -176,6 +176,7 @@ daemon_status(void)
>  /*
>   * I love you too, systemd ...
>   */
> +#ifdef USE_SYSTEMD
>  static const char *
>  sd_notify_status(enum daemon_status state)
>  {
> @@ -195,7 +196,6 @@ sd_notify_status(enum daemon_status state)
>  	return NULL;
>  }
>  
> -#ifdef USE_SYSTEMD
>  static void do_sd_notify(enum daemon_status old_state,
>  			 enum daemon_status new_state)
>  {
> @@ -247,7 +247,9 @@ enum daemon_status wait_for_state_change_if(enum
> daemon_status oldstate,
>  static void __post_config_state(enum daemon_status state)
>  {
>  	if (state != running_state && running_state != DAEMON_SHUTDOWN)
> {
> -		enum daemon_status old_state = running_state;
> +		/* save state for sd_notify */
> +		enum daemon_status
> +			__attribute__((unused)) old_state =
> running_state;

again, please put the declaration of old_state in an #if conditional.

>  
>  		running_state = state;
>  		pthread_cond_broadcast(&config_cond);
> @@ -272,7 +274,9 @@ int set_config_state(enum daemon_status state)
>  	pthread_cleanup_push(config_cleanup, NULL);
>  	pthread_mutex_lock(&config_lock);
>  	if (running_state != state) {
> -		enum daemon_status old_state = running_state;
> +		/* save state for sd_notify */
> +		enum daemon_status
> +			__attribute__((unused)) old_state =
> running_state;

likewise

>  
>  		if (running_state == DAEMON_SHUTDOWN)
>  			rc = EINVAL;
> @@ -2280,7 +2284,6 @@ checkerloop (void *ap)
>  	struct timespec last_time;
>  	struct config *conf;
>  	int foreign_tick = 0;
> -	bool use_watchdog;
>  
>  	pthread_cleanup_push(rcu_unregister, NULL);
>  	rcu_register_thread();
> @@ -2292,11 +2295,15 @@ checkerloop (void *ap)
>  	get_monotonic_time(&last_time);
>  	last_time.tv_sec -= 1;
>  
> -	/* use_watchdog is set from process environment and never
> changes */
>  	conf = get_multipath_config();
> -	use_watchdog = conf->use_watchdog;
>  	put_multipath_config(conf);
>  
> +#ifdef USE_SYSTEMD
> +	/* use_watchdog is set from process environment and never
> changes */
> +	bool use_watchdog;
> +	use_watchdog = conf->use_watchdog;
> +#endif
> +

This is wrong. You have to access conf between get_multipath_config()
and put_multipath_config().

I'd prefer to simply add conditionals around the declaration of
use_watchdog, both in the function header and where it's set.

Regards,
Martin

>  	while (1) {
>  		struct timespec diff_time, start_time, end_time;
>  		int num_paths = 0, strict_timing, rc = 0;

[PATCH v2] multipath: Fix compiler warnings when built without systemd.

[Marius Bakke]

Add ifdef guards for code that is unused when systemd is not available.
---
 libmultipath/config.c |  6 ++++--
 multipathd/main.c     | 10 +++++++++-
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index b4d87689..658bec8b 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -696,9 +696,9 @@ process_config_dir(struct config *conf, char *dir)
 	pthread_cleanup_pop(1);
 }
 
+#ifdef USE_SYSTEMD
 static void set_max_checkint_from_watchdog(struct config *conf)
 {
-#ifdef USE_SYSTEMD
 	char *envp = getenv("WATCHDOG_USEC");
 	unsigned long checkint;
 
@@ -714,8 +714,8 @@ static void set_max_checkint_from_watchdog(struct config *conf)
 		condlog(3, "enabling watchdog, interval %ld", checkint);
 		conf->use_watchdog = true;
 	}
-#endif
 }
+#endif
 
 struct config *
 load_config (char * file)
@@ -789,7 +789,9 @@ load_config (char * file)
 	/*
 	 * fill the voids left in the config file
 	 */
+#ifdef USE_SYSTEMD
 	set_max_checkint_from_watchdog(conf);
+#endif
 	if (conf->max_checkint == 0) {
 		if (conf->checkint == CHECKINT_UNDEF)
 			conf->checkint = DEFAULT_CHECKINT;
diff --git a/multipathd/main.c b/multipathd/main.c
index 8baf9abe..ac8ae4c2 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -176,6 +176,7 @@ daemon_status(void)
 /*
  * I love you too, systemd ...
  */
+#ifdef USE_SYSTEMD
 static const char *
 sd_notify_status(enum daemon_status state)
 {
@@ -195,7 +196,6 @@ sd_notify_status(enum daemon_status state)
 	return NULL;
 }
 
-#ifdef USE_SYSTEMD
 static void do_sd_notify(enum daemon_status old_state,
 			 enum daemon_status new_state)
 {
@@ -247,7 +247,9 @@ enum daemon_status wait_for_state_change_if(enum daemon_status oldstate,
 static void __post_config_state(enum daemon_status state)
 {
 	if (state != running_state && running_state != DAEMON_SHUTDOWN) {
+#ifdef USE_SYSTEMD
 		enum daemon_status old_state = running_state;
+#endif
 
 		running_state = state;
 		pthread_cond_broadcast(&config_cond);
@@ -272,7 +274,9 @@ int set_config_state(enum daemon_status state)
 	pthread_cleanup_push(config_cleanup, NULL);
 	pthread_mutex_lock(&config_lock);
 	if (running_state != state) {
+#ifdef USE_SYSTEMD
 		enum daemon_status old_state = running_state;
+#endif
 
 		if (running_state == DAEMON_SHUTDOWN)
 			rc = EINVAL;
@@ -2280,7 +2284,9 @@ checkerloop (void *ap)
 	struct timespec last_time;
 	struct config *conf;
 	int foreign_tick = 0;
+#ifdef USE_SYSTEMD
 	bool use_watchdog;
+#endif
 
 	pthread_cleanup_push(rcu_unregister, NULL);
 	rcu_register_thread();
@@ -2294,7 +2300,9 @@ checkerloop (void *ap)
 
 	/* use_watchdog is set from process environment and never changes */
 	conf = get_multipath_config();
+#ifdef USE_SYSTEMD
 	use_watchdog = conf->use_watchdog;
+#endif
 	put_multipath_config(conf);
 
 	while (1) {
-- 
2.26.2

Re: [PATCH] multipath-tools: Fix compiler warnings when built without systemd.

[Marius Bakke]

Martin Wilck <mwilck@suse.com> writes:

> Hello Marius,
>
> On Sat, 2020-05-16 at 19:55 +0200, Marius Bakke wrote:
>> ---
>>  libmultipath/config.c |  2 +-
>>  multipathd/main.c     | 19 +++++++++++++------
>>  2 files changed, 14 insertions(+), 7 deletions(-)
>> 
>
> thank you for the patch, and sorry for the late reply.
> Thanks to Xose, too, for making me aware of it.
>
> This patch needs some improvements, see remarks below.
>
> In general, please note that the development and in particular the
> testing of multipath-tools has been done almost exclusively on systems
> using systemd for several years now. The compilation issues you
> encountered may only be the tip of the iceberg. In particular, the udev
> rules shipped with multipath-tools rely on systemd for proper device
> setup.

Thanks for reviewing.  I'm mainly here for 'kpartx' anyway, but "good"
to know that multipath-tools may need extra care outside of systemd.

I'm hardly a C programmer, so thanks for bearing with me.  Adding just
plain old ifdef guards is much clearer than sprinkling __attribute__
around.

V2 sent in a separate message.

Thanks,
Marius

Re: [PATCH v2] multipath: Fix compiler warnings when built without systemd.

[Martin Wilck]

On Wed, 2020-06-17 at 01:11 +0200, Marius Bakke wrote:
> Add ifdef guards for code that is unused when systemd is not
> available.
> ---
>  libmultipath/config.c |  6 ++++--
>  multipathd/main.c     | 10 +++++++++-
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 

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

Thanks!
Martin