[PATCH 0/9] Mdmonitor refactor and udev event handling improvements

[PATCH 0/9] Mdmonitor refactor and udev event handling improvements

[Mateusz Grzonka]

Along the way we observed many problems with current approach to event handling in mdmonitor.
It frequently doesn't report Fail and DeviceDisappeared events.
It's due to time races with udev, and too long delay in some cases.
While there was a patch intending to address time races with udev, it didn't remove them completely.
This patch series presents alternative approach, where mdmonitor wakes up on udev events, so that
there should be no more conflicts with udev we saw before.

Additionally some code quality improvements were done, to make the code more maintainable.

Mateusz Grzonka (9):
  Mdmonitor: Split alert() into separate functions
  Mdmonitor: Make alert_info global
  Mdmonitor: Pass events to alert() using enums instead of strings
  Mdmonitor: Add helper functions
  Add helpers to determine whether directories or files are soft links
  Mdmonitor: Refactor write_autorebuild_pid()
  Mdmonitor: Refactor check_one_sharer() for better error handling
  Mdmonitor: Improve udev event handling
  udev: Move udev_block() and udev_unblock() into udev.c

 Create.c  |   1 +
 Makefile  |   3 +-
 Manage.c  |   3 +-
 Monitor.c | 707 ++++++++++++++++++++++++++++++++----------------------
 lib.c     |  42 ----
 mdadm.h   |   6 +-
 mdopen.c  |  19 +-
 udev.c    | 191 +++++++++++++++
 udev.h    |  38 +++
 util.c    |  46 ++++
 10 files changed, 713 insertions(+), 343 deletions(-)
 create mode 100644 udev.c
 create mode 100644 udev.h

-- 
2.26.2

[PATCH 1/9] Mdmonitor: Split alert() into separate functions

[Mateusz Grzonka]

Signed-off-by: Mateusz Grzonka <mateusz.grzonka@intel.com>
---
 Monitor.c | 186 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 95 insertions(+), 91 deletions(-)

diff --git a/Monitor.c b/Monitor.c
index c0ab5412..65e66474 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -66,7 +66,7 @@ struct alert_info {
 static int make_daemon(char *pidfile);
 static int check_one_sharer(int scan);
 static void write_autorebuild_pid(void);
-static void alert(char *event, char *dev, char *disc, struct alert_info *info);
+static void alert(const char *event, const char *dev, const char *disc, struct alert_info *info);
 static int check_array(struct state *st, struct mdstat_ent *mdstat,
 		       int test, struct alert_info *info,
 		       int increments, char *prefer);
@@ -402,111 +402,115 @@ static void write_autorebuild_pid()
 	}
 }
 
-static void alert(char *event, char *dev, char *disc, struct alert_info *info)
+static void execute_alert_cmd(const char *event, const char *dev, const char *disc, struct alert_info *info)
+{
+	int pid = fork();
+
+	switch (pid) {
+	default:
+		waitpid(pid, NULL, 0);
+		break;
+	case -1:
+		pr_err("Cannot fork to execute alert command");
+		break;
+	case 0:
+		execl(info->alert_cmd, info->alert_cmd, event, dev, disc, NULL);
+		exit(2);
+	}
+}
+
+static void send_event_email(const char *event, const char *dev, const char *disc, struct alert_info *info)
+{
+	FILE *mp, *mdstat;
+	char hname[256];
+	char buf[BUFSIZ];
+	int n;
+
+	mp = popen(Sendmail, "w");
+	if (!mp) {
+		pr_err("Cannot open pipe stream for sendmail.\n");
+		return;
+	}
+
+	gethostname(hname, sizeof(hname));
+	signal(SIGPIPE, SIG_IGN);
+	if (info->mailfrom)
+		fprintf(mp, "From: %s\n", info->mailfrom);
+	else
+		fprintf(mp, "From: %s monitoring <root>\n", Name);
+	fprintf(mp, "To: %s\n", info->mailaddr);
+	fprintf(mp, "Subject: %s event on %s:%s\n\n", event, dev, hname);
+	fprintf(mp, "This is an automatically generated mail message. \n");
+	fprintf(mp, "A %s event had been detected on md device %s.\n\n", event, dev);
+
+	if (disc && disc[0] != ' ')
+		fprintf(mp,
+			"It could be related to component device %s.\n\n", disc);
+	if (disc && disc[0] == ' ')
+		fprintf(mp, "Extra information:%s.\n\n", disc);
+
+	mdstat = fopen("/proc/mdstat", "r");
+	if (!mdstat) {
+		pr_err("Cannot open /proc/mdstat\n");
+		pclose(mp);
+		return;
+	}
+
+	fprintf(mp, "The /proc/mdstat file currently contains the following:\n\n");
+	while ((n = fread(buf, 1, sizeof(buf), mdstat)) > 0)
+		n = fwrite(buf, 1, n, mp);
+	fclose(mdstat);
+	pclose(mp);
+}
+
+static void log_event_to_syslog(const char *event, const char *dev, const char *disc)
 {
 	int priority;
+	/* Log at a different severity depending on the event.
+	 *
+	 * These are the critical events:  */
+	if (strncmp(event, "Fail", 4) == 0 ||
+		strncmp(event, "Degrade", 7) == 0 ||
+		strncmp(event, "DeviceDisappeared", 17) == 0)
+		priority = LOG_CRIT;
+	/* Good to know about, but are not failures: */
+	else if (strncmp(event, "Rebuild", 7) == 0 ||
+			strncmp(event, "MoveSpare", 9) == 0 ||
+			strncmp(event, "Spares", 6) != 0)
+		priority = LOG_WARNING;
+	/* Everything else: */
+	else
+		priority = LOG_INFO;
 
+	if (disc && disc[0] != ' ')
+		syslog(priority,
+			"%s event detected on md device %s, component device %s", event, dev, disc);
+	else if (disc)
+		syslog(priority, "%s event detected on md device %s: %s", event, dev, disc);
+	else
+		syslog(priority, "%s event detected on md device %s", event, dev);
+}
+
+static void alert(const char *event, const char *dev, const char *disc, struct alert_info *info)
+{
 	if (!info->alert_cmd && !info->mailaddr && !info->dosyslog) {
 		time_t now = time(0);
 
 		printf("%1.15s: %s on %s %s\n", ctime(&now) + 4,
 		       event, dev, disc?disc:"unknown device");
 	}
-	if (info->alert_cmd) {
-		int pid = fork();
-		switch(pid) {
-		default:
-			waitpid(pid, NULL, 0);
-			break;
-		case -1:
-			break;
-		case 0:
-			execl(info->alert_cmd, info->alert_cmd,
-			      event, dev, disc, NULL);
-			exit(2);
-		}
-	}
+	if (info->alert_cmd)
+		execute_alert_cmd(event, dev, disc, info);
+
 	if (info->mailaddr && (strncmp(event, "Fail", 4) == 0 ||
 			       strncmp(event, "Test", 4) == 0 ||
 			       strncmp(event, "Spares", 6) == 0 ||
 			       strncmp(event, "Degrade", 7) == 0)) {
-		FILE *mp = popen(Sendmail, "w");
-		if (mp) {
-			FILE *mdstat;
-			char hname[256];
-
-			gethostname(hname, sizeof(hname));
-			signal_s(SIGPIPE, SIG_IGN);
-
-			if (info->mailfrom)
-				fprintf(mp, "From: %s\n", info->mailfrom);
-			else
-				fprintf(mp, "From: %s monitoring <root>\n",
-					Name);
-			fprintf(mp, "To: %s\n", info->mailaddr);
-			fprintf(mp, "Subject: %s event on %s:%s\n\n",
-				event, dev, hname);
-
-			fprintf(mp,
-				"This is an automatically generated mail message from %s\n", Name);
-			fprintf(mp, "running on %s\n\n", hname);
-
-			fprintf(mp,
-				"A %s event had been detected on md device %s.\n\n", event, dev);
-
-			if (disc && disc[0] != ' ')
-				fprintf(mp,
-					"It could be related to component device %s.\n\n", disc);
-			if (disc && disc[0] == ' ')
-				fprintf(mp, "Extra information:%s.\n\n", disc);
-
-			fprintf(mp, "Faithfully yours, etc.\n");
-
-			mdstat = fopen("/proc/mdstat", "r");
-			if (mdstat) {
-				char buf[8192];
-				int n;
-				fprintf(mp,
-					"\nP.S. The /proc/mdstat file currently contains the following:\n\n");
-				while ((n = fread(buf, 1, sizeof(buf),
-						  mdstat)) > 0)
-					n = fwrite(buf, 1, n, mp);
-				fclose(mdstat);
-			}
-			pclose(mp);
-		}
+		send_event_email(event, dev, disc, info);
 	}
 
-	/* log the event to syslog maybe */
-	if (info->dosyslog) {
-		/* Log at a different severity depending on the event.
-		 *
-		 * These are the critical events:  */
-		if (strncmp(event, "Fail", 4) == 0 ||
-		    strncmp(event, "Degrade", 7) == 0 ||
-		    strncmp(event, "DeviceDisappeared", 17) == 0)
-			priority = LOG_CRIT;
-		/* Good to know about, but are not failures: */
-		else if (strncmp(event, "Rebuild", 7) == 0 ||
-			 strncmp(event, "MoveSpare", 9) == 0 ||
-			 strncmp(event, "Spares", 6) != 0)
-			priority = LOG_WARNING;
-		/* Everything else: */
-		else
-			priority = LOG_INFO;
-
-		if (disc && disc[0] != ' ')
-			syslog(priority,
-			       "%s event detected on md device %s, component device %s", event, dev, disc);
-		else if (disc)
-			syslog(priority,
-			       "%s event detected on md device %s: %s",
-			       event, dev, disc);
-		else
-			syslog(priority,
-			       "%s event detected on md device %s",
-			       event, dev);
-	}
+	if (info->dosyslog)
+		log_event_to_syslog(event, dev, disc);
 }
 
 static int check_array(struct state *st, struct mdstat_ent *mdstat,
-- 
2.26.2

Re: [PATCH 1/9] Mdmonitor: Split alert() into separate functions

[Coly Li]

On 9/7/22 8:56 PM, Mateusz Grzonka wrote:
> Signed-off-by: Mateusz Grzonka<mateusz.grzonka@intel.com>
> ---
>   Monitor.c | 186 ++++++++++++++++++++++++++++--------------------------
>   1 file changed, 95 insertions(+), 91 deletions(-)
>
> diff --git a/Monitor.c b/Monitor.c
> index c0ab5412..65e66474 100644
> --- a/Monitor.c
> +++ b/Monitor.c
> @@ -66,7 +66,7 @@ struct alert_info {
>   static int make_daemon(char *pidfile);
>   static int check_one_sharer(int scan);
>   static void write_autorebuild_pid(void);
> -static void alert(char *event, char *dev, char *disc, struct alert_info *info);
> +static void alert(const char *event, const char *dev, const char *disc, struct alert_info *info);
>   static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   		       int test, struct alert_info *info,
>   		       int increments, char *prefer);
> @@ -402,111 +402,115 @@ static void write_autorebuild_pid()
>   	}
>   }
>   
> -static void alert(char *event, char *dev, char *disc, struct alert_info *info)
> +static void execute_alert_cmd(const char *event, const char *dev, const char *disc, struct alert_info *info)
> +{


The above line is too long, it would be better to modify the parameter 
list into multiple lines.


> +	int pid = fork();
> +
> +	switch (pid) {
> +	default:
> +		waitpid(pid, NULL, 0);
> +		break;
> +	case -1:
> +		pr_err("Cannot fork to execute alert command");


‘\n’ at the tail missed ?


> +		break;
> +	case 0:
> +		execl(info->alert_cmd, info->alert_cmd, event, dev, disc, NULL);
> +		exit(2);
> +	}
> +}
> +
> +static void send_event_email(const char *event, const char *dev, const char *disc, struct alert_info *info)
> +{
> +	FILE *mp, *mdstat;
> +	char hname[256];
> +	char buf[BUFSIZ];
> +	int n;
> +
> +	mp = popen(Sendmail, "w");
> +	if (!mp) {
> +		pr_err("Cannot open pipe stream for sendmail.\n");
> +		return;
> +	}
> +
> +	gethostname(hname, sizeof(hname));
> +	signal(SIGPIPE, SIG_IGN);
> +	if (info->mailfrom)
> +		fprintf(mp, "From: %s\n", info->mailfrom);
> +	else
> +		fprintf(mp, "From: %s monitoring <root>\n", Name);
> +	fprintf(mp, "To: %s\n", info->mailaddr);
> +	fprintf(mp, "Subject: %s event on %s:%s\n\n", event, dev, hname);
> +	fprintf(mp, "This is an automatically generated mail message. \n");
> +	fprintf(mp, "A %s event had been detected on md device %s.\n\n", event, dev);
> +
> +	if (disc && disc[0] != ' ')
> +		fprintf(mp,
> +			"It could be related to component device %s.\n\n", disc);
> +	if (disc && disc[0] == ' ')
> +		fprintf(mp, "Extra information:%s.\n\n", disc);
> +
> +	mdstat = fopen("/proc/mdstat", "r");
> +	if (!mdstat) {
> +		pr_err("Cannot open /proc/mdstat\n");
> +		pclose(mp);
> +		return;
> +	}
> +
> +	fprintf(mp, "The /proc/mdstat file currently contains the following:\n\n");
> +	while ((n = fread(buf, 1, sizeof(buf), mdstat)) > 0)
> +		n = fwrite(buf, 1, n, mp);
> +	fclose(mdstat);
> +	pclose(mp);
> +}
> +
> +static void log_event_to_syslog(const char *event, const char *dev, const char *disc)
>   {
>   	int priority;
> +	/* Log at a different severity depending on the event.
> +	 *
> +	 * These are the critical events:  */

The original code comment didn’t follow existed mdadm code style, maybe 
we can fix the above mismatched code style here?


The rested part looks good to me.

Thanks.

Coly Li


> +	if (strncmp(event, "Fail", 4) == 0 ||
> +		strncmp(event, "Degrade", 7) == 0 ||
> +		strncmp(event, "DeviceDisappeared", 17) == 0)
> +		priority = LOG_CRIT;
> +	/* Good to know about, but are not failures: */
> +	else if (strncmp(event, "Rebuild", 7) == 0 ||
> +			strncmp(event, "MoveSpare", 9) == 0 ||
> +			strncmp(event, "Spares", 6) != 0)
> +		priority = LOG_WARNING;
> +	/* Everything else: */
> +	else
> +		priority = LOG_INFO;
>   
> +	if (disc && disc[0] != ' ')
> +		syslog(priority,
> +			"%s event detected on md device %s, component device %s", event, dev, disc);
> +	else if (disc)
> +		syslog(priority, "%s event detected on md device %s: %s", event, dev, disc);
> +	else
> +		syslog(priority, "%s event detected on md device %s", event, dev);
> +}
> +
> +static void alert(const char *event, const char *dev, const char *disc, struct alert_info *info)
> +{
>   	if (!info->alert_cmd && !info->mailaddr && !info->dosyslog) {
>   		time_t now = time(0);
>   
>   		printf("%1.15s: %s on %s %s\n", ctime(&now) + 4,
>   		       event, dev, disc?disc:"unknown device");
>   	}
> -	if (info->alert_cmd) {
> -		int pid = fork();
> -		switch(pid) {
> -		default:
> -			waitpid(pid, NULL, 0);
> -			break;
> -		case -1:
> -			break;
> -		case 0:
> -			execl(info->alert_cmd, info->alert_cmd,
> -			      event, dev, disc, NULL);
> -			exit(2);
> -		}
> -	}
> +	if (info->alert_cmd)
> +		execute_alert_cmd(event, dev, disc, info);
> +
>   	if (info->mailaddr && (strncmp(event, "Fail", 4) == 0 ||
>   			       strncmp(event, "Test", 4) == 0 ||
>   			       strncmp(event, "Spares", 6) == 0 ||
>   			       strncmp(event, "Degrade", 7) == 0)) {
> -		FILE *mp = popen(Sendmail, "w");
> -		if (mp) {
> -			FILE *mdstat;
> -			char hname[256];
> -
> -			gethostname(hname, sizeof(hname));
> -			signal_s(SIGPIPE, SIG_IGN);
> -
> -			if (info->mailfrom)
> -				fprintf(mp, "From: %s\n", info->mailfrom);
> -			else
> -				fprintf(mp, "From: %s monitoring <root>\n",
> -					Name);
> -			fprintf(mp, "To: %s\n", info->mailaddr);
> -			fprintf(mp, "Subject: %s event on %s:%s\n\n",
> -				event, dev, hname);
> -
> -			fprintf(mp,
> -				"This is an automatically generated mail message from %s\n", Name);
> -			fprintf(mp, "running on %s\n\n", hname);
> -
> -			fprintf(mp,
> -				"A %s event had been detected on md device %s.\n\n", event, dev);
> -
> -			if (disc && disc[0] != ' ')
> -				fprintf(mp,
> -					"It could be related to component device %s.\n\n", disc);
> -			if (disc && disc[0] == ' ')
> -				fprintf(mp, "Extra information:%s.\n\n", disc);
> -
> -			fprintf(mp, "Faithfully yours, etc.\n");
> -
> -			mdstat = fopen("/proc/mdstat", "r");
> -			if (mdstat) {
> -				char buf[8192];
> -				int n;
> -				fprintf(mp,
> -					"\nP.S. The /proc/mdstat file currently contains the following:\n\n");
> -				while ((n = fread(buf, 1, sizeof(buf),
> -						  mdstat)) > 0)
> -					n = fwrite(buf, 1, n, mp);
> -				fclose(mdstat);
> -			}
> -			pclose(mp);
> -		}
> +		send_event_email(event, dev, disc, info);
>   	}
>   
> -	/* log the event to syslog maybe */
> -	if (info->dosyslog) {
> -		/* Log at a different severity depending on the event.
> -		 *
> -		 * These are the critical events:  */
> -		if (strncmp(event, "Fail", 4) == 0 ||
> -		    strncmp(event, "Degrade", 7) == 0 ||
> -		    strncmp(event, "DeviceDisappeared", 17) == 0)
> -			priority = LOG_CRIT;
> -		/* Good to know about, but are not failures: */
> -		else if (strncmp(event, "Rebuild", 7) == 0 ||
> -			 strncmp(event, "MoveSpare", 9) == 0 ||
> -			 strncmp(event, "Spares", 6) != 0)
> -			priority = LOG_WARNING;
> -		/* Everything else: */
> -		else
> -			priority = LOG_INFO;
> -
> -		if (disc && disc[0] != ' ')
> -			syslog(priority,
> -			       "%s event detected on md device %s, component device %s", event, dev, disc);
> -		else if (disc)
> -			syslog(priority,
> -			       "%s event detected on md device %s: %s",
> -			       event, dev, disc);
> -		else
> -			syslog(priority,
> -			       "%s event detected on md device %s",
> -			       event, dev);
> -	}
> +	if (info->dosyslog)
> +		log_event_to_syslog(event, dev, disc);
>   }
>   
>   static int check_array(struct state *st, struct mdstat_ent *mdstat,

Re: [PATCH 1/9] Mdmonitor: Split alert() into separate functions

[Jes Sorensen]

On 9/7/22 08:56, Mateusz Grzonka wrote:
> Signed-off-by: Mateusz Grzonka <mateusz.grzonka@intel.com>
> ---
>  Monitor.c | 186 ++++++++++++++++++++++++++++--------------------------
>  1 file changed, 95 insertions(+), 91 deletions(-)

Applied!

Thanks,
Jes

[PATCH 2/9] Mdmonitor: Make alert_info global

[Mateusz Grzonka]

Move information about --test flag and hostname into alert_info.

Signed-off-by: Mateusz Grzonka <mateusz.grzonka@intel.com>
---
 Monitor.c | 124 +++++++++++++++++++++++++++---------------------------
 1 file changed, 61 insertions(+), 63 deletions(-)

diff --git a/Monitor.c b/Monitor.c
index 65e66474..73c5790d 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -58,21 +58,20 @@ struct state {
 };
 
 struct alert_info {
+	char hostname[HOST_NAME_MAX];
 	char *mailaddr;
 	char *mailfrom;
 	char *alert_cmd;
 	int dosyslog;
-};
+	int test;
+} info;
 static int make_daemon(char *pidfile);
 static int check_one_sharer(int scan);
 static void write_autorebuild_pid(void);
-static void alert(const char *event, const char *dev, const char *disc, struct alert_info *info);
-static int check_array(struct state *st, struct mdstat_ent *mdstat,
-		       int test, struct alert_info *info,
-		       int increments, char *prefer);
-static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist,
-			  int test, struct alert_info *info);
-static void try_spare_migration(struct state *statelist, struct alert_info *info);
+static void alert(const char *event, const char *dev, const char *disc);
+static int check_array(struct state *st, struct mdstat_ent *mdstat, int increments, char *prefer);
+static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist);
+static void try_spare_migration(struct state *statelist);
 static void link_containers_with_subarrays(struct state *list);
 #ifndef NO_LIBUDEV
 static int check_udev_activity(void);
