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:
signature.asc
Description: PGP signature
