GET_ARRAY_INFO assumptions?

GET_ARRAY_INFO assumptions?

[Jes Sorensen]

Hi Neil,

Looking at trying to phase out the ioctl usage, I am trying to introduce 
a helper for the 'is the array valid' situation.

Now looking at places like Incremental.c (around like 557 in my current 
tree):
	/* 7b/ if yes, */
	/* - if number of OK devices match expected, or -R and there */
	/*             are enough, */
	/*   + add any bitmap file  */
	/*   + start the array (auto-readonly). */

	if (md_get_array_info(mdfd, &ainf) == 0) {
		if (c->export) {
			printf("MD_STARTED=already\n");
		} else if (c->verbose >= 0)
			pr_err("%s attached to %s which is already active.\n",
			       devname, chosen_name);
		rv = 0;
		goto out_unlock;
	}

I am wondering if there are any side effects/assumptions about 
GET_ARRAY_INFO that I am not considering? Basically I am making the 
assumption that if /sys/block/md<X>/md exists, the array is valid.

The code in Incremental.c already deals with sysfs higher up in the 
code, so I guess the question is if the above test is even relevant anymore?

Alternative, do we need export a new state in sysfs 'running'?

Thoughts?

Jes

diff --git a/util.c b/util.c
index a695c45..99ed015 100644
--- a/util.c
+++ b/util.c
@@ -200,6 +200,22 @@ out:
         return ret;
  }

+int md_valid_array(int fd)
+{
+       struct mdinfo *sra;
+       struct mdu_array_info_s array;
+       int ret;
+
+       sra = xcalloc(1, sizeof(*sra));
+       ret = sysfs_init(sra, fd, NULL);
+       free(sra);
+
+       if (ret)
+               ret = ioctl(fd, GET_ARRAY_INFO, &array);
+
+       return !ret;
+}
+
  /*
   * Get array info from the kernel. Longer term we want to deprecate the
   * ioctl and get it from sysfs.

Re: GET_ARRAY_INFO assumptions?

[Shaohua Li]

On Thu, Apr 13, 2017 at 01:50:06PM -0400, Jes Sorensen wrote:
> Hi Neil,
> 
> Looking at trying to phase out the ioctl usage, I am trying to introduce a
> helper for the 'is the array valid' situation.
> 
> Now looking at places like Incremental.c (around like 557 in my current
> tree):
> 	/* 7b/ if yes, */
> 	/* - if number of OK devices match expected, or -R and there */
> 	/*             are enough, */
> 	/*   + add any bitmap file  */
> 	/*   + start the array (auto-readonly). */
> 
> 	if (md_get_array_info(mdfd, &ainf) == 0) {
> 		if (c->export) {
> 			printf("MD_STARTED=already\n");
> 		} else if (c->verbose >= 0)
> 			pr_err("%s attached to %s which is already active.\n",
> 			       devname, chosen_name);
> 		rv = 0;
> 		goto out_unlock;
> 	}
> 
> I am wondering if there are any side effects/assumptions about
> GET_ARRAY_INFO that I am not considering? Basically I am making the
> assumption that if /sys/block/md<X>/md exists, the array is valid.

what does 'valid' really mean? md<x>/md exists after a md device is allocated,
the md device might not have any under layer disks bound yet.

> The code in Incremental.c already deals with sysfs higher up in the code, so
> I guess the question is if the above test is even relevant anymore?
> 
> Alternative, do we need export a new state in sysfs 'running'?

I'd assume 'running' means the md device has a personality attached. See
array_state_show(), !running == 'clear' or 'inactive'.


