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. */
