mdopen: call "modprobe md_mod" if it might be needed.
PATCH 1/1 mdopen: call "modprobe md_mod" if it might be needed.
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.
--- mdopen.c | 4 ++++ 1 file changed, 4 insertions(+)
mdopen.c +4 -0
@@ -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" ); + fd =open ("/sys/module/md_mod/parameters/new_array" ,O_WRONLY ); + }if (fd >=0 ) { n =write (fd, devnm,strlen (devnm));close (fd);
Signature · 1 line
2.14.0.rc0.dirty
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"NeilBrown" == NeilBrown <[email protected]> writes:
NeilBrown> Creating an array by opening a block-device with major number of 9 NeilBrown> will transparently load the md module if needed. NeilBrown> Creating an array by opening
NeilBrown> /sys/module/md_mod/parameters/new_array
NeilBrown> and writing to it won't, it will just fail if md_mod isn't loaded.
NeilBrown> So when opening that file fails with ENOENT, run "modprobe md_mod" and NeilBrown> try again.
NeilBrown> This fixes a bug whereby if you have "CREATE names=yes" in mdadm.conf, NeilBrown> and the md modules isn't loaded, then creating or assembling an NeilBrown> array will not honor the "names=yes" configuration.
NeilBrown> Signed-off-by: NeilBrown <[email protected]> NeilBrown> ---
NeilBrown> mdopen.c | 4 ++++
NeilBrown> 1 file changed, 4 insertions(+)
NeilBrown> diff --git a/mdopen.c b/mdopen.c
NeilBrown> index 3c0052f2db23..dcdc6f23e6c1 100644
NeilBrown> --- a/mdopen.c
NeilBrown> +++ b/mdopen.c
NeilBrown> @@ -312,6 +312,10 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
NeilBrown> if (block_udev)
NeilBrown> udev_block(devnm);
NeilBrown> fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);NeilBrown> + if (fd < 0 && errno == ENOENT) {
NeilBrown> + system("modprobe md_mod");
NeilBrown> + fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
NeilBrown> + }
NeilBrown> if (fd >= 0) {
NeilBrown> n = write(fd, devnm, strlen(devnm));
NeilBrown> close(fd);NeilBrown> --
NeilBrown> 2.14.0.rc0.dirty
I haven't looked, but shouldn't the path for modprobe be hardcoded here to /sbin/modprobe? Or the PATH sanitized so that random people can't put something into the system PATH and cause problems?
Quoted 35 lines · John Stoffel
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"NeilBrown" == NeilBrown <[email protected]> writes:
NeilBrown> Creating an array by opening a block-device with major number of 9 NeilBrown> will transparently load the md module if needed. NeilBrown> Creating an array by opening
NeilBrown>/sys/module/md_mod/parameters/new_array
NeilBrown> and writing to it won't, it will just fail ifmd_modisn't loaded.NeilBrown> So when opening that file fails with ENOENT, run "modprobe
md_mod" and NeilBrown> try again.NeilBrown> This fixes a bug whereby if you have "CREATE names=yes" in mdadm.conf, NeilBrown> and the md modules isn't loaded, then creating or assembling an NeilBrown> array will not honor the "names=yes" configuration.
NeilBrown> Signed-off-by: NeilBrown <[email protected]> NeilBrown> ---
NeilBrown>mdopen.c| 4 ++++
NeilBrown> 1 file changed, 4 insertions(+)NeilBrown> diff --git
a/mdopen.cb/mdopen.c
NeilBrown> index3c0052f2db23..dcdc6f23e6c1100644
NeilBrown> ---a/mdopen.c
NeilBrown> +++b/mdopen.c
NeilBrown> @@ -312,6 +312,10 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,NeilBrown> if (block_udev) NeilBrown> udev_block(devnm); NeilBrown> fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);NeilBrown> + if (fd < 0 && errno == ENOENT) {
NeilBrown> +system("modprobe md_mod");
NeilBrown> + fd =open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
NeilBrown> + }NeilBrown> if (fd >= 0) { NeilBrown> n = write(fd, devnm, strlen(devnm)); NeilBrown> close(fd);
Quoted 6 lines
NeilBrown> --
NeilBrown> 2.14.0.rc0.dirtyI haven't looked, but shouldn't the path for modprobe be hardcoded here to
/sbin/modprobe? Or the PATH sanitized so that random people can't put something into the system PATH and cause problems?
That issue briefly crossed my mind as I wrote the code (is it OK to use system()? should I use /sbin/modprobe or just modprobe?) but as mdadm is not set-uid and cannot be run in an environment created by a non-privileged user, there is no security risk.
Certainly a careless sysadmin might set path wrongs, but the most likely wrong outcome is that modprobe won't be found, and there is very little cost to that.
So thanks for asking, but I don't think there is any need for any extra care, in which case "simplest is best".
Thanks,
NeilBrown
Quoted 14 lines · NeilBrown
Quoted 3 lines · John Stoffel
I haven't looked, but shouldn't the path for modprobe be hardcoded here to
/sbin/modprobe? Or the PATH sanitized so that random people can't put something into the system PATH and cause problems?That issue briefly crossed my mind as I wrote the code (is it OK to use
system()? should I use/sbin/modprobeor just modprobe?) but as mdadm is not set-uid and cannot be run in an environment created by a non-privileged user, there is no security risk.
Certainly a careless sysadmin might set path wrongs, but the most likely wrong outcome is that modprobe won't be found, and there is very little cost to that.
Quoted 2 lines
So thanks for asking, but I don't think there is any need for any extra care, in which case "simplest is best".
I completely agree, I also think we shouldn't hardcode imposed layouts unless there are very strong reasons for it. Someone may want to put this into an initramfs or somewhere else and want to put it in a different location.
Cheers,
Jes
Quoted 4 lines · Jes Sorensen
I completely agree, I also think we shouldn't hardcode imposed layouts unless there are very strong reasons for it. Someone may want to put this into an initramfs or somewhere else and want to put it in a different location.
Plus isn't /sbin deprecated?
Iirc (very debatable :-) the end goal of the FHS is to have a read-only / with all executables in /bin. Okay it's going to take a looonngg time to get there, but if I'm right hard-coding already-deprecated standards isn't the best move.
(Again, iirc :-) sbin stood for *static* binaries needed for system recovery, so its original purpose has long been abused ... :-)
Cheers,
Wol
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Wols" == Wols Lists <[email protected]> writes:
Quoted 4 lines · Jes Sorensen
Quoted 4 lines
I completely agree, I also think we shouldn't hardcode imposed layouts unless there are very strong reasons for it. Someone may want to put this into an initramfs or somewhere else and want to put it in a different location.
Wols> Plus isn't /sbin deprecated?
LOL!
Wols> Iirc (very debatable :-) the end goal of the FHS is to have a Wols> read-only / with all executables in /bin. Okay it's going to Wols> take a looonngg time to get there, but if I'm right hard-coding Wols> already-deprecated standards isn't the best move.
Heh heh.
Wols> (Again, iirc :-) sbin stood for *static* binaries needed for Wols> system recovery, so its original purpose has long been abused Wols> ... :-)
I agree. I more think of it as 'system binaries' myself. Stuff that's needed by the barebones system to just even run.
But others haven't asuaged my concerns, so I think we're ok.
John
Jes Sorensen
AppliedQuoted 10 lines · NeilBrown
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 ifmd_modisn't loaded.So when opening that file fails with ENOENT, run "modprobe
md_mod" and try again.
Quoted 8 lines
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 <[email protected]>--- mdopen.c | 4 ++++ 1 file changed, 4 insertions(+)
Applied!
Thanks,
Jes
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 73 lines · Zhilong Liu
Quoted 23 lines · NeilBrown
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 ifmd_modisn'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 <[email protected]>--- mdopen.c | 4 ++++ 1 file changed, 4 insertions(+)mdopen.c +0
-0mdopen.c int create_mddev(char *dev, char *name, int autof, int trustworthy, · line 316
Quoted 5 lines
mdopen.c +2
-0@@ -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;
thissystem()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 attributewarn_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?
mdopen.c +4
-1@@ -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 =
Quoted 4 lines
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
Quoted 21 lines · NeilBrown
Quoted 19 lines · Zhilong Liu
Quoted 18 lines · NeilBrown
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 ifmd_modisn'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 <[email protected]>--- mdopen.c | 4 ++++ 1 file changed, 4 insertions(+)
Quoted 10 lines
Quoted 10 lines
mdopen.c int create_mddev(char *dev, char *name, int autof, int trustworthy, · line 316
Quoted 10 lines
mdopen.c +2
-0@@ -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]
Quoted 2 lines
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.
Quoted 5 lines
In this case I really don't care - I'm not convinced an extra error message will really help.
Maybeif (system("modprobe md_mod") == 0) fd = open("/sys/......", O_WRONLY);
Agree.
Quoted 5 lines
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?
mdopen.c +0 -0
@@ -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();
}Thanks,
-Zhilong
Quoted 2 lines
Thanks,
NeilBrown
Quoted 84 lines · Zhilong Liu
Quoted 21 lines · NeilBrown
Quoted 19 lines · Zhilong Liu
Quoted 18 lines · NeilBrown
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 ifmd_modisn'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 <[email protected]>--- mdopen.c | 4 ++++ 1 file changed, 4 insertions(+)Quoted 10 lines
Quoted 10 lines
mdopen.c int create_mddev(char *dev, char *name, int autof, int trustworthy, · line 316
Quoted 10 lines
mdopen.c +2
-0@@ -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]
Quoted 2 lines
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__wurparameter, re-compile mdadm after delete the '__wur',
it works.Quoted 5 lines
In this case I really don't care - I'm not convinced an extra error message will really help.
Maybeif (system("modprobe md_mod") == 0) fd = open("/sys/......", O_WRONLY);Agree.
Quoted 5 lines
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?
mdopen.c +3
-2@@ -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 ) {
mdopen.c int create_mddev(char *dev, char *name, int autof,
Quoted 10 lines
mdopen.c +3
-1n =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,
NeilBrown
Quoted 88 lines · NeilBrown
Quoted 86 lines · Zhilong Liu
Quoted 21 lines · NeilBrown
Quoted 19 lines · Zhilong Liu
Quoted 18 lines · NeilBrown
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 ifmd_modisn'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 <[email protected]>--- mdopen.c | 4 ++++ 1 file changed, 4 insertions(+)Quoted 10 lines
Quoted 10 lines
mdopen.c int create_mddev(char *dev, char *name, int autof, int trustworthy, · line 316
Quoted 10 lines
mdopen.c +2
-0@@ -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]
Quoted 2 lines
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__wurparameter, re-compile mdadm after delete the '__wur',
it works.Quoted 5 lines
In this case I really don't care - I'm not convinced an extra error message will really help.
Maybeif (system("modprobe md_mod") == 0) fd = open("/sys/......", O_WRONLY);Agree.
Quoted 4 lines
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?
mdopen.c +6
-3@@ -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" );
Quoted 8 lines
Quoted 4 lines
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?
mdopen.c +10 -6
@@ -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 (); }
Signature · 5 lines
2.6.6 Thanks, -Zhilong
Quoted 2 lines
Thanks,
NeilBrown
Quoted 98 lines · Zhilong Liu
Quoted 88 lines · NeilBrown
Quoted 86 lines · Zhilong Liu
Quoted 21 lines · NeilBrown
Quoted 19 lines · Zhilong Liu
Quoted 18 lines · NeilBrown
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 ifmd_modisn'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 <[email protected]>--- mdopen.c | 4 ++++ 1 file changed, 4 insertions(+)Quoted 10 lines
Quoted 10 lines
mdopen.c int create_mddev(char *dev, char *name, int autof, int trustworthy, · line 316
Quoted 10 lines
mdopen.c +2
-0@@ -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]
Quoted 2 lines
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__wurparameter, re-compile mdadm after delete the '__wur',
it works.Quoted 5 lines
In this case I really don't care - I'm not convinced an extra error message will really help.
Maybeif (system("modprobe md_mod") == 0) fd = open("/sys/......", O_WRONLY);Agree.
Quoted 4 lines
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?
mdopen.c +6
-3@@ -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" );Quoted 8 lines
Quoted 4 lines
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.
Quoted 2 lines
Thanks for your detail explanation. I draft it like this, and already checked it via to
./linux/scripts/checkpatch.pl.
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.
mdopen.c line 30
Quoted 12 lines
Is this changing ok to you?
mdopen.c +2
-0@@ -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.
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.
Thanks,
NeilBrown
Quoted 52 lines
mdopen.c +5
-4void 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 ();}mdopen.c +3
-2@@ -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 ();}Signature · 5 lines
2.6.6 Thanks, -ZhilongQuoted 2 lines
Thanks,
NeilBrown
Quoted 16 lines · NeilBrown
Quoted 1 line · NeilBrown
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.Quoted 10 lines
Is this changing ok to you?
mdopen.c +0
-0@@ -26,6 +26,8 @@#include "md_p.h" #include <ctype.h>
Quoted 6 lines
mdopen.c line 30
Quoted 2 lines
mdopen.c +2
-0+#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.
Quoted 3 lines
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.
mdopen.c +25 -20
@@ -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
Quoted 2 lines
Thanks,
NeilBrown
Quoted 24 lines · Zhilong Liu
Quoted 16 lines · NeilBrown
Quoted 1 line · NeilBrown
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.Quoted 10 lines
Is this changing ok to you?
mdopen.c +0
-0@@ -26,6 +26,8 @@#include "md_p.h" #include <ctype.h> Quoted 6 lines
mdopen.c line 30
Quoted 2 lines
mdopen.c +2
-0+#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.
Quoted 3 lines
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
Quoted 92 lines
Quoted 4 lines
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.
mdopen.c +25
-20@@ -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,
-ZhilongQuoted 2 lines
Thanks,
NeilBrown
Quoted 4 lines · NeilBrown
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.
Isn't that the kernel standard anyway?
"Wrap at 80 chars *unless* that would mean breaking a string literal".
Iirc an lwn story or something about how all subsystems tended to follow this rule except one, so they were going to try and standardise across everything.
Cheers,
Wol