Thanks,
Shaohua
> Thoughts?
> 
> Jes
> 
> diff --git a/util.c b/util.c
> index a695c45..99ed015 100644
> --- a/util.c
> +++ b/util.c
> @@ -200,6 +200,22 @@ out:
>         return ret;
>  }
> 
> +int md_valid_array(int fd)
> +{
> +       struct mdinfo *sra;
> +       struct mdu_array_info_s array;
> +       int ret;
> +
> +       sra = xcalloc(1, sizeof(*sra));
> +       ret = sysfs_init(sra, fd, NULL);
> +       free(sra);
> +
> +       if (ret)
> +               ret = ioctl(fd, GET_ARRAY_INFO, &array);
> +
> +       return !ret;
> +}
> +
>  /*
>   * Get array info from the kernel. Longer term we want to deprecate the
>   * ioctl and get it from sysfs.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: GET_ARRAY_INFO assumptions?

[Jes Sorensen]

On 04/13/2017 04:37 PM, Shaohua Li wrote:
> On Thu, Apr 13, 2017 at 01:50:06PM -0400, Jes Sorensen wrote:
>> Hi Neil,
>>
>> Looking at trying to phase out the ioctl usage, I am trying to introduce a
>> helper for the 'is the array valid' situation.
>>
>> Now looking at places like Incremental.c (around like 557 in my current
>> tree):
>> 	/* 7b/ if yes, */
>> 	/* - if number of OK devices match expected, or -R and there */
>> 	/*             are enough, */
>> 	/*   + add any bitmap file  */
>> 	/*   + start the array (auto-readonly). */
>>
>> 	if (md_get_array_info(mdfd, &ainf) == 0) {
>> 		if (c->export) {
>> 			printf("MD_STARTED=already\n");
>> 		} else if (c->verbose >= 0)
>> 			pr_err("%s attached to %s which is already active.\n",
>> 			       devname, chosen_name);
>> 		rv = 0;
>> 		goto out_unlock;
>> 	}
>>
>> I am wondering if there are any side effects/assumptions about
>> GET_ARRAY_INFO that I am not considering? Basically I am making the
>> assumption that if /sys/block/md<X>/md exists, the array is valid.
>
> what does 'valid' really mean? md<x>/md exists after a md device is allocated,
> the md device might not have any under layer disks bound yet.
>
>> The code in Incremental.c already deals with sysfs higher up in the code, so
>> I guess the question is if the above test is even relevant anymore?
>>
>> Alternative, do we need export a new state in sysfs 'running'?
>
> I'd assume 'running' means the md device has a personality attached. See
> array_state_show(), !running == 'clear' or 'inactive'.

Good point, I guess what I am trying to figure out is what is assumed 
when ioctl(GET_ARRAY_INFO) returns 0 and how do we map it to sysfs?

Jes

Re: GET_ARRAY_INFO assumptions?

[Jes Sorensen]

On 04/13/2017 05:06 PM, Jes Sorensen wrote:
> On 04/13/2017 04:37 PM, Shaohua Li wrote:
>> On Thu, Apr 13, 2017 at 01:50:06PM -0400, Jes Sorensen wrote:
>>> Hi Neil,
>>>
>>> Looking at trying to phase out the ioctl usage, I am trying to
>>> introduce a
>>> helper for the 'is the array valid' situation.
>>>
>>> Now looking at places like Incremental.c (around like 557 in my current
>>> tree):
>>>     /* 7b/ if yes, */
>>>     /* - if number of OK devices match expected, or -R and there */
>>>     /*             are enough, */
>>>     /*   + add any bitmap file  */
>>>     /*   + start the array (auto-readonly). */
>>>
>>>     if (md_get_array_info(mdfd, &ainf) == 0) {
>>>         if (c->export) {
>>>             printf("MD_STARTED=already\n");
>>>         } else if (c->verbose >= 0)
>>>             pr_err("%s attached to %s which is already active.\n",
>>>                    devname, chosen_name);
>>>         rv = 0;
>>>         goto out_unlock;
>>>     }
>>>
>>> I am wondering if there are any side effects/assumptions about
>>> GET_ARRAY_INFO that I am not considering? Basically I am making the
>>> assumption that if /sys/block/md<X>/md exists, the array is valid.
>>
>> what does 'valid' really mean? md<x>/md exists after a md device is
>> allocated,
>> the md device might not have any under layer disks bound yet.
>>
>>> The code in Incremental.c already deals with sysfs higher up in the
>>> code, so
>>> I guess the question is if the above test is even relevant anymore?
>>>
>>> Alternative, do we need export a new state in sysfs 'running'?
>>
>> I'd assume 'running' means the md device has a personality attached. See
>> array_state_show(), !running == 'clear' or 'inactive'.
>
> Good point, I guess what I am trying to figure out is what is assumed
> when ioctl(GET_ARRAY_INFO) returns 0 and how do we map it to sysfs?

