Message ID | 20190814130732.23572-2-christian.brauner@ubuntu.com |
---|---|
State | New |
Headers | show |
Series | waitid: process group enhancement | expand |
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. 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); ? Oleg.
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
On 08/14, Christian Brauner wrote: > > 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. ;) find_get_pid() actually tries to find a pid. The helper above does "find" or "use current" depending on nr != 0. > > 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); > } Who else can ever use it? It saves 4 lines in kernel_waitid() but adds 7 lines outside, and you need another ^] to see these lines if you try to understand what PIDTYPE_PGID actually does. IOW, I think this helper will make waitid less readable for no reason. Christian, I try to never argue when it comes to cosmetic issues, and in this case I won't insist too. Oleg.
On Wed, Aug 14, 2019 at 05:27:12PM +0200, Oleg Nesterov wrote: > On 08/14, Christian Brauner wrote: > > > > 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. ;) > > find_get_pid() actually tries to find a pid. The helper above does "find" > or "use current" depending on nr != 0. > > > > 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); > > } > > Who else can ever use it? > > It saves 4 lines in kernel_waitid() but adds 7 lines outside, and you > need another ^] to see these lines if you try to understand what > PIDTYPE_PGID actually does. IOW, I think this helper will make waitid > less readable for no reason. > > > Christian, I try to never argue when it comes to cosmetic issues, and > in this case I won't insist too. Yeah, I know. I'm not insisisting either. We can do your thing since you do after all seem to care at least a tiny bit. ;) Christian
diff --git a/kernel/exit.c b/kernel/exit.c index 5b4a5dcce8f8..d02715a39f68 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -1554,6 +1554,20 @@ static long do_wait(struct wait_opts *wo) return retval; } +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; +} + static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop, int options, struct rusage *ru) { @@ -1576,19 +1590,20 @@ static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop, type = PIDTYPE_PID; if (upid <= 0) return -EINVAL; + + pid = find_get_pid(upid); break; case P_PGID: type = PIDTYPE_PGID; - if (upid <= 0) + if (upid < 0) return -EINVAL; + + pid = find_get_pgrp(upid); break; default: return -EINVAL; } - if (type < PIDTYPE_MAX) - pid = find_get_pid(upid); - wo.wo_type = type; wo.wo_pid = pid; wo.wo_flags = options;