Date: Wed, 14 Jul 2004 22:48:39 -0700 From: Alfred Perlstein <alfred@freebsd.org> To: Nate Lawson <nate@root.org> Cc: cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/kern kern_shutdown.c vfs_subr.c Message-ID: <20040715054839.GM95729@elvis.mu.org> In-Reply-To: <40F616E2.7030601@root.org> References: <20040715042952.C726216A541@hub.freebsd.org> <40F60A42.2060607@root.org> <40F60DA9.8000206@freebsd.org> <40F616E2.7030601@root.org>
next in thread | previous in thread | raw e-mail | index | archive | help
hooboy...
* Nate Lawson <nate@root.org> [040714 22:33] wrote:
>
> 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.
So we one time print .. buffers remaining.. but then nothing?
That's confusing and not acceptable.
> ###
> @@ -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 you look later that stack variable is used because the process
may be gone. Even if it is not gone, that's not the impression
I get.
> ###
> 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
Forever defined as "the last six months or year".
Change is good, although change with some proper punctuation
would be nice. I'll have to add the periods at the end of those
printfs now. :)
> ###
> @@ -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.
What a bikeshed. Not interested anymore. If you have a way to make
it suck less go ahead and fix it. Otherwise...
hi. :)
--
- Alfred Perlstein
- Research Engineering Development Inc.
- email: bright@mu.org cell: 408-480-4684
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040715054839.GM95729>