Looking some more at this, it may be simpler than I thought. How about 
this approach (only compile tested):

int md_array_active(int fd)
{
	struct mdinfo *sra;
	struct mdu_array_info_s array;
	int ret;

	sra = sysfs_read(fd, NULL, GET_VERSION | GET_DISKS);
	if (sra) {
		if (!sra->array.raid_disks &&
		    !(sra->array.major_version == -1 &&
		      sra->array.minor_version == -2))
			ret = -ENODEV;
		else
			ret = 0;

		free(sra);
	} else {
		ret = ioctl(fd, GET_ARRAY_INFO, &array);
	}

	return !ret;
}

Note 'major = -1 && minor = -2' is sysfs_read's way of saying 'external'.

This pretty much mimics what the kernel does in the ioctl handler for 
GET_ARRAY_INFO:

	case GET_ARRAY_INFO:
		if (!mddev->raid_disks && !mddev->external)
			err = -ENODEV;
		else
			err = get_array_info(mddev, argp);
		goto out;

What do you think?

Cheers,
Jes

Re: GET_ARRAY_INFO assumptions?

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3522 bytes --]

On Fri, Apr 14 2017, Jes Sorensen wrote:

> On 04/13/2017 05:06 PM, Jes Sorensen wrote:
>> On 04/13/2017 04:37 PM, Shaohua Li wrote:
>>> On Thu, Apr 13, 2017 at 01:50:06PM -0400, Jes Sorensen wrote:
>>>> Hi Neil,
>>>>
>>>> Looking at trying to phase out the ioctl usage, I am trying to
>>>> introduce a
>>>> helper for the 'is the array valid' situation.
>>>>
>>>> Now looking at places like Incremental.c (around like 557 in my current
>>>> tree):
>>>>     /* 7b/ if yes, */
>>>>     /* - if number of OK devices match expected, or -R and there */
>>>>     /*             are enough, */
>>>>     /*   + add any bitmap file  */
>>>>     /*   + start the array (auto-readonly). */
>>>>
>>>>     if (md_get_array_info(mdfd, &ainf) == 0) {
>>>>         if (c->export) {
>>>>             printf("MD_STARTED=already\n");
>>>>         } else if (c->verbose >= 0)
>>>>             pr_err("%s attached to %s which is already active.\n",
>>>>                    devname, chosen_name);
>>>>         rv = 0;
>>>>         goto out_unlock;
>>>>     }
>>>>
>>>> I am wondering if there are any side effects/assumptions about
>>>> GET_ARRAY_INFO that I am not considering? Basically I am making the
>>>> assumption that if /sys/block/md<X>/md exists, the array is valid.
>>>
>>> what does 'valid' really mean? md<x>/md exists after a md device is
>>> allocated,
>>> the md device might not have any under layer disks bound yet.
>>>
>>>> The code in Incremental.c already deals with sysfs higher up in the
>>>> code, so
>>>> I guess the question is if the above test is even relevant anymore?
>>>>
>>>> Alternative, do we need export a new state in sysfs 'running'?
>>>
>>> I'd assume 'running' means the md device has a personality attached. See
>>> array_state_show(), !running == 'clear' or 'inactive'.
>>
>> Good point, I guess what I am trying to figure out is what is assumed
>> when ioctl(GET_ARRAY_INFO) returns 0 and how do we map it to sysfs?
>
> Looking some more at this, it may be simpler than I thought. How about 
> this approach (only compile tested):
>
> int md_array_active(int fd)
> {
> 	struct mdinfo *sra;
> 	struct mdu_array_info_s array;
> 	int ret;
>
> 	sra = sysfs_read(fd, NULL, GET_VERSION | GET_DISKS);
> 	if (sra) {
> 		if (!sra->array.raid_disks &&
> 		    !(sra->array.major_version == -1 &&
> 		      sra->array.minor_version == -2))
> 			ret = -ENODEV;
> 		else
> 			ret = 0;
>
> 		free(sra);
> 	} else {
> 		ret = ioctl(fd, GET_ARRAY_INFO, &array);
> 	}
>
> 	return !ret;
> }
>
> Note 'major = -1 && minor = -2' is sysfs_read's way of saying 'external'.
>
> This pretty much mimics what the kernel does in the ioctl handler for 
> GET_ARRAY_INFO:
>
> 	case GET_ARRAY_INFO:
> 		if (!mddev->raid_disks && !mddev->external)
> 			err = -ENODEV;
> 		else
> 			err = get_array_info(mddev, argp);
> 		goto out;
>
> What do you think?

