| Message ID | 20250121151603.235606-1-colyli@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mdopen: add sbin path to env PATH when call system("modprobe md_mod") | expand |
| Context | Check | Description |
|---|---|---|
| mdraidci/vmtest-md-6_14-PR | fail | merge-conflict |
On Tue, 21 Jan 2025 23:16:03 +0800 Coly Li <colyli@suse.de> wrote: > During the boot process if mdadm is called in udev context, sbin paths > like /sbin, /usr/sbin, /usr/local/sbin normally not defined in PATH > env variable, calling system("modprobe md_mod") in > create_named_array() may fail with 'sh: modprobe: command not found' > error message. > > We don't want to move modprobe binary into udev private directory, so > setting the PATH env is a more proper method to avoid the above issue. > > This patch sets PATH env variable with > "/sbin:/usr/sbin:/usr/local/sbin" before calling system("modprobe > md_mod"). The change only takes effect within the udev worker > context, not seen by global udev environment. Hi Coly, Nice explanation, thanks! > > Signed-off-by: Coly Li <colyli@suse.de> > --- > mdopen.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/mdopen.c b/mdopen.c > index 26f0c716..30cf781b 100644 > --- a/mdopen.c > +++ b/mdopen.c > @@ -39,6 +39,18 @@ int create_named_array(char *devnm) > > fd = open(new_array_file, O_WRONLY); > if (fd < 0 && errno == ENOENT) { > + char buf[PATH_MAX]; > + > + /* > + * When called by udev worker context, path of > modprobe > + * might not be in env PATH. Set sbin paths into PATH > + * env to avoid potential failure when run modprobe > here. > + */ > + memset(buf, 0, PATH_MAX); just: char buf[PATH_MAX] = {0}; > + snprintf(buf, PATH_MAX - 1, "%s:%s", getenv("PATH"), > + "/sbin:/usr/sbin:/usr/local/sbin"); > + setenv("PATH", buf, 1); Isn't it over-complicated? Why not simply: system("/sbin/modprobe md_mod"); If modprobe is not always in /sbin (checked on my opensuse only) we can make in configured during compilation, simple call `which modprobe` should do the job. What do you think? Thanks, Mariusz
> 2025年1月22日 02:09，Mariusz Tkaczyk <mtkaczyk@kernel.org> 写道： > > On Tue, 21 Jan 2025 23:16:03 +0800 > Coly Li <colyli@suse.de> wrote: > >> During the boot process if mdadm is called in udev context, sbin paths >> like /sbin, /usr/sbin, /usr/local/sbin normally not defined in PATH >> env variable, calling system("modprobe md_mod") in >> create_named_array() may fail with 'sh: modprobe: command not found' >> error message. >> >> We don't want to move modprobe binary into udev private directory, so >> setting the PATH env is a more proper method to avoid the above issue. >> >> This patch sets PATH env variable with >> "/sbin:/usr/sbin:/usr/local/sbin" before calling system("modprobe >> md_mod"). The change only takes effect within the udev worker >> context, not seen by global udev environment. > > Hi Coly, > Nice explanation, thanks! > >> >> Signed-off-by: Coly Li <colyli@suse.de> >> --- >> mdopen.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/mdopen.c b/mdopen.c >> index 26f0c716..30cf781b 100644 >> --- a/mdopen.c >> +++ b/mdopen.c >> @@ -39,6 +39,18 @@ int create_named_array(char *devnm) >> >> fd = open(new_array_file, O_WRONLY); >> if (fd < 0 && errno == ENOENT) { >> + char buf[PATH_MAX]; >> + >> + /* >> + * When called by udev worker context, path of >> modprobe >> + * might not be in env PATH. Set sbin paths into PATH >> + * env to avoid potential failure when run modprobe >> here. >> + */ >> + memset(buf, 0, PATH_MAX); > > just: > char buf[PATH_MAX] = {0}; OK, let me change this. > >> + snprintf(buf, PATH_MAX - 1, "%s:%s", getenv("PATH"), >> + "/sbin:/usr/sbin:/usr/local/sbin"); >> + setenv("PATH", buf, 1); > > Isn't it over-complicated? Why not simply: > system("/sbin/modprobe md_mod"); > > If modprobe is not always in /sbin (checked on my opensuse only) Yes, it might be in /usr/local/sbin, /usr/sbin/ or somewhere else which I am not able to take it for granted on other known or unknown distributions. > we can make in configured during compilation, simple call `which > modprobe` should do the job. > What do you think? Sure, it should work. But might be more complicated, I mean lines of code in total. Thanks for the review. Coly Li
diff --git a/mdopen.c b/mdopen.c index 26f0c716..30cf781b 100644 --- a/mdopen.c +++ b/mdopen.c @@ -39,6 +39,18 @@ int create_named_array(char *devnm) fd = open(new_array_file, O_WRONLY); if (fd < 0 && errno == ENOENT) { + char buf[PATH_MAX]; + + /* + * When called by udev worker context, path of modprobe + * might not be in env PATH. Set sbin paths into PATH + * env to avoid potential failure when run modprobe here. + */ + memset(buf, 0, PATH_MAX); + snprintf(buf, PATH_MAX - 1, "%s:%s", getenv("PATH"), + "/sbin:/usr/sbin:/usr/local/sbin"); + setenv("PATH", buf, 1); + if (system("modprobe md_mod") == 0) fd = open(new_array_file, O_WRONLY); }
During the boot process if mdadm is called in udev context, sbin paths like /sbin, /usr/sbin, /usr/local/sbin normally not defined in PATH env variable, calling system("modprobe md_mod") in create_named_array() may fail with 'sh: modprobe: command not found' error message. We don't want to move modprobe binary into udev private directory, so setting the PATH env is a more proper method to avoid the above issue. This patch sets PATH env variable with "/sbin:/usr/sbin:/usr/local/sbin" before calling system("modprobe md_mod"). The change only takes effect within the udev worker context, not seen by global udev environment. Signed-off-by: Coly Li <colyli@suse.de> --- mdopen.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)