@@ -132,7 +131,6 @@ int Monitor(struct mddev_dev *devlist,
 	int finished = 0;
 	struct mdstat_ent *mdstat = NULL;
 	char *mailfrom;
-	struct alert_info info;
 	struct mddev_ident *mdlist;
 	int delay_for_event = c->delay;
 
@@ -158,6 +156,13 @@ int Monitor(struct mddev_dev *devlist,
 	info.mailaddr = mailaddr;
 	info.mailfrom = mailfrom;
 	info.dosyslog = dosyslog;
+	info.test = c->test;
+
+	if (gethostname(info.hostname, sizeof(info.hostname)) != 0) {
+		pr_err("Cannot get hostname.\n");
+		return 1;
+	}
+	info.hostname[sizeof(info.hostname) - 1] = '\0';
 
 	if (share){
 		if (check_one_sharer(c->scan))
@@ -230,8 +235,7 @@ int Monitor(struct mddev_dev *devlist,
 		mdstat = mdstat_read(oneshot ? 0 : 1, 0);
 
 		for (st = statelist; st; st = st->next) {
-			if (check_array(st, mdstat, c->test, &info,
-					increments, c->prefer))
+			if (check_array(st, mdstat, increments, c->prefer))
 				anydegraded = 1;
 			/* for external arrays, metadata is filled for
 			 * containers only
@@ -244,15 +248,14 @@ int Monitor(struct mddev_dev *devlist,
 
 		/* now check if there are any new devices found in mdstat */
 		if (c->scan)
-			new_found = add_new_arrays(mdstat, &statelist, c->test,
-						   &info);
+			new_found = add_new_arrays(mdstat, &statelist);
 
 		/* If an array has active < raid && spare == 0 && spare_group != NULL
 		 * Look for another array with spare > 0 and active == raid and same spare_group
 		 * if found, choose a device and hotremove/hotadd
 		 */
 		if (share && anydegraded)
-			try_spare_migration(statelist, &info);
+			try_spare_migration(statelist);
 		if (!new_found) {
 			if (oneshot)
 				break;
@@ -283,7 +286,7 @@ int Monitor(struct mddev_dev *devlist,
 				mdstat_close();
 			}
 		}
-		c->test = 0;
+		info.test = 0;
 
 		for (stp = &statelist; (st = *stp) != NULL; ) {
 			if (st->from_auto && st->err > 5) {
@@ -402,7 +405,7 @@ static void write_autorebuild_pid()
 	}
 }
 
-static void execute_alert_cmd(const char *event, const char *dev, const char *disc, struct alert_info *info)
+static void execute_alert_cmd(const char *event, const char *dev, const char *disc)
 {
 	int pid = fork();
 
@@ -414,15 +417,14 @@ static void execute_alert_cmd(const char *event, const char *dev, const char *di
 		pr_err("Cannot fork to execute alert command");
 		break;
 	case 0:
-		execl(info->alert_cmd, info->alert_cmd, event, dev, disc, NULL);
+		execl(info.alert_cmd, info.alert_cmd, event, dev, disc, NULL);
 		exit(2);
 	}
 }
 
-static void send_event_email(const char *event, const char *dev, const char *disc, struct alert_info *info)
+static void send_event_email(const char *event, const char *dev, const char *disc)
 {
 	FILE *mp, *mdstat;
-	char hname[256];
 	char buf[BUFSIZ];
 	int n;
 
@@ -432,14 +434,13 @@ static void send_event_email(const char *event, const char *dev, const char *dis
 		return;
 	}
 
-	gethostname(hname, sizeof(hname));
 	signal(SIGPIPE, SIG_IGN);
-	if (info->mailfrom)
-		fprintf(mp, "From: %s\n", info->mailfrom);
+	if (info.mailfrom)
+		fprintf(mp, "From: %s\n", info.mailfrom);
 	else
 		fprintf(mp, "From: %s monitoring <root>\n", Name);
-	fprintf(mp, "To: %s\n", info->mailaddr);
-	fprintf(mp, "Subject: %s event on %s:%s\n\n", event, dev, hname);
+	fprintf(mp, "To: %s\n", info.mailaddr);
+	fprintf(mp, "Subject: %s event on %s:%s\n\n", event, dev, info.hostname);
 	fprintf(mp, "This is an automatically generated mail message. \n");
 	fprintf(mp, "A %s event had been detected on md device %s.\n\n", event, dev);
 
@@ -491,37 +492,36 @@ static void log_event_to_syslog(const char *event, const char *dev, const char *
 		syslog(priority, "%s event detected on md device %s", event, dev);
 }
 
-static void alert(const char *event, const char *dev, const char *disc, struct alert_info *info)
+static void alert(const char *event, const char *dev, const char *disc)
 {
-	if (!info->alert_cmd && !info->mailaddr && !info->dosyslog) {
+	if (!info.alert_cmd && !info.mailaddr && !info.dosyslog) {
 		time_t now = time(0);
 
 		printf("%1.15s: %s on %s %s\n", ctime(&now) + 4,
 		       event, dev, disc?disc:"unknown device");
 	}
-	if (info->alert_cmd)
-		execute_alert_cmd(event, dev, disc, info);
+	if (info.alert_cmd)
+		execute_alert_cmd(event, dev, disc);
 
-	if (info->mailaddr && (strncmp(event, "Fail", 4) == 0 ||
+	if (info.mailaddr && (strncmp(event, "Fail", 4) == 0 ||
 			       strncmp(event, "Test", 4) == 0 ||
 			       strncmp(event, "Spares", 6) == 0 ||
 			       strncmp(event, "Degrade", 7) == 0)) {
-		send_event_email(event, dev, disc, info);
+		send_event_email(event, dev, disc);
 	}
 
-	if (info->dosyslog)
+	if (info.dosyslog)
 		log_event_to_syslog(event, dev, disc);
 }
 
 static int check_array(struct state *st, struct mdstat_ent *mdstat,
-		       int test, struct alert_info *ainfo,
 		       int increments, char *prefer)
 {
 	/* Update the state 'st' to reflect any changes shown in mdstat,
 	 * or found by directly examining the array, and return
 	 * '1' if the array is degraded, or '0' if it is optimal (or dead).
 	 */
-	struct { int state, major, minor; } info[MAX_DISKS];
+	struct { int state, major, minor; } disks_info[MAX_DISKS];
 	struct mdinfo *sra = NULL;
 	mdu_array_info_t array;
 	struct mdstat_ent *mse = NULL, *mse2;
@@ -535,8 +535,8 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 	int is_container = 0;
 	unsigned long redundancy_only_flags = 0;
 
-	if (test)
-		alert("TestMessage", dev, NULL, ainfo);
+	if (info.test)
+		alert("TestMessage", dev, NULL);
 
 	retval = 0;
 
@@ -585,7 +585,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 	 */
 	if (sra->array.level == 0 || sra->array.level == -1) {
 		if (!st->err && !st->from_config)
-			alert("DeviceDisappeared", dev, " Wrong-Level", ainfo);
+			alert("DeviceDisappeared", dev, " Wrong-Level");
 		st->err++;
 		goto out;
 	}
@@ -602,7 +602,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 		st->percent = RESYNC_NONE;
 		new_array = 1;
 		if (!is_container)
-			alert("NewArray", st->devname, NULL, ainfo);
+			alert("NewArray", st->devname, NULL);
 	}
 
 	if (st->utime == array.utime && st->failed == sra->array.failed_disks &&
@@ -615,14 +615,14 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 	}
 	if (st->utime == 0 && /* new array */
 	    mse->pattern && strchr(mse->pattern, '_') /* degraded */)
-		alert("DegradedArray", dev, NULL, ainfo);
+		alert("DegradedArray", dev, NULL);
 
 	if (st->utime == 0 && /* new array */ st->expected_spares > 0 &&
 	    sra->array.spare_disks < st->expected_spares)
-		alert("SparesMissing", dev, NULL, ainfo);
+		alert("SparesMissing", dev, NULL);
 	if (st->percent < 0 && st->percent != RESYNC_UNKNOWN &&
 	    mse->percent >= 0)
-		alert("RebuildStarted", dev, NULL, ainfo);
+		alert("RebuildStarted", dev, NULL);
 	if (st->percent >= 0 && mse->percent >= 0 &&
 	    (mse->percent / increments) > (st->percent / increments)) {
 		char percentalert[18];
@@ -637,7 +637,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 			snprintf(percentalert, sizeof(percentalert),
 				 "Rebuild%02d", mse->percent);
 
-		alert(percentalert, dev, NULL, ainfo);
+		alert(percentalert, dev, NULL);
 	}
 
 	if (mse->percent == RESYNC_NONE && st->percent >= 0) {
@@ -650,9 +650,9 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 			snprintf(cnt, sizeof(cnt),
 				 " mismatches found: %d (on raid level %d)",
 				 sra->mismatch_cnt, sra->array.level);
-			alert("RebuildFinished", dev, cnt, ainfo);
+			alert("RebuildFinished", dev, cnt);
 		} else
-			alert("RebuildFinished", dev, NULL, ainfo);
+			alert("RebuildFinished", dev, NULL);
 	}
 	st->percent = mse->percent;
 
@@ -661,13 +661,13 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 		mdu_disk_info_t disc;
 		disc.number = i;
 		if (md_get_disk_info(fd, &disc) >= 0) {
-			info[i].state = disc.state;
-			info[i].major = disc.major;
-			info[i].minor = disc.minor;
+			disks_info[i].state = disc.state;
+			disks_info[i].major = disc.major;
+			disks_info[i].minor = disc.minor;
 			if (disc.major || disc.minor)
 				remaining_disks --;
 		} else
-			info[i].major = info[i].minor = 0;
+			disks_info[i].major = disks_info[i].minor = 0;
 	}
 	last_disk = i;
 
@@ -690,13 +690,13 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 		int change;
 		char *dv = NULL;
 		disc.number = i;
-		if (i < last_disk && (info[i].major || info[i].minor)) {
-			newstate = info[i].state;
-			dv = map_dev_preferred(info[i].major, info[i].minor, 1,
+		if (i < last_disk && (disks_info[i].major || disks_info[i].minor)) {
+			newstate = disks_info[i].state;
+			dv = map_dev_preferred(disks_info[i].major, disks_info[i].minor, 1,
 					       prefer);
 			disc.state = newstate;
-			disc.major = info[i].major;
-			disc.minor = info[i].minor;
+			disc.major = disks_info[i].major;
+			disc.minor = disks_info[i].minor;
 		} else
 			newstate = (1 << MD_DISK_REMOVED);
 
@@ -706,14 +706,14 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 		change = newstate ^ st->devstate[i];
 		if (st->utime && change && !st->err && !new_array) {
 			if ((st->devstate[i]&change) & (1 << MD_DISK_SYNC))
-				alert("Fail", dev, dv, ainfo);
+				alert("Fail", dev, dv);
 			else if ((newstate & (1 << MD_DISK_FAULTY)) &&
 				 (disc.major || disc.minor) &&
 				 st->devid[i] == makedev(disc.major,
 							 disc.minor))
-				alert("FailSpare", dev, dv, ainfo);
+				alert("FailSpare", dev, dv);
 			else if ((newstate&change) & (1 << MD_DISK_SYNC))
-				alert("SpareActive", dev, dv, ainfo);
+				alert("SpareActive", dev, dv);
 		}
 		st->devstate[i] = newstate;
 		st->devid[i] = makedev(disc.major, disc.minor);
@@ -737,13 +737,12 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 
  disappeared:
 	if (!st->err && !is_container)
-		alert("DeviceDisappeared", dev, NULL, ainfo);
+		alert("DeviceDisappeared", dev, NULL);
 	st->err++;
 	goto out;
 }
 
-static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist,
-			  int test, struct alert_info *info)
+static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist)
 {
 	struct mdstat_ent *mse;
 	int new_found = 0;
@@ -797,8 +796,8 @@ static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist,
 			} else
 				st->parent_devnm[0] = 0;
 			*statelist = st;
-			if (test)
-				alert("TestMessage", st->devname, NULL, info);
+			if (info.test)
+				alert("TestMessage", st->devname, NULL);
 			new_found = 1;
 		}
 	return new_found;
@@ -962,7 +961,7 @@ static dev_t container_choose_spare(struct state *from, struct state *to,
 	return dev;
 }
 
-static void try_spare_migration(struct state *statelist, struct alert_info *info)
+static void try_spare_migration(struct state *statelist)
 {
 	struct state *from;
 	struct state *st;
@@ -1021,8 +1020,7 @@ static void try_spare_migration(struct state *statelist, struct alert_info *info
 				if (devid > 0 &&
 				    move_spare(from->devname, to->devname,
 					       devid)) {
-					alert("MoveSpare", to->devname,
-					      from->devname, info);
+					alert("MoveSpare", to->devname, from->devname);
 					break;
 				}
 			}
-- 
2.26.2

Re: [PATCH 2/9] Mdmonitor: Make alert_info global

[Coly Li]

On 9/7/22 8:56 PM, Mateusz Grzonka wrote:
> Move information about --test flag and hostname into alert_info.
>
> Signed-off-by: Mateusz Grzonka<mateusz.grzonka@intel.com>

Looks good to me.

Acked-by: Coly Li <colyli@suse.de>


Thanks.

Coly Li

> ---
>   Monitor.c | 124 +++++++++++++++++++++++++++---------------------------
>   1 file changed, 61 insertions(+), 63 deletions(-)
>
> diff --git a/Monitor.c b/Monitor.c
> index 65e66474..73c5790d 100644
> --- a/Monitor.c
> +++ b/Monitor.c
> @@ -58,21 +58,20 @@ struct state {
>   };
>   
>   struct alert_info {
> +	char hostname[HOST_NAME_MAX];
>   	char *mailaddr;
>   	char *mailfrom;
>   	char *alert_cmd;
>   	int dosyslog;
> -};
> +	int test;
> +} info;
>   static int make_daemon(char *pidfile);
>   static int check_one_sharer(int scan);
>   static void write_autorebuild_pid(void);
> -static void alert(const char *event, const char *dev, const char *disc, struct alert_info *info);
> -static int check_array(struct state *st, struct mdstat_ent *mdstat,
> -		       int test, struct alert_info *info,
> -		       int increments, char *prefer);
> -static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist,
> -			  int test, struct alert_info *info);
> -static void try_spare_migration(struct state *statelist, struct alert_info *info);
> +static void alert(const char *event, const char *dev, const char *disc);
> +static int check_array(struct state *st, struct mdstat_ent *mdstat, int increments, char *prefer);
> +static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist);
> +static void try_spare_migration(struct state *statelist);
>   static void link_containers_with_subarrays(struct state *list);
>   #ifndef NO_LIBUDEV
>   static int check_udev_activity(void);
> @@ -132,7 +131,6 @@ int Monitor(struct mddev_dev *devlist,
>   	int finished = 0;
>   	struct mdstat_ent *mdstat = NULL;
>   	char *mailfrom;
> -	struct alert_info info;
>   	struct mddev_ident *mdlist;
>   	int delay_for_event = c->delay;
>   
> @@ -158,6 +156,13 @@ int Monitor(struct mddev_dev *devlist,
>   	info.mailaddr = mailaddr;
>   	info.mailfrom = mailfrom;
>   	info.dosyslog = dosyslog;
> +	info.test = c->test;
> +
> +	if (gethostname(info.hostname, sizeof(info.hostname)) != 0) {
> +		pr_err("Cannot get hostname.\n");
> +		return 1;
> +	}
> +	info.hostname[sizeof(info.hostname) - 1] = '\0';
>   
>   	if (share){
>   		if (check_one_sharer(c->scan))
> @@ -230,8 +235,7 @@ int Monitor(struct mddev_dev *devlist,
>   		mdstat = mdstat_read(oneshot ? 0 : 1, 0);
>   
>   		for (st = statelist; st; st = st->next) {
> -			if (check_array(st, mdstat, c->test, &info,
> -					increments, c->prefer))
> +			if (check_array(st, mdstat, increments, c->prefer))
>   				anydegraded = 1;
>   			/* for external arrays, metadata is filled for
>   			 * containers only
> @@ -244,15 +248,14 @@ int Monitor(struct mddev_dev *devlist,
>   
>   		/* now check if there are any new devices found in mdstat */
>   		if (c->scan)
> -			new_found = add_new_arrays(mdstat, &statelist, c->test,
> -						   &info);
> +			new_found = add_new_arrays(mdstat, &statelist);
>   
>   		/* If an array has active < raid && spare == 0 && spare_group != NULL
>   		 * Look for another array with spare > 0 and active == raid and same spare_group
>   		 * if found, choose a device and hotremove/hotadd
>   		 */
>   		if (share && anydegraded)
> -			try_spare_migration(statelist, &info);
> +			try_spare_migration(statelist);
>   		if (!new_found) {
>   			if (oneshot)
>   				break;
> @@ -283,7 +286,7 @@ int Monitor(struct mddev_dev *devlist,
>   				mdstat_close();
>   			}
>   		}
> -		c->test = 0;
> +		info.test = 0;
>   
>   		for (stp = &statelist; (st = *stp) != NULL; ) {
>   			if (st->from_auto && st->err > 5) {
> @@ -402,7 +405,7 @@ static void write_autorebuild_pid()
>   	}
>   }
>   
> -static void execute_alert_cmd(const char *event, const char *dev, const char *disc, struct alert_info *info)
> +static void execute_alert_cmd(const char *event, const char *dev, const char *disc)
>   {
>   	int pid = fork();
>   
> @@ -414,15 +417,14 @@ static void execute_alert_cmd(const char *event, const char *dev, const char *di
>   		pr_err("Cannot fork to execute alert command");
>   		break;
>   	case 0:
> -		execl(info->alert_cmd, info->alert_cmd, event, dev, disc, NULL);
> +		execl(info.alert_cmd, info.alert_cmd, event, dev, disc, NULL);
>   		exit(2);
>   	}
>   }
>   
> -static void send_event_email(const char *event, const char *dev, const char *disc, struct alert_info *info)
> +static void send_event_email(const char *event, const char *dev, const char *disc)
>   {
>   	FILE *mp, *mdstat;
> -	char hname[256];
>   	char buf[BUFSIZ];
>   	int n;
>   
> @@ -432,14 +434,13 @@ static void send_event_email(const char *event, const char *dev, const char *dis
>   		return;
>   	}
>   
> -	gethostname(hname, sizeof(hname));
>   	signal(SIGPIPE, SIG_IGN);
> -	if (info->mailfrom)
> -		fprintf(mp, "From: %s\n", info->mailfrom);
> +	if (info.mailfrom)
> +		fprintf(mp, "From: %s\n", info.mailfrom);
>   	else
>   		fprintf(mp, "From: %s monitoring <root>\n", Name);
> -	fprintf(mp, "To: %s\n", info->mailaddr);
> -	fprintf(mp, "Subject: %s event on %s:%s\n\n", event, dev, hname);
> +	fprintf(mp, "To: %s\n", info.mailaddr);
> +	fprintf(mp, "Subject: %s event on %s:%s\n\n", event, dev, info.hostname);
>   	fprintf(mp, "This is an automatically generated mail message. \n");
>   	fprintf(mp, "A %s event had been detected on md device %s.\n\n", event, dev);
>   
> @@ -491,37 +492,36 @@ static void log_event_to_syslog(const char *event, const char *dev, const char *
>   		syslog(priority, "%s event detected on md device %s", event, dev);
>   }
>   
> -static void alert(const char *event, const char *dev, const char *disc, struct alert_info *info)
> +static void alert(const char *event, const char *dev, const char *disc)
>   {
> -	if (!info->alert_cmd && !info->mailaddr && !info->dosyslog) {
> +	if (!info.alert_cmd && !info.mailaddr && !info.dosyslog) {
>   		time_t now = time(0);
>   
>   		printf("%1.15s: %s on %s %s\n", ctime(&now) + 4,
>   		       event, dev, disc?disc:"unknown device");
>   	}
> -	if (info->alert_cmd)
> -		execute_alert_cmd(event, dev, disc, info);
> +	if (info.alert_cmd)
> +		execute_alert_cmd(event, dev, disc);
>   
> -	if (info->mailaddr && (strncmp(event, "Fail", 4) == 0 ||
> +	if (info.mailaddr && (strncmp(event, "Fail", 4) == 0 ||
>   			       strncmp(event, "Test", 4) == 0 ||
>   			       strncmp(event, "Spares", 6) == 0 ||
>   			       strncmp(event, "Degrade", 7) == 0)) {
> -		send_event_email(event, dev, disc, info);
> +		send_event_email(event, dev, disc);
>   	}
>   
> -	if (info->dosyslog)
> +	if (info.dosyslog)
>   		log_event_to_syslog(event, dev, disc);
>   }
>   
>   static int check_array(struct state *st, struct mdstat_ent *mdstat,
> -		       int test, struct alert_info *ainfo,
>   		       int increments, char *prefer)
>   {
>   	/* Update the state 'st' to reflect any changes shown in mdstat,
>   	 * or found by directly examining the array, and return
>   	 * '1' if the array is degraded, or '0' if it is optimal (or dead).
>   	 */
> -	struct { int state, major, minor; } info[MAX_DISKS];
> +	struct { int state, major, minor; } disks_info[MAX_DISKS];
>   	struct mdinfo *sra = NULL;
>   	mdu_array_info_t array;
>   	struct mdstat_ent *mse = NULL, *mse2;
> @@ -535,8 +535,8 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   	int is_container = 0;
>   	unsigned long redundancy_only_flags = 0;
>   
> -	if (test)
> -		alert("TestMessage", dev, NULL, ainfo);
> +	if (info.test)
> +		alert("TestMessage", dev, NULL);
>   
>   	retval = 0;
>   
> @@ -585,7 +585,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   	 */
>   	if (sra->array.level == 0 || sra->array.level == -1) {
>   		if (!st->err && !st->from_config)
> -			alert("DeviceDisappeared", dev, " Wrong-Level", ainfo);
> +			alert("DeviceDisappeared", dev, " Wrong-Level");
>   		st->err++;
>   		goto out;
>   	}
> @@ -602,7 +602,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   		st->percent = RESYNC_NONE;
>   		new_array = 1;
>   		if (!is_container)
> -			alert("NewArray", st->devname, NULL, ainfo);
> +			alert("NewArray", st->devname, NULL);
>   	}
>   
>   	if (st->utime == array.utime && st->failed == sra->array.failed_disks &&
> @@ -615,14 +615,14 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   	}
>   	if (st->utime == 0 && /* new array */
>   	    mse->pattern && strchr(mse->pattern, '_') /* degraded */)
> -		alert("DegradedArray", dev, NULL, ainfo);
> +		alert("DegradedArray", dev, NULL);
>   
>   	if (st->utime == 0 && /* new array */ st->expected_spares > 0 &&
>   	    sra->array.spare_disks < st->expected_spares)
> -		alert("SparesMissing", dev, NULL, ainfo);
> +		alert("SparesMissing", dev, NULL);
>   	if (st->percent < 0 && st->percent != RESYNC_UNKNOWN &&
>   	    mse->percent >= 0)
> -		alert("RebuildStarted", dev, NULL, ainfo);
> +		alert("RebuildStarted", dev, NULL);
>   	if (st->percent >= 0 && mse->percent >= 0 &&
>   	    (mse->percent / increments) > (st->percent / increments)) {
>   		char percentalert[18];
> @@ -637,7 +637,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   			snprintf(percentalert, sizeof(percentalert),
>   				 "Rebuild%02d", mse->percent);
>   
> -		alert(percentalert, dev, NULL, ainfo);
> +		alert(percentalert, dev, NULL);
>   	}
>   
>   	if (mse->percent == RESYNC_NONE && st->percent >= 0) {
> @@ -650,9 +650,9 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   			snprintf(cnt, sizeof(cnt),
>   				 " mismatches found: %d (on raid level %d)",
>   				 sra->mismatch_cnt, sra->array.level);
> -			alert("RebuildFinished", dev, cnt, ainfo);
> +			alert("RebuildFinished", dev, cnt);
>   		} else
> -			alert("RebuildFinished", dev, NULL, ainfo);
> +			alert("RebuildFinished", dev, NULL);
>   	}
>   	st->percent = mse->percent;
>   
> @@ -661,13 +661,13 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   		mdu_disk_info_t disc;
>   		disc.number = i;
>   		if (md_get_disk_info(fd, &disc) >= 0) {
> -			info[i].state = disc.state;
> -			info[i].major = disc.major;
> -			info[i].minor = disc.minor;
> +			disks_info[i].state = disc.state;
> +			disks_info[i].major = disc.major;
> +			disks_info[i].minor = disc.minor;
>   			if (disc.major || disc.minor)
>   				remaining_disks --;
>   		} else
> -			info[i].major = info[i].minor = 0;
> +			disks_info[i].major = disks_info[i].minor = 0;
>   	}
>   	last_disk = i;
>   
> @@ -690,13 +690,13 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   		int change;
>   		char *dv = NULL;
>   		disc.number = i;
> -		if (i < last_disk && (info[i].major || info[i].minor)) {
> -			newstate = info[i].state;
> -			dv = map_dev_preferred(info[i].major, info[i].minor, 1,
> +		if (i < last_disk && (disks_info[i].major || disks_info[i].minor)) {
> +			newstate = disks_info[i].state;
> +			dv = map_dev_preferred(disks_info[i].major, disks_info[i].minor, 1,
>   					       prefer);
>   			disc.state = newstate;
> -			disc.major = info[i].major;
> -			disc.minor = info[i].minor;
> +			disc.major = disks_info[i].major;
> +			disc.minor = disks_info[i].minor;
>   		} else
>   			newstate = (1 << MD_DISK_REMOVED);
>   
> @@ -706,14 +706,14 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   		change = newstate ^ st->devstate[i];
>   		if (st->utime && change && !st->err && !new_array) {
>   			if ((st->devstate[i]&change) & (1 << MD_DISK_SYNC))
> -				alert("Fail", dev, dv, ainfo);
> +				alert("Fail", dev, dv);
>   			else if ((newstate & (1 << MD_DISK_FAULTY)) &&
>   				 (disc.major || disc.minor) &&
>   				 st->devid[i] == makedev(disc.major,
>   							 disc.minor))
> -				alert("FailSpare", dev, dv, ainfo);
> +				alert("FailSpare", dev, dv);
>   			else if ((newstate&change) & (1 << MD_DISK_SYNC))
> -				alert("SpareActive", dev, dv, ainfo);
> +				alert("SpareActive", dev, dv);
>   		}
>   		st->devstate[i] = newstate;
>   		st->devid[i] = makedev(disc.major, disc.minor);
> @@ -737,13 +737,12 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   
>    disappeared:
>   	if (!st->err && !is_container)
> -		alert("DeviceDisappeared", dev, NULL, ainfo);
> +		alert("DeviceDisappeared", dev, NULL);
>   	st->err++;
>   	goto out;
>   }
>   
> -static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist,
> -			  int test, struct alert_info *info)
> +static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist)
>   {
>   	struct mdstat_ent *mse;
>   	int new_found = 0;
> @@ -797,8 +796,8 @@ static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist,
>   			} else
>   				st->parent_devnm[0] = 0;
>   			*statelist = st;
> -			if (test)
> -				alert("TestMessage", st->devname, NULL, info);
> +			if (info.test)
> +				alert("TestMessage", st->devname, NULL);
>   			new_found = 1;
>   		}
>   	return new_found;
> @@ -962,7 +961,7 @@ static dev_t container_choose_spare(struct state *from, struct state *to,
>   	return dev;
>   }
>   
> -static void try_spare_migration(struct state *statelist, struct alert_info *info)
> +static void try_spare_migration(struct state *statelist)
>   {
>   	struct state *from;
>   	struct state *st;
> @@ -1021,8 +1020,7 @@ static void try_spare_migration(struct state *statelist, struct alert_info *info
>   				if (devid > 0 &&
>   				    move_spare(from->devname, to->devname,
>   					       devid)) {
> -					alert("MoveSpare", to->devname,
> -					      from->devname, info);
> +					alert("MoveSpare", to->devname, from->devname);
>   					break;
>   				}
>   			}

Re: [PATCH 2/9] Mdmonitor: Make alert_info global

[Jes Sorensen]

On 9/7/22 08:56, Mateusz Grzonka wrote:
> Move information about --test flag and hostname into alert_info.
> 
> Signed-off-by: Mateusz Grzonka <mateusz.grzonka@intel.com>

Hi Mateusz,

This one no longer applies, any chance you can do an update?

Thanks,
Jes

Re: [PATCH 2/9] Mdmonitor: Make alert_info global

[Grzonka, Mateusz]

On 12/28/2022 3:49 PM, Jes Sorensen wrote:
> On 9/7/22 08:56, Mateusz Grzonka wrote:
>> Move information about --test flag and hostname into alert_info.
>>
>> Signed-off-by: Mateusz Grzonka <mateusz.grzonka@intel.com>
> 
> Hi Mateusz,
> 
> This one no longer applies, any chance you can do an update?
> 
> Thanks,
> Jes
> 

Hi Jes,
I fixed some problems with comments, and rebased the series.

Unfortunately got some problems while sending them.
All patches marked as v2 are the correct ones.
Sorry for inconvenience.

Thanks,
Mateusz

[PATCH 3/9] Mdmonitor: Pass events to alert() using enums instead of strings

[Mateusz Grzonka]

Add events enum, and mapping_t struct, that maps them to strings, so
that enums are passed around instead of strings.

Signed-off-by: Mateusz Grzonka <mateusz.grzonka@intel.com>
---
 Monitor.c | 136 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 83 insertions(+), 53 deletions(-)

diff --git a/Monitor.c b/Monitor.c
index 73c5790d..aa0ee047 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -32,6 +32,8 @@
 #include	<libudev.h>
 #endif
 
+#define EVENT_NAME_MAX 32
+
 struct state {
 	char *devname;
 	char devnm[32];	/* to sync with mdstat info */
@@ -65,10 +67,43 @@ struct alert_info {
 	int dosyslog;
 	int test;
 } info;
+
+enum event {
+	EVENT_SPARE_ACTIVE = 0,
+	EVENT_NEW_ARRAY,
+	EVENT_MOVE_SPARE,
+	EVENT_TEST_MESSAGE,
+	EVENT_REBUILD_STARTED,
+	EVENT_REBUILD,
+	EVENT_REBUILD_FINISHED,
+	EVENT_SPARES_MISSING,
+	EVENT_DEVICE_DISAPPEARED,
+	EVENT_FAIL,
+	EVENT_FAIL_SPARE,
+	EVENT_DEGRADED_ARRAY,
+	EVENT_UNKNOWN
+};
+
+mapping_t events_map[] = {
+	{"SpareActive", EVENT_SPARE_ACTIVE},
+	{"NewArray", EVENT_NEW_ARRAY},
+	{"MoveSpare", EVENT_MOVE_SPARE},
+	{"TestMessage", EVENT_TEST_MESSAGE},
+	{"RebuildStarted", EVENT_REBUILD_STARTED},
+	{"Rebuild", EVENT_REBUILD},
+	{"RebuildFinished", EVENT_REBUILD_FINISHED},
+	{"SparesMissing", EVENT_SPARES_MISSING},
+	{"DeviceDisappeared", EVENT_DEVICE_DISAPPEARED},
+	{"Fail", EVENT_FAIL},
+	{"FailSpare", EVENT_FAIL_SPARE},
+	{"DegradedArray", EVENT_DEGRADED_ARRAY},
+	{NULL, EVENT_UNKNOWN}
+};
+
 static int make_daemon(char *pidfile);
 static int check_one_sharer(int scan);
 static void write_autorebuild_pid(void);
-static void alert(const char *event, const char *dev, const char *disc);
+static void alert(const enum event event_enum, const unsigned int progress, const char *dev, const char *disc);
 static int check_array(struct state *st, struct mdstat_ent *mdstat, int increments, char *prefer);
 static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist);
 static void try_spare_migration(struct state *statelist);
@@ -405,7 +440,7 @@ static void write_autorebuild_pid()
 	}
 }
 
-static void execute_alert_cmd(const char *event, const char *dev, const char *disc)
+static void execute_alert_cmd(const char *event_name, const char *dev, const char *disc)
 {
 	int pid = fork();
 
@@ -417,12 +452,12 @@ static void execute_alert_cmd(const char *event, const char *dev, const char *di
 		pr_err("Cannot fork to execute alert command");
 		break;
 	case 0:
-		execl(info.alert_cmd, info.alert_cmd, event, dev, disc, NULL);
+		execl(info.alert_cmd, info.alert_cmd, event_name, dev, disc, NULL);
 		exit(2);
 	}
 }
 
-static void send_event_email(const char *event, const char *dev, const char *disc)
+static void send_event_email(const char *event_name, const char *dev, const char *disc)
 {
 	FILE *mp, *mdstat;
 	char buf[BUFSIZ];
@@ -440,9 +475,9 @@ static void send_event_email(const char *event, const char *dev, const char *dis
 	else
 		fprintf(mp, "From: %s monitoring <root>\n", Name);
 	fprintf(mp, "To: %s\n", info.mailaddr);
-	fprintf(mp, "Subject: %s event on %s:%s\n\n", event, dev, info.hostname);
+	fprintf(mp, "Subject: %s event on %s:%s\n\n", event_name, dev, info.hostname);
 	fprintf(mp, "This is an automatically generated mail message. \n");
-	fprintf(mp, "A %s event had been detected on md device %s.\n\n", event, dev);
+	fprintf(mp, "A %s event had been detected on md device %s.\n\n", event_name, dev);
 
 	if (disc && disc[0] != ' ')
 		fprintf(mp,
@@ -464,20 +499,20 @@ static void send_event_email(const char *event, const char *dev, const char *dis
 	pclose(mp);
 }
 
-static void log_event_to_syslog(const char *event, const char *dev, const char *disc)
+static void log_event_to_syslog(const enum event event_enum, const char *event_name, const char *dev, const char *disc)
 {
 	int priority;
 	/* Log at a different severity depending on the event.
 	 *
 	 * These are the critical events:  */
-	if (strncmp(event, "Fail", 4) == 0 ||
-		strncmp(event, "Degrade", 7) == 0 ||
-		strncmp(event, "DeviceDisappeared", 17) == 0)
+	if (event_enum == EVENT_FAIL ||
+	    event_enum == EVENT_DEGRADED_ARRAY ||
+	    event_enum == EVENT_DEVICE_DISAPPEARED)
 		priority = LOG_CRIT;
 	/* Good to know about, but are not failures: */
-	else if (strncmp(event, "Rebuild", 7) == 0 ||
-			strncmp(event, "MoveSpare", 9) == 0 ||
-			strncmp(event, "Spares", 6) != 0)
+	else if (event_enum == EVENT_REBUILD ||
+		 event_enum == EVENT_MOVE_SPARE ||
+		 event_enum == EVENT_SPARES_MISSING)
 		priority = LOG_WARNING;
 	/* Everything else: */
 	else
@@ -485,33 +520,37 @@ static void log_event_to_syslog(const char *event, const char *dev, const char *
 
 	if (disc && disc[0] != ' ')
 		syslog(priority,
-			"%s event detected on md device %s, component device %s", event, dev, disc);
+		       "%s event detected on md device %s, component device %s",
+		       event_name, dev, disc);
 	else if (disc)
-		syslog(priority, "%s event detected on md device %s: %s", event, dev, disc);
+		syslog(priority, "%s event detected on md device %s: %s", event_name, dev, disc);
 	else
-		syslog(priority, "%s event detected on md device %s", event, dev);
+		syslog(priority, "%s event detected on md device %s", event_name, dev);
 }
 
-static void alert(const char *event, const char *dev, const char *disc)
+static void alert(const enum event event_enum, const unsigned int progress, const char *dev, const char *disc)
 {
-	if (!info.alert_cmd && !info.mailaddr && !info.dosyslog) {
-		time_t now = time(0);
+	char event_name[EVENT_NAME_MAX];
 
-		printf("%1.15s: %s on %s %s\n", ctime(&now) + 4,
-		       event, dev, disc?disc:"unknown device");
+	if (event_enum == EVENT_REBUILD) {
+		snprintf(event_name, sizeof(event_name), "%s%02d",
+			 map_num_s(events_map, EVENT_REBUILD), progress);
+	} else {
+		snprintf(event_name, sizeof(event_name), "%s", map_num_s(events_map, event_enum));
 	}
+
 	if (info.alert_cmd)
-		execute_alert_cmd(event, dev, disc);
+		execute_alert_cmd(event_name, dev, disc);
 
-	if (info.mailaddr && (strncmp(event, "Fail", 4) == 0 ||
-			       strncmp(event, "Test", 4) == 0 ||
-			       strncmp(event, "Spares", 6) == 0 ||
-			       strncmp(event, "Degrade", 7) == 0)) {
-		send_event_email(event, dev, disc);
+	if (info.mailaddr && (event_enum == EVENT_FAIL ||
+			      event_enum == EVENT_TEST_MESSAGE ||
+			      event_enum == EVENT_SPARES_MISSING ||
+			      event_enum == EVENT_DEGRADED_ARRAY)) {
+		send_event_email(event_name, dev, disc);
 	}
 
 	if (info.dosyslog)
-		log_event_to_syslog(event, dev, disc);
+		log_event_to_syslog(event_enum, event_name, dev, disc);
 }
 
 static int check_array(struct state *st, struct mdstat_ent *mdstat,
@@ -536,7 +575,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 	unsigned long redundancy_only_flags = 0;
 
 	if (info.test)
-		alert("TestMessage", dev, NULL);
+		alert(EVENT_TEST_MESSAGE, 0, dev, NULL);
 
 	retval = 0;
 
@@ -585,7 +624,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 	 */
 	if (sra->array.level == 0 || sra->array.level == -1) {
 		if (!st->err && !st->from_config)
-			alert("DeviceDisappeared", dev, " Wrong-Level");
+			alert(EVENT_DEVICE_DISAPPEARED, 0, dev, " Wrong-Level");
 		st->err++;
 		goto out;
 	}
@@ -602,7 +641,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 		st->percent = RESYNC_NONE;
 		new_array = 1;
 		if (!is_container)
-			alert("NewArray", st->devname, NULL);
+			alert(EVENT_NEW_ARRAY, 0, st->devname, NULL);
 	}
 
 	if (st->utime == array.utime && st->failed == sra->array.failed_disks &&
@@ -615,29 +654,20 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 	}
 	if (st->utime == 0 && /* new array */
 	    mse->pattern && strchr(mse->pattern, '_') /* degraded */)
-		alert("DegradedArray", dev, NULL);
+		alert(EVENT_DEGRADED_ARRAY, 0, dev, NULL);
 
 	if (st->utime == 0 && /* new array */ st->expected_spares > 0 &&
 	    sra->array.spare_disks < st->expected_spares)
-		alert("SparesMissing", dev, NULL);
+		alert(EVENT_SPARES_MISSING, 0, dev, NULL);
 	if (st->percent < 0 && st->percent != RESYNC_UNKNOWN &&
 	    mse->percent >= 0)
-		alert("RebuildStarted", dev, NULL);
+		alert(EVENT_REBUILD_STARTED, 0, dev, NULL);
 	if (st->percent >= 0 && mse->percent >= 0 &&
 	    (mse->percent / increments) > (st->percent / increments)) {
-		char percentalert[18];
-		/*
-		 * "RebuildNN" (10 chars) or "RebuildStarted" (15 chars)
-		 */
-
 		if((mse->percent / increments) == 0)
-			snprintf(percentalert, sizeof(percentalert),
-				 "RebuildStarted");
+			alert(EVENT_REBUILD_STARTED, 0, dev, NULL);
 		else
-			snprintf(percentalert, sizeof(percentalert),
-				 "Rebuild%02d", mse->percent);
-
-		alert(percentalert, dev, NULL);
+			alert(EVENT_REBUILD, mse->percent, dev, NULL);
 	}
 
 	if (mse->percent == RESYNC_NONE && st->percent >= 0) {
@@ -650,9 +680,9 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 			snprintf(cnt, sizeof(cnt),
 				 " mismatches found: %d (on raid level %d)",
 				 sra->mismatch_cnt, sra->array.level);
-			alert("RebuildFinished", dev, cnt);
+			alert(EVENT_REBUILD_FINISHED, 0, dev, cnt);
 		} else
-			alert("RebuildFinished", dev, NULL);
+			alert(EVENT_REBUILD_FINISHED, 0, dev, NULL);
 	}
 	st->percent = mse->percent;
 
@@ -706,14 +736,14 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 		change = newstate ^ st->devstate[i];
 		if (st->utime && change && !st->err && !new_array) {
 			if ((st->devstate[i]&change) & (1 << MD_DISK_SYNC))
-				alert("Fail", dev, dv);
+				alert(EVENT_FAIL, 0, dev, dv);
 			else if ((newstate & (1 << MD_DISK_FAULTY)) &&
 				 (disc.major || disc.minor) &&
 				 st->devid[i] == makedev(disc.major,
 							 disc.minor))
-				alert("FailSpare", dev, dv);
+				alert(EVENT_FAIL_SPARE, 0, dev, dv);
 			else if ((newstate&change) & (1 << MD_DISK_SYNC))
-				alert("SpareActive", dev, dv);
+				alert(EVENT_SPARE_ACTIVE, 0, dev, dv);
 		}
 		st->devstate[i] = newstate;
 		st->devid[i] = makedev(disc.major, disc.minor);
@@ -737,7 +767,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 
  disappeared:
 	if (!st->err && !is_container)
-		alert("DeviceDisappeared", dev, NULL);
+		alert(EVENT_DEVICE_DISAPPEARED, 0, dev, NULL);
 	st->err++;
 	goto out;
 }
@@ -797,7 +827,7 @@ static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist)
 				st->parent_devnm[0] = 0;
 			*statelist = st;
 			if (info.test)
-				alert("TestMessage", st->devname, NULL);
+				alert(EVENT_TEST_MESSAGE, 0, st->devname, NULL);
 			new_found = 1;
 		}
 	return new_found;
@@ -1020,7 +1050,7 @@ static void try_spare_migration(struct state *statelist)
 				if (devid > 0 &&
 				    move_spare(from->devname, to->devname,
 					       devid)) {
-					alert("MoveSpare", to->devname, from->devname);
+					alert(EVENT_MOVE_SPARE, 0, to->devname, from->devname);
 					break;
 				}
 			}
-- 
2.26.2

Re: [PATCH 3/9] Mdmonitor: Pass events to alert() using enums instead of strings

[Coly Li]

On 9/7/22 8:56 PM, Mateusz Grzonka wrote:
> Add events enum, and mapping_t struct, that maps them to strings, so
> that enums are passed around instead of strings.
>
> Signed-off-by: Mateusz Grzonka<mateusz.grzonka@intel.com>
> ---
>   Monitor.c | 136 +++++++++++++++++++++++++++++++++---------------------
>   1 file changed, 83 insertions(+), 53 deletions(-)

[snipped]

>   
> -static void alert(const char *event, const char *dev, const char *disc)
> +static void alert(const enum event event_enum, const unsigned int progress, const char *dev, const char *disc)
>   {
> -	if (!info.alert_cmd && !info.mailaddr && !info.dosyslog) {
> -		time_t now = time(0);
> +	char event_name[EVENT_NAME_MAX];
>   
> -		printf("%1.15s: %s on %s %s\n", ctime(&now) + 4,
> -		       event, dev, disc?disc:"unknown device");
> +	if (event_enum == EVENT_REBUILD) {
> +		snprintf(event_name, sizeof(event_name), "%s%02d",
> +			 map_num_s(events_map, EVENT_REBUILD), progress);
> +	} else {
> +		snprintf(event_name, sizeof(event_name), "%s", map_num_s(events_map, event_enum));
>   	}
> +

I am not sure whether it is fine to change the output message. I don’t 
object it, just not sure it is right or not right.

All rested part looks good to me.


Acked-by: Coly Li <colyli@suse.de>


Thanks.

Coly Li


>   	if (info.alert_cmd)
> -		execute_alert_cmd(event, dev, disc);
> +		execute_alert_cmd(event_name, dev, disc);
>   
> -	if (info.mailaddr && (strncmp(event, "Fail", 4) == 0 ||
> -			       strncmp(event, "Test", 4) == 0 ||
> -			       strncmp(event, "Spares", 6) == 0 ||
> -			       strncmp(event, "Degrade", 7) == 0)) {
> -		send_event_email(event, dev, disc);
> +	if (info.mailaddr && (event_enum == EVENT_FAIL ||
> +			      event_enum == EVENT_TEST_MESSAGE ||
> +			      event_enum == EVENT_SPARES_MISSING ||
> +			      event_enum == EVENT_DEGRADED_ARRAY)) {
> +		send_event_email(event_name, dev, disc);
>   	}
>   
>   	if (info.dosyslog)
> -		log_event_to_syslog(event, dev, disc);
> +		log_event_to_syslog(event_enum, event_name, dev, disc);
>   }
>   
>   static int check_array(struct state *st, struct mdstat_ent *mdstat,
> @@ -536,7 +575,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   	unsigned long redundancy_only_flags = 0;
>   
>   	if (info.test)
> -		alert("TestMessage", dev, NULL);
> +		alert(EVENT_TEST_MESSAGE, 0, dev, NULL);
>   
>   	retval = 0;
>   
> @@ -585,7 +624,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   	 */
>   	if (sra->array.level == 0 || sra->array.level == -1) {
>   		if (!st->err && !st->from_config)
> -			alert("DeviceDisappeared", dev, " Wrong-Level");
> +			alert(EVENT_DEVICE_DISAPPEARED, 0, dev, " Wrong-Level");
>   		st->err++;
>   		goto out;
>   	}
> @@ -602,7 +641,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   		st->percent = RESYNC_NONE;
>   		new_array = 1;
>   		if (!is_container)
> -			alert("NewArray", st->devname, NULL);
> +			alert(EVENT_NEW_ARRAY, 0, st->devname, NULL);
>   	}
>   
>   	if (st->utime == array.utime && st->failed == sra->array.failed_disks &&
> @@ -615,29 +654,20 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   	}
>   	if (st->utime == 0 && /* new array */
>   	    mse->pattern && strchr(mse->pattern, '_') /* degraded */)
> -		alert("DegradedArray", dev, NULL);
> +		alert(EVENT_DEGRADED_ARRAY, 0, dev, NULL);
>   
>   	if (st->utime == 0 && /* new array */ st->expected_spares > 0 &&
>   	    sra->array.spare_disks < st->expected_spares)
> -		alert("SparesMissing", dev, NULL);
> +		alert(EVENT_SPARES_MISSING, 0, dev, NULL);
>   	if (st->percent < 0 && st->percent != RESYNC_UNKNOWN &&
>   	    mse->percent >= 0)
> -		alert("RebuildStarted", dev, NULL);
> +		alert(EVENT_REBUILD_STARTED, 0, dev, NULL);
>   	if (st->percent >= 0 && mse->percent >= 0 &&
>   	    (mse->percent / increments) > (st->percent / increments)) {
> -		char percentalert[18];
> -		/*
> -		 * "RebuildNN" (10 chars) or "RebuildStarted" (15 chars)
> -		 */
> -
>   		if((mse->percent / increments) == 0)
> -			snprintf(percentalert, sizeof(percentalert),
> -				 "RebuildStarted");
> +			alert(EVENT_REBUILD_STARTED, 0, dev, NULL);
>   		else
> -			snprintf(percentalert, sizeof(percentalert),
> -				 "Rebuild%02d", mse->percent);
> -
> -		alert(percentalert, dev, NULL);
> +			alert(EVENT_REBUILD, mse->percent, dev, NULL);
>   	}
>   
>   	if (mse->percent == RESYNC_NONE && st->percent >= 0) {
> @@ -650,9 +680,9 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   			snprintf(cnt, sizeof(cnt),
>   				 " mismatches found: %d (on raid level %d)",
>   				 sra->mismatch_cnt, sra->array.level);
> -			alert("RebuildFinished", dev, cnt);
> +			alert(EVENT_REBUILD_FINISHED, 0, dev, cnt);
>   		} else
> -			alert("RebuildFinished", dev, NULL);
> +			alert(EVENT_REBUILD_FINISHED, 0, dev, NULL);
>   	}
>   	st->percent = mse->percent;
>   
> @@ -706,14 +736,14 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   		change = newstate ^ st->devstate[i];
>   		if (st->utime && change && !st->err && !new_array) {
>   			if ((st->devstate[i]&change) & (1 << MD_DISK_SYNC))
> -				alert("Fail", dev, dv);
> +				alert(EVENT_FAIL, 0, dev, dv);
>   			else if ((newstate & (1 << MD_DISK_FAULTY)) &&
>   				 (disc.major || disc.minor) &&
>   				 st->devid[i] == makedev(disc.major,
>   							 disc.minor))
> -				alert("FailSpare", dev, dv);
> +				alert(EVENT_FAIL_SPARE, 0, dev, dv);
>   			else if ((newstate&change) & (1 << MD_DISK_SYNC))
> -				alert("SpareActive", dev, dv);
> +				alert(EVENT_SPARE_ACTIVE, 0, dev, dv);
>   		}
>   		st->devstate[i] = newstate;
>   		st->devid[i] = makedev(disc.major, disc.minor);
> @@ -737,7 +767,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   
>    disappeared:
>   	if (!st->err && !is_container)
> -		alert("DeviceDisappeared", dev, NULL);
> +		alert(EVENT_DEVICE_DISAPPEARED, 0, dev, NULL);
>   	st->err++;
>   	goto out;
>   }
> @@ -797,7 +827,7 @@ static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist)
>   				st->parent_devnm[0] = 0;
>   			*statelist = st;
>   			if (info.test)
> -				alert("TestMessage", st->devname, NULL);
> +				alert(EVENT_TEST_MESSAGE, 0, st->devname, NULL);
>   			new_found = 1;
>   		}
>   	return new_found;
> @@ -1020,7 +1050,7 @@ static void try_spare_migration(struct state *statelist)
>   				if (devid > 0 &&
>   				    move_spare(from->devname, to->devname,
>   					       devid)) {
> -					alert("MoveSpare", to->devname, from->devname);
> +					alert(EVENT_MOVE_SPARE, 0, to->devname, from->devname);
>   					break;
>   				}
>   			}

[PATCH 4/9] Mdmonitor: Add helper functions

[Mateusz Grzonka]

Add functions:
- is_email_event(),
- get_syslog_event_priority(),
- sprint_event_message(),
with kernel style comments containing more detailed descriptions.

Also update event syslog priorities to be consistent with man. MoveSpare event was described in man as priority info, while implemented as warning. Move event data into a struct, so that it is passed between different functions if needed.
Sort function declarations alphabetically and remove redundant alert() declaration.

Signed-off-by: Mateusz Grzonka <mateusz.grzonka@intel.com>
---
 Monitor.c | 230 +++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 158 insertions(+), 72 deletions(-)

diff --git a/Monitor.c b/Monitor.c
index aa0ee047..80e43ebe 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -73,10 +73,12 @@ enum event {
 	EVENT_NEW_ARRAY,
 	EVENT_MOVE_SPARE,
 	EVENT_TEST_MESSAGE,
+	__SYSLOG_PRIORITY_WARNING,
 	EVENT_REBUILD_STARTED,
 	EVENT_REBUILD,
 	EVENT_REBUILD_FINISHED,
 	EVENT_SPARES_MISSING,
+	__SYSLOG_PRIORITY_CRITICAL,
 	EVENT_DEVICE_DISAPPEARED,
 	EVENT_FAIL,
 	EVENT_FAIL_SPARE,
@@ -100,17 +102,28 @@ mapping_t events_map[] = {
 	{NULL, EVENT_UNKNOWN}
 };
 
-static int make_daemon(char *pidfile);
-static int check_one_sharer(int scan);
-static void write_autorebuild_pid(void);
-static void alert(const enum event event_enum, const unsigned int progress, const char *dev, const char *disc);
-static int check_array(struct state *st, struct mdstat_ent *mdstat, int increments, char *prefer);
+struct event_data {
+	enum event event_enum;
+	/**
+	 * @event_name: Rebuild event name must be in form "RebuildXX", where XX is rebuild progress.
+	 */
+	char event_name[EVENT_NAME_MAX];
+	char message[BUFSIZ];
+	const char *description;
+	const char *dev;
+	const char *disc;
+};
+
 static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist);
-static void try_spare_migration(struct state *statelist);
-static void link_containers_with_subarrays(struct state *list);
+static int check_array(struct state *st, struct mdstat_ent *mdstat, int increments, char *prefer);
+static int check_one_sharer(int scan);
 #ifndef NO_LIBUDEV
 static int check_udev_activity(void);
 #endif
+static void link_containers_with_subarrays(struct state *list);
+static int make_daemon(char *pidfile);
+static void try_spare_migration(struct state *statelist);
+static void write_autorebuild_pid(void);
 
 int Monitor(struct mddev_dev *devlist,
 	    char *mailaddr, char *alert_cmd,
@@ -440,7 +453,80 @@ static void write_autorebuild_pid()
 	}
 }
 
-static void execute_alert_cmd(const char *event_name, const char *dev, const char *disc)
+#define BASE_MESSAGE "%s event detected on md device %s"
+#define COMPONENT_DEVICE_MESSAGE ", component device %s"
+#define DESCRIPTION_MESSAGE ": %s"
+/**
+ * sprint_event_message() - Writes basic message about detected event to destination ptr.
+ * @dest: message destination, should be at least the size of BUFSIZ
+ * @data: event data
+ *
+ * Return: 0 on success, 1 on error
+ */
+static int sprint_event_message(char *dest, const struct event_data *data)
+{
+	if (!dest || !data)
+		return 1;
+
+	if (data->disc && data->description)
+		snprintf(dest, BUFSIZ, BASE_MESSAGE COMPONENT_DEVICE_MESSAGE DESCRIPTION_MESSAGE,
+			 data->event_name, data->dev, data->disc, data->description);
+	else if (data->disc)
+		snprintf(dest, BUFSIZ, BASE_MESSAGE COMPONENT_DEVICE_MESSAGE,
+			 data->event_name, data->dev, data->disc);
+	else if (data->description)
+		snprintf(dest, BUFSIZ, BASE_MESSAGE DESCRIPTION_MESSAGE,
+			 data->event_name, data->dev, data->description);
+	else
+		snprintf(dest, BUFSIZ, BASE_MESSAGE, data->event_name, data->dev);
+
+	return 0;
+}
+
+/**
+ * get_syslog_event_priority() - Determines event priority.
+ * @event_enum: event to be checked
+ *
+ * Return: LOG_CRIT, LOG_WARNING or LOG_INFO
+ */
+static int get_syslog_event_priority(const enum event event_enum)
+{
+	if (event_enum > __SYSLOG_PRIORITY_CRITICAL)
+		return LOG_CRIT;
+	if (event_enum > __SYSLOG_PRIORITY_WARNING)
+		return LOG_WARNING;
+	return LOG_INFO;
+}
+
+/**
+ * is_email_event() - Determines whether email for event should be sent or not.
+ * @event_enum: event to be checked
+ *
+ * Return: true if email should be sent, false otherwise
+ */
+static bool is_email_event(const enum event event_enum)
+{
+	static const enum event email_events[] = {
+	EVENT_FAIL,
+	EVENT_FAIL_SPARE,
+	EVENT_DEGRADED_ARRAY,
+	EVENT_SPARES_MISSING,
+	EVENT_TEST_MESSAGE
+	};
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(email_events); ++i) {
+		if (event_enum == email_events[i])
+			return true;
+	}
+	return false;
+}
+
+/**
+ * execute_alert_cmd() - Forks and executes command provided as alert_cmd.
+ * @data: event data
+ */
+static void execute_alert_cmd(const struct event_data *data)
 {
 	int pid = fork();
 
@@ -452,12 +538,16 @@ static void execute_alert_cmd(const char *event_name, const char *dev, const cha
 		pr_err("Cannot fork to execute alert command");
 		break;
 	case 0:
-		execl(info.alert_cmd, info.alert_cmd, event_name, dev, disc, NULL);
+		execl(info.alert_cmd, info.alert_cmd, data->event_name, data->dev, data->disc, NULL);
 		exit(2);
 	}
 }
 
-static void send_event_email(const char *event_name, const char *dev, const char *disc)
+/**
+ * send_event_email() - Sends an email about event detected by monitor.
+ * @data: event data
+ */
+static void send_event_email(const struct event_data *data)
 {
 	FILE *mp, *mdstat;
 	char buf[BUFSIZ];
@@ -475,15 +565,9 @@ static void send_event_email(const char *event_name, const char *dev, const char
 	else
 		fprintf(mp, "From: %s monitoring <root>\n", Name);
 	fprintf(mp, "To: %s\n", info.mailaddr);
-	fprintf(mp, "Subject: %s event on %s:%s\n\n", event_name, dev, info.hostname);
-	fprintf(mp, "This is an automatically generated mail message. \n");
-	fprintf(mp, "A %s event had been detected on md device %s.\n\n", event_name, dev);
-
-	if (disc && disc[0] != ' ')
-		fprintf(mp,
-			"It could be related to component device %s.\n\n", disc);
-	if (disc && disc[0] == ' ')
-		fprintf(mp, "Extra information:%s.\n\n", disc);
+	fprintf(mp, "Subject: %s event on %s:%s\n\n", data->event_name, data->dev, info.hostname);
+	fprintf(mp, "This is an automatically generated mail message.\n");
+	fprintf(mp, "%s\n", data->message);
 
 	mdstat = fopen("/proc/mdstat", "r");
 	if (!mdstat) {
@@ -499,58 +583,60 @@ static void send_event_email(const char *event_name, const char *dev, const char
 	pclose(mp);
 }
 
-static void log_event_to_syslog(const enum event event_enum, const char *event_name, const char *dev, const char *disc)
+/**
+ * log_event_to_syslog() - Logs an event into syslog.
+ * @data: event data
+ */
+static void log_event_to_syslog(const struct event_data *data)
 {
 	int priority;
-	/* Log at a different severity depending on the event.
-	 *
-	 * These are the critical events:  */
-	if (event_enum == EVENT_FAIL ||
-	    event_enum == EVENT_DEGRADED_ARRAY ||
-	    event_enum == EVENT_DEVICE_DISAPPEARED)
-		priority = LOG_CRIT;
-	/* Good to know about, but are not failures: */
-	else if (event_enum == EVENT_REBUILD ||
-		 event_enum == EVENT_MOVE_SPARE ||
-		 event_enum == EVENT_SPARES_MISSING)
-		priority = LOG_WARNING;
-	/* Everything else: */
-	else
-		priority = LOG_INFO;
-
-	if (disc && disc[0] != ' ')
-		syslog(priority,
-		       "%s event detected on md device %s, component device %s",
-		       event_name, dev, disc);
-	else if (disc)
-		syslog(priority, "%s event detected on md device %s: %s", event_name, dev, disc);
-	else
-		syslog(priority, "%s event detected on md device %s", event_name, dev);
+
+	priority = get_syslog_event_priority(data->event_enum);
+
+	syslog(priority, "%s\n", data->message);
 }
 
-static void alert(const enum event event_enum, const unsigned int progress, const char *dev, const char *disc)
+/**
+ * alert() - Alerts about the monitor event.
+ * @event_enum: event to be sent
+ * @description: event description
+ * @progress: rebuild progress
+ * @dev: md device name
+ * @disc: component device
+ *
+ * If needed function executes alert command, sends an email or logs event to syslog.
+ */
+static void alert(const enum event event_enum, const char *description, const uint8_t progress,
+		  const char *dev, const char *disc)
 {
-	char event_name[EVENT_NAME_MAX];
+	struct event_data data = {.dev = dev, .disc = disc, .description = description};
+
+	if (!dev)
+		return;
 
 	if (event_enum == EVENT_REBUILD) {
-		snprintf(event_name, sizeof(event_name), "%s%02d",
+		snprintf(data.event_name, sizeof(data.event_name), "%s%02d",
 			 map_num_s(events_map, EVENT_REBUILD), progress);
 	} else {
-		snprintf(event_name, sizeof(event_name), "%s", map_num_s(events_map, event_enum));
+		snprintf(data.event_name, sizeof(data.event_name), "%s", map_num_s(events_map, event_enum));
 	}
 
-	if (info.alert_cmd)
-		execute_alert_cmd(event_name, dev, disc);
+	data.event_enum = event_enum;
 
-	if (info.mailaddr && (event_enum == EVENT_FAIL ||
-			      event_enum == EVENT_TEST_MESSAGE ||
-			      event_enum == EVENT_SPARES_MISSING ||
-			      event_enum == EVENT_DEGRADED_ARRAY)) {
-		send_event_email(event_name, dev, disc);
+	if (sprint_event_message(data.message, &data) != 0) {
+		pr_err("Cannot create event message.\n");
+		return;
 	}
+	pr_err("%s\n", data.message);
+
+	if (info.alert_cmd)
+		execute_alert_cmd(&data);
+
+	if (info.mailaddr && is_email_event(event_enum))
+		send_event_email(&data);
 
 	if (info.dosyslog)
-		log_event_to_syslog(event_enum, event_name, dev, disc);
+		log_event_to_syslog(&data);
 }
 
 static int check_array(struct state *st, struct mdstat_ent *mdstat,
@@ -575,7 +661,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 	unsigned long redundancy_only_flags = 0;
 
 	if (info.test)
-		alert(EVENT_TEST_MESSAGE, 0, dev, NULL);
+		alert(EVENT_TEST_MESSAGE, NULL, 0, dev, NULL);
 
 	retval = 0;
 
@@ -624,7 +710,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 	 */
 	if (sra->array.level == 0 || sra->array.level == -1) {
 		if (!st->err && !st->from_config)
-			alert(EVENT_DEVICE_DISAPPEARED, 0, dev, " Wrong-Level");
+			alert(EVENT_DEVICE_DISAPPEARED, "Wrong-Level", 0, dev, NULL);
 		st->err++;
 		goto out;
 	}
@@ -641,7 +727,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 		st->percent = RESYNC_NONE;
 		new_array = 1;
 		if (!is_container)
-			alert(EVENT_NEW_ARRAY, 0, st->devname, NULL);
+			alert(EVENT_NEW_ARRAY, NULL, 0, st->devname, NULL);
 	}
 
 	if (st->utime == array.utime && st->failed == sra->array.failed_disks &&
@@ -654,20 +740,20 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 	}
 	if (st->utime == 0 && /* new array */
 	    mse->pattern && strchr(mse->pattern, '_') /* degraded */)
-		alert(EVENT_DEGRADED_ARRAY, 0, dev, NULL);
+		alert(EVENT_DEGRADED_ARRAY, NULL, 0, dev, NULL);
 
 	if (st->utime == 0 && /* new array */ st->expected_spares > 0 &&
 	    sra->array.spare_disks < st->expected_spares)
-		alert(EVENT_SPARES_MISSING, 0, dev, NULL);
+		alert(EVENT_SPARES_MISSING, NULL, 0, dev, NULL);
 	if (st->percent < 0 && st->percent != RESYNC_UNKNOWN &&
 	    mse->percent >= 0)
-		alert(EVENT_REBUILD_STARTED, 0, dev, NULL);
+		alert(EVENT_REBUILD_STARTED, NULL, 0, dev, NULL);
 	if (st->percent >= 0 && mse->percent >= 0 &&
 	    (mse->percent / increments) > (st->percent / increments)) {
 		if((mse->percent / increments) == 0)
-			alert(EVENT_REBUILD_STARTED, 0, dev, NULL);
+			alert(EVENT_REBUILD_STARTED, NULL, 0, dev, NULL);
 		else
-			alert(EVENT_REBUILD, mse->percent, dev, NULL);
+			alert(EVENT_REBUILD, NULL, mse->percent, dev, NULL);
 	}
 
 	if (mse->percent == RESYNC_NONE && st->percent >= 0) {
@@ -680,9 +766,9 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 			snprintf(cnt, sizeof(cnt),
 				 " mismatches found: %d (on raid level %d)",
 				 sra->mismatch_cnt, sra->array.level);
-			alert(EVENT_REBUILD_FINISHED, 0, dev, cnt);
+			alert(EVENT_REBUILD_FINISHED, NULL, 0, dev, cnt);
 		} else
-			alert(EVENT_REBUILD_FINISHED, 0, dev, NULL);
+			alert(EVENT_REBUILD_FINISHED, NULL, 0, dev, NULL);
 	}
 	st->percent = mse->percent;
 
@@ -736,14 +822,14 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 		change = newstate ^ st->devstate[i];
 		if (st->utime && change && !st->err && !new_array) {
 			if ((st->devstate[i]&change) & (1 << MD_DISK_SYNC))
-				alert(EVENT_FAIL, 0, dev, dv);
+				alert(EVENT_FAIL, NULL, 0, dev, dv);
 			else if ((newstate & (1 << MD_DISK_FAULTY)) &&
 				 (disc.major || disc.minor) &&
 				 st->devid[i] == makedev(disc.major,
 							 disc.minor))
-				alert(EVENT_FAIL_SPARE, 0, dev, dv);
+				alert(EVENT_FAIL_SPARE, NULL, 0, dev, dv);
 			else if ((newstate&change) & (1 << MD_DISK_SYNC))
-				alert(EVENT_SPARE_ACTIVE, 0, dev, dv);
+				alert(EVENT_SPARE_ACTIVE, NULL, 0, dev, dv);
 		}
 		st->devstate[i] = newstate;
 		st->devid[i] = makedev(disc.major, disc.minor);
@@ -767,7 +853,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 
  disappeared:
 	if (!st->err && !is_container)
-		alert(EVENT_DEVICE_DISAPPEARED, 0, dev, NULL);
+		alert(EVENT_DEVICE_DISAPPEARED, NULL, 0, dev, NULL);
 	st->err++;
 	goto out;
 }
@@ -827,7 +913,7 @@ static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist)
 				st->parent_devnm[0] = 0;
 			*statelist = st;
 			if (info.test)
-				alert(EVENT_TEST_MESSAGE, 0, st->devname, NULL);
+				alert(EVENT_TEST_MESSAGE, NULL, 0, st->devname, NULL);
 			new_found = 1;
 		}
 	return new_found;
@@ -1050,7 +1136,7 @@ static void try_spare_migration(struct state *statelist)
 				if (devid > 0 &&
 				    move_spare(from->devname, to->devname,
 					       devid)) {
-					alert(EVENT_MOVE_SPARE, 0, to->devname, from->devname);
+					alert(EVENT_MOVE_SPARE, NULL, 0, to->devname, from->devname);
 					break;
 				}
 			}
-- 
2.26.2

Re: [PATCH 4/9] Mdmonitor: Add helper functions

[Coly Li]

On 9/7/22 8:56 PM, Mateusz Grzonka wrote:
> Add functions:
> - is_email_event(),
> - get_syslog_event_priority(),
> - sprint_event_message(),
> with kernel style comments containing more detailed descriptions.
>
> Also update event syslog priorities to be consistent with man. MoveSpare event was described in man as priority info, while implemented as warning. Move event data into a struct, so that it is passed between different functions if needed.
> Sort function declarations alphabetically and remove redundant alert() declaration.
>
> Signed-off-by: Mateusz Grzonka<mateusz.grzonka@intel.com>

Looks good to me.


Acked-by: Coly Li <colyli@suse.de>

Thanks.


Coly Li

> ---
>   Monitor.c | 230 +++++++++++++++++++++++++++++++++++++-----------------
>   1 file changed, 158 insertions(+), 72 deletions(-)
>
> diff --git a/Monitor.c b/Monitor.c
> index aa0ee047..80e43ebe 100644
> --- a/Monitor.c
> +++ b/Monitor.c
> @@ -73,10 +73,12 @@ enum event {
>   	EVENT_NEW_ARRAY,
>   	EVENT_MOVE_SPARE,
>   	EVENT_TEST_MESSAGE,
> +	__SYSLOG_PRIORITY_WARNING,
>   	EVENT_REBUILD_STARTED,
>   	EVENT_REBUILD,
>   	EVENT_REBUILD_FINISHED,
>   	EVENT_SPARES_MISSING,
> +	__SYSLOG_PRIORITY_CRITICAL,
>   	EVENT_DEVICE_DISAPPEARED,
>   	EVENT_FAIL,
>   	EVENT_FAIL_SPARE,
> @@ -100,17 +102,28 @@ mapping_t events_map[] = {
>   	{NULL, EVENT_UNKNOWN}
>   };
>   
> -static int make_daemon(char *pidfile);
> -static int check_one_sharer(int scan);
> -static void write_autorebuild_pid(void);
> -static void alert(const enum event event_enum, const unsigned int progress, const char *dev, const char *disc);
> -static int check_array(struct state *st, struct mdstat_ent *mdstat, int increments, char *prefer);
> +struct event_data {
> +	enum event event_enum;
> +	/**
> +	 * @event_name: Rebuild event name must be in form "RebuildXX", where XX is rebuild progress.
> +	 */
> +	char event_name[EVENT_NAME_MAX];
> +	char message[BUFSIZ];
> +	const char *description;
> +	const char *dev;
> +	const char *disc;
> +};
> +
>   static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist);
> -static void try_spare_migration(struct state *statelist);
> -static void link_containers_with_subarrays(struct state *list);
> +static int check_array(struct state *st, struct mdstat_ent *mdstat, int increments, char *prefer);
> +static int check_one_sharer(int scan);
>   #ifndef NO_LIBUDEV
>   static int check_udev_activity(void);
>   #endif
> +static void link_containers_with_subarrays(struct state *list);
> +static int make_daemon(char *pidfile);
> +static void try_spare_migration(struct state *statelist);
> +static void write_autorebuild_pid(void);
>   
>   int Monitor(struct mddev_dev *devlist,
>   	    char *mailaddr, char *alert_cmd,
> @@ -440,7 +453,80 @@ static void write_autorebuild_pid()
>   	}
>   }
>   
> -static void execute_alert_cmd(const char *event_name, const char *dev, const char *disc)
> +#define BASE_MESSAGE "%s event detected on md device %s"
> +#define COMPONENT_DEVICE_MESSAGE ", component device %s"
> +#define DESCRIPTION_MESSAGE ": %s"
> +/**
> + * sprint_event_message() - Writes basic message about detected event to destination ptr.
> + * @dest: message destination, should be at least the size of BUFSIZ
> + * @data: event data
> + *
> + * Return: 0 on success, 1 on error
> + */
> +static int sprint_event_message(char *dest, const struct event_data *data)
> +{
> +	if (!dest || !data)
> +		return 1;
> +
> +	if (data->disc && data->description)
> +		snprintf(dest, BUFSIZ, BASE_MESSAGE COMPONENT_DEVICE_MESSAGE DESCRIPTION_MESSAGE,
> +			 data->event_name, data->dev, data->disc, data->description);
> +	else if (data->disc)
> +		snprintf(dest, BUFSIZ, BASE_MESSAGE COMPONENT_DEVICE_MESSAGE,
> +			 data->event_name, data->dev, data->disc);
> +	else if (data->description)
> +		snprintf(dest, BUFSIZ, BASE_MESSAGE DESCRIPTION_MESSAGE,
> +			 data->event_name, data->dev, data->description);
> +	else
> +		snprintf(dest, BUFSIZ, BASE_MESSAGE, data->event_name, data->dev);
> +
> +	return 0;
> +}
> +
> +/**
> + * get_syslog_event_priority() - Determines event priority.
> + * @event_enum: event to be checked
> + *
> + * Return: LOG_CRIT, LOG_WARNING or LOG_INFO
> + */
> +static int get_syslog_event_priority(const enum event event_enum)
> +{
> +	if (event_enum > __SYSLOG_PRIORITY_CRITICAL)
> +		return LOG_CRIT;
> +	if (event_enum > __SYSLOG_PRIORITY_WARNING)
> +		return LOG_WARNING;
> +	return LOG_INFO;
> +}
> +
> +/**
> + * is_email_event() - Determines whether email for event should be sent or not.
> + * @event_enum: event to be checked
> + *
> + * Return: true if email should be sent, false otherwise
> + */
> +static bool is_email_event(const enum event event_enum)
> +{
> +	static const enum event email_events[] = {
> +	EVENT_FAIL,
> +	EVENT_FAIL_SPARE,
> +	EVENT_DEGRADED_ARRAY,
> +	EVENT_SPARES_MISSING,
> +	EVENT_TEST_MESSAGE
> +	};
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(email_events); ++i) {
> +		if (event_enum == email_events[i])
> +			return true;
> +	}
> +	return false;
> +}
> +
> +/**
> + * execute_alert_cmd() - Forks and executes command provided as alert_cmd.
> + * @data: event data
> + */
> +static void execute_alert_cmd(const struct event_data *data)
>   {
>   	int pid = fork();
>   
> @@ -452,12 +538,16 @@ static void execute_alert_cmd(const char *event_name, const char *dev, const cha
>   		pr_err("Cannot fork to execute alert command");
>   		break;
>   	case 0:
> -		execl(info.alert_cmd, info.alert_cmd, event_name, dev, disc, NULL);
> +		execl(info.alert_cmd, info.alert_cmd, data->event_name, data->dev, data->disc, NULL);
>   		exit(2);
>   	}
>   }
>   
> -static void send_event_email(const char *event_name, const char *dev, const char *disc)
> +/**
> + * send_event_email() - Sends an email about event detected by monitor.
> + * @data: event data
> + */
> +static void send_event_email(const struct event_data *data)
>   {
>   	FILE *mp, *mdstat;
>   	char buf[BUFSIZ];
> @@ -475,15 +565,9 @@ static void send_event_email(const char *event_name, const char *dev, const char
>   	else
>   		fprintf(mp, "From: %s monitoring <root>\n", Name);
>   	fprintf(mp, "To: %s\n", info.mailaddr);
> -	fprintf(mp, "Subject: %s event on %s:%s\n\n", event_name, dev, info.hostname);
> -	fprintf(mp, "This is an automatically generated mail message. \n");
> -	fprintf(mp, "A %s event had been detected on md device %s.\n\n", event_name, dev);
> -
> -	if (disc && disc[0] != ' ')
> -		fprintf(mp,
> -			"It could be related to component device %s.\n\n", disc);
> -	if (disc && disc[0] == ' ')
> -		fprintf(mp, "Extra information:%s.\n\n", disc);
> +	fprintf(mp, "Subject: %s event on %s:%s\n\n", data->event_name, data->dev, info.hostname);
> +	fprintf(mp, "This is an automatically generated mail message.\n");
> +	fprintf(mp, "%s\n", data->message);
>   
>   	mdstat = fopen("/proc/mdstat", "r");
>   	if (!mdstat) {
> @@ -499,58 +583,60 @@ static void send_event_email(const char *event_name, const char *dev, const char
>   	pclose(mp);
>   }
>   
> -static void log_event_to_syslog(const enum event event_enum, const char *event_name, const char *dev, const char *disc)
> +/**
> + * log_event_to_syslog() - Logs an event into syslog.
> + * @data: event data
> + */
> +static void log_event_to_syslog(const struct event_data *data)
>   {
>   	int priority;
> -	/* Log at a different severity depending on the event.
> -	 *
> -	 * These are the critical events:  */
> -	if (event_enum == EVENT_FAIL ||
> -	    event_enum == EVENT_DEGRADED_ARRAY ||
> -	    event_enum == EVENT_DEVICE_DISAPPEARED)
> -		priority = LOG_CRIT;
> -	/* Good to know about, but are not failures: */
> -	else if (event_enum == EVENT_REBUILD ||
> -		 event_enum == EVENT_MOVE_SPARE ||
> -		 event_enum == EVENT_SPARES_MISSING)
> -		priority = LOG_WARNING;
> -	/* Everything else: */
> -	else
> -		priority = LOG_INFO;
> -
> -	if (disc && disc[0] != ' ')
> -		syslog(priority,
> -		       "%s event detected on md device %s, component device %s",
> -		       event_name, dev, disc);
> -	else if (disc)
> -		syslog(priority, "%s event detected on md device %s: %s", event_name, dev, disc);
> -	else
> -		syslog(priority, "%s event detected on md device %s", event_name, dev);
> +
> +	priority = get_syslog_event_priority(data->event_enum);
> +
> +	syslog(priority, "%s\n", data->message);
>   }
>   
> -static void alert(const enum event event_enum, const unsigned int progress, const char *dev, const char *disc)
> +/**
> + * alert() - Alerts about the monitor event.
> + * @event_enum: event to be sent
> + * @description: event description
> + * @progress: rebuild progress
> + * @dev: md device name
> + * @disc: component device
> + *
> + * If needed function executes alert command, sends an email or logs event to syslog.
> + */
> +static void alert(const enum event event_enum, const char *description, const uint8_t progress,
> +		  const char *dev, const char *disc)
>   {
> -	char event_name[EVENT_NAME_MAX];
> +	struct event_data data = {.dev = dev, .disc = disc, .description = description};
> +
> +	if (!dev)
> +		return;
>   
>   	if (event_enum == EVENT_REBUILD) {
> -		snprintf(event_name, sizeof(event_name), "%s%02d",
> +		snprintf(data.event_name, sizeof(data.event_name), "%s%02d",
>   			 map_num_s(events_map, EVENT_REBUILD), progress);
>   	} else {
> -		snprintf(event_name, sizeof(event_name), "%s", map_num_s(events_map, event_enum));
> +		snprintf(data.event_name, sizeof(data.event_name), "%s", map_num_s(events_map, event_enum));
>   	}
>   
> -	if (info.alert_cmd)
> -		execute_alert_cmd(event_name, dev, disc);
> +	data.event_enum = event_enum;
>   
> -	if (info.mailaddr && (event_enum == EVENT_FAIL ||
> -			      event_enum == EVENT_TEST_MESSAGE ||
> -			      event_enum == EVENT_SPARES_MISSING ||
> -			      event_enum == EVENT_DEGRADED_ARRAY)) {
> -		send_event_email(event_name, dev, disc);
> +	if (sprint_event_message(data.message, &data) != 0) {
> +		pr_err("Cannot create event message.\n");
> +		return;
>   	}
> +	pr_err("%s\n", data.message);
> +
> +	if (info.alert_cmd)
> +		execute_alert_cmd(&data);
> +
> +	if (info.mailaddr && is_email_event(event_enum))
> +		send_event_email(&data);
>   
>   	if (info.dosyslog)
> -		log_event_to_syslog(event_enum, event_name, dev, disc);
> +		log_event_to_syslog(&data);
>   }
>   
>   static int check_array(struct state *st, struct mdstat_ent *mdstat,
> @@ -575,7 +661,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   	unsigned long redundancy_only_flags = 0;
>   
>   	if (info.test)
> -		alert(EVENT_TEST_MESSAGE, 0, dev, NULL);
> +		alert(EVENT_TEST_MESSAGE, NULL, 0, dev, NULL);
>   
>   	retval = 0;
>   
> @@ -624,7 +710,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   	 */
>   	if (sra->array.level == 0 || sra->array.level == -1) {
>   		if (!st->err && !st->from_config)
> -			alert(EVENT_DEVICE_DISAPPEARED, 0, dev, " Wrong-Level");
> +			alert(EVENT_DEVICE_DISAPPEARED, "Wrong-Level", 0, dev, NULL);
>   		st->err++;
>   		goto out;
>   	}
> @@ -641,7 +727,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   		st->percent = RESYNC_NONE;
>   		new_array = 1;
>   		if (!is_container)
> -			alert(EVENT_NEW_ARRAY, 0, st->devname, NULL);
> +			alert(EVENT_NEW_ARRAY, NULL, 0, st->devname, NULL);
>   	}
>   
>   	if (st->utime == array.utime && st->failed == sra->array.failed_disks &&
> @@ -654,20 +740,20 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   	}
>   	if (st->utime == 0 && /* new array */
>   	    mse->pattern && strchr(mse->pattern, '_') /* degraded */)
> -		alert(EVENT_DEGRADED_ARRAY, 0, dev, NULL);
> +		alert(EVENT_DEGRADED_ARRAY, NULL, 0, dev, NULL);
>   
>   	if (st->utime == 0 && /* new array */ st->expected_spares > 0 &&
>   	    sra->array.spare_disks < st->expected_spares)
> -		alert(EVENT_SPARES_MISSING, 0, dev, NULL);
> +		alert(EVENT_SPARES_MISSING, NULL, 0, dev, NULL);
>   	if (st->percent < 0 && st->percent != RESYNC_UNKNOWN &&
>   	    mse->percent >= 0)
> -		alert(EVENT_REBUILD_STARTED, 0, dev, NULL);
> +		alert(EVENT_REBUILD_STARTED, NULL, 0, dev, NULL);
>   	if (st->percent >= 0 && mse->percent >= 0 &&
>   	    (mse->percent / increments) > (st->percent / increments)) {
>   		if((mse->percent / increments) == 0)
> -			alert(EVENT_REBUILD_STARTED, 0, dev, NULL);
> +			alert(EVENT_REBUILD_STARTED, NULL, 0, dev, NULL);
>   		else
> -			alert(EVENT_REBUILD, mse->percent, dev, NULL);
> +			alert(EVENT_REBUILD, NULL, mse->percent, dev, NULL);
>   	}
>   
>   	if (mse->percent == RESYNC_NONE && st->percent >= 0) {
> @@ -680,9 +766,9 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   			snprintf(cnt, sizeof(cnt),
>   				 " mismatches found: %d (on raid level %d)",
>   				 sra->mismatch_cnt, sra->array.level);
> -			alert(EVENT_REBUILD_FINISHED, 0, dev, cnt);
> +			alert(EVENT_REBUILD_FINISHED, NULL, 0, dev, cnt);
>   		} else
> -			alert(EVENT_REBUILD_FINISHED, 0, dev, NULL);
> +			alert(EVENT_REBUILD_FINISHED, NULL, 0, dev, NULL);
>   	}
>   	st->percent = mse->percent;
>   
> @@ -736,14 +822,14 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   		change = newstate ^ st->devstate[i];
>   		if (st->utime && change && !st->err && !new_array) {
>   			if ((st->devstate[i]&change) & (1 << MD_DISK_SYNC))
> -				alert(EVENT_FAIL, 0, dev, dv);
> +				alert(EVENT_FAIL, NULL, 0, dev, dv);
>   			else if ((newstate & (1 << MD_DISK_FAULTY)) &&
>   				 (disc.major || disc.minor) &&
>   				 st->devid[i] == makedev(disc.major,
>   							 disc.minor))
> -				alert(EVENT_FAIL_SPARE, 0, dev, dv);
> +				alert(EVENT_FAIL_SPARE, NULL, 0, dev, dv);
>   			else if ((newstate&change) & (1 << MD_DISK_SYNC))
> -				alert(EVENT_SPARE_ACTIVE, 0, dev, dv);
> +				alert(EVENT_SPARE_ACTIVE, NULL, 0, dev, dv);
>   		}
>   		st->devstate[i] = newstate;
>   		st->devid[i] = makedev(disc.major, disc.minor);
> @@ -767,7 +853,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   
>    disappeared:
>   	if (!st->err && !is_container)
> -		alert(EVENT_DEVICE_DISAPPEARED, 0, dev, NULL);
> +		alert(EVENT_DEVICE_DISAPPEARED, NULL, 0, dev, NULL);
>   	st->err++;
>   	goto out;
>   }
> @@ -827,7 +913,7 @@ static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist)
>   				st->parent_devnm[0] = 0;
>   			*statelist = st;
>   			if (info.test)
> -				alert(EVENT_TEST_MESSAGE, 0, st->devname, NULL);
> +				alert(EVENT_TEST_MESSAGE, NULL, 0, st->devname, NULL);
>   			new_found = 1;
>   		}
>   	return new_found;
> @@ -1050,7 +1136,7 @@ static void try_spare_migration(struct state *statelist)
>   				if (devid > 0 &&
>   				    move_spare(from->devname, to->devname,
>   					       devid)) {
> -					alert(EVENT_MOVE_SPARE, 0, to->devname, from->devname);
> +					alert(EVENT_MOVE_SPARE, NULL, 0, to->devname, from->devname);
>   					break;
>   				}
>   			}

