From owner-cvs-src@FreeBSD.ORG Thu Jul 15 05:33:17 2004 Return-Path: Delivered-To: cvs-src@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 6FD0016A4CE; Thu, 15 Jul 2004 05:33:17 +0000 (GMT) Received: from www.cryptography.com (li-22.members.linode.com [64.5.53.22]) by mx1.FreeBSD.org (Postfix) with ESMTP id 3238C43D45; Thu, 15 Jul 2004 05:33:17 +0000 (GMT) (envelope-from nate@root.org) Received: from [10.0.5.50] (adsl-64-171-186-94.dsl.snfc21.pacbell.net [64.171.186.94]) by www.cryptography.com (8.12.8/8.12.8) with ESMTP id i6F5XFrb028518 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NOT); Wed, 14 Jul 2004 22:33:16 -0700 Message-ID: <40F616E2.7030601@root.org> Date: Wed, 14 Jul 2004 22:32:18 -0700 From: Nate Lawson User-Agent: Mozilla Thunderbird 0.7.1 (Windows/20040626) X-Accept-Language: en-us, en MIME-Version: 1.0 To: Scott Long References: <20040715042952.C726216A541@hub.freebsd.org> <40F60A42.2060607@root.org> <40F60DA9.8000206@freebsd.org> In-Reply-To: <40F60DA9.8000206@freebsd.org> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit cc: cvs-src@freebsd.org cc: Alfred Perlstein cc: src-committers@freebsd.org cc: cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/kern kern_shutdown.c vfs_subr.c X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 15 Jul 2004 05:33:17 -0000 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