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

[Zhilong Liu <zlliu@suse.com>]

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