From owner-svn-src-all@FreeBSD.ORG Fri Apr 30 17:11:40 2010 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 4541D1065670; Fri, 30 Apr 2010 17:11:40 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail09.syd.optusnet.com.au (mail09.syd.optusnet.com.au [211.29.132.190]) by mx1.freebsd.org (Postfix) with ESMTP id 8CC2F8FC12; Fri, 30 Apr 2010 17:11:39 +0000 (UTC) Received: from c122-106-173-187.carlnfd1.nsw.optusnet.com.au (c122-106-173-187.carlnfd1.nsw.optusnet.com.au [122.106.173.187]) by mail09.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id o3UHBY7Z001445 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 1 May 2010 03:11:36 +1000 Date: Sat, 1 May 2010 03:11:35 +1000 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Jeff Roberson In-Reply-To: <201004300421.o3U4LMmV056220@svn.freebsd.org> Message-ID: <20100501024249.C13811@delplex.bde.org> References: <201004300421.o3U4LMmV056220@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r207421 - head/sbin/tunefs X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 30 Apr 2010 17:11:40 -0000 On Fri, 30 Apr 2010, Jeff Roberson wrote: > Log: > - Use the path to the filesystem mountpoint to look up the statfs > structure so that we correctly reload. Note that tunefs doesn't > properly detect the need to reload if the disk device is specified > for a read-only mounted filesystem. I think I fixed this long ago (using the getmntpt() function which should be in a library). The patch also fixes many style bugs (but far from all, and there are many more now). % Index: tunefs.c % =================================================================== % RCS file: /home/ncvs/src/sbin/tunefs/tunefs.c,v % retrieving revision 1.42 % diff -u -2 -r1.42 tunefs.c % --- tunefs.c 9 Apr 2004 19:58:40 -0000 1.42 % +++ tunefs.c 10 Apr 2004 02:10:11 -0000 % @@ -46,6 +46,6 @@ % */ % #include % -#include % #include % +#include % #include % % @@ -58,4 +58,5 @@ % #include % #include % +#define _LIBUFS % #include % #include % @@ -71,6 +72,7 @@ % #define sblock disk.d_fs % % -void usage(void); % +struct statfs *getmntpt(const char *); % void printfs(void); % +void usage(void); % % int % @@ -78,7 +80,6 @@ % { % char *avalue, *Lvalue, *lvalue, *nvalue; % - const char *special, *on; % - const char *name; % - int active; % + const char *name, *special; % + struct statfs *stfsp; % int Aflag, aflag, eflag, evalue, fflag, fvalue, Lflag, lflag; % int mflag, mvalue, nflag, oflag, ovalue, pflag, sflag, svalue; % @@ -86,5 +87,4 @@ % const char *chg[2]; % struct ufs_args args; % - struct statfs stfs; % % if (argc < 3) % @@ -94,5 +94,4 @@ % avalue = Lvalue = lvalue = nvalue = NULL; % evalue = fvalue = mvalue = ovalue = svalue = 0; % - active = 0; % found_arg = 0; /* At least one arg is required. */ % while ((ch = getopt(argc, argv, "Aa:e:f:L:l:m:n:o:ps:")) != -1) % @@ -101,5 +100,5 @@ % case 'A': % found_arg = 1; % - Aflag++; % + Aflag = 1; % break; % % @@ -108,7 +107,7 @@ % name = "ACLs"; % avalue = optarg; % - if (strcmp(avalue, "enable") && % - strcmp(avalue, "disable")) { % - errx(10, "bad %s (options are %s)", % + if (strcmp(avalue, "enable") != 0 && % + strcmp(avalue, "disable") != 0) { % + errx(10, "bad %s option (options are %s)", % name, "`enable' or `disable'"); % } % @@ -141,12 +140,13 @@ % Lvalue = optarg; % i = -1; % - while (isalnum(Lvalue[++i])); % + while (isalnum(Lvalue[++i])) % + ; % if (Lvalue[i] != '\0') { % errx(10, % - "bad %s. Valid characters are alphanumerics.", % + "bad %s (valid characters are alphanumerics)", % name); % } % if (strlen(Lvalue) >= MAXVOLLEN) { % - errx(10, "bad %s. Length is longer than %d.", % + errx(10, "bad %s (length is longer than %d)", % name, MAXVOLLEN - 1); % } % @@ -158,7 +158,7 @@ % name = "multilabel MAC file system"; % lvalue = optarg; % - if (strcmp(lvalue, "enable") && % - strcmp(lvalue, "disable")) { % - errx(10, "bad %s (options are %s)", % + if (strcmp(lvalue, "enable") != 0 && % + strcmp(lvalue, "disable") != 0) { % + errx(10, "bad %s option (options are %s)", % name, "`enable' or `disable'"); % } % @@ -170,5 +170,5 @@ % name = "minimum percentage of free space"; % mvalue = atoi(optarg); % - if (mvalue < 0 || mvalue > 99) % + if (mvalue < 0 || mvalue > 100) % errx(10, "bad %s (%s)", name, optarg); % mflag = 1; % @@ -181,5 +181,5 @@ % if (strcmp(nvalue, "enable") != 0 && % strcmp(nvalue, "disable") != 0) { % - errx(10, "bad %s (options are %s)", % + errx(10, "bad %s option (options are %s)", % name, "`enable' or `disable'"); % } % @@ -224,12 +224,14 @@ % usage(); % % - on = special = argv[0]; % + special = argv[0]; % if (ufs_disk_fillout(&disk, special) == -1) % goto err; % - if (disk.d_name != special) { % + if (disk.d_name != special) % special = disk.d_name; % - if (statfs(special, &stfs) == 0 && % - strcmp(special, stfs.f_mntonname) == 0) % - active = 1; % + stfsp = getmntpt(special); % + if (stfsp != NULL) { % + if (pflag == 0 && (stfsp->f_flags & MNT_RDONLY) == 0) % + errx(1, % + "cannot work on read-write mounted file system"); % } % % @@ -238,23 +240,17 @@ % exit(0); % } % - if (Lflag) { % - name = "volume label"; % - strlcpy(sblock.fs_volname, Lvalue, MAXVOLLEN); % - } % if (aflag) { % name = "ACLs"; % if (strcmp(avalue, "enable") == 0) { % - if (sblock.fs_flags & FS_ACLS) { % + if (sblock.fs_flags & FS_ACLS) % warnx("%s remains unchanged as enabled", name); % - } else { % + else { % sblock.fs_flags |= FS_ACLS; % warnx("%s set", name); % } % } else if (strcmp(avalue, "disable") == 0) { % - if ((~sblock.fs_flags & FS_ACLS) == % - FS_ACLS) { % - warnx("%s remains unchanged as disabled", % - name); % - } else { % + if ((~sblock.fs_flags & FS_ACLS) == FS_ACLS) % + warnx("%s remains unchanged as disabled", name); % + else { % sblock.fs_flags &= ~FS_ACLS; % warnx("%s cleared", name); % @@ -274,28 +270,29 @@ % if (fflag) { % name = "average file size"; % - if (sblock.fs_avgfilesize == fvalue) { % + if (sblock.fs_avgfilesize == fvalue) % warnx("%s remains unchanged as %d", name, fvalue); % - } % else { % warnx("%s changes from %d to %d", % - name, sblock.fs_avgfilesize, fvalue); % + name, sblock.fs_avgfilesize, fvalue); % sblock.fs_avgfilesize = fvalue; % } % } % + if (Lflag) { % + name = "volume label"; % + strlcpy(sblock.fs_volname, Lvalue, MAXVOLLEN); % + } % if (lflag) { % name = "multilabel"; % if (strcmp(lvalue, "enable") == 0) { % - if (sblock.fs_flags & FS_MULTILABEL) { % + if (sblock.fs_flags & FS_MULTILABEL) % warnx("%s remains unchanged as enabled", name); % - } else { % + else { % sblock.fs_flags |= FS_MULTILABEL; % warnx("%s set", name); % } % } else if (strcmp(lvalue, "disable") == 0) { % - if ((~sblock.fs_flags & FS_MULTILABEL) == % - FS_MULTILABEL) { % - warnx("%s remains unchanged as disabled", % - name); % - } else { % + if ((~sblock.fs_flags & FS_MULTILABEL) == FS_MULTILABEL) % + warnx("%s remains unchanged as disabled", name); % + else { % sblock.fs_flags &= ~FS_MULTILABEL; % warnx("%s cleared", name); % @@ -309,5 +306,5 @@ % else { % warnx("%s changes from %d%% to %d%%", % - name, sblock.fs_minfree, mvalue); % + name, sblock.fs_minfree, mvalue); % sblock.fs_minfree = mvalue; % if (mvalue >= MINFREE && sblock.fs_optim == FS_OPTSPACE) % @@ -318,6 +315,6 @@ % } % if (nflag) { % - name = "soft updates"; % - if (strcmp(nvalue, "enable") == 0) { % + name = "soft updates"; % + if (strcmp(nvalue, "enable") == 0) { % if (sblock.fs_flags & FS_DOSOFTDEP) % warnx("%s remains unchanged as enabled", name); % @@ -326,15 +323,17 @@ % name); % } else { % - sblock.fs_flags |= FS_DOSOFTDEP; % - warnx("%s set", name); % + sblock.fs_flags |= FS_DOSOFTDEP; % + warnx("%s changes from disabled to enabled", % + name); % } % - } else if (strcmp(nvalue, "disable") == 0) { % + } 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); % + sblock.fs_flags &= ~FS_DOSOFTDEP; % + warnx("%s changes from enabled to disabled", % + name); % } % - } % + } % } % if (oflag) { % @@ -346,5 +345,5 @@ % else { % warnx("%s changes from %s to %s", % - name, chg[sblock.fs_optim], chg[ovalue]); % + name, chg[sblock.fs_optim], chg[ovalue]); % sblock.fs_optim = ovalue; % if (sblock.fs_minfree >= MINFREE && % @@ -357,10 +356,9 @@ % if (sflag) { % name = "expected number of files per directory"; % - if (sblock.fs_avgfpdir == svalue) { % + if (sblock.fs_avgfpdir == svalue) % warnx("%s remains unchanged as %d", name, svalue); % - } % else { % warnx("%s changes from %d to %d", % - name, sblock.fs_avgfpdir, svalue); % + name, sblock.fs_avgfpdir, svalue); % sblock.fs_avgfpdir = svalue; % } % @@ -370,12 +368,13 @@ % goto err; % ufs_disk_close(&disk); % - if (active) { % + if (stfsp != NULL) { % bzero(&args, sizeof(args)); % - if (mount("ufs", on, % - stfs.f_flags | MNT_UPDATE | MNT_RELOAD, &args) < 0) % + if (mount("ufs", stfsp->f_mntonname, % + stfsp->f_flags | MNT_UPDATE | MNT_RELOAD, &args) < 0) % err(9, "%s: reload", special); % warnx("file system reloaded"); % } % exit(0); % + % err: % if (disk.d_error != NULL) % @@ -399,4 +398,5 @@ % printfs(void) % { % + /* XXX mounds of style bugs. */ % warnx("ACLs: (-a) %s", % (sblock.fs_flags & FS_ACLS)? "enabled" : "disabled"); % @@ -424,2 +424,36 @@ % sblock.fs_volname); % } % + % +/* % + * Get the mount info for a ufs file system mounted on `name'. % + * XXX adapted from fsck_ffs/main.c. % + */ % +struct statfs * % +getmntpt(name) % + const char *name; % +{ % + struct stat devstat, mntdevstat; % + char devnamebuf[sizeof(_PATH_DEV) - 1 + MNAMELEN]; % + char *devnamep; % + struct statfs *mntbuf; % + int i, mntsize; % + % + if (stat(name, &devstat) != 0 || % + !(S_ISCHR(devstat.st_mode) || S_ISBLK(devstat.st_mode))) % + return (NULL); % + mntsize = getmntinfo(&mntbuf, MNT_NOWAIT); % + for (i = 0; i < mntsize; i++) { % + if (strcmp(mntbuf[i].f_fstypename, "ufs") != 0) % + continue; % + devnamep = mntbuf[i].f_mntfromname; % + if (*devnamep != '/') { % + strcpy(devnamebuf, _PATH_DEV); % + strcat(devnamebuf, devnamep); % + devnamep = devnamebuf; % + } % + if (stat(devnamep, &mntdevstat) == 0 && % + mntdevstat.st_rdev == devstat.st_rdev) % + return (&mntbuf[i]); % + } % + return (NULL); % +} Back to the change in the commit... > Modified: head/sbin/tunefs/tunefs.c > ============================================================================== > --- head/sbin/tunefs/tunefs.c Fri Apr 30 03:35:05 2010 (r207420) > +++ head/sbin/tunefs/tunefs.c Fri Apr 30 04:21:22 2010 (r207421) > @@ -280,9 +280,9 @@ main(int argc, char *argv[]) > if (ufs_disk_fillout(&disk, special) == -1) > goto err; > if (disk.d_name != special) { > - special = disk.d_name; > - if (statfs(special, &stfs) == 0 && > - strcmp(special, stfs.f_mntonname) == 0) > + if (statfs(special, &stfs) != 0) > + warn("Can't stat %s", special); > + if (strcmp(special, stfs.f_mntonname) == 0) > active = 1; This introduces undefined behaviour when statfs() failed. stfs then consists of stack garbage, but f_mntonname in it is used. Since f_mntonname is an array whose contents is unlikely to match the leading bytes of `special', the undefined behaviour is likely to be benign. This has 1 style bug (capitalization of the warning message). There used to be no other instance of this bug in tunefs, but recent changes added many more instances, mostly of the form "Failed to ..." and "Journal file ...". This doesn't actually implement the beheviour described in the log message. It doesn't change the path to the filesystem mountpoint to look up the statfs struct; it only adds a warning when the lookup fails. > } > Bruce