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>