[PATCH 5/9] Add helpers to determine whether directories or files are soft links

[Mateusz Grzonka]

Signed-off-by: Mateusz Grzonka <mateusz.grzonka@intel.com>
---
 mdadm.h |  3 +++
 util.c  | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/mdadm.h b/mdadm.h
index 09915a00..9183ee70 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1707,6 +1707,9 @@ extern int cluster_get_dlmlock(void);
 extern int cluster_release_dlmlock(void);
 extern void set_dlm_hooks(void);
 
+extern bool is_directory(const char *path);
+extern bool is_file(const char *path);
+
 #define _ROUND_UP(val, base)	(((val) + (base) - 1) & ~(base - 1))
 #define ROUND_UP(val, base)	_ROUND_UP(val, (typeof(val))(base))
 #define ROUND_UP_PTR(ptr, base)	((typeof(ptr)) \
diff --git a/util.c b/util.c
index cc94f96e..97926f19 100644
--- a/util.c
+++ b/util.c
@@ -2375,3 +2375,49 @@ out:
 	close(fd_zero);
 	return ret;
 }
+
+/**
+ * is_directory() - Checks if directory provided by path is indeed a regular directory.
+ * @path: directory path to be checked
+ *
+ * Doesn't accept symlinks.
+ *
+ * Return: true if is a directory, false if not
+ */
+bool is_directory(const char *path)
+{
+	struct stat st;
+
+	if (lstat(path, &st) != 0) {
+		pr_err("%s: %s\n", strerror(errno), path);
+		return false;
+	}
+
+	if (!S_ISDIR(st.st_mode))
+		return false;
+
+	return true;
+}
+
+/**
+ * is_file() - Checks if file provided by path is indeed a regular file.
+ * @path: file path to be checked
+ *
+ * Doesn't accept symlinks.
+ *
+ * Return: true if is  a file, false if not
+ */
+bool is_file(const char *path)
+{
+	struct stat st;
+
+	if (lstat(path, &st) != 0) {
+		pr_err("%s: %s\n", strerror(errno), path);
+		return false;
+	}
+
+	if (!S_ISREG(st.st_mode))
+		return false;
+
+	return true;
+}
-- 
2.26.2

