Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757050AbZFWIYE (ORCPT ); Tue, 23 Jun 2009 04:24:04 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751495AbZFWIXv (ORCPT ); Tue, 23 Jun 2009 04:23:51 -0400 Received: from zelda.netsplit.com ([87.194.19.211]:33971 "EHLO zelda.netsplit.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751410AbZFWIXt (ORCPT ); Tue, 23 Jun 2009 04:23:49 -0400 Subject: Re: [PATCH] proc connector: add event for process becoming session leader From: Scott James Remnant To: Andrew Morton Cc: linux-kernel@vger.kernel.org, Matt Helsley In-Reply-To: <20090622161909.e5706885.akpm@linux-foundation.org> References: <20090622161909.e5706885.akpm@linux-foundation.org> Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-DnFNN84UR4ufDIiUsIuP" Date: Tue, 23 Jun 2009 09:23:40 +0100 Message-Id: <1245745420.3477.16.camel@wing-commander> Mime-Version: 1.0 X-Mailer: Evolution 2.27.3 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6218 Lines: 182 --=-DnFNN84UR4ufDIiUsIuP Content-Type: text/plain Content-Transfer-Encoding: quoted-printable On Mon, 2009-06-22 at 16:19 -0700, Andrew Morton wrote: > Let's cc the Process Events developer.. >=20 Ah great, thanks - I couldn't find an e-mail address. > On Mon, 15 Jun 2009 13:03:08 +0100 > Scott James Remnant wrote: >=20 > > The act of a process becoming a session leader is a useful signal to a > > supervising init daemon such as Upstart. > >=20 > > While a daemon will normally do this as part of the process of becoming > > a daemon, it is rare for its children to do so. When the children do, > > it is nearly always a sign that the child should be considered detached > > from the parent and not supervised along with it. > >=20 > > The poster-child example is OpenSSH; the per-login children call setsid= () > > so that they may control the pty connected to them. If the primary dae= mon > > dies or is restarted, we do not want to consider the per-login children > > and want to respawn the primary daemon without killing the children. > >=20 > > This patch adds a new PROC_SID_EVENT and associated structure to the > > proc_event event_data union, it arranges for this to be emitted when > > the special PIDTYPE_SID pid is set. > >=20 >=20 > hm, well, I don't have much useful to say about the overall idea, but > it seems to slot into the existing code simply enough. >=20 > > --- > > drivers/connector/cn_proc.c | 25 +++++++++++++++++++++++++ > > include/linux/cn_proc.h | 10 ++++++++++ > > kernel/exit.c | 4 +++- > > 3 files changed, 38 insertions(+), 1 deletions(-) >=20 > We seem to have forgotten to document this entire interface, so I can't > ding you for forgetting to update the forgotten documentation. >=20 > > diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c > > index c5afc98..7d48cd9 100644 > > --- a/drivers/connector/cn_proc.c > > +++ b/drivers/connector/cn_proc.c > > @@ -139,6 +139,31 @@ void proc_id_connector(struct task_struct *task, i= nt which_id) > > cn_netlink_send(msg, CN_IDX_PROC, GFP_KERNEL); > > } > > =20 > > +void proc_sid_connector(struct task_struct *task) >=20 > It would be nice to have a nice comment explaining what this function > does. Ditto all the others in there, really. >=20 > > +{ > > + struct cn_msg *msg; > > + struct proc_event *ev; > > + struct timespec ts; > > + __u8 buffer[CN_PROC_MSG_SIZE]; > > + > > + if (atomic_read(&proc_event_num_listeners) < 1) > > + return; > > + > > + msg =3D (struct cn_msg*)buffer; > > + ev =3D (struct proc_event*)msg->data; >=20 > Please pass all patches through scripts/checkpatch.pl. >=20 > > + get_seq(&msg->seq, &ev->cpu); > > + ktime_get_ts(&ts); /* get high res monotonic timestamp */ > > + put_unaligned(timespec_to_ns(&ts), (__u64 *)&ev->timestamp_ns); > > + ev->what =3D PROC_EVENT_SID; > > + ev->event_data.sid.process_pid =3D task->pid; >=20 > This is a bit of a worry. In a containerised environment, pids are not > unique. Now what do we do? >=20 I must admit, that I just copy and pasted the existing functions - bugs and all :-) > > + ev->event_data.sid.process_tgid =3D task->tgid; > > + > > + memcpy(&msg->id, &cn_proc_event_id, sizeof(msg->id)); > > + msg->ack =3D 0; /* not used */ > > + msg->len =3D sizeof(*ev); > > + cn_netlink_send(msg, CN_IDX_PROC, GFP_KERNEL); > > +} > > + > > void proc_exit_connector(struct task_struct *task) > > { > > struct cn_msg *msg; > > diff --git a/include/linux/cn_proc.h b/include/linux/cn_proc.h > > index b8125b2..47dac5e 100644 > > --- a/include/linux/cn_proc.h > > +++ b/include/linux/cn_proc.h > > @@ -52,6 +52,7 @@ struct proc_event { > > PROC_EVENT_EXEC =3D 0x00000002, > > PROC_EVENT_UID =3D 0x00000004, > > PROC_EVENT_GID =3D 0x00000040, > > + PROC_EVENT_SID =3D 0x00000080, > > /* "next" should be 0x00000400 */ > > /* "last" is the last process event: exit */ > > PROC_EVENT_EXIT =3D 0x80000000 > > @@ -89,6 +90,11 @@ struct proc_event { > > } e; > > } id; > > =20 > > + struct sid_proc_event { > > + __kernel_pid_t process_pid; > > + __kernel_pid_t process_tgid; > > + } sid; > > + > > struct exit_proc_event { > > __kernel_pid_t process_pid; > > __kernel_pid_t process_tgid; > > @@ -102,6 +108,7 @@ struct proc_event { > > void proc_fork_connector(struct task_struct *task); > > void proc_exec_connector(struct task_struct *task); > > void proc_id_connector(struct task_struct *task, int which_id); > > +void proc_sid_connector(struct task_struct *task); > > void proc_exit_connector(struct task_struct *task); > > #else > > static inline void proc_fork_connector(struct task_struct *task) > > @@ -114,6 +121,9 @@ static inline void proc_id_connector(struct task_st= ruct *task, > > int which_id) > > {} > > =20 > > +static inline void proc_sid_connector(struct task_struct *task) > > +{} > > + > > static inline void proc_exit_connector(struct task_struct *task) > > {} > > #endif /* CONFIG_PROC_EVENTS */ > > diff --git a/kernel/exit.c b/kernel/exit.c > > index b6c90b5..5162589 100644 > > --- a/kernel/exit.c > > +++ b/kernel/exit.c > > @@ -360,8 +360,10 @@ void __set_special_pids(struct pid *pid) > > { > > struct task_struct *curr =3D current->group_leader; > > =20 > > - if (task_session(curr) !=3D pid) > > + if (task_session(curr) !=3D pid) { > > change_pid(curr, PIDTYPE_SID, pid); > > + proc_sid_connector(curr); > > + } > > =20 > > if (task_pgrp(curr) !=3D pid) > > change_pid(curr, PIDTYPE_PGID, pid); >=20 Scott --=20 Scott James Remnant scott@ubuntu.com --=-DnFNN84UR4ufDIiUsIuP Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) iEYEABECAAYFAkpAkQwACgkQSnQiFMl4yK6/TgCgjKac+2iHUeLFXlZnjes58QDH M/AAoKyXRhILnHtWSz94e4woi70F1djf =bijE -----END PGP SIGNATURE----- --=-DnFNN84UR4ufDIiUsIuP-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/