This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2 1/1] waitid: Add support for waiting for the current process group
- From: Christian Brauner <christian dot brauner at ubuntu dot com>
- To: Oleg Nesterov <oleg at redhat dot com>
- Cc: linux-kernel at vger dot kernel dot org, libc-alpha at sourceware dot org, alistair23 at gmail dot com, ebiederm at xmission dot com, arnd at arndb dot de, dalias at libc dot org, torvalds at linux-foundation dot org, adhemerval dot zanella at linaro dot org, fweimer at redhat dot com, palmer at sifive dot com, macro at wdc dot com, zongbox at gmail dot com, akpm at linux-foundation dot org, viro at zeniv dot linux dot org dot uk, hpa at zytor dot com
- Date: Wed, 14 Aug 2019 17:30:23 +0200
- Subject: Re: [PATCH v2 1/1] waitid: Add support for waiting for the current process group
- References: <CAKmqyKMJPQAOKn11xepzAwXOd4e9dU0Cyz=A0T-uMEgUp5yJjA@mail.gmail.com> <20190814130732.23572-1-christian.brauner@ubuntu.com> <20190814130732.23572-2-christian.brauner@ubuntu.com> <20190814141956.GC11595@redhat.com> <20190814143545.tu6xfp2mxmnzwkx4@wittgenstein> <20190814152712.GE11595@redhat.com>
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