I think that it accurately mimics what the current code does.
I'm not sure that is what we really want.
For testing in Incremental.c if an array is "active" we really
should be testing more than "raid_disks != 0".
We should be testing, as Shaohua suggested, if
array_state != 'clear' or 'inactive'.
You cannot get that info through the ioctl interface, so I suppose
I decided the current test was 'close enough'.
If we are going to stop supported kernels that don't have (e.g.)
array_state, then we should really fo the right thing and test
array_state.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: GET_ARRAY_INFO assumptions?

[Jes Sorensen]

On 04/17/2017 07:48 PM, NeilBrown wrote:
> On Fri, Apr 14 2017, Jes Sorensen wrote:
>
>> On 04/13/2017 05:06 PM, Jes Sorensen wrote:
>>> On 04/13/2017 04:37 PM, Shaohua Li wrote:
>>>> On Thu, Apr 13, 2017 at 01:50:06PM -0400, Jes Sorensen wrote:
>>>>> Hi Neil,
>>>>>
>>>>> Looking at trying to phase out the ioctl usage, I am trying to
>>>>> introduce a
>>>>> helper for the 'is the array valid' situation.
>>>>>
>>>>> Now looking at places like Incremental.c (around like 557 in my current
>>>>> tree):
>>>>>     /* 7b/ if yes, */
>>>>>     /* - if number of OK devices match expected, or -R and there */
>>>>>     /*             are enough, */
>>>>>     /*   + add any bitmap file  */
>>>>>     /*   + start the array (auto-readonly). */
>>>>>
>>>>>     if (md_get_array_info(mdfd, &ainf) == 0) {
>>>>>         if (c->export) {
>>>>>             printf("MD_STARTED=already\n");
>>>>>         } else if (c->verbose >= 0)
>>>>>             pr_err("%s attached to %s which is already active.\n",
>>>>>                    devname, chosen_name);
>>>>>         rv = 0;
>>>>>         goto out_unlock;
>>>>>     }
>>>>>
>>>>> I am wondering if there are any side effects/assumptions about
>>>>> GET_ARRAY_INFO that I am not considering? Basically I am making the
>>>>> assumption that if /sys/block/md<X>/md exists, the array is valid.
>>>>
>>>> what does 'valid' really mean? md<x>/md exists after a md device is
>>>> allocated,
>>>> the md device might not have any under layer disks bound yet.
>>>>
>>>>> The code in Incremental.c already deals with sysfs higher up in the
>>>>> code, so
>>>>> I guess the question is if the above test is even relevant anymore?
>>>>>
>>>>> Alternative, do we need export a new state in sysfs 'running'?
>>>>
>>>> I'd assume 'running' means the md device has a personality attached. See
>>>> array_state_show(), !running == 'clear' or 'inactive'.
>>>
>>> Good point, I guess what I am trying to figure out is what is assumed
>>> when ioctl(GET_ARRAY_INFO) returns 0 and how do we map it to sysfs?
>>
>> Looking some more at this, it may be simpler than I thought. How about
>> this approach (only compile tested):
>>
>> int md_array_active(int fd)
>> {
>> 	struct mdinfo *sra;
>> 	struct mdu_array_info_s array;
>> 	int ret;
>>
>> 	sra = sysfs_read(fd, NULL, GET_VERSION | GET_DISKS);
>> 	if (sra) {
>> 		if (!sra->array.raid_disks &&
>> 		    !(sra->array.major_version == -1 &&
>> 		      sra->array.minor_version == -2))
>> 			ret = -ENODEV;
>> 		else
>> 			ret = 0;
>>
>> 		free(sra);
>> 	} else {
>> 		ret = ioctl(fd, GET_ARRAY_INFO, &array);
>> 	}
>>
>> 	return !ret;
>> }
>>
>> Note 'major = -1 && minor = -2' is sysfs_read's way of saying 'external'.
>>
>> This pretty much mimics what the kernel does in the ioctl handler for
>> GET_ARRAY_INFO:
>>
>> 	case GET_ARRAY_INFO:
>> 		if (!mddev->raid_disks && !mddev->external)
>> 			err = -ENODEV;
>> 		else
>> 			err = get_array_info(mddev, argp);
>> 		goto out;
>>
>> What do you think?
>
> I think that it accurately mimics what the current code does.
> I'm not sure that is what we really want.
> For testing in Incremental.c if an array is "active" we really
> should be testing more than "raid_disks != 0".
> We should be testing, as Shaohua suggested, if
> array_state != 'clear' or 'inactive'.
> You cannot get that info through the ioctl interface, so I suppose
> I decided the current test was 'close enough'.
> If we are going to stop supported kernels that don't have (e.g.)
> array_state, then we should really fo the right thing and test
> array_state.

