Date: Sat, 1 May 2010 03:11:35 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Jeff Roberson <jeff@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r207421 - head/sbin/tunefs Message-ID: <20100501024249.C13811@delplex.bde.org> In-Reply-To: <201004300421.o3U4LMmV056220@svn.freebsd.org> References: <201004300421.o3U4LMmV056220@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <sys/param.h>
% -#include <sys/mount.h>
% #include <sys/disklabel.h>
% +#include <sys/mount.h>
% #include <sys/stat.h>
%
% @@ -58,4 +58,5 @@
% #include <fcntl.h>
% #include <fstab.h>
% +#define _LIBUFS
% #include <libufs.h>
% #include <paths.h>
% @@ -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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100501024249.C13811>