cvs commit: src/sys/kern kern_shutdown.c vfs_subr.c

[Nate Lawson]

Scott Long wrote:
> Nate Lawson wrote:
> 
>> Alfred Perlstein wrote:
>>
>>> alfred      2004-07-15 04:29:48 UTC
>>>
>>>   FreeBSD src repository
>>>
>>>   Modified files:
>>>     sys/kern             kern_shutdown.c vfs_subr.c   Log:
>>>   Tidy up system shutdown.
>>>     Revision  Changes    Path
>>>   1.156     +13 -5     src/sys/kern/kern_shutdown.c
>>>   1.511     +11 -1     src/sys/kern/vfs_subr.c
>>
>> This is a step backwards with more newlines and duplicate output 
>> (i.e., procname).  Please revert.
> 
> It's getting a little tedious that whenever someone objects to a commit, 
> their response includes (sometimes little more than) 'please revert.'
> What don't you like about it?  How would you change it?

As I said above, at a minimum it adds more newlines (which my commit 1 
hour before had just removed) and duplicates output.  But since you want 
the full analysis, which is as long as the commit itself:

####
@@ -245,6 +245,9 @@
  static void
  boot(int howto)
  {
+	int first_buf_printf;
+
+	first_buf_printf = 1;

  	/* collect extra flags that shutdown_nice might have set */
  	howto |= shutdown_howto;
###

Adding unnecessary loop variable.

###
@@ -272,7 +275,6 @@
  #endif

  		waittime = 0;
-		printf("syncing disks, buffers remaining... ");

  		sync(&thread0, NULL);

@@ -295,6 +297,10 @@
  			}
  			if (nbusy == 0)
  				break;
+			if (first_buf_printf) {
+				printf("syncing disks, buffers remaining... ");
+				first_buf_printf = 0;
+			}
  			printf("%d ", nbusy);
  			if (nbusy < pbusy)
  				iter = 0;
###

Moving one-time printf into a loop protected by useless flag.

###
@@ -576,20 +582,22 @@
  kproc_shutdown(void *arg, int howto)
  {
  	struct proc *p;
+	char procname[MAXCOMLEN + 1];
  	int error;

  	if (panicstr)
  		return;

  	p = (struct proc *)arg;
-	printf("Waiting (max %d seconds) for system process `%s' to stop...",
-	    kproc_shutdown_wait, p->p_comm);
+	strlcpy(procname, p->p_comm, sizeof(procname));
+	printf("Waiting (max %d seconds) for system process `%s' to stop...\n",
+	    kproc_shutdown_wait, procname);
  	error = kthread_suspend(p, kproc_shutdown_wait * hz);
###

Adds unnecessary stack variable and copy of an informational name when 
the proc can't be switched out.

###
  	if (error == EWOULDBLOCK)
-		printf("timed out\n");
+		printf("Stop of '%s' timed out\n", procname);
  	else
-		printf("stopped\n");
+		printf("Process '%s' stopped\n", procname);
  }
###

Adds unnecessary repetition of previous word on line ("stop..."), the 
proc's name, and an extra line (instead of keeping the result on the 
same line.

Just in case that isn't clear, this makes the output:
Waiting (max 60 seconds) for system process `foo' to stop...
Process 'foo' stopped

Versus what has been there forever:
Waiting (max 60 seconds) for system process `foo' to stop... stopped

###
@@ -1546,10 +1546,12 @@
  	int last_work_seen;
  	int net_worklist_len;
  	int syncer_final_iter;
+	int first_printf;

  	mtx_lock(&Giant);
  	last_work_seen = 0;
  	syncer_final_iter = 0;
+	first_printf = 1;
  	syncer_state = SYNCER_RUNNING;
  	starttime = time_second;
###

Another useless loop variable.

###
@@ -1561,12 +1563,20 @@
  		if (syncer_state == SYNCER_FINAL_DELAY &&
  		    syncer_final_iter == 0) {
  			mtx_unlock(&sync_mtx);
+			printf("done.\n");
  			kthread_suspend_check(td->td_proc);
  			mtx_lock(&sync_mtx);
  		}
  		net_worklist_len = syncer_worklist_len - sync_vnode_count;
-		if (syncer_state != SYNCER_RUNNING && starttime != time_second)
+		if (syncer_state != SYNCER_RUNNING &&
+		    starttime != time_second) {
+			if (first_printf) {
+				printf("Syncer syncing disks, "
+				    "buffers remaining... ");
+				first_printf = 0;
+			}
  			printf("%d ", net_worklist_len);
+		}
  		starttime = time_second;
###

Probably unneeded style change (was at 80 cols before).  Moving single 
shot printf into loop.

Remember, you asked for all the details.  I thought a short email with a 
few comments should have been enough for this change and less likely to 
be taken as confrontational.  It appears Bosko thought the same thing.

-- 
-Nate