From owner-p4-projects@FreeBSD.ORG Sat Mar 18 16:39:27 2006 Return-Path: X-Original-To: p4-projects@freebsd.org Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id 2AE4C16A423; Sat, 18 Mar 2006 16:39:27 +0000 (UTC) X-Original-To: perforce@freebsd.org Delivered-To: perforce@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id E4AC516A41F for ; Sat, 18 Mar 2006 16:39:26 +0000 (UTC) (envelope-from bb+lists.freebsd.perforce@cyrus.watson.org) Received: from repoman.freebsd.org (repoman.freebsd.org [216.136.204.115]) by mx1.FreeBSD.org (Postfix) with ESMTP id 4F35143D48 for ; Sat, 18 Mar 2006 16:39:26 +0000 (GMT) (envelope-from bb+lists.freebsd.perforce@cyrus.watson.org) Received: from repoman.freebsd.org (localhost [127.0.0.1]) by repoman.freebsd.org (8.13.1/8.13.1) with ESMTP id k2IGdQRF095564 for ; Sat, 18 Mar 2006 16:39:26 GMT (envelope-from bb+lists.freebsd.perforce@cyrus.watson.org) Received: (from perforce@localhost) by repoman.freebsd.org (8.13.1/8.13.1/Submit) id k2IGdPnb095561 for perforce@freebsd.org; Sat, 18 Mar 2006 16:39:25 GMT (envelope-from bb+lists.freebsd.perforce@cyrus.watson.org) Date: Sat, 18 Mar 2006 16:39:25 GMT Message-Id: <200603181639.k2IGdPnb095561@repoman.freebsd.org> X-Authentication-Warning: repoman.freebsd.org: perforce set sender to bb+lists.freebsd.perforce@cyrus.watson.org using -f From: Robert Watson To: Perforce Change Reviews Cc: Subject: PERFORCE change 93509 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 18 Mar 2006 16:39:27 -0000 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