Messages in this thread | | | Date | Wed, 28 Mar 2012 19:38:58 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH 4/6 OPTION-A version 3] completion: Add new wait_for_completion_timeout_state |
| |
On 03/26, Boaz Harrosh wrote: > > +int __sched > +wait_for_completion_timeout_state(struct completion *x, > + unsigned long timeout, int state) > +{ > + long t; > + int ret; > + > + if (!timeout) > + timeout = MAX_SCHEDULE_TIMEOUT; > + > + switch (state) { > + default: > + WARN_ON_ONCE(1); > + /* fall through */ > + case 0: > + state = TASK_UNINTERRUPTIBLE; > + break;
Well, this looks strange, imho. If the caller wants TASK_UNINTERRUPTIBLE, it should simply pass TASK_UNINTERRUPTIBLE. wait_for_completion_timeout_state(state => 0) looks confusing, and this is not symmetrical wrt other states.
> + t = wait_for_common(x, timeout, state); > + if (likely(t > 0)) { > + ret = 0; > + } else { > + if (t < 0) > + ret = t; > + else > + ret = -ETIMEDOUT; > + } > + return ret;
I tend to agree with Peter. This is the common helper, probably it will have more users. We shouldn't throw out the positive return value, it can be useful.
call_usermodehelper_exec() can simply do
retval = wait_for_common(...);
if (retval > 0) retval = sub_info->retval; else if (!retval) retval = -ETIMEDOUT;
Oleg.
| |