Re: In-kernel process exit hooks?

[Paul Goyette <paul%whooppee.com@localhost>]

On Fri, 8 Jan 2016, Paul Goyette wrote:

> Hmmm, option #3 doesn't work so well, after all!
>
> I tried it when both the target and monitor processes were children of the 
> same shell process.  Thus all three of these share the same 'struct file' for 
> their stdout, and the filemon_fd_close() callback cannot tell which one is 
> being closed.  The all have the same fd (just in different descriptor 
> tables?)
>
> So next I'm going to try kre's suggestion about using a "proper" refcnt 
> mechanism and see if that helps.

Robert Elz wrote:

> Actually, thinking through this more, why not just "fix" filemon to
> make a proper reference to the file, instead of the half baked thing
> it is currently doing.
>
> That is, instead of a fd_getfile() without an (almost immediate)
> fd_putfile() so keeping ff_refcount incremented, in filemon, just do
>
>        fp = fd_getfile(...);
>        fp->f_count++;
>        fd_putfile(...);
>
> so filemon has a proper reference to the file*.   When it is done, it
> just closes it, like any other file would be closed (which decrements
> f_count and frees the struct file if it goes to 0).
>
> Wouldn't this solve all the problems, keep the semantics the same, and
> just generally be cleaner?

Option 4 works!  See attached diffs.  Thanks, kre!


I'll commit this in a day or two unless I receive some extremely 
negative feedback.  I know that filemon(4) is not the best code, and it 
really needs a total redesign and rewrite, but that's going to take a 
fair amount of time.  Until then, I think with these changes that we'll 
have a safe-and-stable implementation.  (And if it is decided to remove 
the current code completely, at least we can have a safe-and-stable copy 
in the attic!)


+------------------+--------------------------+------------------------+
| Paul Goyette     | PGP Key fingerprint:     | E-mail addresses:      |
| (Retired)        | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com   |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+------------------+--------------------------+------------------------+

Index: filemon.c
===================================================================
RCS file: /cvsroot/src/sys/dev/filemon/filemon.c,v
retrieving revision 1.24
diff -u -p -r1.24 filemon.c
--- filemon.c	5 Jan 2016 22:08:54 -0000	1.24
+++ filemon.c	8 Jan 2016 06:39:58 -0000
@@ -222,7 +222,6 @@ filemon_open(dev_t dev, int oflags __unu
 
 	filemon = kmem_alloc(sizeof(struct filemon), KM_SLEEP);
 	rw_init(&filemon->fm_mtx);
-	filemon->fm_fd = -1;
 	filemon->fm_fp = NULL;
 	filemon->fm_pid = curproc->p_pid;
 
@@ -265,7 +264,7 @@ filemon_close(struct file * fp)
 	 */
 	rw_enter(&filemon->fm_mtx, RW_WRITER);
 	if (filemon->fm_fp) {
-		fd_putfile(filemon->fm_fd);	/* release our reference */
+		closef(filemon->fm_fp);	/* release our reference */
 		filemon->fm_fp = NULL;
 	}
 	rw_exit(&filemon->fm_mtx);
@@ -279,6 +278,7 @@ static int
 filemon_ioctl(struct file * fp, u_long cmd, void *data)
 {
 	int error = 0;
+	int fd;
 	struct filemon *filemon;
 	struct proc *tp;
 
@@ -301,14 +301,16 @@ filemon_ioctl(struct file * fp, u_long c
 
 		/* First, release any current output file descriptor */
 		if (filemon->fm_fp)
-			fd_putfile(filemon->fm_fd);
+			closef(filemon->fm_fp);
 
 		/* Now set up the new one */
-		filemon->fm_fd = *((int *) data);
-		if ((filemon->fm_fp = fd_getfile(filemon->fm_fd)) == NULL) {
+		fd = *((int *) data);
+		if ((filemon->fm_fp = fd_getfile(fd)) == NULL) {
 			error = EBADF;
 			break;
 		}
+		filemon->fm_fp->f_count++;
+		fd_putfile(fd);
 		/* Write the file header. */
 		filemon_comment(filemon);
 		break;
Index: filemon.h
===================================================================
RCS file: /cvsroot/src/sys/dev/filemon/filemon.h,v
retrieving revision 1.8
diff -u -p -r1.8 filemon.h
--- filemon.h	25 Nov 2015 07:34:49 -0000	1.8
+++ filemon.h	8 Jan 2016 06:39:58 -0000
@@ -41,7 +41,6 @@ struct filemon {
 	char fm_fname1[MAXPATHLEN];/* Temporary filename buffer. */
 	char fm_fname2[MAXPATHLEN];/* Temporary filename buffer. */
 	char fm_msgbufr[32 + 2 * MAXPATHLEN];	/* Output message buffer. */
-	int fm_fd;			/* Output fd */
 	struct file *fm_fp;	/* Output file pointer. */
 	krwlock_t fm_mtx;		/* Lock mutex for this filemon. */
 	TAILQ_ENTRY(filemon) fm_link;	/* Link into the in-use list. */