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

[NeilBrown <neilb@suse.com>]

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