Neil,

That makes a lot of sense - let me look into this. There are other cases 
in the code where the intention isn't 100% clear, so expect me to nag 
you as I work through them :)

Cheers,
Jes

Re: GET_ARRAY_INFO assumptions?

[Jes Sorensen]

On 04/17/2017 07:48 PM, NeilBrown wrote:
> On Fri, Apr 14 2017, Jes Sorensen wrote:
>> Looking some more at this, it may be simpler than I thought. How about
>> this approach (only compile tested):
>>
>> int md_array_active(int fd)
>> {
>> 	struct mdinfo *sra;
>> 	struct mdu_array_info_s array;
>> 	int ret;
>>
>> 	sra = sysfs_read(fd, NULL, GET_VERSION | GET_DISKS);
>> 	if (sra) {
>> 		if (!sra->array.raid_disks &&
>> 		    !(sra->array.major_version == -1 &&
>> 		      sra->array.minor_version == -2))
>> 			ret = -ENODEV;
>> 		else
>> 			ret = 0;
>>
>> 		free(sra);
>> 	} else {
>> 		ret = ioctl(fd, GET_ARRAY_INFO, &array);
>> 	}
>>
>> 	return !ret;
>> }
>>
>> Note 'major = -1 && minor = -2' is sysfs_read's way of saying 'external'.
>>
>> This pretty much mimics what the kernel does in the ioctl handler for
>> GET_ARRAY_INFO:
>>
>> 	case GET_ARRAY_INFO:
>> 		if (!mddev->raid_disks && !mddev->external)
>> 			err = -ENODEV;
>> 		else
>> 			err = get_array_info(mddev, argp);
>> 		goto out;
>>
>> What do you think?
>
> I think that it accurately mimics what the current code does.
> I'm not sure that is what we really want.
> For testing in Incremental.c if an array is "active" we really
> should be testing more than "raid_disks != 0".
> We should be testing, as Shaohua suggested, if
> array_state != 'clear' or 'inactive'.
> You cannot get that info through the ioctl interface, so I suppose
> I decided the current test was 'close enough'.
> If we are going to stop supported kernels that don't have (e.g.)
> array_state, then we should really fo the right thing and test
> array_state.

I think I got it right this time and pushed it into git. It made things 
a lot prettier too IMHO :)

