[PATCH] Monitor: containers don't have the same sysfs properties as arrays

[PATCH] Monitor: containers don't have the same sysfs properties as arrays

[Mariusz Tkaczyk]

GET_MISMATCH option doesn't exist for containers so sysfs_read fails if
this information is requested. Set options according to the device using
information from /proc/mdstat.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@intel.com>
Reviewed-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
---
 Monitor.c | 50 ++++++++++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 22 deletions(-)

diff --git a/Monitor.c b/Monitor.c
index 48c451c..8d753ca 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -459,22 +459,41 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 	mdu_array_info_t array;
 	struct mdstat_ent *mse = NULL, *mse2;
 	char *dev = st->devname;
-	int fd;
+	int fd = -1;
 	int i;
 	int remaining_disks;
 	int last_disk;
 	int new_array = 0;
-	int retval;
+	int retval = 0;
+	int is_container;
+	unsigned long array_only_flags = 0;
 
 	if (test)
 		alert("TestMessage", dev, NULL, ainfo);
 
-	retval = 0;
-
 	fd = open(dev, O_RDONLY);
 	if (fd < 0)
 		goto disappeared;
 
+	if (st->devnm[0] == 0)
+		strcpy(st->devnm, fd2devnm(fd));
+
+	for (mse2 = mdstat; mse2; mse2 = mse2->next)
+		if (strcmp(mse2->devnm, st->devnm) == 0) {
+			mse2->devnm[0] = 0; /* flag it as "used" */
+			mse = mse2;
+		}
+
+	if (!mse) {
+		/* duplicated array in statelist
+		 * or re-created after reading mdstat
+		 */
+		st->err++;
+		goto out;
+	}
+
+	is_container = mse->level == NULL;
+
 	if (!md_array_active(fd))
 		goto disappeared;
 
@@ -482,11 +501,11 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 	if (md_get_array_info(fd, &array) < 0)
 		goto disappeared;
 
-	if (st->devnm[0] == 0)
-		strcpy(st->devnm, fd2devnm(fd));
+	array_only_flags |= !is_container ? GET_MISMATCH : 0;
+
+	sra = sysfs_read(-1, st->devnm, GET_LEVEL | GET_DISKS | GET_DEVS |
+			GET_STATE | array_only_flags);
 
-	sra = sysfs_read(-1, st->devnm, GET_LEVEL | GET_DISKS | GET_MISMATCH |
-			 GET_DEVS | GET_STATE);
 	if (!sra)
 		goto disappeared;
 
@@ -500,19 +519,6 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
 		goto out;
 	}
 
-	for (mse2 = mdstat; mse2; mse2 = mse2->next)
-		if (strcmp(mse2->devnm, st->devnm) == 0) {
-			mse2->devnm[0] = 0; /* flag it as "used" */
-			mse = mse2;
-		}
-
-	if (!mse) {
-		/* duplicated array in statelist
-		 * or re-created after reading mdstat*/
-		st->err++;
-		close(fd);
-		goto out;
-	}
 	/* this array is in /proc/mdstat */
 	if (array.utime == 0)
 		/* external arrays don't update utime, so
@@ -653,7 +659,7 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
  out:
 	if (sra)
 		sysfs_free(sra);
-	if (fd > 0)
+	if (fd >= 0)
 		close(fd);
 	return retval;
 
-- 
1.8.3.1

Re: [PATCH] Monitor: containers don't have the same sysfs properties as arrays

[Jes Sorensen]

On 07/21/2017 08:15 AM, Mariusz Tkaczyk wrote:
> GET_MISMATCH option doesn't exist for containers so sysfs_read fails if
> this information is requested. Set options according to the device using
> information from /proc/mdstat.
> 
> Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@intel.com>
> Reviewed-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
> ---
>   Monitor.c | 50 ++++++++++++++++++++++++++++----------------------
>   1 file changed, 28 insertions(+), 22 deletions(-)

Hi Mariusz,

Sorry for the late response, I am currently traveling so there have been 
some delays.

I am fine with the principle of this patch, but I have a few nits:

> diff --git a/Monitor.c b/Monitor.c
> index 48c451c..8d753ca 100644
> --- a/Monitor.c
> +++ b/Monitor.c
> @@ -459,22 +459,41 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   	mdu_array_info_t array;
>   	struct mdstat_ent *mse = NULL, *mse2;
>   	char *dev = st->devname;
> -	int fd;
> +	int fd = -1;
>   	int i;
>   	int remaining_disks;
>   	int last_disk;
>   	int new_array = 0;
> -	int retval;
> +	int retval = 0;
> +	int is_container;
> +	unsigned long array_only_flags = 0;

You initialize array_only_flags here.

[snip]

> @@ -482,11 +501,11 @@ static int check_array(struct state *st, struct mdstat_ent *mdstat,
>   	if (md_get_array_info(fd, &array) < 0)
>   		goto disappeared;
>   
> -	if (st->devnm[0] == 0)
> -		strcpy(st->devnm, fd2devnm(fd));
> +	array_only_flags |= !is_container ? GET_MISMATCH : 0;
> +

... and again here.

Could we try and avoid that obfuscated approach and simply do

	if (!is_container)
		array_only_flags |= GET_MISMATCH

That would avoid the double initialization and is a lot more readable.

Second issue, you are moving to global initialization for a number of 
variables. That is pretty much always bad as it hides things - please 
avoid that unless absolutely necessary.

Thanks,
Jes