> 2025年1月22日 20:01，Mariusz Tkaczyk <mtkaczyk@xxxxxxxxxx> 写道：
>
> Hi Coly,
> I read that once again and I have more comments. Even if it
> looks simple, we are calling many env commands in mdadm, that is why I'm
> trying to be careful.
>
> On Wed, 22 Jan 2025 11:53:59 +0800
> Coly Li <colyli@xxxxxxx> 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
>
> normally defined? Please remove "not".
This is correct, normally NOT defined. In boot time udev tasks just normally called
binaries in their private directory, they don’t call system binaries.
>
>> 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.
>
> Curios, did you verified what is happening to our "systemctl" calls?
>
> mdmon and grow-continue are started this way, they are later followed by
> "WANTS=" in udev rule so the issue there is probably hidden, maybe we
> should fix these calls too?
For this specific case, md kernel module was not loaded yet, it was in quite early
stage from observation of me and bug reporter.
>
>>
>> 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.
>
> If we are running app from terminal (i.e mdadm -I, or ./mdadm -I) this
> change should not affect the terminal environment. I verified it to be
> sure. Could you please mention that in description?
>
OK, let me do it in next version.
>>
>> Signed-off-by: Coly Li <colyli@xxxxxxx>
>> ---
>> Changelog,
>> v2: set buf[PATH_MAX] to 0 in stack variable announcement.
>> v1: the original version.
>>
>>
>> mdopen.c | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/mdopen.c b/mdopen.c
>> index 26f0c716..65bd8a1b 100644
>> --- a/mdopen.c
>> +++ b/mdopen.c
>> @@ -39,6 +39,17 @@ int create_named_array(char *devnm)
>>
>> fd = open(new_array_file, O_WRONLY);
>> if (fd < 0 && errno == ENOENT) {
>> + char buf[PATH_MAX] = {0};
>> +
>> + /*
>> + * 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.
>> + */
>> + snprintf(buf, PATH_MAX - 1, "%s:%s", getenv("PATH"),
>> + "/sbin:/usr/sbin:/usr/local/sbin");
>
> We can get NULL returned by getenv("PATH"), should we handle it? We
> probably rely on compiler behavior here.
>
> I did simple test. I tried: printf("%s\n", getenv("NOT_EXISTING"));
> I got segmentation fault.
Yes, this was my fault, I took it for granted that PATH should always be set in udev context.
You are right, I will add a NULL check in next version.
>
>> + setenv("PATH", buf, 1);
>
> I see here portability issues. We, assume that these binaries must be
> in locations we added here. We may even double them, if they are
> already defined.
> Even if I know that probably no one is enough brave to
> not have base binaries there, we should not force our PATH. I think it
> is not our task and responsibility to deal with binaries location
> issues. We should take what system provided.
>
> I still think that we should pass locations during compilation. Here
> example with EXTRAVERSION, of course it may require some adjustments
> but it is generally the way it can be achieved:
> https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=03ab9763f51ddf2030f60f83e76cf9c1b50b726c
>
> I'm not strong convinced to the option I proposed, I just need
> argument because more or less code is not an argument. What we will
> choose today will stay here for years, we need to choose the best
> possible way we see.
The installation path might vary, depends on the way how mdadm is installed. If from rpm or other
installation pack, the dest location can be predicted. If mdadm is installed from source code compiling,
the destination varies depends on the pre-defined installation location, similar situation happens in containers.
So the patch is just a best-effort-try, if the binary is not installed in /sbin, /usr/sbin or /usr/local/sbin, my patch just gives up.
>
>> +
>> if (system("modprobe md_mod") == 0)
>> fd = open(new_array_file, O_WRONLY);
>
>> }
>
> The change will affect code executed later, probably we don't want
> that. Shouldn't we restore old PATH here to minimize risk?
For my understanding if the code was called after boot up, these path should be set already by shell initialization scripts.
And for udev context, it is called and exited, and NOT shared with other udev tasks, almost no influence.
Thanks for your detailed comments.
Coly Li