Re: [PATCH 5/9] Add helpers to determine whether directories or files are soft links

[Coly Li]

On 9/7/22 8:56 PM, Mateusz Grzonka wrote:
> Signed-off-by: Mateusz Grzonka<mateusz.grzonka@intel.com>

Except for the comment style, the rested part is good to me.


Acked-by: Coly Li <colyli@suse.de>

Thanks.


Coly Li



> ---
>   mdadm.h |  3 +++
>   util.c  | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 49 insertions(+)
>
> diff --git a/mdadm.h b/mdadm.h
> index 09915a00..9183ee70 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -1707,6 +1707,9 @@ extern int cluster_get_dlmlock(void);
>   extern int cluster_release_dlmlock(void);
>   extern void set_dlm_hooks(void);
>   
> +extern bool is_directory(const char *path);
> +extern bool is_file(const char *path);
> +
>   #define _ROUND_UP(val, base)	(((val) + (base) - 1) & ~(base - 1))
>   #define ROUND_UP(val, base)	_ROUND_UP(val, (typeof(val))(base))
>   #define ROUND_UP_PTR(ptr, base)	((typeof(ptr)) \
> diff --git a/util.c b/util.c
> index cc94f96e..97926f19 100644
> --- a/util.c
> +++ b/util.c
> @@ -2375,3 +2375,49 @@ out:
>   	close(fd_zero);
>   	return ret;
>   }
> +
> +/**
> + * is_directory() - Checks if directory provided by path is indeed a regular directory.
> + * @path: directory path to be checked
> + *
> + * Doesn't accept symlinks.
> + *
> + * Return: true if is a directory, false if not
> + */
> +bool is_directory(const char *path)
> +{
> +	struct stat st;
> +
> +	if (lstat(path, &st) != 0) {
> +		pr_err("%s: %s\n", strerror(errno), path);
> +		return false;
> +	}
> +
> +	if (!S_ISDIR(st.st_mode))
> +		return false;
> +
> +	return true;
> +}
> +
> +/**
> + * is_file() - Checks if file provided by path is indeed a regular file.
> + * @path: file path to be checked
> + *
> + * Doesn't accept symlinks.
> + *
> + * Return: true if is  a file, false if not
> + */
> +bool is_file(const char *path)
> +{
> +	struct stat st;
> +
> +	if (lstat(path, &st) != 0) {
> +		pr_err("%s: %s\n", strerror(errno), path);
> +		return false;
> +	}
> +
> +	if (!S_ISREG(st.st_mode))
> +		return false;
> +
> +	return true;
> +}

