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

[NeilBrown <neilb@suse.com>]

[-- 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 --]