Messages in this thread | | | Date | Wed, 28 Mar 2012 19:04:01 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH 5/6] kmod: Add new call_usermodehelper_timeout() API |
| |
On 03/27, Oleg Nesterov wrote: > > On 03/26, Boaz Harrosh wrote: > > > > int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait) > > { > > DECLARE_COMPLETION_ONSTACK(done); > > + int wait_state; > > int retval = 0; > > > > helper_lock(); > > @@ -540,19 +541,15 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait) > > if (wait == UMH_NO_WAIT) /* task has freed sub_info */ > > goto unlock; > > > > - if (wait & UMH_KILLABLE) { > > - retval = wait_for_completion_killable(&done); > > - if (!retval) > > - goto wait_done; > > - > > + wait_state = (wait & UMH_KILLABLE) ? TASK_KILLABLE : 0; > > + retval = wait_for_completion_timeout_state(&done, sub_info->timeout, > > + wait_state); > > + if (unlikely(retval)) { > > /* umh_complete() will see NULL and free sub_info */ > > if (xchg(&sub_info->complete, NULL)) > > goto unlock; > > - /* fallthrough, umh_complete() was already called */ > > } > > > > - wait_for_completion(&done); > > at first glance this looks certainly wrong, or I misread the patch. > > We can't remove the "fallback to wait_for_completion" logic until > you move the completion into subprocess_info (the next patch seems > to do this).
Yes, I think this is true after I re-checked the code.
One more nit. With this patch call_usermodehelper_fns() does
info = call_usermodehelper_setup(path, argv, envp, gfp_mask);
call_usermodehelper_setfns(info, init, cleanup, data);
info->timeout = timeout;
We have 2 (static!) helpers to initialize info, yet we initialize ->timeout by hand. Of course I do not blame this patch, but imho this looks a bit messy and deserves another minor cleanup.
Oleg.
| |