Date: Tue, 2 Jul 2002 23:23:48 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Johan Karlsson <johan@FreeBSD.ORG> Cc: freebsd-audit@FreeBSD.ORG, <sheldonh@FreeBSD.ORG> Subject: Re: accounting to appen only file Message-ID: <20020702224153.U12090-100000@gamplex.bde.org> In-Reply-To: <20020701204140.A49191@numeri.campus.luth.se>
next in thread | previous in thread | raw e-mail | index | archive | help
> I have been running with the equivalent patch on stable
> for a while and it works as supposed for me. However, since
> I'm not that familiar with the vn code I would like to get
> feedback if this might be harmfull in any way.
> Index: src/sys/kern//kern_acct.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/kern/kern_acct.c,v
> retrieving revision 1.44
> diff -u -r1.44 kern_acct.c
> --- src/sys/kern//kern_acct.c 16 May 2002 21:28:11 -0000 1.44
> +++ src/sys/kern//kern_acct.c 30 Jun 2002 19:39:55 -0000
> @@ -119,6 +119,9 @@
> struct nameidata nd;
> int error, flags;
>
> + /* Open file for append writing only */
> + flags = FWRITE | FAPPEND;
> +
This has some style bugs: excessive commenting, an unterminated sentence,
and some less than useful code restructuring (moving the initialization
of `flags'. (This file is excessively commented in several places. I
think the comments in acct_process() are just to show that it was
implemented in a clean room starting from documentation.)
> /* Make sure that the caller is root. */
> error = suser(td);
Excessive commenting in old code.
> if (error)
> @@ -126,20 +129,19 @@
>
> mtx_lock(&Giant);
> /*
> - * If accounting is to be started to a file, open that file for
> - * writing and make sure it's a 'normal'.
> + * If accounting is to be started to a file, open that file
> + * and make sure it's a 'normal'.
> */
> if (SCARG(uap, path) != NULL) {
> NDINIT(&nd, LOOKUP, NOFOLLOW, UIO_USERSPACE, SCARG(uap, path),
> td);
> - flags = FWRITE;
I would just change FWRITE globally to FWRITE | FAPPEND (and adjust
the comment to say "appending"). That would be a little chummy with
the implementation, but so is the existing and new code, and fixing
this is beyond the scope of the PR.
> error = vn_open(&nd, &flags, 0);
Note that the flags variable was only used to discard some changes made
by vn_open(). At least in the direct descendant of vn_open(), these changes
are limited to clearing O_CREAT and O_TRUNC, but we really shouldn't assume
this. Strictly, we should record returned flags for use late like the `file'
layer would do.
> if (error)
> goto done2;
> NDFREE(&nd, NDF_ONLY_PNBUF);
> VOP_UNLOCK(nd.ni_vp, 0, td);
> if (nd.ni_vp->v_type != VREG) {
> - vn_close(nd.ni_vp, FWRITE, td->td_ucred, td);
> + vn_close(nd.ni_vp, flags, td->td_ucred, td);
This fixes an error case to use the returned flags.
> error = EACCES;
> goto done2;
> }
> @@ -151,7 +153,7 @@
> */
> if (acctp != NULLVP || savacctp != NULLVP) {
> callout_stop(&acctwatch_callout);
> - error = vn_close((acctp != NULLVP ? acctp : savacctp), FWRITE,
> + error = vn_close((acctp != NULLVP ? acctp : savacctp), flags,
Here using the returned flags is (logically) worse than using hard-coded
flags, since we are closing a different file so we should strictly use the
flags that were returned when we opened that file, nit the flags for the
file that we just opened.
> td->td_ucred, td);
> acctp = savacctp = NULLVP;
> }
I think FAPPEND should be spelled O_APPEND in the above. The former
spelling is currently not used anywhere in *.c under /sys except in
alpha/osf1. It should be only compatibility cruft for userland.
I doubt that vn_close() actually cares about the FAPPEND flag.
There are some more FWRITE's in acctwatch(). These are only for closes,
so the correctness of the above patches depends on vn_close() not caring.
Bruce
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-audit" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020702224153.U12090-100000>