In the process I also changed the behavior of 
sysfs_read(GET_ARRAY_STATE) as I really didn't like how it was copying 
in the string rather than parsing it.

I am traveling at the moment and don't yet have my new raid test box 
setup back at the office, so my testing is limited. If I broke something 
badly, feel free to throw rotten tomatoes at me.

Cheers,
Jes

Re: GET_ARRAY_INFO assumptions?

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1230 bytes --]

On Thu, Apr 20 2017, Jes Sorensen wrote:

>
> I think I got it right this time and pushed it into git. It made things 
> a lot prettier too IMHO :)
>
> In the process I also changed the behavior of 
> sysfs_read(GET_ARRAY_STATE) as I really didn't like how it was copying 
> in the string rather than parsing it.
>
> I am traveling at the moment and don't yet have my new raid test box 
> setup back at the office, so my testing is limited. If I broke something 
> badly, feel free to throw rotten tomatoes at me.
>
> Cheers,
> Jes

Looks good to me, except...

	/*
	 * Beware map_name() uses strcmp() so active-idle must come before
	 * active, to be detected correctly.
	 */

???
If map_name() used strncmp() you might need to be careful, but not with
strcmp.
Also:

int map_name(mapping_t *map, char *name)
{
	while (map->name) {
		if (strcmp(map->name, name)==0)
			return map->num;
		map++;
	}

	return map->num;
}

Both returns do the same thing... so there should (could) just be one.
is:

 while (map->name && strcmp(map->name, name) != 0)
 	map++;
 return map->num;

too terse??

I do like that you could use map_name to parse the array_state file.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: GET_ARRAY_INFO assumptions?

[Tomasz Majchrzak]

On Thu, Apr 20, 2017 at 12:05:54PM -0400, Jes Sorensen wrote:
> On 04/17/2017 07:48 PM, NeilBrown wrote:
> >On Fri, Apr 14 2017, Jes Sorensen wrote:
> >>Looking some more at this, it may be simpler than I thought. How about
> >>this approach (only compile tested):
> >>
> >>int md_array_active(int fd)
> >>{
> >>	struct mdinfo *sra;
> >>	struct mdu_array_info_s array;
> >>	int ret;
> >>
> >>	sra = sysfs_read(fd, NULL, GET_VERSION | GET_DISKS);
> >>	if (sra) {
> >>		if (!sra->array.raid_disks &&
> >>		    !(sra->array.major_version == -1 &&
> >>		      sra->array.minor_version == -2))
> >>			ret = -ENODEV;
> >>		else
> >>			ret = 0;
> >>
> >>		free(sra);
> >>	} else {
> >>		ret = ioctl(fd, GET_ARRAY_INFO, &array);
> >>	}
> >>
> >>	return !ret;
> >>}
> >>
> >>Note 'major = -1 && minor = -2' is sysfs_read's way of saying 'external'.
> >>
> >>This pretty much mimics what the kernel does in the ioctl handler for
> >>GET_ARRAY_INFO:
> >>
> >>	case GET_ARRAY_INFO:
> >>		if (!mddev->raid_disks && !mddev->external)
> >>			err = -ENODEV;
> >>		else
> >>			err = get_array_info(mddev, argp);
> >>		goto out;
> >>
> >>What do you think?
> >
> >I think that it accurately mimics what the current code does.
> >I'm not sure that is what we really want.
> >For testing in Incremental.c if an array is "active" we really
> >should be testing more than "raid_disks != 0".
> >We should be testing, as Shaohua suggested, if
> >array_state != 'clear' or 'inactive'.
> >You cannot get that info through the ioctl interface, so I suppose
> >I decided the current test was 'close enough'.
> >If we are going to stop supported kernels that don't have (e.g.)
> >array_state, then we should really fo the right thing and test
> >array_state.
> 
> I think I got it right this time and pushed it into git. It made
> things a lot prettier too IMHO :)
> 
> In the process I also changed the behavior of
> sysfs_read(GET_ARRAY_STATE) as I really didn't like how it was
> copying in the string rather than parsing it.
> 
> I am traveling at the moment and don't yet have my new raid test box
> setup back at the office, so my testing is limited. If I broke
> something badly, feel free to throw rotten tomatoes at me.
> 

