Hi Oleg,
On Mon, Feb 25, 2013 at 3:08 PM, Oleg Nesterov <o...@redhat.com> wrote:
> On 02/25, Lucas De Marchi wrote:
>>
>> Yep. The current interface is confusing. I agree that a separate
>> setup() + exec() would make more sense.
>
> Great,
>
>> > @@ -98,8 +98,14 @@ static int call_modprobe(char *module_na
>> > argv[3] = module_name; /* check free_modprobe_argv() */
>> > argv[4] = NULL;
>> >
>> > - return call_usermodehelper_fns(modprobe_path, argv, envp,
>> > - wait | UMH_KILLABLE, NULL, free_modprobe_argv, NULL);
>> > + info = call_usermodehelper_setup(...); // better name, please...
>> > + if (!info)
>> > + goto free_modname;
>> > +
>> > + return call_usermodehelper_exec(info, wait);
>>
>> I'd say that in these cases the "call_" prefix has no meaning, and we
>> could just use a "usermodehelper" as the namespace.
>
> Oh, I agree with any naming.
>
> So, I hope you will send v2. I'd suggest to split the fixes. 1/3
> should create/export the new helpers, and 2-3 fix should call_modprobe()
> and call_usermodehelper_keys(). But this is up to you, I won't insist.
I was implementing this today, but looking into call_modprobe(), it is
never called with UMH_NO_WAIT. Doesn't it mean we can use a similar
trick as you used in __orderly_poweroff()? Then something similar to
this would be sufficient in this case (whitespace damaged):
diff --git a/kernel/kmod.c b/kernel/kmod.c
index b39f240..e4d6f6f 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -69,12 +69,6 @@ static DECLARE_RWSEM(umhelper_sem);
*/
char modprobe_path[KMOD_PATH_LEN] = "/sbin/modprobe";
-static void free_modprobe_argv(struct subprocess_info *info)
-{
- kfree(info->argv[3]); /* check call_modprobe() */
- kfree(info->argv);
-}
-
static int call_modprobe(char *module_name, int wait)
{
static char *envp[] = {
@@ -83,6 +77,7 @@ static int call_modprobe(char *module_name, int wait)
"PATH=/sbin:/usr/sbin:/bin:/usr/bin",
NULL
};
+ int ret = -ENOMEM;
char **argv = kmalloc(sizeof(char *[5]), GFP_KERNEL);
if (!argv)
@@ -98,12 +93,13 @@ static int call_modprobe(char *module_name, int wait)
argv[3] = module_name; /* check free_modprobe_argv() */
argv[4] = NULL;
- return call_usermodehelper_fns(modprobe_path, argv, envp,
- wait | UMH_KILLABLE, NULL, free_modprobe_argv, NULL);
+ ret = call_usermodehelper(modprobe_path, argv, envp,
+ wait | UMH_KILLABLE);
+ kfree(module_name);
free_argv:
kfree(argv);
out:
- return -ENOMEM;
+ return ret;
}
/**
Lucas De Marchi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
