PERFORCE change 93509 for review

[Robert Watson]

http://perforce.freebsd.org/chv.cgi?CH=93509

Change 93509 by rwatson at 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