Hi Jes,

Compilation with "-O2" flag fails:

CXFLAGS=-O2 make

cc -Wall -Werror -Wstrict-prototypes -Wextra -Wno-unused-parameter -O2
-DSendmail=\""/usr/sbin/sendmail -t"\" -DCONFFILE=\"/etc/mdadm.conf\"
-DCONFFILE2=\"/etc/mdadm/mdadm.conf\" -DMAP_DIR=\"/run/mdadm\"
-DMAP_FILE=\"map\" -DMDMON_DIR=\"/run/mdadm\"
-DFAILED_SLOTS_DIR=\"/run/mdadm/failed-slots\" -DNO_COROSYNC -DNO_DLM
-DVERSION=\"4.0-85-g4435675\" -DVERS_DATE="\"2017-04-20\"" -DUSE_PTHREADS
-DBINDIR=\"/sbin\"  -c -o Query.o Query.c

Query.c: In function ‘Query’:
Query.c:92:9: error: ‘level’ may be used uninitialized in this function
[-Werror=maybe-uninitialized]
   printf("%s: %s %s %d devices, %d spare%s. Use mdadm --detail for more
   detail.\n",
            ^
Query.c:92:9: error: ‘spare_disks’ may be used uninitialized in
this function [-Werror=maybe-uninitialized]
Query.c:92:9: error: ‘raid_disks’ may be used uninitialized in
this function [-Werror=maybe-uninitialized]
cc1: all warnings being treated as errors
make: *** [Query.o] Error 1

Regards,

Tomek

Re: GET_ARRAY_INFO assumptions?

[Jes Sorensen]

On 04/21/2017 10:06 AM, Tomasz Majchrzak wrote:
> On Thu, Apr 20, 2017 at 12:05:54PM -0400, Jes Sorensen wrote:
>> On 04/17/2017 07:48 PM, NeilBrown wrote:
>>> On Fri, Apr 14 2017, Jes Sorensen wrote:
>>>> Looking some more at this, it may be simpler than I thought. How about
>>>> this approach (only compile tested):
>>>>
>>>> int md_array_active(int fd)
>>>> {
>>>> 	struct mdinfo *sra;
>>>> 	struct mdu_array_info_s array;
>>>> 	int ret;
>>>>
>>>> 	sra = sysfs_read(fd, NULL, GET_VERSION | GET_DISKS);
>>>> 	if (sra) {
>>>> 		if (!sra->array.raid_disks &&
>>>> 		    !(sra->array.major_version == -1 &&
>>>> 		      sra->array.minor_version == -2))
>>>> 			ret = -ENODEV;
>>>> 		else
>>>> 			ret = 0;
>>>>
>>>> 		free(sra);
>>>> 	} else {
>>>> 		ret = ioctl(fd, GET_ARRAY_INFO, &array);
>>>> 	}
>>>>
>>>> 	return !ret;
>>>> }
>>>>
>>>> Note 'major = -1 && minor = -2' is sysfs_read's way of saying 'external'.
>>>>
>>>> This pretty much mimics what the kernel does in the ioctl handler for
>>>> GET_ARRAY_INFO:
>>>>
>>>> 	case GET_ARRAY_INFO:
>>>> 		if (!mddev->raid_disks && !mddev->external)
>>>> 			err = -ENODEV;
>>>> 		else
>>>> 			err = get_array_info(mddev, argp);
>>>> 		goto out;
>>>>
>>>> What do you think?
>>>
>>> I think that it accurately mimics what the current code does.
>>> I'm not sure that is what we really want.
>>> For testing in Incremental.c if an array is "active" we really
>>> should be testing more than "raid_disks != 0".
>>> We should be testing, as Shaohua suggested, if
>>> array_state != 'clear' or 'inactive'.
>>> You cannot get that info through the ioctl interface, so I suppose
>>> I decided the current test was 'close enough'.
>>> If we are going to stop supported kernels that don't have (e.g.)
>>> array_state, then we should really fo the right thing and test
>>> array_state.
>>
>> I think I got it right this time and pushed it into git. It made
>> things a lot prettier too IMHO :)
>>
>> In the process I also changed the behavior of
>> sysfs_read(GET_ARRAY_STATE) as I really didn't like how it was
>> copying in the string rather than parsing it.
>>
>> I am traveling at the moment and don't yet have my new raid test box
>> setup back at the office, so my testing is limited. If I broke
>> something badly, feel free to throw rotten tomatoes at me.
>>
>
> Hi Jes,
>
> Compilation with "-O2" flag fails:
>
> CXFLAGS=-O2 make
>
> cc -Wall -Werror -Wstrict-prototypes -Wextra -Wno-unused-parameter -O2
> -DSendmail=\""/usr/sbin/sendmail -t"\" -DCONFFILE=\"/etc/mdadm.conf\"
> -DCONFFILE2=\"/etc/mdadm/mdadm.conf\" -DMAP_DIR=\"/run/mdadm\"
> -DMAP_FILE=\"map\" -DMDMON_DIR=\"/run/mdadm\"
> -DFAILED_SLOTS_DIR=\"/run/mdadm/failed-slots\" -DNO_COROSYNC -DNO_DLM
> -DVERSION=\"4.0-85-g4435675\" -DVERS_DATE="\"2017-04-20\"" -DUSE_PTHREADS
> -DBINDIR=\"/sbin\"  -c -o Query.o Query.c
>
> Query.c: In function ‘Query’:
> Query.c:92:9: error: ‘level’ may be used uninitialized in this function
> [-Werror=maybe-uninitialized]
>    printf("%s: %s %s %d devices, %d spare%s. Use mdadm --detail for more
>    detail.\n",
>             ^
> Query.c:92:9: error: ‘spare_disks’ may be used uninitialized in
> this function [-Werror=maybe-uninitialized]
> Query.c:92:9: error: ‘raid_disks’ may be used uninitialized in
> this function [-Werror=maybe-uninitialized]
> cc1: all warnings being treated as errors
> make: *** [Query.o] Error 1

Fixed!

I do take patches ;)

