Date: Sat, 18 Mar 2006 16:39:25 GMT From: Robert Watson <rwatson@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 93509 for review Message-ID: <200603181639.k2IGdPnb095561@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=93509 Change 93509 by rwatson@rwatson_peppercorn on 2006/03/18 16:38:25 Move log rotation logic out of audit_worker() into audit_worker_rotate(). This makes the event loop a bit easier to read. Comment on some ways in which the event loop could be made better. Affected files ... .. //depot/projects/trustedbsd/audit3/sys/security/audit/audit.c#19 edit Differences ... ==== //depot/projects/trustedbsd/audit3/sys/security/audit/audit.c#19 (text+ko) ==== @@ -459,6 +459,71 @@ } /* + * If an appropriate signal has been received rotate the audit log based on + * the global replacement variables. Signal consumers as needed that the + * rotation has taken place. + * + * XXXRW: The global variables and CVs used to signal the audit_worker to + * perform a rotation are essentially a message queue of depth 1. It would + * be much nicer to actually use a message queue. + */ +static void +audit_worker_rotate(struct ucred **audit_credp, struct vnode **audit_vpp, + struct thread *audit_td) +{ + int do_replacement_signal, vfslocked; + struct ucred *old_cred; + struct vnode *old_vp; + + mtx_assert(&audit_mtx, MA_OWNED); + + do_replacement_signal = 0; + while (audit_replacement_flag != 0) { + old_cred = *audit_credp; + old_vp = *audit_vpp; + *audit_credp = audit_replacement_cred; + *audit_vpp = audit_replacement_vp; + audit_replacement_cred = NULL; + audit_replacement_vp = NULL; + audit_replacement_flag = 0; + + audit_enabled = (*audit_vpp != NULL); + + /* + * XXX: What to do about write failures here? + */ + if (old_vp != NULL) { + AUDIT_PRINTF(("Closing old audit file\n")); + mtx_unlock(&audit_mtx); + vfslocked = VFS_LOCK_GIANT(old_vp->v_mount); + vn_close(old_vp, AUDIT_CLOSE_FLAGS, old_cred, + audit_td); + VFS_UNLOCK_GIANT(vfslocked); + crfree(old_cred); + mtx_lock(&audit_mtx); + old_cred = NULL; + old_vp = NULL; + AUDIT_PRINTF(("Audit file closed\n")); + } + if (*audit_vpp != NULL) { + AUDIT_PRINTF(("Opening new audit file\n")); + } + do_replacement_signal = 1; + } + + /* + * Signal that replacement have occurred to wake up and + * start any other replacements started in parallel. We can + * continue about our business in the mean time. We + * broadcast so that both new replacements can be inserted, + * but also so that the source(s) of replacement can return + * successfully. + */ + if (do_replacement_signal) + cv_broadcast(&audit_replacement_cv); +} + +/* * The audit_worker thread is responsible for watching the event queue, * dequeueing records, converting them to BSM format, and committing them to * disk. In order to minimize lock thrashing, records are dequeued in sets @@ -469,14 +534,12 @@ static void audit_worker(void *arg) { - int do_replacement_signal, error; TAILQ_HEAD(, kaudit_record) ar_worklist; struct kaudit_record *ar; - struct vnode *audit_vp, *old_vp; - int vfslocked; - - struct ucred *audit_cred, *old_cred; + struct ucred *audit_cred; struct thread *audit_td; + struct vnode *audit_vp; + int error; AUDIT_PRINTF(("audit_worker starting\n")); @@ -490,59 +553,18 @@ mtx_lock(&audit_mtx); while (1) { + mtx_assert(&audit_mtx, MA_OWNED); + /* - * First priority: replace the audit log target if requested. - * Accessing the vnode here requires dropping the audit_mtx; - * in case another replacement was scheduled while the mutex - * was released, we loop. - * - * XXX It could well be we should drain existing records - * first to ensure that the timestamps and ordering - * are right. + * XXXRW: Logic here should really be: while (!events and + * !records) cv_wait(), then process events, and then + * records. */ - do_replacement_signal = 0; - while (audit_replacement_flag != 0) { - old_cred = audit_cred; - old_vp = audit_vp; - audit_cred = audit_replacement_cred; - audit_vp = audit_replacement_vp; - audit_replacement_cred = NULL; - audit_replacement_vp = NULL; - audit_replacement_flag = 0; - - audit_enabled = (audit_vp != NULL); - /* - * XXX: What to do about write failures here? - */ - if (old_vp != NULL) { - AUDIT_PRINTF(("Closing old audit file\n")); - mtx_unlock(&audit_mtx); - vfslocked = VFS_LOCK_GIANT(old_vp->v_mount); - vn_close(old_vp, AUDIT_CLOSE_FLAGS, old_cred, - audit_td); - VFS_UNLOCK_GIANT(vfslocked); - crfree(old_cred); - mtx_lock(&audit_mtx); - old_cred = NULL; - old_vp = NULL; - AUDIT_PRINTF(("Audit file closed\n")); - } - if (audit_vp != NULL) { - AUDIT_PRINTF(("Opening new audit file\n")); - } - do_replacement_signal = 1; - } /* - * Signal that replacement have occurred to wake up and - * start any other replacements started in parallel. We can - * continue about our business in the mean time. We - * broadcast so that both new replacements can be inserted, - * but also so that the source(s) of replacement can return - * successfully. + * First priority: replace the audit log target if requested. */ - if (do_replacement_signal) - cv_broadcast(&audit_replacement_cv); + audit_worker_rotate(&audit_cred, &audit_vp, audit_td); /* * Next, check to see if we have any records to drain into
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200603181639.k2IGdPnb095561>