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