Cheers,
Jes

Re: GET_ARRAY_INFO assumptions?

[Jes Sorensen]

On 04/20/2017 05:49 PM, NeilBrown wrote:
> On Thu, Apr 20 2017, Jes Sorensen wrote:
>
>>
>> I think I got it right this time and pushed it into git. It made things
>> a lot prettier too IMHO :)
>>
>> In the process I also changed the behavior of
>> sysfs_read(GET_ARRAY_STATE) as I really didn't like how it was copying
>> in the string rather than parsing it.
>>
>> I am traveling at the moment and don't yet have my new raid test box
>> setup back at the office, so my testing is limited. If I broke something
>> badly, feel free to throw rotten tomatoes at me.
>>
>> Cheers,
>> Jes
>
> Looks good to me, except...
>
> 	/*
> 	 * Beware map_name() uses strcmp() so active-idle must come before
> 	 * active, to be detected correctly.
> 	 */
>
> ???
> If map_name() used strncmp() you might need to be careful, but not with
> strcmp.

Hmmm I should know better than that! I did work on glibc back in the day 
after all :(

> Also:
>
> int map_name(mapping_t *map, char *name)
> {
> 	while (map->name) {
> 		if (strcmp(map->name, name)==0)
> 			return map->num;
> 		map++;
> 	}
>
> 	return map->num;
> }
>
> Both returns do the same thing... so there should (could) just be one.
> is:
>
>  while (map->name && strcmp(map->name, name) != 0)
>  	map++;
>  return map->num;
>
> too terse??

Looks good to me - applied a patch for it.

> I do like that you could use map_name to parse the array_state file.

I actually had it done first with a large if list, but then discovered 
maps.c and felt I had to change it :)

Cheers,
Jes


> Thanks,
> NeilBrown
>