[mdadm PATCH] mdopen: call "modprobe md_mod" if it might be needed.

[mdadm PATCH] mdopen: call "modprobe md_mod" if it might be needed.

[NeilBrown]

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


Creating an array by opening a block-device with major number of 9
will transparently load the md module if needed.
Creating an array by opening
   /sys/module/md_mod/parameters/new_array
and writing to it won't, it will just fail if md_mod isn't loaded.

So when opening that file fails with ENOENT, run "modprobe md_mod" and
try again.

This fixes a bug whereby if you have "CREATE names=yes" in mdadm.conf,
and the md modules isn't loaded, then creating or assembling an
array will not honor the "names=yes" configuration.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 mdopen.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/mdopen.c b/mdopen.c
index 3c0052f2db23..dcdc6f23e6c1 100644
--- a/mdopen.c
+++ b/mdopen.c
@@ -312,6 +312,10 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
 		if (block_udev)
 			udev_block(devnm);
 		fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
+		if (fd < 0 && errno == ENOENT) {
+			system("modprobe md_mod");
+			fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
+		}
 		if (fd >= 0) {
 			n = write(fd, devnm, strlen(devnm));
 			close(fd);
-- 
2.14.0.rc0.dirty


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

Re: [mdadm PATCH] mdopen: call "modprobe md_mod" if it might be needed.

[John Stoffel]

>>>>> "NeilBrown" == NeilBrown  <neilb@suse.com> writes:

NeilBrown> Creating an array by opening a block-device with major number of 9
NeilBrown> will transparently load the md module if needed.
NeilBrown> Creating an array by opening
NeilBrown>    /sys/module/md_mod/parameters/new_array
NeilBrown> and writing to it won't, it will just fail if md_mod isn't loaded.

NeilBrown> So when opening that file fails with ENOENT, run "modprobe md_mod" and
NeilBrown> try again.

NeilBrown> This fixes a bug whereby if you have "CREATE names=yes" in mdadm.conf,
NeilBrown> and the md modules isn't loaded, then creating or assembling an
NeilBrown> array will not honor the "names=yes" configuration.

NeilBrown> Signed-off-by: NeilBrown <neilb@suse.com>
NeilBrown> ---
NeilBrown>  mdopen.c | 4 ++++
NeilBrown>  1 file changed, 4 insertions(+)

NeilBrown> diff --git a/mdopen.c b/mdopen.c
NeilBrown> index 3c0052f2db23..dcdc6f23e6c1 100644
NeilBrown> --- a/mdopen.c
NeilBrown> +++ b/mdopen.c
NeilBrown> @@ -312,6 +312,10 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
NeilBrown>  		if (block_udev)
NeilBrown>  			udev_block(devnm);
NeilBrown>  		fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
NeilBrown> +		if (fd < 0 && errno == ENOENT) {
NeilBrown> +			system("modprobe md_mod");
NeilBrown> +			fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
NeilBrown> +		}
NeilBrown>  		if (fd >= 0) {
NeilBrown>  			n = write(fd, devnm, strlen(devnm));
NeilBrown>  			close(fd);
NeilBrown> -- 
NeilBrown> 2.14.0.rc0.dirty

I haven't looked, but shouldn't the path for modprobe be hardcoded
here to /sbin/modprobe?  Or the PATH sanitized so that random people
can't put something into the system PATH and cause problems? 

Re: [mdadm PATCH] mdopen: call "modprobe md_mod" if it might be needed.

[NeilBrown]

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

On Mon, Sep 25 2017, John Stoffel wrote:

>>>>>> "NeilBrown" == NeilBrown  <neilb@suse.com> writes:
>
> NeilBrown> Creating an array by opening a block-device with major number of 9
> NeilBrown> will transparently load the md module if needed.
> NeilBrown> Creating an array by opening
> NeilBrown>    /sys/module/md_mod/parameters/new_array
> NeilBrown> and writing to it won't, it will just fail if md_mod isn't loaded.
>
> NeilBrown> So when opening that file fails with ENOENT, run "modprobe md_mod" and
> NeilBrown> try again.
>
> NeilBrown> This fixes a bug whereby if you have "CREATE names=yes" in mdadm.conf,
> NeilBrown> and the md modules isn't loaded, then creating or assembling an
> NeilBrown> array will not honor the "names=yes" configuration.
>
> NeilBrown> Signed-off-by: NeilBrown <neilb@suse.com>
> NeilBrown> ---
> NeilBrown>  mdopen.c | 4 ++++
> NeilBrown>  1 file changed, 4 insertions(+)
>
> NeilBrown> diff --git a/mdopen.c b/mdopen.c
> NeilBrown> index 3c0052f2db23..dcdc6f23e6c1 100644
> NeilBrown> --- a/mdopen.c
> NeilBrown> +++ b/mdopen.c
> NeilBrown> @@ -312,6 +312,10 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
> NeilBrown>  		if (block_udev)
> NeilBrown>  			udev_block(devnm);
> NeilBrown>  		fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
> NeilBrown> +		if (fd < 0 && errno == ENOENT) {
> NeilBrown> +			system("modprobe md_mod");
> NeilBrown> +			fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
> NeilBrown> +		}
> NeilBrown>  		if (fd >= 0) {
> NeilBrown>  			n = write(fd, devnm, strlen(devnm));
> NeilBrown>  			close(fd);
> NeilBrown> -- 
> NeilBrown> 2.14.0.rc0.dirty
>
> I haven't looked, but shouldn't the path for modprobe be hardcoded
> here to /sbin/modprobe?  Or the PATH sanitized so that random people
> can't put something into the system PATH and cause problems? 

That issue briefly crossed my mind as I wrote the code (is it OK to use
system()? should I use /sbin/modprobe or just modprobe?) but as mdadm is
not set-uid and cannot be run in an environment created by a
non-privileged user, there is no security risk.
Certainly a careless sysadmin might set path wrongs, but the most likely
wrong outcome is that modprobe won't be found, and there is very little
cost to that.

So thanks for asking, but I don't think there is any need for any extra
care, in which case "simplest is best".

Thanks,
NeilBrown

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

Re: [mdadm PATCH] mdopen: call "modprobe md_mod" if it might be needed.

[Jes Sorensen]

On 09/25/2017 07:50 PM, NeilBrown wrote:
> On Mon, Sep 25 2017, John Stoffel wrote:
> 
>> I haven't looked, but shouldn't the path for modprobe be hardcoded
>> here to /sbin/modprobe?  Or the PATH sanitized so that random people
>> can't put something into the system PATH and cause problems?
> 
> That issue briefly crossed my mind as I wrote the code (is it OK to use
> system()? should I use /sbin/modprobe or just modprobe?) but as mdadm is
> not set-uid and cannot be run in an environment created by a
> non-privileged user, there is no security risk.
> Certainly a careless sysadmin might set path wrongs, but the most likely
> wrong outcome is that modprobe won't be found, and there is very little
> cost to that.
> 
> So thanks for asking, but I don't think there is any need for any extra
> care, in which case "simplest is best".

I completely agree, I also think we shouldn't hardcode imposed layouts 
unless there are very strong reasons for it. Someone may want to put 
this into an initramfs or somewhere else and want to put it in a 
different location.

Cheers,
Jes

Re: [mdadm PATCH] mdopen: call "modprobe md_mod" if it might be needed.

[Wols Lists]

On 26/09/17 16:11, Jes Sorensen wrote:
> I completely agree, I also think we shouldn't hardcode imposed layouts
> unless there are very strong reasons for it. Someone may want to put
> this into an initramfs or somewhere else and want to put it in a
> different location.

Plus isn't /sbin deprecated?

Iirc (very debatable :-) the end goal of the FHS is to have a read-only
/ with all executables in /bin. Okay it's going to take a looonngg time
to get there, but if I'm right hard-coding already-deprecated standards
isn't the best move.

(Again, iirc :-) sbin stood for *static* binaries needed for system
recovery, so its original purpose has long been abused ... :-)

Cheers,
Wol

Re: [mdadm PATCH] mdopen: call "modprobe md_mod" if it might be needed.

[John Stoffel]

>>>>> "Wols" == Wols Lists <antlists@youngman.org.uk> writes:

Wols> On 26/09/17 16:11, Jes Sorensen wrote:
>> I completely agree, I also think we shouldn't hardcode imposed layouts
>> unless there are very strong reasons for it. Someone may want to put
>> this into an initramfs or somewhere else and want to put it in a
>> different location.

Wols> Plus isn't /sbin deprecated?

LOL!  

Wols> Iirc (very debatable :-) the end goal of the FHS is to have a
Wols> read-only / with all executables in /bin. Okay it's going to
Wols> take a looonngg time to get there, but if I'm right hard-coding
Wols> already-deprecated standards isn't the best move.

Heh heh.

Wols> (Again, iirc :-) sbin stood for *static* binaries needed for
Wols> system recovery, so its original purpose has long been abused
Wols> ... :-)

I agree.  I more think of it as 'system binaries' myself.  Stuff
that's needed by the barebones system to just even run.

But others haven't asuaged my concerns, so I think we're ok.

John

Re: [mdadm PATCH] mdopen: call "modprobe md_mod" if it might be needed.

[Jes Sorensen]

On 09/25/2017 01:52 AM, NeilBrown wrote:
> 
> Creating an array by opening a block-device with major number of 9
> will transparently load the md module if needed.
> Creating an array by opening
>     /sys/module/md_mod/parameters/new_array
> and writing to it won't, it will just fail if md_mod isn't loaded.
> 
> So when opening that file fails with ENOENT, run "modprobe md_mod" and
> try again.
> 
> This fixes a bug whereby if you have "CREATE names=yes" in mdadm.conf,
> and the md modules isn't loaded, then creating or assembling an
> array will not honor the "names=yes" configuration.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>   mdopen.c | 4 ++++
>   1 file changed, 4 insertions(+)

Applied!

Thanks,
Jes

Re: [mdadm PATCH] mdopen: call "modprobe md_mod" if it might be needed.

[NeilBrown]

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

On Tue, Oct 10 2017, Zhilong Liu wrote:

> On 09/25/2017 01:52 PM, NeilBrown wrote:
>> Creating an array by opening a block-device with major number of 9
>> will transparently load the md module if needed.
>> Creating an array by opening
>>     /sys/module/md_mod/parameters/new_array
>> and writing to it won't, it will just fail if md_mod isn't loaded.
>>
>> So when opening that file fails with ENOENT, run "modprobe md_mod" and
>> try again.
>>
>> This fixes a bug whereby if you have "CREATE names=yes" in mdadm.conf,
>> and the md modules isn't loaded, then creating or assembling an
>> array will not honor the "names=yes" configuration.
>>
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>>   mdopen.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/mdopen.c b/mdopen.c
>> index 3c0052f2db23..dcdc6f23e6c1 100644
>> --- a/mdopen.c
>> +++ b/mdopen.c
>> @@ -312,6 +312,10 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
>>   		if (block_udev)
>>   			udev_block(devnm);
>>   		fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
>> +		if (fd < 0 && errno == ENOENT) {
>> +			system("modprobe md_mod");
>
> Hi, Neil;
> this system() line would be treated warning as error when issue
> make everything-test.
> It complains:
> ... ...
> mdopen.c: In function ‘create_mddev’:
> mdopen.c:316:10: error: ignoring return value of ‘system’, declared with 
> attribute warn_unused_result [-Werror=unused-result]
>      system("modprobe md_mod");
>            ^
> cc1: all warnings being treated as errors
> Makefile:196: recipe for target 'mdadm.O2' failed
> make: *** [mdadm.O2] Error 1
> ... ...
>
> It shows that mdadm cannot assume that "system" will always succeed. The 
> code becomes
> unreliable in this way.
>
> It should meets three conditions at the same time to ensure system is 
> successful.
> 1. -1 != status
> 2. WIFEXITED(status) is true
> 3. 0 == WEXITSTATUS(status)
>
> Maybe add a test like this?
>
> diff --git a/mdopen.c b/mdopen.c
> index dcdc6f2..51bf2d3 100644
> --- a/mdopen.c
> +++ b/mdopen.c
> @@ -313,7 +313,10 @@ int create_mddev(char *dev, char *name, int autof, 
> int trustworthy,
>                          udev_block(devnm);
>                  fd = open("/sys/module/md_mod/parameters/new_array", 
> O_WRONLY);
>                  if (fd < 0 && errno == ENOENT) {
> -                       system("modprobe md_mod");
> +                       int ret = system("modprobe md_mod");
> +                       if (ret) {
> +                               pr_err("modprobe md_mod got failed!\n");
> +                       }
>                          fd = 
> open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
>                  }
>                  if (fd >= 0) {
>

Hmmm.. that's annoying.  I wonder why "system" is marked
"warn_unused_result".
In this case I really don't care - I'm not convinced an extra error
message will really help.
Maybe
   if (system("modprobe md_mod") == 0)
        fd = open("/sys/......", O_WRONLY);


We do what a better error message, then it should be based on 'fd < 0'.
e.g.
  if (fd < 0 || n != strlen(devnm))
      pr_err("Fail create array using /sys/module/md_mod/parameters/new_array\n");

Thanks,
NeilBrown

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

Re: [mdadm PATCH] mdopen: call "modprobe md_mod" if it might be needed.

[Zhilong Liu]

On 10/11/2017 04:16 AM, NeilBrown wrote:
> On Tue, Oct 10 2017, Zhilong Liu wrote:
>
>> On 09/25/2017 01:52 PM, NeilBrown wrote:
>>> Creating an array by opening a block-device with major number of 9
>>> will transparently load the md module if needed.
>>> Creating an array by opening
>>>      /sys/module/md_mod/parameters/new_array
>>> and writing to it won't, it will just fail if md_mod isn't loaded.
>>>
>>> So when opening that file fails with ENOENT, run "modprobe md_mod" and
>>> try again.
>>>
>>> This fixes a bug whereby if you have "CREATE names=yes" in mdadm.conf,
>>> and the md modules isn't loaded, then creating or assembling an
>>> array will not honor the "names=yes" configuration.
>>>
>>> Signed-off-by: NeilBrown <neilb@suse.com>
>>> ---
>>>    mdopen.c | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>>
>>> diff --git a/mdopen.c b/mdopen.c
>>> index 3c0052f2db23..dcdc6f23e6c1 100644
>>> --- a/mdopen.c
>>> +++ b/mdopen.c
>>> @@ -312,6 +312,10 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
>>>    		if (block_udev)
>>>    			udev_block(devnm);
>>>    		fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
>>> +		if (fd < 0 && errno == ENOENT) {
>>> +			system("modprobe md_mod");
[nip]
> Hmmm.. that's annoying.  I wonder why "system" is marked
> "warn_unused_result".

in /usr/include/stdlib.h:
...
712 /* Execute the given line as a shell command.
713
714    This function is a cancellation point and therefore not marked with
715    __THROW.  */
716 extern int system (const char *__command) __wur;
...

the "warn_unused_result" is from the __wur parameter, re-compile mdadm 
after delete the '__wur',
it works.

> In this case I really don't care - I'm not convinced an extra error
> message will really help.
> Maybe
>     if (system("modprobe md_mod") == 0)
>          fd = open("/sys/......", O_WRONLY);

Agree.
>
> We do what a better error message, then it should be based on 'fd < 0'.
> e.g.
>    if (fd < 0 || n != strlen(devnm))
>        pr_err("Fail create array using /sys/module/md_mod/parameters/new_array\n");

you mean something like this?

diff --git a/mdopen.c b/mdopen.c
index dcdc6f2..9de347e 100644
--- a/mdopen.c
+++ b/mdopen.c
@@ -313,14 +313,17 @@ int create_mddev(char *dev, char *name, int autof, 
int trustworthy,
                         udev_block(devnm);
                 fd = open("/sys/module/md_mod/parameters/new_array", 
O_WRONLY);
                 if (fd < 0 && errno == ENOENT) {
-                       system("modprobe md_mod");
-                       fd = 
open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
+                       if (system("modprobe md_mod") == 0)
+                               fd = 
open("/sys/module/md_mod/parameters/new_array",
+                                         O_WRONLY);
                 }
                 if (fd >= 0) {
                         n = write(fd, devnm, strlen(devnm));
                         close(fd);
                 }
-               if (n < 0) {
+               if (fd < 0 || n != strlen(devnm)) {
+                       pr_err("Fail create array using "
+ "/sys/module/md_mod/parameters/new_array\n");
                         devnm[0] = 0;
                         udev_unblock();
                 }


Thanks,
-Zhilong

> Thanks,
> NeilBrown

Re: [mdadm PATCH] mdopen: call "modprobe md_mod" if it might be needed.

[NeilBrown]

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

On Wed, Oct 11 2017, Zhilong Liu wrote:

> On 10/11/2017 04:16 AM, NeilBrown wrote:
>> On Tue, Oct 10 2017, Zhilong Liu wrote:
>>
>>> On 09/25/2017 01:52 PM, NeilBrown wrote:
>>>> Creating an array by opening a block-device with major number of 9
>>>> will transparently load the md module if needed.
>>>> Creating an array by opening
>>>>      /sys/module/md_mod/parameters/new_array
>>>> and writing to it won't, it will just fail if md_mod isn't loaded.
>>>>
>>>> So when opening that file fails with ENOENT, run "modprobe md_mod" and
>>>> try again.
>>>>
>>>> This fixes a bug whereby if you have "CREATE names=yes" in mdadm.conf,
>>>> and the md modules isn't loaded, then creating or assembling an
>>>> array will not honor the "names=yes" configuration.
>>>>
>>>> Signed-off-by: NeilBrown <neilb@suse.com>
>>>> ---
>>>>    mdopen.c | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/mdopen.c b/mdopen.c
>>>> index 3c0052f2db23..dcdc6f23e6c1 100644
>>>> --- a/mdopen.c
>>>> +++ b/mdopen.c
>>>> @@ -312,6 +312,10 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
>>>>    		if (block_udev)
>>>>    			udev_block(devnm);
>>>>    		fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
>>>> +		if (fd < 0 && errno == ENOENT) {
>>>> +			system("modprobe md_mod");
> [nip]
>> Hmmm.. that's annoying.  I wonder why "system" is marked
>> "warn_unused_result".
>
> in /usr/include/stdlib.h:
> ...
> 712 /* Execute the given line as a shell command.
> 713
> 714    This function is a cancellation point and therefore not marked with
> 715    __THROW.  */
> 716 extern int system (const char *__command) __wur;
> ...
>
> the "warn_unused_result" is from the __wur parameter, re-compile mdadm 
> after delete the '__wur',
> it works.
>
>> In this case I really don't care - I'm not convinced an extra error
>> message will really help.
>> Maybe
>>     if (system("modprobe md_mod") == 0)
>>          fd = open("/sys/......", O_WRONLY);
>
> Agree.
>>
>> We do what a better error message, then it should be based on 'fd < 0'.
>> e.g.
>>    if (fd < 0 || n != strlen(devnm))
>>        pr_err("Fail create array using /sys/module/md_mod/parameters/new_array\n");
>
> you mean something like this?
>
> diff --git a/mdopen.c b/mdopen.c
> index dcdc6f2..9de347e 100644
> --- a/mdopen.c
> +++ b/mdopen.c
> @@ -313,14 +313,17 @@ int create_mddev(char *dev, char *name, int autof, 
> int trustworthy,
>                          udev_block(devnm);
>                  fd = open("/sys/module/md_mod/parameters/new_array", 
> O_WRONLY);
>                  if (fd < 0 && errno == ENOENT) {
> -                       system("modprobe md_mod");
> -                       fd = 
> open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
> +                       if (system("modprobe md_mod") == 0)
> +                               fd = 
> open("/sys/module/md_mod/parameters/new_array",
> +                                         O_WRONLY);
>                  }
>                  if (fd >= 0) {
>                          n = write(fd, devnm, strlen(devnm));
>                          close(fd);
>                  }
> -               if (n < 0) {
> +               if (fd < 0 || n != strlen(devnm)) {
> +                       pr_err("Fail create array using "
> + "/sys/module/md_mod/parameters/new_array\n");
>                          devnm[0] = 0;
>                          udev_unblock();
>                  }
>

Yes - exactly like that except that I wouldn't wrap the long string.
Lines longer than 80 chars are good to avoid, but breaking string
literals is worse than having long lines.  e.g. it makes searching for
the string hard.

Thanks,
NeilBrown

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

Re: [mdadm PATCH] mdopen: call "modprobe md_mod" if it might be needed.

[Zhilong Liu]

On 10/12/2017 08:06 AM, NeilBrown wrote:
> On Wed, Oct 11 2017, Zhilong Liu wrote:
>
>> On 10/11/2017 04:16 AM, NeilBrown wrote:
>>> On Tue, Oct 10 2017, Zhilong Liu wrote:
>>>
>>>> On 09/25/2017 01:52 PM, NeilBrown wrote:
>>>>> Creating an array by opening a block-device with major number of 9
>>>>> will transparently load the md module if needed.
>>>>> Creating an array by opening
>>>>>       /sys/module/md_mod/parameters/new_array
>>>>> and writing to it won't, it will just fail if md_mod isn't loaded.
>>>>>
>>>>> So when opening that file fails with ENOENT, run "modprobe md_mod" and
>>>>> try again.
>>>>>
>>>>> This fixes a bug whereby if you have "CREATE names=yes" in mdadm.conf,
>>>>> and the md modules isn't loaded, then creating or assembling an
>>>>> array will not honor the "names=yes" configuration.
>>>>>
>>>>> Signed-off-by: NeilBrown <neilb@suse.com>
>>>>> ---
>>>>>     mdopen.c | 4 ++++
>>>>>     1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/mdopen.c b/mdopen.c
>>>>> index 3c0052f2db23..dcdc6f23e6c1 100644
>>>>> --- a/mdopen.c
>>>>> +++ b/mdopen.c
>>>>> @@ -312,6 +312,10 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
>>>>>     		if (block_udev)
>>>>>     			udev_block(devnm);
>>>>>     		fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
>>>>> +		if (fd < 0 && errno == ENOENT) {
>>>>> +			system("modprobe md_mod");
>> [nip]
>>> Hmmm.. that's annoying.  I wonder why "system" is marked
>>> "warn_unused_result".
>> in /usr/include/stdlib.h:
>> ...
>> 712 /* Execute the given line as a shell command.
>> 713
>> 714    This function is a cancellation point and therefore not marked with
>> 715    __THROW.  */
>> 716 extern int system (const char *__command) __wur;
>> ...
>>
>> the "warn_unused_result" is from the __wur parameter, re-compile mdadm
>> after delete the '__wur',
>> it works.
>>
>>> In this case I really don't care - I'm not convinced an extra error
>>> message will really help.
>>> Maybe
>>>      if (system("modprobe md_mod") == 0)
>>>           fd = open("/sys/......", O_WRONLY);
>> Agree.
>>> We do what a better error message, then it should be based on 'fd < 0'.
>>> e.g.
>>>     if (fd < 0 || n != strlen(devnm))
>>>         pr_err("Fail create array using /sys/module/md_mod/parameters/new_array\n");
>> you mean something like this?
>>
>> diff --git a/mdopen.c b/mdopen.c
>> index dcdc6f2..9de347e 100644
>> --- a/mdopen.c
>> +++ b/mdopen.c
>> @@ -313,14 +313,17 @@ int create_mddev(char *dev, char *name, int autof,
>> int trustworthy,
>>                           udev_block(devnm);
>>                   fd = open("/sys/module/md_mod/parameters/new_array",
>> O_WRONLY);
>>                   if (fd < 0 && errno == ENOENT) {
>> -                       system("modprobe md_mod");
>> -                       fd =
>> open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
>> +                       if (system("modprobe md_mod") == 0)
>> +                               fd =
>> open("/sys/module/md_mod/parameters/new_array",
>> +                                         O_WRONLY);
>>                   }
>>                   if (fd >= 0) {
>>                           n = write(fd, devnm, strlen(devnm));
>>                           close(fd);
>>                   }
>> -               if (n < 0) {
>> +               if (fd < 0 || n != strlen(devnm)) {
>> +                       pr_err("Fail create array using "
>> + "/sys/module/md_mod/parameters/new_array\n");
>>                           devnm[0] = 0;
>>                           udev_unblock();
>>                   }
>>
> Yes - exactly like that except that I wouldn't wrap the long string.
> Lines longer than 80 chars are good to avoid, but breaking string
> literals is worse than having long lines.  e.g. it makes searching for
> the string hard.

Thanks for your detail explanation. I draft it like this, and
already checked it via to ./linux/scripts/checkpatch.pl.
Is this changing ok to you?

diff --git a/mdopen.c b/mdopen.c
index dcdc6f2..da8d9d1 100644
--- a/mdopen.c
+++ b/mdopen.c
@@ -26,6 +26,8 @@
  #include "md_p.h"
  #include <ctype.h>

+#define NEW_ARRAY_FILE "/sys/module/md_mod/parameters/new_array"
+
  void make_parts(char *dev, int cnt)
  {
      /* make 'cnt' partition devices for 'dev'
@@ -311,16 +313,17 @@ int create_mddev(char *dev, char *name, int autof, 
int trustworthy,
          sprintf(devnm, "md_%s", cname);
          if (block_udev)
              udev_block(devnm);
-        fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
+        fd = open(NEW_ARRAY_FILE, O_WRONLY);
          if (fd < 0 && errno == ENOENT) {
-            system("modprobe md_mod");
-            fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
+            if (system("modprobe md_mod") == 0)
+                fd = open(NEW_ARRAY_FILE, O_WRONLY);
          }
          if (fd >= 0) {
              n = write(fd, devnm, strlen(devnm));
              close(fd);
          }
-        if (n < 0) {
+        if (fd < 0 || n != strlen(devnm)) {
+            pr_err("Fail create array using %s\n", NEW_ARRAY_FILE);
              devnm[0] = 0;
              udev_unblock();
          }
@@ -331,12 +334,13 @@ int create_mddev(char *dev, char *name, int autof, 
int trustworthy,
          sprintf(devnm, "md%d", num);
          if (block_udev)
              udev_block(devnm);
-        fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
+        fd = open(NEW_ARRAY_FILE, O_WRONLY);
          if (fd >= 0) {
              n = write(fd, devnm, strlen(devnm));
              close(fd);
          }
-        if (n < 0) {
+        if (fd < 0 || n != strlen(devnm)) {
+            pr_err("Fail create array using %s\n", NEW_ARRAY_FILE);
              devnm[0] = 0;
              udev_unblock();
          }
-- 
2.6.6


Thanks,
-Zhilong

> Thanks,
> NeilBrown

Re: [mdadm PATCH] mdopen: call "modprobe md_mod" if it might be needed.

[NeilBrown]

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

On Thu, Oct 12 2017, Zhilong Liu wrote:

> On 10/12/2017 08:06 AM, NeilBrown wrote:
>> On Wed, Oct 11 2017, Zhilong Liu wrote:
>>
>>> On 10/11/2017 04:16 AM, NeilBrown wrote:
>>>> On Tue, Oct 10 2017, Zhilong Liu wrote:
>>>>
>>>>> On 09/25/2017 01:52 PM, NeilBrown wrote:
>>>>>> Creating an array by opening a block-device with major number of 9
>>>>>> will transparently load the md module if needed.
>>>>>> Creating an array by opening
>>>>>>       /sys/module/md_mod/parameters/new_array
>>>>>> and writing to it won't, it will just fail if md_mod isn't loaded.
>>>>>>
>>>>>> So when opening that file fails with ENOENT, run "modprobe md_mod" and
>>>>>> try again.
>>>>>>
>>>>>> This fixes a bug whereby if you have "CREATE names=yes" in mdadm.conf,
>>>>>> and the md modules isn't loaded, then creating or assembling an
>>>>>> array will not honor the "names=yes" configuration.
>>>>>>
>>>>>> Signed-off-by: NeilBrown <neilb@suse.com>
>>>>>> ---
>>>>>>     mdopen.c | 4 ++++
>>>>>>     1 file changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/mdopen.c b/mdopen.c
>>>>>> index 3c0052f2db23..dcdc6f23e6c1 100644
>>>>>> --- a/mdopen.c
>>>>>> +++ b/mdopen.c
>>>>>> @@ -312,6 +312,10 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
>>>>>>     		if (block_udev)
>>>>>>     			udev_block(devnm);
>>>>>>     		fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
>>>>>> +		if (fd < 0 && errno == ENOENT) {
>>>>>> +			system("modprobe md_mod");
>>> [nip]
>>>> Hmmm.. that's annoying.  I wonder why "system" is marked
>>>> "warn_unused_result".
>>> in /usr/include/stdlib.h:
>>> ...
>>> 712 /* Execute the given line as a shell command.
>>> 713
>>> 714    This function is a cancellation point and therefore not marked with
>>> 715    __THROW.  */
>>> 716 extern int system (const char *__command) __wur;
>>> ...
>>>
>>> the "warn_unused_result" is from the __wur parameter, re-compile mdadm
>>> after delete the '__wur',
>>> it works.
>>>
>>>> In this case I really don't care - I'm not convinced an extra error
>>>> message will really help.
>>>> Maybe
>>>>      if (system("modprobe md_mod") == 0)
>>>>           fd = open("/sys/......", O_WRONLY);
>>> Agree.
>>>> We do what a better error message, then it should be based on 'fd < 0'.
>>>> e.g.
>>>>     if (fd < 0 || n != strlen(devnm))
>>>>         pr_err("Fail create array using /sys/module/md_mod/parameters/new_array\n");
>>> you mean something like this?
>>>
>>> diff --git a/mdopen.c b/mdopen.c
>>> index dcdc6f2..9de347e 100644
>>> --- a/mdopen.c
>>> +++ b/mdopen.c
>>> @@ -313,14 +313,17 @@ int create_mddev(char *dev, char *name, int autof,
>>> int trustworthy,
>>>                           udev_block(devnm);
>>>                   fd = open("/sys/module/md_mod/parameters/new_array",
>>> O_WRONLY);
>>>                   if (fd < 0 && errno == ENOENT) {
>>> -                       system("modprobe md_mod");
>>> -                       fd =
>>> open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
>>> +                       if (system("modprobe md_mod") == 0)
>>> +                               fd =
>>> open("/sys/module/md_mod/parameters/new_array",
>>> +                                         O_WRONLY);
>>>                   }
>>>                   if (fd >= 0) {
>>>                           n = write(fd, devnm, strlen(devnm));
>>>                           close(fd);
>>>                   }
>>> -               if (n < 0) {
>>> +               if (fd < 0 || n != strlen(devnm)) {
>>> +                       pr_err("Fail create array using "
>>> + "/sys/module/md_mod/parameters/new_array\n");
>>>                           devnm[0] = 0;
>>>                           udev_unblock();
>>>                   }
>>>
>> Yes - exactly like that except that I wouldn't wrap the long string.
>> Lines longer than 80 chars are good to avoid, but breaking string
>> literals is worse than having long lines.  e.g. it makes searching for
>> the string hard.
>
> Thanks for your detail explanation. I draft it like this, and
> already checked it via to ./linux/scripts/checkpatch.pl.

I wouldn't worry too much about checkpatch.pl.
It is worth looking at what it reports, but if you don't agree or the
maintainer doesn't agree, then feel free to ignore it.


> Is this changing ok to you?
>
> diff --git a/mdopen.c b/mdopen.c
> index dcdc6f2..da8d9d1 100644
> --- a/mdopen.c
> +++ b/mdopen.c
> @@ -26,6 +26,8 @@
>   #include "md_p.h"
>   #include <ctype.h>
>
> +#define NEW_ARRAY_FILE "/sys/module/md_mod/parameters/new_array"
> +

Splitting this out into a separate string does make sense.
However I would use
  static const char new_array_file[] = ....;
rather than #define.

I hadn't noticed that there are two places where we write to new_array.
Maybe that should be split out into a function.
Both should use modprobe if the open fails.

Thanks,
NeilBrown


>   void make_parts(char *dev, int cnt)
>   {
>       /* make 'cnt' partition devices for 'dev'
> @@ -311,16 +313,17 @@ int create_mddev(char *dev, char *name, int autof, 
> int trustworthy,
>           sprintf(devnm, "md_%s", cname);
>           if (block_udev)
>               udev_block(devnm);
> -        fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
> +        fd = open(NEW_ARRAY_FILE, O_WRONLY);
>           if (fd < 0 && errno == ENOENT) {
> -            system("modprobe md_mod");
> -            fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
> +            if (system("modprobe md_mod") == 0)
> +                fd = open(NEW_ARRAY_FILE, O_WRONLY);
>           }
>           if (fd >= 0) {
>               n = write(fd, devnm, strlen(devnm));
>               close(fd);
>           }
> -        if (n < 0) {
> +        if (fd < 0 || n != strlen(devnm)) {
> +            pr_err("Fail create array using %s\n", NEW_ARRAY_FILE);
>               devnm[0] = 0;
>               udev_unblock();
>           }
> @@ -331,12 +334,13 @@ int create_mddev(char *dev, char *name, int autof, 
> int trustworthy,
>           sprintf(devnm, "md%d", num);
>           if (block_udev)
>               udev_block(devnm);
> -        fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
> +        fd = open(NEW_ARRAY_FILE, O_WRONLY);
>           if (fd >= 0) {
>               n = write(fd, devnm, strlen(devnm));
>               close(fd);
>           }
> -        if (n < 0) {
> +        if (fd < 0 || n != strlen(devnm)) {
> +            pr_err("Fail create array using %s\n", NEW_ARRAY_FILE);
>               devnm[0] = 0;
>               udev_unblock();
>           }
> -- 
> 2.6.6
>
>
> Thanks,
> -Zhilong
>
>> Thanks,
>> NeilBrown

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

Re: [mdadm PATCH] mdopen: call "modprobe md_mod" if it might be needed.

[Wols Lists]

On 12/10/17 01:06, NeilBrown wrote:
> Yes - exactly like that except that I wouldn't wrap the long string.
> Lines longer than 80 chars are good to avoid, but breaking string
> literals is worse than having long lines.  e.g. it makes searching for
> the string hard.

Isn't that the kernel standard anyway?

"Wrap at 80 chars *unless* that would mean breaking a string literal".

Iirc an lwn story or something about how all subsystems tended to follow
this rule except one, so they were going to try and standardise across
everything.

Cheers,
Wol

Re: [mdadm PATCH] mdopen: call "modprobe md_mod" if it might be needed.

[Zhilong Liu]

On 10/12/2017 04:48 PM, NeilBrown wrote:
>> On 10/12/2017 08:06 AM, NeilBrown wrote:
> I wouldn't worry too much about checkpatch.pl.
> It is worth looking at what it reports, but if you don't agree or the
> maintainer doesn't agree, then feel free to ignore it.
>
>
>> Is this changing ok to you?
>>
>> diff --git a/mdopen.c b/mdopen.c
>> index dcdc6f2..da8d9d1 100644
>> --- a/mdopen.c
>> +++ b/mdopen.c
>> @@ -26,6 +26,8 @@
>>    #include "md_p.h"
>>    #include <ctype.h>
>>
>> +#define NEW_ARRAY_FILE "/sys/module/md_mod/parameters/new_array"
>> +
> Splitting this out into a separate string does make sense.
> However I would use
>    static const char new_array_file[] = ....;
> rather than #define.

Maybe "const char *" is enough for this long string? Because this file 
only used when
creating named array, not global using.

> I hadn't noticed that there are two places where we write to new_array.
> Maybe that should be split out into a function.
> Both should use modprobe if the open fails.

how about this changing? Thanks for your patience to have a look at it.


diff --git a/mdopen.c b/mdopen.c
index dcdc6f2..6d8402d 100644
--- a/mdopen.c
+++ b/mdopen.c
@@ -100,6 +100,29 @@ void make_parts(char *dev, int cnt)
         free(name);
  }

+int create_named_array(char *devnm)
+{
+       const char *file = "/sys/module/md_mod/parameters/new_array";
+       int fd;
+       int n = -1;
+
+       fd = open(file, O_WRONLY);
+       if (fd < 0 && errno == ENOENT) {
+               if (system("modprobe md_mod") == 0)
+                       fd = open(file, O_WRONLY);
+       }
+       if (fd >= 0) {
+               n = write(fd, devnm, strlen(devnm));
+               close(fd);
+       }
+       if (fd < 0 || n != (int)strlen(devnm)) {
+               pr_err("Fail create %s when using %s\n", devnm, file);
+               return 0;
+       }
+
+       return 1;
+}
+
  /*
   * We need a new md device to assemble/build/create an array.
   * 'dev' is a name given us by the user (command line or mdadm.conf)
@@ -306,37 +329,19 @@ int create_mddev(char *dev, char *name, int autof, 
int trustworthy,

         devnm[0] = 0;
         if (num < 0 && cname && ci->names) {
-               int fd;
-               int n = -1;
                 sprintf(devnm, "md_%s", cname);
                 if (block_udev)
                         udev_block(devnm);
-               fd = open("/sys/module/md_mod/parameters/new_array", 
O_WRONLY);
-               if (fd < 0 && errno == ENOENT) {
-                       system("modprobe md_mod");
-                       fd = 
open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
-               }
-               if (fd >= 0) {
-                       n = write(fd, devnm, strlen(devnm));
-                       close(fd);
-               }
-               if (n < 0) {
+               if (!create_named_array(devnm)) {
                         devnm[0] = 0;
                         udev_unblock();
                 }
         }
         if (num >= 0) {
-               int fd;
-               int n = -1;
                 sprintf(devnm, "md%d", num);
                 if (block_udev)
                         udev_block(devnm);
-               fd = open("/sys/module/md_mod/parameters/new_array", 
O_WRONLY);
-               if (fd >= 0) {
-                       n = write(fd, devnm, strlen(devnm));
-                       close(fd);
-               }
-               if (n < 0) {
+               if (!create_named_array(devnm)) {
                         devnm[0] = 0;
                         udev_unblock();
                 }

Thanks,
-Zhilong

> Thanks,
> NeilBrown

Re: [mdadm PATCH] mdopen: call "modprobe md_mod" if it might be needed.

[NeilBrown]

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

On Fri, Oct 13 2017, Zhilong Liu wrote:

> On 10/12/2017 04:48 PM, NeilBrown wrote:
>>> On 10/12/2017 08:06 AM, NeilBrown wrote:
>> I wouldn't worry too much about checkpatch.pl.
>> It is worth looking at what it reports, but if you don't agree or the
>> maintainer doesn't agree, then feel free to ignore it.
>>
>>
>>> Is this changing ok to you?
>>>
>>> diff --git a/mdopen.c b/mdopen.c
>>> index dcdc6f2..da8d9d1 100644
>>> --- a/mdopen.c
>>> +++ b/mdopen.c
>>> @@ -26,6 +26,8 @@
>>>    #include "md_p.h"
>>>    #include <ctype.h>
>>>
>>> +#define NEW_ARRAY_FILE "/sys/module/md_mod/parameters/new_array"
>>> +
>> Splitting this out into a separate string does make sense.
>> However I would use
>>    static const char new_array_file[] = ....;
>> rather than #define.
>
> Maybe "const char *" is enough for this long string? Because this file 
> only used when
> creating named array, not global using.

I would still suggest "const char new_array_file[] = " because you
don't need a variable to hold a pointer to the string, you just need the
string.

But if you want to make it static inside the function, rather then
static outside the function, I see no problem with that.

NeilBrown

>
>> I hadn't noticed that there are two places where we write to new_array.
>> Maybe that should be split out into a function.
>> Both should use modprobe if the open fails.
>
> how about this changing? Thanks for your patience to have a look at it.
>
>
> diff --git a/mdopen.c b/mdopen.c
> index dcdc6f2..6d8402d 100644
> --- a/mdopen.c
> +++ b/mdopen.c
> @@ -100,6 +100,29 @@ void make_parts(char *dev, int cnt)
>          free(name);
>   }
>
> +int create_named_array(char *devnm)
> +{
> +       const char *file = "/sys/module/md_mod/parameters/new_array";
> +       int fd;
> +       int n = -1;
> +
> +       fd = open(file, O_WRONLY);
> +       if (fd < 0 && errno == ENOENT) {
> +               if (system("modprobe md_mod") == 0)
> +                       fd = open(file, O_WRONLY);
> +       }
> +       if (fd >= 0) {
> +               n = write(fd, devnm, strlen(devnm));
> +               close(fd);
> +       }
> +       if (fd < 0 || n != (int)strlen(devnm)) {
> +               pr_err("Fail create %s when using %s\n", devnm, file);
> +               return 0;
> +       }
> +
> +       return 1;
> +}
> +
>   /*
>    * We need a new md device to assemble/build/create an array.
>    * 'dev' is a name given us by the user (command line or mdadm.conf)
> @@ -306,37 +329,19 @@ int create_mddev(char *dev, char *name, int autof, 
> int trustworthy,
>
>          devnm[0] = 0;
>          if (num < 0 && cname && ci->names) {
> -               int fd;
> -               int n = -1;
>                  sprintf(devnm, "md_%s", cname);
>                  if (block_udev)
>                          udev_block(devnm);
> -               fd = open("/sys/module/md_mod/parameters/new_array", 
> O_WRONLY);
> -               if (fd < 0 && errno == ENOENT) {
> -                       system("modprobe md_mod");
> -                       fd = 
> open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
> -               }
> -               if (fd >= 0) {
> -                       n = write(fd, devnm, strlen(devnm));
> -                       close(fd);
> -               }
> -               if (n < 0) {
> +               if (!create_named_array(devnm)) {
>                          devnm[0] = 0;
>                          udev_unblock();
>                  }
>          }
>          if (num >= 0) {
> -               int fd;
> -               int n = -1;
>                  sprintf(devnm, "md%d", num);
>                  if (block_udev)
>                          udev_block(devnm);
> -               fd = open("/sys/module/md_mod/parameters/new_array", 
> O_WRONLY);
> -               if (fd >= 0) {
> -                       n = write(fd, devnm, strlen(devnm));
> -                       close(fd);
> -               }
> -               if (n < 0) {
> +               if (!create_named_array(devnm)) {
>                          devnm[0] = 0;
>                          udev_unblock();
>                  }
>
> Thanks,
> -Zhilong
>
>> Thanks,
>> NeilBrown

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