[PATCH 6/9] Mdmonitor: Refactor write_autorebuild_pid()

[Mateusz Grzonka]

Add better error handling and check for symlinks when opening MDMON_DIR.

Signed-off-by: Mateusz Grzonka <mateusz.grzonka@intel.com>
---
 Monitor.c | 55 ++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 19 deletions(-)

diff --git a/Monitor.c b/Monitor.c
index 80e43ebe..3e09f2a2 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -33,6 +33,7 @@
 #endif
 
 #define EVENT_NAME_MAX 32
+#define AUTOREBUILD_PID_PATH MDMON_DIR "/autorebuild.pid"
 
 struct state {
 	char *devname;
@@ -123,7 +124,7 @@ static int check_udev_activity(void);
 static void link_containers_with_subarrays(struct state *list);
 static int make_daemon(char *pidfile);
 static void try_spare_migration(struct state *statelist);
-static void write_autorebuild_pid(void);
+static int write_autorebuild_pid(void);
 
 int Monitor(struct mddev_dev *devlist,
 	    char *mailaddr, char *alert_cmd,
@@ -224,7 +225,8 @@ int Monitor(struct mddev_dev *devlist,
 	}
 
 	if (share)
-		write_autorebuild_pid();
+		if (write_autorebuild_pid() != 0)
+			return 1;
 
 	if (devlist == NULL) {
 		mdlist = conf_get_ident(NULL);
@@ -428,29 +430,44 @@ static int check_one_sharer(int scan)
 	return 0;
 }
 
-static void write_autorebuild_pid()
+/**
+ * write_autorebuild_pid() - Writes pid to autorebuild.pid file.
+ *
+ * Return: 0 on success, 1 on error
+ */
+static int write_autorebuild_pid(void)
 {
-	char path[PATH_MAX];
-	int pid;
-	FILE *fp = NULL;
-	sprintf(path, "%s/autorebuild.pid", MDMON_DIR);
+	FILE *fp;
+	int fd;
 
 	if (mkdir(MDMON_DIR, 0700) < 0 && errno != EEXIST) {
-		pr_err("Can't create autorebuild.pid file\n");
-	} else {
-		int fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0700);
+		pr_err("%s: %s\n", strerror(errno), MDMON_DIR);
+		return 1;
+	}
 
-		if (fd >= 0)
-			fp = fdopen(fd, "w");
+	if (!is_directory(MDMON_DIR)) {
+		pr_err("%s is not a regular directory.\n", MDMON_DIR);
+		return 1;
+	}
 
-		if (!fp)
-			pr_err("Can't create autorebuild.pid file\n");
-		else {
-			pid = getpid();
-			fprintf(fp, "%d\n", pid);
-			fclose(fp);
-		}
+	fd = open(AUTOREBUILD_PID_PATH, O_WRONLY | O_CREAT | O_TRUNC, 0700);
+
+	if (fd < 0) {
+		pr_err("Error opening %s file.\n", AUTOREBUILD_PID_PATH);
+		return 1;
 	}
+
+	fp = fdopen(fd, "w");
+
+	if (!fp) {
+		pr_err("Error opening fd for %s file.\n", AUTOREBUILD_PID_PATH);
+		return 1;
+	}
+
+	fprintf(fp, "%d\n", getpid());
+
+	fclose(fp);
+	return 0;
 }
 
 #define BASE_MESSAGE "%s event detected on md device %s"
-- 
2.26.2

Re: [PATCH 6/9] Mdmonitor: Refactor write_autorebuild_pid()

[Coly Li]

On 9/7/22 8:56 PM, Mateusz Grzonka wrote:
> Add better error handling and check for symlinks when opening MDMON_DIR.
>
> Signed-off-by: Mateusz Grzonka<mateusz.grzonka@intel.com>


Except for the code comment style, the rested part is good to me.

Acked-by: Coly Li <colyli@suse.de>


Thanks.


Coly Li




> ---
>   Monitor.c | 55 ++++++++++++++++++++++++++++++++++++-------------------
>   1 file changed, 36 insertions(+), 19 deletions(-)
>
> diff --git a/Monitor.c b/Monitor.c
> index 80e43ebe..3e09f2a2 100644
> --- a/Monitor.c
> +++ b/Monitor.c
> @@ -33,6 +33,7 @@
>   #endif
>   
>   #define EVENT_NAME_MAX 32
> +#define AUTOREBUILD_PID_PATH MDMON_DIR "/autorebuild.pid"
>   
>   struct state {
>   	char *devname;
> @@ -123,7 +124,7 @@ static int check_udev_activity(void);
>   static void link_containers_with_subarrays(struct state *list);
>   static int make_daemon(char *pidfile);
>   static void try_spare_migration(struct state *statelist);
> -static void write_autorebuild_pid(void);
> +static int write_autorebuild_pid(void);
>   
>   int Monitor(struct mddev_dev *devlist,
>   	    char *mailaddr, char *alert_cmd,
> @@ -224,7 +225,8 @@ int Monitor(struct mddev_dev *devlist,
>   	}
>   
>   	if (share)
> -		write_autorebuild_pid();
> +		if (write_autorebuild_pid() != 0)
> +			return 1;
>   
>   	if (devlist == NULL) {
>   		mdlist = conf_get_ident(NULL);
> @@ -428,29 +430,44 @@ static int check_one_sharer(int scan)
>   	return 0;
>   }
>   
> -static void write_autorebuild_pid()
> +/**
> + * write_autorebuild_pid() - Writes pid to autorebuild.pid file.
> + *
> + * Return: 0 on success, 1 on error
> + */
> +static int write_autorebuild_pid(void)
>   {
> -	char path[PATH_MAX];
> -	int pid;
> -	FILE *fp = NULL;
> -	sprintf(path, "%s/autorebuild.pid", MDMON_DIR);
> +	FILE *fp;
> +	int fd;
>   
>   	if (mkdir(MDMON_DIR, 0700) < 0 && errno != EEXIST) {
> -		pr_err("Can't create autorebuild.pid file\n");
> -	} else {
> -		int fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0700);
> +		pr_err("%s: %s\n", strerror(errno), MDMON_DIR);
> +		return 1;
> +	}
>   
> -		if (fd >= 0)
> -			fp = fdopen(fd, "w");
> +	if (!is_directory(MDMON_DIR)) {
> +		pr_err("%s is not a regular directory.\n", MDMON_DIR);
> +		return 1;
> +	}
>   
> -		if (!fp)
> -			pr_err("Can't create autorebuild.pid file\n");
> -		else {
> -			pid = getpid();
> -			fprintf(fp, "%d\n", pid);
> -			fclose(fp);
> -		}
> +	fd = open(AUTOREBUILD_PID_PATH, O_WRONLY | O_CREAT | O_TRUNC, 0700);
> +
> +	if (fd < 0) {
> +		pr_err("Error opening %s file.\n", AUTOREBUILD_PID_PATH);
> +		return 1;
>   	}
> +
> +	fp = fdopen(fd, "w");
> +
> +	if (!fp) {
> +		pr_err("Error opening fd for %s file.\n", AUTOREBUILD_PID_PATH);
> +		return 1;
> +	}
> +
> +	fprintf(fp, "%d\n", getpid());
> +
> +	fclose(fp);
> +	return 0;
>   }
>   
>   #define BASE_MESSAGE "%s event detected on md device %s"

[PATCH 7/9] Mdmonitor: Refactor check_one_sharer() for better error handling

[Mateusz Grzonka]

Also check if autorebuild.pid is a symlink, which we shouldn't accept.

Signed-off-by: Mateusz Grzonka <mateusz.grzonka@intel.com>
---
 Monitor.c | 89 ++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 62 insertions(+), 27 deletions(-)

diff --git a/Monitor.c b/Monitor.c
index 3e09f2a2..aa79cf6c 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -32,6 +32,7 @@
 #include	<libudev.h>
 #endif
 
+#define TASK_COMM_LEN 16
 #define EVENT_NAME_MAX 32
 #define AUTOREBUILD_PID_PATH MDMON_DIR "/autorebuild.pid"
 
@@ -214,7 +215,7 @@ int Monitor(struct mddev_dev *devlist,
 	info.hostname[sizeof(info.hostname) - 1] = '\0';
 
 	if (share){
-		if (check_one_sharer(c->scan))
+		if (check_one_sharer(c->scan) == 2)
 			return 1;
 	}
 
@@ -394,39 +395,73 @@ static int make_daemon(char *pidfile)
 	return -1;
 }
 
+/**
+ * check_one_sharer() - Checks for other mdmon processes running.
+ *
+ * Return:
+ * 0 - no other processes running,
+ * 1 - warning,
+ * 2 - error, or when scan mode is enabled, and one mdmon process already exists
+ */
 static int check_one_sharer(int scan)
 {
 	int pid;
-	FILE *comm_fp;
-	FILE *fp;
+	FILE *fp, *comm_fp;
 	char comm_path[PATH_MAX];
-	char path[PATH_MAX];
-	char comm[20];
-
-	sprintf(path, "%s/autorebuild.pid", MDMON_DIR);
-	fp = fopen(path, "r");
-	if (fp) {
-		if (fscanf(fp, "%d", &pid) != 1)
-			pid = -1;
-		snprintf(comm_path, sizeof(comm_path),
-			 "/proc/%d/comm", pid);
-		comm_fp = fopen(comm_path, "r");
-		if (comm_fp) {
-			if (fscanf(comm_fp, "%19s", comm) &&
-			    strncmp(basename(comm), Name, strlen(Name)) == 0) {
-				if (scan) {
-					pr_err("Only one autorebuild process allowed in scan mode, aborting\n");
-					fclose(comm_fp);
-					fclose(fp);
-					return 1;
-				} else {
-					pr_err("Warning: One autorebuild process already running.\n");
-				}
-			}
+	char comm[TASK_COMM_LEN];
+
+	if (!is_directory(MDMON_DIR)) {
+		pr_err("%s is not a regular directory.\n", MDMON_DIR);
+		return 2;
+	}
+
+	if (access(AUTOREBUILD_PID_PATH, F_OK) != 0)
+		return 0;
+
+	if (!is_file(AUTOREBUILD_PID_PATH)) {
+		pr_err("%s is not a regular file.\n", AUTOREBUILD_PID_PATH);
+		return 2;
+	}
+
+	fp = fopen(AUTOREBUILD_PID_PATH, "r");
+	if (!fp) {
+		pr_err("Cannot open %s file.\n", AUTOREBUILD_PID_PATH);
+		return 2;
+	}
+
+	if (fscanf(fp, "%d", &pid) != 1) {
+		pr_err("Cannot read pid from %s file.\n", AUTOREBUILD_PID_PATH);
+		fclose(fp);
+		return 2;
+	}
+
+	snprintf(comm_path, sizeof(comm_path), "/proc/%d/comm", pid);
+
+	comm_fp = fopen(comm_path, "r");
+	if (!comm_fp) {
+		dprintf("Warning: Cannot open %s, continuing\n", comm_path);
+		fclose(fp);
+		return 1;
+	}
+
+	if (fscanf(comm_fp, "%15s", comm) == 0) {
+		dprintf("Warning: Cannot read comm from %s, continuing\n", comm_path);
+		fclose(comm_fp);
+		fclose(fp);
+		return 1;
+	}
+
+	if (strncmp(basename(comm), Name, strlen(Name)) == 0) {
+		if (scan) {
+			pr_err("Only one autorebuild process allowed in scan mode, aborting\n");
 			fclose(comm_fp);
+			fclose(fp);
+			return 2;
 		}
-		fclose(fp);
+		pr_err("Warning: One autorebuild process already running.\n");
 	}
+	fclose(comm_fp);
+	fclose(fp);
 	return 0;
 }
 
-- 
2.26.2

Re: [PATCH 7/9] Mdmonitor: Refactor check_one_sharer() for better error handling

[Coly Li]

On 9/7/22 8:56 PM, Mateusz Grzonka wrote:
> Also check if autorebuild.pid is a symlink, which we shouldn't accept.
>
> Signed-off-by: Mateusz Grzonka<mateusz.grzonka@intel.com>

Except for the code comment style, the rested part is fine to me.

Acked-by: Coly Li <colyli@suse.de>


Thanks.

Coly Li


> ---
>   Monitor.c | 89 ++++++++++++++++++++++++++++++++++++++-----------------
>   1 file changed, 62 insertions(+), 27 deletions(-)
>
> diff --git a/Monitor.c b/Monitor.c
> index 3e09f2a2..aa79cf6c 100644
> --- a/Monitor.c
> +++ b/Monitor.c
> @@ -32,6 +32,7 @@
>   #include	<libudev.h>
>   #endif
>   
> +#define TASK_COMM_LEN 16
>   #define EVENT_NAME_MAX 32
>   #define AUTOREBUILD_PID_PATH MDMON_DIR "/autorebuild.pid"
>   
> @@ -214,7 +215,7 @@ int Monitor(struct mddev_dev *devlist,
>   	info.hostname[sizeof(info.hostname) - 1] = '\0';
>   
>   	if (share){
> -		if (check_one_sharer(c->scan))
> +		if (check_one_sharer(c->scan) == 2)
>   			return 1;
>   	}
>   
> @@ -394,39 +395,73 @@ static int make_daemon(char *pidfile)
>   	return -1;
>   }
>   
> +/**
> + * check_one_sharer() - Checks for other mdmon processes running.
> + *
> + * Return:
> + * 0 - no other processes running,
> + * 1 - warning,
> + * 2 - error, or when scan mode is enabled, and one mdmon process already exists
> + */
>   static int check_one_sharer(int scan)
>   {
>   	int pid;
> -	FILE *comm_fp;
> -	FILE *fp;
> +	FILE *fp, *comm_fp;
>   	char comm_path[PATH_MAX];
> -	char path[PATH_MAX];
> -	char comm[20];
> -
> -	sprintf(path, "%s/autorebuild.pid", MDMON_DIR);
> -	fp = fopen(path, "r");
> -	if (fp) {
> -		if (fscanf(fp, "%d", &pid) != 1)
> -			pid = -1;
> -		snprintf(comm_path, sizeof(comm_path),
> -			 "/proc/%d/comm", pid);
> -		comm_fp = fopen(comm_path, "r");
> -		if (comm_fp) {
> -			if (fscanf(comm_fp, "%19s", comm) &&
> -			    strncmp(basename(comm), Name, strlen(Name)) == 0) {
> -				if (scan) {
> -					pr_err("Only one autorebuild process allowed in scan mode, aborting\n");
> -					fclose(comm_fp);
> -					fclose(fp);
> -					return 1;
> -				} else {
> -					pr_err("Warning: One autorebuild process already running.\n");
> -				}
> -			}
> +	char comm[TASK_COMM_LEN];
> +
> +	if (!is_directory(MDMON_DIR)) {
> +		pr_err("%s is not a regular directory.\n", MDMON_DIR);
> +		return 2;
> +	}
> +
> +	if (access(AUTOREBUILD_PID_PATH, F_OK) != 0)
> +		return 0;
> +
> +	if (!is_file(AUTOREBUILD_PID_PATH)) {
> +		pr_err("%s is not a regular file.\n", AUTOREBUILD_PID_PATH);
> +		return 2;
> +	}
> +
> +	fp = fopen(AUTOREBUILD_PID_PATH, "r");
> +	if (!fp) {
> +		pr_err("Cannot open %s file.\n", AUTOREBUILD_PID_PATH);
> +		return 2;
> +	}
> +
> +	if (fscanf(fp, "%d", &pid) != 1) {
> +		pr_err("Cannot read pid from %s file.\n", AUTOREBUILD_PID_PATH);
> +		fclose(fp);
> +		return 2;
> +	}
> +
> +	snprintf(comm_path, sizeof(comm_path), "/proc/%d/comm", pid);
> +
> +	comm_fp = fopen(comm_path, "r");
> +	if (!comm_fp) {
> +		dprintf("Warning: Cannot open %s, continuing\n", comm_path);
> +		fclose(fp);
> +		return 1;
> +	}
> +
> +	if (fscanf(comm_fp, "%15s", comm) == 0) {
> +		dprintf("Warning: Cannot read comm from %s, continuing\n", comm_path);
> +		fclose(comm_fp);
> +		fclose(fp);
> +		return 1;
> +	}
> +
> +	if (strncmp(basename(comm), Name, strlen(Name)) == 0) {
> +		if (scan) {
> +			pr_err("Only one autorebuild process allowed in scan mode, aborting\n");
>   			fclose(comm_fp);
> +			fclose(fp);
> +			return 2;
>   		}
> -		fclose(fp);
> +		pr_err("Warning: One autorebuild process already running.\n");
>   	}
> +	fclose(comm_fp);
> +	fclose(fp);
>   	return 0;
>   }
>   

[PATCH 8/9] Mdmonitor: Improve udev event handling

[Mateusz Grzonka]

Mdmonitor is waiting for udev queue to become empty.
Even if the queue becomes empty, udev might still be processing last event.
However we want to wait and wake up mdmonitor when udev finished
processing events.

Also, the udev queue interface is considered legacy and should not be
used outside of udev.

Use udev monitor instead, and wake up mdmonitor on every event triggered
by udev for md block device.

We need to generate more change events from kernel, because they are
missing in some situations, for example, when rebuild started.
This will be addressed in a separate patch.

Move udev specific code into separate functions, and place them in udev.c file.
Also move use_udev() logic from lib.c into newly created file.

Signed-off-by: Mateusz Grzonka <mateusz.grzonka@intel.com>
---
 Makefile  |   3 +-
 Manage.c  |   3 +-
 Monitor.c | 137 +++++++++++++++++++-------------------------------
 lib.c     |  13 -----
 mdadm.h   |   1 -
 mdopen.c  |   7 +--
 udev.c    | 147 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 udev.h    |  36 +++++++++++++
 8 files changed, 241 insertions(+), 106 deletions(-)
 create mode 100644 udev.c
 create mode 100644 udev.h

diff --git a/Makefile b/Makefile
index bf126033..ed371006 100644
--- a/Makefile
+++ b/Makefile
@@ -124,6 +124,7 @@ LDLIBS = -ldl
 # To explicitly disable libudev, set -DNO_LIBUDEV in CXFLAGS
 ifeq (, $(findstring -DNO_LIBUDEV,  $(CXFLAGS)))
 	LDLIBS += -ludev
+	OBJS = udev.o
 endif
 
 INSTALL = /usr/bin/install
@@ -145,7 +146,7 @@ else
 	ECHO=:
 endif
 
-OBJS =  mdadm.o config.o policy.o mdstat.o  ReadMe.o uuid.o util.o maps.o lib.o \
+OBJS += mdadm.o config.o policy.o mdstat.o  ReadMe.o uuid.o util.o maps.o lib.o \
 	Manage.o Assemble.o Build.o \
 	Create.o Detail.o Examine.o Grow.o Monitor.o dlink.o Kill.o Query.o \
 	Incremental.o Dump.o \
diff --git a/Manage.c b/Manage.c
index f789e0c1..dae09901 100644
--- a/Manage.c
+++ b/Manage.c
@@ -25,6 +25,7 @@
 #include "mdadm.h"
 #include "md_u.h"
 #include "md_p.h"
+#include "udev.h"
 #include <ctype.h>
 
 int Manage_ro(char *devname, int fd, int readonly)
@@ -472,7 +473,7 @@ done:
 			sysfs_uevent(mdi, "change");
 	}
 
-	if (devnm[0] && use_udev()) {
+	if (devnm[0] && udev_is_available()) {
 		struct map_ent *mp = map_by_devnm(&map, devnm);
 		remove_devices(devnm, mp ? mp->path : NULL);
 	}
diff --git a/Monitor.c b/Monitor.c
index aa79cf6c..dd3ea347 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -23,18 +23,17 @@
  */
 
 #include	"mdadm.h"
+#include	"udev.h"
 #include	"md_p.h"
 #include	"md_u.h"
 #include	<sys/wait.h>
 #include	<limits.h>
 #include	<syslog.h>
-#ifndef NO_LIBUDEV
-#include	<libudev.h>
-#endif
 
 #define TASK_COMM_LEN 16
 #define EVENT_NAME_MAX 32
 #define AUTOREBUILD_PID_PATH MDMON_DIR "/autorebuild.pid"
+#define FALLBACK_DELAY 5
 
 struct state {
 	char *devname;
@@ -119,12 +118,11 @@ struct event_data {
 static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist);
 static int check_array(struct state *st, struct mdstat_ent *mdstat, int increments, char *prefer);
 static int check_one_sharer(int scan);
-#ifndef NO_LIBUDEV
-static int check_udev_activity(void);
-#endif
 static void link_containers_with_subarrays(struct state *list);
 static int make_daemon(char *pidfile);
 static void try_spare_migration(struct state *statelist);
+static void wait_for_events(int *delay_for_event, int c_delay);
+static void wait_for_events_mdstat(int *delay_for_event, int c_delay);
 static int write_autorebuild_pid(void);
 
 int Monitor(struct mddev_dev *devlist,
@@ -310,32 +308,12 @@ int Monitor(struct mddev_dev *devlist,
 		if (!new_found) {
 			if (oneshot)
 				break;
-			else if (!anyredundant) {
+			if (!anyredundant) {
 				pr_err("No array with redundancy detected, stopping\n");
 				break;
 			}
-			else {
-#ifndef NO_LIBUDEV
-				/*
-				 * Wait for udevd to finish new devices
-				 * processing.
-				 */
-				if (mdstat_wait(delay_for_event) &&
-				    check_udev_activity())
-					pr_err("Error while waiting for UDEV to complete new devices processing\n");
-#else
-				int wait_result = mdstat_wait(delay_for_event);
-				/*
-				 * Give chance to process new device
-				 */
-				if (wait_result != 0) {
-					if (c->delay > 5)
-						delay_for_event = 5;
-				} else
-					delay_for_event = c->delay;
-#endif
-				mdstat_close();
-			}
+
+			wait_for_events(&delay_for_event, c->delay);
 		}
 		info.test = 0;
 
@@ -359,6 +337,49 @@ int Monitor(struct mddev_dev *devlist,
 	return 0;
 }
 
+/**
+ * wait_for_events() - Waits for events on md devices.
+ * @delay_for_event: pointer to current event delay
+ * @c_delay: delay from config
+ */
+static void wait_for_events(int *delay_for_event, int c_delay)
+{
+#ifndef NO_LIBUDEV
+	if (udev_is_available()) {
+		if (udev_wait_for_events(*delay_for_event) == UDEV_STATUS_ERROR)
+			pr_err("Error while waiting for udev events.\n");
+		return;
+	}
+#endif
+	wait_for_events_mdstat(delay_for_event, c_delay);
+}
+
+/**
+ * wait_for_events_mdstat() - Waits for events on mdstat.
+ * @delay_for_event: pointer to current event delay
+ * @c_delay: delay from config
+ */
+static void wait_for_events_mdstat(int *delay_for_event, int c_delay)
+{
+	int wait_result = mdstat_wait(*delay_for_event);
+
+	if (wait_result < 0) {
+		pr_err("Error while waiting for events on mdstat.\n");
+		return;
+	}
+
+	/*
+	 * Give chance to process new device
+	 */
+	if (wait_result != 0) {
+		if (c_delay > FALLBACK_DELAY)
+			*delay_for_event = FALLBACK_DELAY;
+	} else {
+		*delay_for_event = c_delay;
+	}
+	mdstat_close();
+}
+
 static int make_daemon(char *pidfile)
 {
 	/* Return:
@@ -1222,64 +1243,6 @@ static void link_containers_with_subarrays(struct state *list)
 				}
 }
 
-#ifndef NO_LIBUDEV
-/* function: check_udev_activity
- * Description: Function waits for udev to finish
- * events processing.
- * Returns:
- *		1 - detected error while opening udev
- *		2 - timeout
- *		0 - successfull completion
- */
-static int check_udev_activity(void)
-{
-	struct udev *udev = NULL;
-	struct udev_queue *udev_queue = NULL;
-	int timeout_cnt = 30;
-	int rc = 0;
-
-	/*
-	 * In rare cases systemd may not have udevm,
-	 * in such cases just exit with rc 0
-	 */
-	if (!use_udev())
-		goto out;
-
-	udev = udev_new();
-	if (!udev) {
-		rc = 1;
-		goto out;
-	}
-
-	udev_queue = udev_queue_new(udev);
-	if (!udev_queue) {
-		rc = 1;
-		goto out;
-	}
-
-	if (udev_queue_get_queue_is_empty(udev_queue))
-		goto out;
-
-	while (!udev_queue_get_queue_is_empty(udev_queue)) {
-		sleep(1);
-
-		if (timeout_cnt)
-			timeout_cnt--;
-		else {
-			rc = 2;
-			goto out;
-		}
-	}
-
-out:
-	if (udev_queue)
-		udev_queue_unref(udev_queue);
-	if (udev)
-		udev_unref(udev);
-	return rc;
-}
-#endif
-
 /* Not really Monitor but ... */
 int Wait(char *dev)
 {
diff --git a/lib.c b/lib.c
index 7e3e3d47..10a6ae40 100644
--- a/lib.c
+++ b/lib.c
@@ -483,19 +483,6 @@ int check_env(char *name)
 	return 0;
 }
 
-int use_udev(void)
-{
-	static int use = -1;
-	struct stat stb;
-
-	if (use < 0) {
-		use = ((stat("/dev/.udev", &stb) == 0 ||
-			stat("/run/udev", &stb) == 0) &&
-		       check_env("MDADM_NO_UDEV") == 0);
-	}
-	return use;
-}
-
 unsigned long GCD(unsigned long a, unsigned long b)
 {
 	while (a != b) {
diff --git a/mdadm.h b/mdadm.h
index 9183ee70..3c08f826 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1559,7 +1559,6 @@ extern char *conf_line(FILE *file);
 extern char *conf_word(FILE *file, int allow_key);
 extern void print_quoted(char *str);
 extern void print_escape(char *str);
-extern int use_udev(void);
 extern unsigned long GCD(unsigned long a, unsigned long b);
 extern int conf_name_is_free(char *name);
 extern int conf_verify_devnames(struct mddev_ident *array_list);
diff --git a/mdopen.c b/mdopen.c
index 245be537..53da4d96 100644
--- a/mdopen.c
+++ b/mdopen.c
@@ -23,6 +23,7 @@
  */
 
 #include "mdadm.h"
+#include "udev.h"
 #include "md_p.h"
 #include <ctype.h>
 
@@ -176,7 +177,7 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
 	char devnm[32];
 	char cbuf[400];
 
-	if (!use_udev())
+	if (!udev_is_available())
 		block_udev = 0;
 
 	if (chosen == NULL)
@@ -383,7 +384,7 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
 	 * If we cannot detect udev, we need to make
 	 * devices and links ourselves.
 	 */
-	if (!use_udev()) {
+	if (!udev_is_available()) {
 		/* Make sure 'devname' exists and 'chosen' is a symlink to it */
 		if (lstat(devname, &stb) == 0) {
 			/* Must be the correct device, else error */
@@ -490,7 +491,7 @@ char *find_free_devnm(int use_partitions)
 			continue;
 		if (!conf_name_is_free(devnm))
 			continue;
-		if (!use_udev()) {
+		if (!udev_is_available()) {
 			/* make sure it is new to /dev too, at least as a
 			 * non-standard */
 			dev_t devid = devnm2devid(devnm);
diff --git a/udev.c b/udev.c
new file mode 100644
index 00000000..18055bfc
--- /dev/null
+++ b/udev.c
@@ -0,0 +1,147 @@
+/*
+ * mdadm - manage Linux "md" devices aka RAID arrays.
+ *
+ * Copyright (C) 2022 Mateusz Grzonka <mateusz.grzonka@intel.com>
+ *
+ *    This program is free software; you can redistribute it and/or modify
+ *    it under the terms of the GNU General Public License as published by
+ *    the Free Software Foundation; either version 2 of the License, or
+ *    (at your option) any later version.
+ *
+ *    This program is distributed in the hope that it will be useful,
+ *    but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *    GNU General Public License for more details.
+ *
+ *    You should have received a copy of the GNU General Public License
+ *    along with this program; if not, write to the Free Software
+ *    Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include	"mdadm.h"
+#include	"udev.h"
+#include	"md_p.h"
+#include	"md_u.h"
+#include	<sys/wait.h>
+#include	<signal.h>
+#include	<limits.h>
+#include	<syslog.h>
+#include	<libudev.h>
+
+static struct udev *udev;
+static struct udev_monitor *udev_monitor;
+
+/**
+ * udev_is_available() - Checks for udev in the system.
+ *
+ * Function looks whether udev binaries are available and MDADM_NO_UDEV env defined.
+ *
+ * Return:
+ * true if udev is available,
+ * false if not
+ */
+bool udev_is_available(void)
+{
+	struct stat stb;
+
+	if (stat("/dev/.udev", &stb) != 0 &&
+	    stat("/run/udev", &stb) != 0)
+		return false;
+	if (check_env("MDADM_NO_UDEV") == 1)
+		return false;
+	return true;
+}
+
+/**
+ * udev_initialize() - Initializes udev and udev_monitor structures.
+ *
+ * Function initializes udev, udev_monitor, and sets udev_monitor filter for block devices.
+ *
+ * Return:
+ * UDEV_STATUS_SUCCESS on success
+ * UDEV_STATUS_ERROR on error
+ * UDEV_STATUS_ERROR_NO_UDEV when udev not available
+ */
+enum udev_status udev_initialize(void)
+{
+	if (!udev_is_available()) {
+		pr_err("No udev.\n");
+		return UDEV_STATUS_ERROR_NO_UDEV;
+	}
+
+	udev = udev_new();
+	if (!udev) {
+		pr_err("Cannot initialize udev.\n");
+		return UDEV_STATUS_ERROR;
+	}
+
+	udev_monitor = udev_monitor_new_from_netlink(udev, "udev");
+	if (!udev_monitor) {
+		pr_err("Cannot initialize udev monitor.\n");
+		udev = udev_unref(udev);
+		return UDEV_STATUS_ERROR;
+	}
+
+	if (udev_monitor_filter_add_match_subsystem_devtype(udev_monitor, "block", 0) < 0) {
+		pr_err("Cannot add udev monitor event filter for md devices.\n");
+		udev_release();
+		return UDEV_STATUS_ERROR;
+	}
+	if (udev_monitor_enable_receiving(udev_monitor) < 0) {
+		pr_err("Cannot enable receiving udev events through udev monitor.\n");
+		udev_release();
+		return UDEV_STATUS_ERROR;
+	}
+	atexit(udev_release);
+	return UDEV_STATUS_SUCCESS;
+}
+
+/**
+ * udev_wait_for_events() - Waits for events from udev.
+ * @seconds: Timeout in seconds.
+ *
+ * Function waits udev events, wakes up on event or timeout.
+ *
+ * Return:
+ * UDEV_STATUS_SUCCESS on detected event
+ * UDEV_STATUS_TIMEOUT on timeout
+ * UDEV_STATUS_ERROR on error
+ */
+enum udev_status udev_wait_for_events(int seconds)
+{
+	int fd;
+	fd_set readfds;
+	struct timeval tv;
+	int ret;
+
+	if (!udev || !udev_monitor) {
+		ret = udev_initialize();
+		if (ret != UDEV_STATUS_SUCCESS)
+			return ret;
+	}
+
+	fd = udev_monitor_get_fd(udev_monitor);
+	if (fd < 0) {
+		pr_err("Cannot access file descriptor associated with udev monitor.\n");
+		return UDEV_STATUS_ERROR;
+	}
+
+	FD_ZERO(&readfds);
+	FD_SET(fd, &readfds);
+	tv.tv_sec = seconds;
+	tv.tv_usec = 0;
+
+	if (select(fd + 1, &readfds, NULL, NULL, &tv) > 0 && FD_ISSET(fd, &readfds))
+		if (udev_monitor_receive_device(udev_monitor))
+			return UDEV_STATUS_SUCCESS; /* event detected */
+	return UDEV_STATUS_TIMEOUT;
+}
+
+/**
+ * udev_release() - Drops references of udev and udev_monitor.
+ */
+void udev_release(void)
+{
+	udev_monitor_unref(udev_monitor);
+	udev_unref(udev);
+}
diff --git a/udev.h b/udev.h
new file mode 100644
index 00000000..515366e7
--- /dev/null
+++ b/udev.h
@@ -0,0 +1,36 @@
+/*
+ * mdadm - manage Linux "md" devices aka RAID arrays.
+ *
+ * Copyright (C) 2022 Mateusz Grzonka <mateusz.grzonka@intel.com>
+ *
+ *    This program is free software; you can redistribute it and/or modify
+ *    it under the terms of the GNU General Public License as published by
+ *    the Free Software Foundation; either version 2 of the License, or
+ *    (at your option) any later version.
+ *
+ *    This program is distributed in the hope that it will be useful,
+ *    but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *    GNU General Public License for more details.
+ *
+ *    You should have received a copy of the GNU General Public License
+ *    along with this program; if not, write to the Free Software
+ *    Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#ifndef MONITOR_UDEV_H
+#define MONITOR_UDEV_H
+
+enum udev_status {
+	UDEV_STATUS_ERROR_NO_UDEV = -2,
+	UDEV_STATUS_ERROR,
+	UDEV_STATUS_SUCCESS = 0,
+	UDEV_STATUS_TIMEOUT
+};
+
+bool udev_is_available(void);
+enum udev_status udev_initialize(void);
+enum udev_status udev_wait_for_events(int seconds);
+void udev_release(void);
+
+#endif
-- 
2.26.2

Re: [PATCH 8/9] Mdmonitor: Improve udev event handling

[Coly Li]

On 9/7/22 8:56 PM, Mateusz Grzonka wrote:
> Mdmonitor is waiting for udev queue to become empty.
> Even if the queue becomes empty, udev might still be processing last event.
> However we want to wait and wake up mdmonitor when udev finished
> processing events.
>
> Also, the udev queue interface is considered legacy and should not be
> used outside of udev.
>
> Use udev monitor instead, and wake up mdmonitor on every event triggered
> by udev for md block device.
>
> We need to generate more change events from kernel, because they are
> missing in some situations, for example, when rebuild started.
> This will be addressed in a separate patch.
>
> Move udev specific code into separate functions, and place them in udev.c file.
> Also move use_udev() logic from lib.c into newly created file.
>
> Signed-off-by: Mateusz Grzonka<mateusz.grzonka@intel.com>

It looks good to me.

Acked-by: Coly Li <colyli@suse.de>


Thanks.

Coly Li



> ---
>   Makefile  |   3 +-
>   Manage.c  |   3 +-
>   Monitor.c | 137 +++++++++++++++++++-------------------------------
>   lib.c     |  13 -----
>   mdadm.h   |   1 -
>   mdopen.c  |   7 +--
>   udev.c    | 147 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   udev.h    |  36 +++++++++++++
>   8 files changed, 241 insertions(+), 106 deletions(-)
>   create mode 100644 udev.c
>   create mode 100644 udev.h
>
> diff --git a/Makefile b/Makefile
> index bf126033..ed371006 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -124,6 +124,7 @@ LDLIBS = -ldl
>   # To explicitly disable libudev, set -DNO_LIBUDEV in CXFLAGS
>   ifeq (, $(findstring -DNO_LIBUDEV,  $(CXFLAGS)))
>   	LDLIBS += -ludev
> +	OBJS = udev.o
>   endif
>   
>   INSTALL = /usr/bin/install
> @@ -145,7 +146,7 @@ else
>   	ECHO=:
>   endif
>   
> -OBJS =  mdadm.o config.o policy.o mdstat.o  ReadMe.o uuid.o util.o maps.o lib.o \
> +OBJS += mdadm.o config.o policy.o mdstat.o  ReadMe.o uuid.o util.o maps.o lib.o \
>   	Manage.o Assemble.o Build.o \
>   	Create.o Detail.o Examine.o Grow.o Monitor.o dlink.o Kill.o Query.o \
>   	Incremental.o Dump.o \
> diff --git a/Manage.c b/Manage.c
> index f789e0c1..dae09901 100644
> --- a/Manage.c
> +++ b/Manage.c
> @@ -25,6 +25,7 @@
>   #include "mdadm.h"
>   #include "md_u.h"
>   #include "md_p.h"
> +#include "udev.h"
>   #include <ctype.h>
>   
>   int Manage_ro(char *devname, int fd, int readonly)
> @@ -472,7 +473,7 @@ done:
>   			sysfs_uevent(mdi, "change");
>   	}
>   
> -	if (devnm[0] && use_udev()) {
> +	if (devnm[0] && udev_is_available()) {
>   		struct map_ent *mp = map_by_devnm(&map, devnm);
>   		remove_devices(devnm, mp ? mp->path : NULL);
>   	}
> diff --git a/Monitor.c b/Monitor.c
> index aa79cf6c..dd3ea347 100644
> --- a/Monitor.c
> +++ b/Monitor.c
> @@ -23,18 +23,17 @@
>    */
>   
>   #include	"mdadm.h"
> +#include	"udev.h"
>   #include	"md_p.h"
>   #include	"md_u.h"
>   #include	<sys/wait.h>
>   #include	<limits.h>
>   #include	<syslog.h>
> -#ifndef NO_LIBUDEV
> -#include	<libudev.h>
> -#endif
>   
>   #define TASK_COMM_LEN 16
>   #define EVENT_NAME_MAX 32
>   #define AUTOREBUILD_PID_PATH MDMON_DIR "/autorebuild.pid"
> +#define FALLBACK_DELAY 5
>   
>   struct state {
>   	char *devname;
> @@ -119,12 +118,11 @@ struct event_data {
>   static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist);
>   static int check_array(struct state *st, struct mdstat_ent *mdstat, int increments, char *prefer);
>   static int check_one_sharer(int scan);
> -#ifndef NO_LIBUDEV
> -static int check_udev_activity(void);
> -#endif
>   static void link_containers_with_subarrays(struct state *list);
>   static int make_daemon(char *pidfile);
>   static void try_spare_migration(struct state *statelist);
> +static void wait_for_events(int *delay_for_event, int c_delay);
> +static void wait_for_events_mdstat(int *delay_for_event, int c_delay);
>   static int write_autorebuild_pid(void);
>   
>   int Monitor(struct mddev_dev *devlist,
> @@ -310,32 +308,12 @@ int Monitor(struct mddev_dev *devlist,
>   		if (!new_found) {
>   			if (oneshot)
>   				break;
> -			else if (!anyredundant) {
> +			if (!anyredundant) {
>   				pr_err("No array with redundancy detected, stopping\n");
>   				break;
>   			}
> -			else {
> -#ifndef NO_LIBUDEV
> -				/*
> -				 * Wait for udevd to finish new devices
> -				 * processing.
> -				 */
> -				if (mdstat_wait(delay_for_event) &&
> -				    check_udev_activity())
> -					pr_err("Error while waiting for UDEV to complete new devices processing\n");
> -#else
> -				int wait_result = mdstat_wait(delay_for_event);
> -				/*
> -				 * Give chance to process new device
> -				 */
> -				if (wait_result != 0) {
> -					if (c->delay > 5)
> -						delay_for_event = 5;
> -				} else
> -					delay_for_event = c->delay;
> -#endif
> -				mdstat_close();
> -			}
> +
> +			wait_for_events(&delay_for_event, c->delay);
>   		}
>   		info.test = 0;
>   
> @@ -359,6 +337,49 @@ int Monitor(struct mddev_dev *devlist,
>   	return 0;
>   }
>   
> +/**
> + * wait_for_events() - Waits for events on md devices.
> + * @delay_for_event: pointer to current event delay
> + * @c_delay: delay from config
> + */
> +static void wait_for_events(int *delay_for_event, int c_delay)
> +{
> +#ifndef NO_LIBUDEV
> +	if (udev_is_available()) {
> +		if (udev_wait_for_events(*delay_for_event) == UDEV_STATUS_ERROR)
> +			pr_err("Error while waiting for udev events.\n");
> +		return;
> +	}
> +#endif
> +	wait_for_events_mdstat(delay_for_event, c_delay);
> +}
> +
> +/**
> + * wait_for_events_mdstat() - Waits for events on mdstat.
> + * @delay_for_event: pointer to current event delay
> + * @c_delay: delay from config
> + */
> +static void wait_for_events_mdstat(int *delay_for_event, int c_delay)
> +{
> +	int wait_result = mdstat_wait(*delay_for_event);
> +
> +	if (wait_result < 0) {
> +		pr_err("Error while waiting for events on mdstat.\n");
> +		return;
> +	}
> +
> +	/*
> +	 * Give chance to process new device
> +	 */
> +	if (wait_result != 0) {
> +		if (c_delay > FALLBACK_DELAY)
> +			*delay_for_event = FALLBACK_DELAY;
> +	} else {
> +		*delay_for_event = c_delay;
> +	}
> +	mdstat_close();
> +}
> +
>   static int make_daemon(char *pidfile)
>   {
>   	/* Return:
> @@ -1222,64 +1243,6 @@ static void link_containers_with_subarrays(struct state *list)
>   				}
>   }
>   
> -#ifndef NO_LIBUDEV
> -/* function: check_udev_activity
> - * Description: Function waits for udev to finish
> - * events processing.
> - * Returns:
> - *		1 - detected error while opening udev
> - *		2 - timeout
> - *		0 - successfull completion
> - */
> -static int check_udev_activity(void)
> -{
> -	struct udev *udev = NULL;
> -	struct udev_queue *udev_queue = NULL;
> -	int timeout_cnt = 30;
> -	int rc = 0;
> -
> -	/*
> -	 * In rare cases systemd may not have udevm,
> -	 * in such cases just exit with rc 0
> -	 */
> -	if (!use_udev())
> -		goto out;
> -
> -	udev = udev_new();
> -	if (!udev) {
> -		rc = 1;
> -		goto out;
> -	}
> -
> -	udev_queue = udev_queue_new(udev);
> -	if (!udev_queue) {
> -		rc = 1;
> -		goto out;
> -	}
> -
> -	if (udev_queue_get_queue_is_empty(udev_queue))
> -		goto out;
> -
> -	while (!udev_queue_get_queue_is_empty(udev_queue)) {
> -		sleep(1);
> -
> -		if (timeout_cnt)
> -			timeout_cnt--;
> -		else {
> -			rc = 2;
> -			goto out;
> -		}
> -	}
> -
> -out:
> -	if (udev_queue)
> -		udev_queue_unref(udev_queue);
> -	if (udev)
> -		udev_unref(udev);
> -	return rc;
> -}
> -#endif
> -
>   /* Not really Monitor but ... */
>   int Wait(char *dev)
>   {
> diff --git a/lib.c b/lib.c
> index 7e3e3d47..10a6ae40 100644
> --- a/lib.c
> +++ b/lib.c
> @@ -483,19 +483,6 @@ int check_env(char *name)
>   	return 0;
>   }
>   
> -int use_udev(void)
> -{
> -	static int use = -1;
> -	struct stat stb;
> -
> -	if (use < 0) {
> -		use = ((stat("/dev/.udev", &stb) == 0 ||
> -			stat("/run/udev", &stb) == 0) &&
> -		       check_env("MDADM_NO_UDEV") == 0);
> -	}
> -	return use;
> -}
> -
>   unsigned long GCD(unsigned long a, unsigned long b)
>   {
>   	while (a != b) {
> diff --git a/mdadm.h b/mdadm.h
> index 9183ee70..3c08f826 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -1559,7 +1559,6 @@ extern char *conf_line(FILE *file);
>   extern char *conf_word(FILE *file, int allow_key);
>   extern void print_quoted(char *str);
>   extern void print_escape(char *str);
> -extern int use_udev(void);
>   extern unsigned long GCD(unsigned long a, unsigned long b);
>   extern int conf_name_is_free(char *name);
>   extern int conf_verify_devnames(struct mddev_ident *array_list);
> diff --git a/mdopen.c b/mdopen.c
> index 245be537..53da4d96 100644
> --- a/mdopen.c
> +++ b/mdopen.c
> @@ -23,6 +23,7 @@
>    */
>   
>   #include "mdadm.h"
> +#include "udev.h"
>   #include "md_p.h"
>   #include <ctype.h>
>   
> @@ -176,7 +177,7 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
>   	char devnm[32];
>   	char cbuf[400];
>   
> -	if (!use_udev())
> +	if (!udev_is_available())
>   		block_udev = 0;
>   
>   	if (chosen == NULL)
> @@ -383,7 +384,7 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
>   	 * If we cannot detect udev, we need to make
>   	 * devices and links ourselves.
>   	 */
> -	if (!use_udev()) {
> +	if (!udev_is_available()) {
>   		/* Make sure 'devname' exists and 'chosen' is a symlink to it */
>   		if (lstat(devname, &stb) == 0) {
>   			/* Must be the correct device, else error */
> @@ -490,7 +491,7 @@ char *find_free_devnm(int use_partitions)
>   			continue;
>   		if (!conf_name_is_free(devnm))
>   			continue;
> -		if (!use_udev()) {
> +		if (!udev_is_available()) {
>   			/* make sure it is new to /dev too, at least as a
>   			 * non-standard */
>   			dev_t devid = devnm2devid(devnm);
> diff --git a/udev.c b/udev.c
> new file mode 100644
> index 00000000..18055bfc
> --- /dev/null
> +++ b/udev.c
> @@ -0,0 +1,147 @@
> +/*
> + * mdadm - manage Linux "md" devices aka RAID arrays.
> + *
> + * Copyright (C) 2022 Mateusz Grzonka<mateusz.grzonka@intel.com>
> + *
> + *    This program is free software; you can redistribute it and/or modify
> + *    it under the terms of the GNU General Public License as published by
> + *    the Free Software Foundation; either version 2 of the License, or
> + *    (at your option) any later version.
> + *
> + *    This program is distributed in the hope that it will be useful,
> + *    but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *    GNU General Public License for more details.
> + *
> + *    You should have received a copy of the GNU General Public License
> + *    along with this program; if not, write to the Free Software
> + *    Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#include	"mdadm.h"
> +#include	"udev.h"
> +#include	"md_p.h"
> +#include	"md_u.h"
> +#include	<sys/wait.h>
> +#include	<signal.h>
> +#include	<limits.h>
> +#include	<syslog.h>
> +#include	<libudev.h>
> +
> +static struct udev *udev;
> +static struct udev_monitor *udev_monitor;
> +
> +/**
> + * udev_is_available() - Checks for udev in the system.
> + *
> + * Function looks whether udev binaries are available and MDADM_NO_UDEV env defined.
> + *
> + * Return:
> + * true if udev is available,
> + * false if not
> + */
> +bool udev_is_available(void)
> +{
> +	struct stat stb;
> +
> +	if (stat("/dev/.udev", &stb) != 0 &&
> +	    stat("/run/udev", &stb) != 0)
> +		return false;
> +	if (check_env("MDADM_NO_UDEV") == 1)
> +		return false;
> +	return true;
> +}
> +
> +/**
> + * udev_initialize() - Initializes udev and udev_monitor structures.
> + *
> + * Function initializes udev, udev_monitor, and sets udev_monitor filter for block devices.
> + *
> + * Return:
> + * UDEV_STATUS_SUCCESS on success
> + * UDEV_STATUS_ERROR on error
> + * UDEV_STATUS_ERROR_NO_UDEV when udev not available
> + */
> +enum udev_status udev_initialize(void)
> +{
> +	if (!udev_is_available()) {
> +		pr_err("No udev.\n");
> +		return UDEV_STATUS_ERROR_NO_UDEV;
> +	}
> +
> +	udev = udev_new();
> +	if (!udev) {
> +		pr_err("Cannot initialize udev.\n");
> +		return UDEV_STATUS_ERROR;
> +	}
> +
> +	udev_monitor = udev_monitor_new_from_netlink(udev, "udev");
> +	if (!udev_monitor) {
> +		pr_err("Cannot initialize udev monitor.\n");
> +		udev = udev_unref(udev);
> +		return UDEV_STATUS_ERROR;
> +	}
> +
> +	if (udev_monitor_filter_add_match_subsystem_devtype(udev_monitor, "block", 0) < 0) {
> +		pr_err("Cannot add udev monitor event filter for md devices.\n");
> +		udev_release();
> +		return UDEV_STATUS_ERROR;
> +	}
> +	if (udev_monitor_enable_receiving(udev_monitor) < 0) {
> +		pr_err("Cannot enable receiving udev events through udev monitor.\n");
> +		udev_release();
> +		return UDEV_STATUS_ERROR;
> +	}
> +	atexit(udev_release);
> +	return UDEV_STATUS_SUCCESS;
> +}
> +
> +/**
> + * udev_wait_for_events() - Waits for events from udev.
> + * @seconds: Timeout in seconds.
> + *
> + * Function waits udev events, wakes up on event or timeout.
> + *
> + * Return:
> + * UDEV_STATUS_SUCCESS on detected event
> + * UDEV_STATUS_TIMEOUT on timeout
> + * UDEV_STATUS_ERROR on error
> + */
> +enum udev_status udev_wait_for_events(int seconds)
> +{
> +	int fd;
> +	fd_set readfds;
> +	struct timeval tv;
> +	int ret;
> +
> +	if (!udev || !udev_monitor) {
> +		ret = udev_initialize();
> +		if (ret != UDEV_STATUS_SUCCESS)
> +			return ret;
> +	}
> +
> +	fd = udev_monitor_get_fd(udev_monitor);
> +	if (fd < 0) {
> +		pr_err("Cannot access file descriptor associated with udev monitor.\n");
> +		return UDEV_STATUS_ERROR;
> +	}
> +
> +	FD_ZERO(&readfds);
> +	FD_SET(fd, &readfds);
> +	tv.tv_sec = seconds;
> +	tv.tv_usec = 0;
> +
> +	if (select(fd + 1, &readfds, NULL, NULL, &tv) > 0 && FD_ISSET(fd, &readfds))
> +		if (udev_monitor_receive_device(udev_monitor))
> +			return UDEV_STATUS_SUCCESS; /* event detected */
> +	return UDEV_STATUS_TIMEOUT;
> +}
> +
> +/**
> + * udev_release() - Drops references of udev and udev_monitor.
> + */
> +void udev_release(void)
> +{
> +	udev_monitor_unref(udev_monitor);
> +	udev_unref(udev);
> +}
> diff --git a/udev.h b/udev.h
> new file mode 100644
> index 00000000..515366e7
> --- /dev/null
> +++ b/udev.h
> @@ -0,0 +1,36 @@
> +/*
> + * mdadm - manage Linux "md" devices aka RAID arrays.
> + *
> + * Copyright (C) 2022 Mateusz Grzonka<mateusz.grzonka@intel.com>
> + *
> + *    This program is free software; you can redistribute it and/or modify
> + *    it under the terms of the GNU General Public License as published by
> + *    the Free Software Foundation; either version 2 of the License, or
> + *    (at your option) any later version.
> + *
> + *    This program is distributed in the hope that it will be useful,
> + *    but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *    GNU General Public License for more details.
> + *
> + *    You should have received a copy of the GNU General Public License
> + *    along with this program; if not, write to the Free Software
> + *    Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#ifndef MONITOR_UDEV_H
> +#define MONITOR_UDEV_H
> +
> +enum udev_status {
> +	UDEV_STATUS_ERROR_NO_UDEV = -2,
> +	UDEV_STATUS_ERROR,
> +	UDEV_STATUS_SUCCESS = 0,
> +	UDEV_STATUS_TIMEOUT
> +};
> +
> +bool udev_is_available(void);
> +enum udev_status udev_initialize(void);
> +enum udev_status udev_wait_for_events(int seconds);
> +void udev_release(void);
> +
> +#endif

[PATCH 9/9] udev: Move udev_block() and udev_unblock() into udev.c

[Mateusz Grzonka]

Add kernel style comments and better error handling.

Change-Id: Ib6b8e7725d6739a2d7f7b8801e3403137c9cc402
---
 Create.c |  1 +
 lib.c    | 29 -----------------------------
 mdadm.h  |  2 --
 mdopen.c | 12 ++++++------
 udev.c   | 44 ++++++++++++++++++++++++++++++++++++++++++++
 udev.h   |  2 ++
 6 files changed, 53 insertions(+), 37 deletions(-)

diff --git a/Create.c b/Create.c
index c84c1ac8..e68894ed 100644
--- a/Create.c
+++ b/Create.c
@@ -23,6 +23,7 @@
  */
 
 #include	"mdadm.h"
+#include	"udev.h"
 #include	"md_u.h"
 #include	"md_p.h"
 #include	<ctype.h>
diff --git a/lib.c b/lib.c
index 10a6ae40..5a1afd49 100644
--- a/lib.c
+++ b/lib.c
@@ -174,35 +174,6 @@ char *fd2devnm(int fd)
 	return NULL;
 }
 
-/* When we create a new array, we don't want the content to
- * be immediately examined by udev - it is probably meaningless.
- * So create /run/mdadm/creating-mdXXX and expect that a udev
- * rule will noticed this and act accordingly.
- */
-static char block_path[] = "/run/mdadm/creating-%s";
-static char *unblock_path = NULL;
-void udev_block(char *devnm)
-{
-	int fd;
-	char *path = NULL;
-
-	xasprintf(&path, block_path, devnm);
-	fd = open(path, O_CREAT|O_RDWR, 0600);
-	if (fd >= 0) {
-		close(fd);
-		unblock_path = path;
-	} else
-		free(path);
-}
-
-void udev_unblock(void)
-{
-	if (unblock_path)
-		unlink(unblock_path);
-	free(unblock_path);
-	unblock_path = NULL;
-}
-
 /*
  * convert a major/minor pair for a block device into a name in /dev, if possible.
  * On the first call, walk /dev collecting name.
diff --git a/mdadm.h b/mdadm.h
index 3c08f826..3f81cce6 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1662,8 +1662,6 @@ extern char *stat2kname(struct stat *st);
 extern char *fd2kname(int fd);
 extern char *stat2devnm(struct stat *st);
 extern char *fd2devnm(int fd);
-extern void udev_block(char *devnm);
-extern void udev_unblock(void);
 
 extern int in_initrd(void);
 
diff --git a/mdopen.c b/mdopen.c
index 53da4d96..93193739 100644
--- a/mdopen.c
+++ b/mdopen.c
@@ -336,8 +336,8 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
 	devnm[0] = 0;
 	if (num < 0 && cname && ci->names) {
 		sprintf(devnm, "md_%s", cname);
-		if (block_udev)
-			udev_block(devnm);
+		if (block_udev && udev_block(devnm) != UDEV_STATUS_SUCCESS)
+			return -1;
 		if (!create_named_array(devnm)) {
 			devnm[0] = 0;
 			udev_unblock();
@@ -345,8 +345,8 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
 	}
 	if (num >= 0) {
 		sprintf(devnm, "md%d", num);
-		if (block_udev)
-			udev_block(devnm);
+		if (block_udev && udev_block(devnm) != UDEV_STATUS_SUCCESS)
+			return -1;
 		if (!create_named_array(devnm)) {
 			devnm[0] = 0;
 			udev_unblock();
@@ -369,8 +369,8 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
 				return -1;
 			}
 		}
-		if (block_udev)
-			udev_block(devnm);
+		if (block_udev && udev_block(devnm) != UDEV_STATUS_SUCCESS)
+			return -1;
 	}
 
 	sprintf(devname, "/dev/%s", devnm);
diff --git a/udev.c b/udev.c
index 18055bfc..d6e3d7b5 100644
--- a/udev.c
+++ b/udev.c
@@ -30,6 +30,7 @@
 
 static struct udev *udev;
 static struct udev_monitor *udev_monitor;
+static char *unblock_path;
 
 /**
  * udev_is_available() - Checks for udev in the system.
@@ -145,3 +146,46 @@ void udev_release(void)
 	udev_monitor_unref(udev_monitor);
 	udev_unref(udev);
 }
+
+/**
+ * udev_block() - Block udev from examining newly created arrays.
+ *
+ * When array is created, we don't want udev to examine it immediately.
+ * Function creates /run/mdadm/creating-mdXXX and expects that udev rule
+ * will notice it and act accordingly.
+ *
+ * Return:
+ * UDEV_STATUS_SUCCESS when successfully blocked udev
+ * UDEV_STATUS_ERROR on error
+ */
+enum udev_status udev_block(char *devnm)
+{
+	int fd;
+	char *path = xcalloc(1, BUFSIZ);
+
+	snprintf(path, BUFSIZ, "/run/mdadm/creating-%s", devnm);
+
+	fd = open(path, O_CREAT | O_RDWR, 0600);
+	if (!is_fd_valid(fd)) {
+		pr_err("Cannot block udev, error creating blocking file.\n");
+		pr_err("%s: %s\n", strerror(errno), path);
+		free(path);
+		return UDEV_STATUS_ERROR;
+	}
+
+	close(fd);
+	unblock_path = path;
+	return UDEV_STATUS_SUCCESS;
+}
+
+/**
+ * udev_unblock() - Unblock udev.
+ */
+void udev_unblock(void)
+{
+	if (unblock_path)
+		unlink(unblock_path);
+	free(unblock_path);
+	unblock_path = NULL;
+}
+
diff --git a/udev.h b/udev.h
index 515366e7..e4da29cc 100644
--- a/udev.h
+++ b/udev.h
@@ -32,5 +32,7 @@ bool udev_is_available(void);
 enum udev_status udev_initialize(void);
 enum udev_status udev_wait_for_events(int seconds);
 void udev_release(void);
+enum udev_status udev_block(char *devnm);
+void udev_unblock(void);
 
 #endif
-- 
2.26.2

Re: [PATCH 9/9] udev: Move udev_block() and udev_unblock() into udev.c

[Paul Menzel]

Dear Mateusz,


Thank you for your patches.

Am 07.09.22 um 14:56 schrieb Mateusz Grzonka:
> Add kernel style comments and better error handling.

Please add a note, why the move is useful even if `lib.c` was just used 
for historic reasons.

> Change-Id: Ib6b8e7725d6739a2d7f7b8801e3403137c9cc402

The tag seems to be a leftover – or share the Gerrit URL.

[…]


Kind regards,

Paul

Re: [PATCH 9/9] udev: Move udev_block() and udev_unblock() into udev.c

[Coly Li]

On 9/7/22 8:56 PM, Mateusz Grzonka wrote:
> Add kernel style comments and better error handling.
>
> Change-Id: Ib6b8e7725d6739a2d7f7b8801e3403137c9cc402

What does the above line mean?


BTW, for the code, it looks fine to me. I want to ack this patch after 
the Change-Id in the commit log explained.

Acked-by: Coly Li <colyli@suse.de>

Thanks.

Coly Li


> ---
>   Create.c |  1 +
>   lib.c    | 29 -----------------------------
>   mdadm.h  |  2 --
>   mdopen.c | 12 ++++++------
>   udev.c   | 44 ++++++++++++++++++++++++++++++++++++++++++++
>   udev.h   |  2 ++
>   6 files changed, 53 insertions(+), 37 deletions(-)
>
> diff --git a/Create.c b/Create.c
> index c84c1ac8..e68894ed 100644
> --- a/Create.c
> +++ b/Create.c
> @@ -23,6 +23,7 @@
>    */
>   
>   #include	"mdadm.h"
> +#include	"udev.h"
>   #include	"md_u.h"
>   #include	"md_p.h"
>   #include	<ctype.h>
> diff --git a/lib.c b/lib.c
> index 10a6ae40..5a1afd49 100644
> --- a/lib.c
> +++ b/lib.c
> @@ -174,35 +174,6 @@ char *fd2devnm(int fd)
>   	return NULL;
>   }
>   
> -/* When we create a new array, we don't want the content to
> - * be immediately examined by udev - it is probably meaningless.
> - * So create /run/mdadm/creating-mdXXX and expect that a udev
> - * rule will noticed this and act accordingly.
> - */
> -static char block_path[] = "/run/mdadm/creating-%s";
> -static char *unblock_path = NULL;
> -void udev_block(char *devnm)
> -{
> -	int fd;
> -	char *path = NULL;
> -
> -	xasprintf(&path, block_path, devnm);
> -	fd = open(path, O_CREAT|O_RDWR, 0600);
> -	if (fd >= 0) {
> -		close(fd);
> -		unblock_path = path;
> -	} else
> -		free(path);
> -}
> -
> -void udev_unblock(void)
> -{
> -	if (unblock_path)
> -		unlink(unblock_path);
> -	free(unblock_path);
> -	unblock_path = NULL;
> -}
> -
>   /*
>    * convert a major/minor pair for a block device into a name in /dev, if possible.
>    * On the first call, walk /dev collecting name.
> diff --git a/mdadm.h b/mdadm.h
> index 3c08f826..3f81cce6 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -1662,8 +1662,6 @@ extern char *stat2kname(struct stat *st);
>   extern char *fd2kname(int fd);
>   extern char *stat2devnm(struct stat *st);
>   extern char *fd2devnm(int fd);
> -extern void udev_block(char *devnm);
> -extern void udev_unblock(void);
>   
>   extern int in_initrd(void);
>   
> diff --git a/mdopen.c b/mdopen.c
> index 53da4d96..93193739 100644
> --- a/mdopen.c
> +++ b/mdopen.c
> @@ -336,8 +336,8 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
>   	devnm[0] = 0;
>   	if (num < 0 && cname && ci->names) {
>   		sprintf(devnm, "md_%s", cname);
> -		if (block_udev)
> -			udev_block(devnm);
> +		if (block_udev && udev_block(devnm) != UDEV_STATUS_SUCCESS)
> +			return -1;
>   		if (!create_named_array(devnm)) {
>   			devnm[0] = 0;
>   			udev_unblock();
> @@ -345,8 +345,8 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
>   	}
>   	if (num >= 0) {
>   		sprintf(devnm, "md%d", num);
> -		if (block_udev)
> -			udev_block(devnm);
> +		if (block_udev && udev_block(devnm) != UDEV_STATUS_SUCCESS)
> +			return -1;
>   		if (!create_named_array(devnm)) {
>   			devnm[0] = 0;
>   			udev_unblock();
> @@ -369,8 +369,8 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
>   				return -1;
>   			}
>   		}
> -		if (block_udev)
> -			udev_block(devnm);
> +		if (block_udev && udev_block(devnm) != UDEV_STATUS_SUCCESS)
> +			return -1;
>   	}
>   
>   	sprintf(devname, "/dev/%s", devnm);
> diff --git a/udev.c b/udev.c
> index 18055bfc..d6e3d7b5 100644
> --- a/udev.c
> +++ b/udev.c
> @@ -30,6 +30,7 @@
>   
>   static struct udev *udev;
>   static struct udev_monitor *udev_monitor;
> +static char *unblock_path;
>   
>   /**
>    * udev_is_available() - Checks for udev in the system.
> @@ -145,3 +146,46 @@ void udev_release(void)
>   	udev_monitor_unref(udev_monitor);
>   	udev_unref(udev);
>   }
> +
> +/**
> + * udev_block() - Block udev from examining newly created arrays.
> + *
> + * When array is created, we don't want udev to examine it immediately.
> + * Function creates /run/mdadm/creating-mdXXX and expects that udev rule
> + * will notice it and act accordingly.
> + *
> + * Return:
> + * UDEV_STATUS_SUCCESS when successfully blocked udev
> + * UDEV_STATUS_ERROR on error
> + */
> +enum udev_status udev_block(char *devnm)
> +{
> +	int fd;
> +	char *path = xcalloc(1, BUFSIZ);
> +
> +	snprintf(path, BUFSIZ, "/run/mdadm/creating-%s", devnm);
> +
> +	fd = open(path, O_CREAT | O_RDWR, 0600);
> +	if (!is_fd_valid(fd)) {
> +		pr_err("Cannot block udev, error creating blocking file.\n");
> +		pr_err("%s: %s\n", strerror(errno), path);
> +		free(path);
> +		return UDEV_STATUS_ERROR;
> +	}
> +
> +	close(fd);
> +	unblock_path = path;
> +	return UDEV_STATUS_SUCCESS;
> +}
> +
> +/**
> + * udev_unblock() - Unblock udev.
> + */
> +void udev_unblock(void)
> +{
> +	if (unblock_path)
> +		unlink(unblock_path);
> +	free(unblock_path);
> +	unblock_path = NULL;
> +}
> +
> diff --git a/udev.h b/udev.h
> index 515366e7..e4da29cc 100644
> --- a/udev.h
> +++ b/udev.h
> @@ -32,5 +32,7 @@ bool udev_is_available(void);
>   enum udev_status udev_initialize(void);
>   enum udev_status udev_wait_for_events(int seconds);
>   void udev_release(void);
> +enum udev_status udev_block(char *devnm);
> +void udev_unblock(void);
>   
>   #endif

Re: [PATCH 0/9] Mdmonitor refactor and udev event handling improvements

[Coly Li]

On 9/7/22 8:56 PM, Mateusz Grzonka wrote:
> Along the way we observed many problems with current approach to event handling in mdmonitor.
> It frequently doesn't report Fail and DeviceDisappeared events.
> It's due to time races with udev, and too long delay in some cases.
> While there was a patch intending to address time races with udev, it didn't remove them completely.
> This patch series presents alternative approach, where mdmonitor wakes up on udev events, so that
> there should be no more conflicts with udev we saw before.
>
> Additionally some code quality improvements were done, to make the code more maintainable.

Hi Mateusz,

I am not familiar with the udev stuffs, and take some time to review all 
this series. Overall I am fine with this series, except for the code 
comment style like,
/**
  * It seems not md kernel code comment style
  */


And I leave my comments in each patch, please check them.

Thanks.


Coly Li



>
> Mateusz Grzonka (9):
>    Mdmonitor: Split alert() into separate functions
>    Mdmonitor: Make alert_info global
>    Mdmonitor: Pass events to alert() using enums instead of strings
>    Mdmonitor: Add helper functions
>    Add helpers to determine whether directories or files are soft links
>    Mdmonitor: Refactor write_autorebuild_pid()
>    Mdmonitor: Refactor check_one_sharer() for better error handling
>    Mdmonitor: Improve udev event handling
>    udev: Move udev_block() and udev_unblock() into udev.c
>
>   Create.c  |   1 +
>   Makefile  |   3 +-
>   Manage.c  |   3 +-
>   Monitor.c | 707 ++++++++++++++++++++++++++++++++----------------------
>   lib.c     |  42 ----
>   mdadm.h   |   6 +-
>   mdopen.c  |  19 +-
>   udev.c    | 191 +++++++++++++++
>   udev.h    |  38 +++
>   util.c    |  46 ++++
>   10 files changed, 713 insertions(+), 343 deletions(-)
>   create mode 100644 udev.c
>   create mode 100644 udev.h
>