Bug 31199
| Summary: | tunefs error is incorrect when enabling softupdates | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Rob Simmons <rsimmons> |
| Component: | bin | Assignee: | freebsd-bugs (Nobody) <bugs> |
| Status: | Closed FIXED | ||
| Severity: | Affects Only Me | ||
| Priority: | Normal | ||
| Version: | 4.4-STABLE | ||
| Hardware: | Any | ||
| OS: | Any | ||
|
Description
Rob Simmons
2001-10-10 16:50:01 UTC
On Wed, Oct 10, 2001 at 11:46:07AM -0400, Rob Simmons wrote: [snip] > When tunefs is used to enable softupdates, it gives conflicting mesages if > the filesystem is mounted. > >How-To-Repeat: > run "tunefs -n enable <fs>" on a mounted filesystem. > > You will get the following output: > > bash-2.05$ tunefs -n enable /dev/ad0s1a > tunefs: soft updates set > tunefs: cannot open /dev/ad0s1a: Permission denied Looking at the code, I think the least obtrusive way to fix this is to slightly reword the messages a little more clear. tunefs(8) goes through all of the options before it actually tries to modify the file system. The messages should reflect that it is collecting the values before it actually tries to write them. Something more like, $ tunefs -n enable /dev/ad0s1a tunefs: setting soft updates... tunefs: cannot open /dev/ad0s1a: Permission denied And in a successful run (since tunefs(8) is already chatty on success), $ tunefs -n enable /dev/ad0s1a tunefs: setting soft updates... tunefs: changes on /dev/ad0s1 done This patch look good? Index: tunefs.c =================================================================== RCS file: /export/ncvs/src/sbin/tunefs/tunefs.c,v retrieving revision 1.20 diff -u -r1.20 tunefs.c --- tunefs.c 2001/09/30 14:57:08 1.20 +++ tunefs.c 2001/10/15 09:43:07 @@ -231,7 +231,7 @@ warnx("%s remains unchanged as %d", name, avalue); } else { - warnx("%s changes from %d to %d", + warnx("changing %s from %d to %d...", name, sblock.fs_maxcontig, avalue); sblock.fs_maxcontig = avalue; } @@ -242,7 +242,7 @@ warnx("%s remains unchanged as %dms", name, dvalue); } else { - warnx("%s changes from %dms to %dms", + warnx("changing %s from %dms to %dms...", name, sblock.fs_rotdelay, dvalue); sblock.fs_rotdelay = dvalue; } @@ -253,7 +253,7 @@ warnx("%s remains unchanged as %d", name, evalue); } else { - warnx("%s changes from %d to %d", + warnx("changing %s from %d to %d...", name, sblock.fs_maxbpg, evalue); sblock.fs_maxbpg = evalue; } @@ -264,7 +264,7 @@ warnx("%s remains unchanged as %d", name, fvalue); } else { - warnx("%s changes from %d to %d", + warnx("changing %s from %d to %d...", name, sblock.fs_avgfilesize, fvalue); sblock.fs_avgfilesize = fvalue; } @@ -275,7 +275,7 @@ warnx("%s remains unchanged as %d%%", name, mvalue); } else { - warnx("%s changes from %d%% to %d%%", + warnx("changing %s from %d%% to %d%%...", name, sblock.fs_minfree, mvalue); sblock.fs_minfree = mvalue; if (mvalue >= MINFREE && sblock.fs_optim == FS_OPTSPACE) @@ -294,14 +294,14 @@ name); } else { sblock.fs_flags |= FS_DOSOFTDEP; - warnx("%s set", name); + warnx("setting %s...", name); } } else if (strcmp(nvalue, "disable") == 0) { if ((~sblock.fs_flags & FS_DOSOFTDEP) == FS_DOSOFTDEP) { warnx("%s remains unchanged as disabled", name); } else { sblock.fs_flags &= ~FS_DOSOFTDEP; - warnx("%s cleared", name); + warnx("clearing %s...", name); } } } @@ -313,7 +313,7 @@ warnx("%s remains unchanged as %s", name, chg[ovalue]); } else { - warnx("%s changes from %s to %s", + warnx("changing %s from %s to %s...", name, chg[sblock.fs_optim], chg[ovalue]); sblock.fs_optim = ovalue; if (sblock.fs_minfree >= MINFREE && @@ -330,7 +330,7 @@ warnx("%s remains unchanged as %d", name, svalue); } else { - warnx("%s changes from %d to %d", + warnx("changing %s from %d to %d...", name, sblock.fs_avgfpdir, svalue); sblock.fs_avgfpdir = svalue; } @@ -396,6 +396,7 @@ for (i = 0; i < fs->fs_ncg; i++) bwrite(fsbtodb(fs, cgsblock(fs, i)), (const char *)fs, SBSIZE); + warnx("changes on %s done", file); close(fi); } -- Crist J. Clark | cjclark@alum.mit.edu | cjclark@jhu.edu http://people.freebsd.org/~cjc/ | cjc@freebsd.org On Mon, 15 Oct 2001, Crist J. Clark wrote: > On Wed, Oct 10, 2001 at 11:46:07AM -0400, Rob Simmons wrote: > ... > > >How-To-Repeat: > > run "tunefs -n enable <fs>" on a mounted filesystem. > > > > You will get the following output: > > > > bash-2.05$ tunefs -n enable /dev/ad0s1a > > tunefs: soft updates set > > tunefs: cannot open /dev/ad0s1a: Permission denied > > Looking at the code, I think the least obtrusive way to fix this is to > slightly reword the messages a little more clear. tunefs(8) goes > through all of the options before it actually tries to modify the file > system. This was broken in rev.1.12. tunefs previously opened the device with mode O_RDWR (spelled as "2") up front in getsb(). This was bogus for the -p case since it required write access to display the settings, but is needed for all other cases since tunefs writes even null changes. > The messages should reflect that it is collecting the values > before it actually tries to write them. Something more like, > > $ tunefs -n enable /dev/ad0s1a > tunefs: setting soft updates... > tunefs: cannot open /dev/ad0s1a: Permission denied The "setting" message used to be "soft updates set". I changed this to "soft updates changes from disabled to enabled" (and similarly for "soft updates cleared" so that the wording is similar for all the messages. > And in a successful run (since tunefs(8) is already chatty on > success), > > $ tunefs -n enable /dev/ad0s1a > tunefs: setting soft updates... > tunefs: changes on /dev/ad0s1 done The "changes done" message is silly if tunefs has only printed "remains unchanged" messages. > This patch look good? > > Index: tunefs.c > =================================================================== > RCS file: /export/ncvs/src/sbin/tunefs/tunefs.c,v > retrieving revision 1.20 > diff -u -r1.20 tunefs.c > --- tunefs.c 2001/09/30 14:57:08 1.20 > +++ tunefs.c 2001/10/15 09:43:07 > @@ -231,7 +231,7 @@ > warnx("%s remains unchanged as %d", name, avalue); > } > else { > - warnx("%s changes from %d to %d", > + warnx("changing %s from %d to %d...", > name, sblock.fs_maxcontig, avalue); > sblock.fs_maxcontig = avalue; > } I don't think this is any better. The unusual wording in the original was apparently chosen to put the subject first. Neither version claims to have completed the changes (that would be "%s changed"). Bruce On Tue, Oct 16, 2001 at 01:22:37AM +1000, Bruce Evans wrote: > On Mon, 15 Oct 2001, Crist J. Clark wrote: [snip] > > The messages should reflect that it is collecting the values > > before it actually tries to write them. Something more like, > > > > $ tunefs -n enable /dev/ad0s1a > > tunefs: setting soft updates... > > tunefs: cannot open /dev/ad0s1a: Permission denied > > The "setting" message used to be "soft updates set". I changed this to > "soft updates changes from disabled to enabled" (and similarly for > "soft updates cleared" so that the wording is similar for all the messages. OK. Probably better. This hasn't been committed yet, though? > > And in a successful run (since tunefs(8) is already chatty on > > success), > > > > $ tunefs -n enable /dev/ad0s1a > > tunefs: setting soft updates... > > tunefs: changes on /dev/ad0s1 done > > The "changes done" message is silly if tunefs has only printed "remains > unchanged" messages. But then again, even if there are no changes, tunefs(8) unconditionally writes the "modified" data. -- Crist J. Clark | cjclark@alum.mit.edu | cjclark@jhu.edu http://people.freebsd.org/~cjc/ | cjc@freebsd.org On Mon, 15 Oct 2001, Crist J. Clark wrote: > On Tue, Oct 16, 2001 at 01:22:37AM +1000, Bruce Evans wrote: > > On Mon, 15 Oct 2001, Crist J. Clark wrote: > > [snip] More comments on [snip]: the problem is easy to fix in -current thanks to the changes in rev.1.14. All args are now parsed before committing to any changes, so pflag is known before getsb() and getsb() can open the device with the least access required. Either pass the open mode to getsb() or make pflag global. > > > The messages should reflect that it is collecting the values > > > before it actually tries to write them. Something more like, > > > > > > $ tunefs -n enable /dev/ad0s1a > > > tunefs: setting soft updates... > > > tunefs: cannot open /dev/ad0s1a: Permission denied > > > > The "setting" message used to be "soft updates set". I changed this to > > "soft updates changes from disabled to enabled" (and similarly for > > "soft updates cleared" so that the wording is similar for all the messages. > > OK. Probably better. This hasn't been committed yet, though? Right. It's hard to see in all my unrelated changes: Index: tunefs.c =================================================================== RCS file: /home/ncvs/src/sbin/tunefs/tunefs.c,v retrieving revision 1.20 diff -u -0 -r1.20 tunefs.c --- tunefs.c 30 Sep 2001 14:57:08 -0000 1.20 +++ tunefs.c 16 Oct 2001 07:30:46 -0000 @@ -297 +309,2 @@ - warnx("%s set", name); + warnx("%s changes from disabled to enabled", + name); @@ -304 +317,2 @@ - warnx("%s cleared", name); + warnx("%s changes from enabled to disabled", + name); > > > > And in a successful run (since tunefs(8) is already chatty on > > > success), > > > > > > $ tunefs -n enable /dev/ad0s1a > > > tunefs: setting soft updates... > > > tunefs: changes on /dev/ad0s1 done > > > > The "changes done" message is silly if tunefs has only printed "remains > > unchanged" messages. > > But then again, even if there are no changes, tunefs(8) > unconditionally writes the "modified" data. Yes. Perhaps it should be even more chatty and print: tunefs: updating superblock tunefs: changes on /dev/ad0s1 done [or error] Bruce I think this PR can safely be closed. Tested on a 6.2-REL machine gives: # tunefs -n enable / tunefs: soft updates cannot be enabled until fsck is run tunefs: /dev/mirror/gm0s1a: failed to write superblock The same message is given when run on a RELENG_7 machine. Also an error code is being set: # echo $? 11 State Changed From-To: open->closed Volker Werth reports in a followup that this error does not recur on 6.x and 7.x. I think we can safely close it now due to its age. |
