Messages in this thread | | | Date | Wed, 14 Aug 2019 16:35:46 +0200 | From | Christian Brauner <> | Subject | Re: [PATCH v2 1/1] waitid: Add support for waiting for the current process group |
| |
On Wed, Aug 14, 2019 at 04:19:57PM +0200, Oleg Nesterov wrote: > On 08/14, Christian Brauner wrote: > > > > +static struct pid *find_get_pgrp(pid_t nr) > > +{ > > + struct pid *pid; > > + > > + if (nr) > > + return find_get_pid(nr); > > + > > + rcu_read_lock(); > > + pid = get_pid(task_pgrp(current)); > > + rcu_read_unlock(); > > + > > + return pid; > > +} > > I can't say I like this helper... even its name doesn't look good to me.
Well, naming scheme obviously stolen from find_get_pid(). Not sure if that doesn't look good as well. ;)
> > I forgot that we already have get_task_pid() when I replied to the previous > version... How about > > case P_PGID: > > if (upid) > pid = find_get_pid(upid); > else > pid = get_task_pid(current, PIDTYPE_PGID); > > ?
Hmyeah, that works but wouldn't it still be nicer to simply have:
static struct pid *get_pgrp(pid_t nr) { if (nr) return find_get_pid(nr);
return get_task_pid(current, PIDTYPE_PGID); }
That allows us to have all cases equivalent with a single call to a function without any if-elses. Imho, this becomes especially nice when considering that P_PIDFD goes on top of that:
switch (which) { case P_ALL: type = PIDTYPE_MAX; break; case P_PID: type = PIDTYPE_PID; if (upid <= 0) return -EINVAL;
pid = find_get_pid(upid); break; case P_PGID: type = PIDTYPE_PGID; if (upid < 0) return -EINVAL;
pid = find_get_pgrp(upid); break; case P_PIDFD: type = PIDTYPE_PID; if (upid < 0) return -EINVAL;
pid = pidfd_get_pid(upid); if (IS_ERR(pid)) return PTR_ERR(pid); break; default: return -EINVAL; }
Christian
| |