On 2011-05-23 11.43, David Coppa wrote:
> I'd like to commit the following diff by Vadim Zhukov.
> This patch allows specifing k/m/g/... suffixes in newfs(8)
> -S and -s options. Useful for mount_mfs, now you can just say:
> 
> # mount_mfs -s 50m swap /tmp
> 
> And it will do what you want, taking in account sector size.
> Old behavior of -s (specifying count of sectors) is, of course,
> preserved.
> 
> Otto already ok'd it. I'm looking for additionals eyes/OKs...

I've only got a general comment to this and other similar patches, which
I like a lot, and that is it is unfortunate that the multiplier suffixes
chosen are all lower case.

This is contrasting to the SI prefix standard, from which the terms kilo,
mega, giga, tera etc originally stems, and it is in some cases ambiguous.
For example, mega is 'M' while milli is 'm'.

Yes, I know that powers of 2 aren't the same as powers of 10, but I would
submit that it's generally accepted to use upper case K for 2^10, M for
2^20 and G for 2^30, and so on.

I haven't tried this patch yet (I just broke my -current machine, have to
fix it first), but I assume it accepts the suffixes in lower case as well
as upper case, in which case my only complaint would be that I feel that
the documentation and usage strings should promote the upper case variants
rather than the lower case ones.

Just throwing this out there. :-)


Regards,
/Benny


> Index: mkfs.c
> ===================================================================
> RCS file: /cvs/src/sbin/newfs/mkfs.c,v
> retrieving revision 1.74
> diff -u -p -r1.74 mkfs.c
> --- mkfs.c    21 Mar 2010 09:13:30 -0000      1.74
> +++ mkfs.c    23 May 2011 09:31:58 -0000
> @@ -87,7 +87,7 @@ extern int  mfs;            /* run as the memory ba
>  extern int   Nflag;          /* run mkfs without writing file system */
>  extern int   Oflag;          /* format as an 4.3BSD file system */
>  extern daddr64_t fssize;     /* file system size */
> -extern int   sectorsize;     /* bytes/sector */
> +extern long long     sectorsize;     /* bytes/sector */
>  extern int   fsize;          /* fragment size */
>  extern int   bsize;          /* block size */
>  extern int   maxfrgspercg;   /* maximum fragments per cylinder group */
> @@ -404,8 +404,8 @@ mkfs(struct partition *pp, char *fsys, i
>               lastminfpg = roundup(sblock.fs_iblkno +
>                   sblock.fs_ipg / INOPF(&sblock), sblock.fs_frag);
>               if (sblock.fs_size < lastminfpg)
> -                     errx(28, "file system size %jd < minimum size of %d",
> -                         (intmax_t)sblock.fs_size, lastminfpg);
> +                     errx(28, "file system size %jd < minimum size of %d "
> +                         "sectors", (intmax_t)sblock.fs_size, lastminfpg);
>  
>               if (sblock.fs_size % sblock.fs_fpg >= lastminfpg ||
>                   sblock.fs_size % sblock.fs_fpg == 0)
> Index: newfs.8
> ===================================================================
> RCS file: /cvs/src/sbin/newfs/newfs.8,v
> retrieving revision 1.69
> diff -u -p -r1.69 newfs.8
> --- newfs.8   31 Mar 2011 11:17:58 -0000      1.69
> +++ newfs.8   23 May 2011 09:31:58 -0000
> @@ -218,6 +218,13 @@ With this option,
>  will not print extraneous information like superblock backups.
>  .It Fl S Ar sector-size
>  The size of a sector in bytes (almost always 512).
> +Alternatively
> +.Ar sector-size
> +may instead use a multiplier, as documented in
> +.Xr scan_scaled 3 .
> +.Ar sector-size
> +should be 512 or a multiple of it because the kernel operates
> +512\-byte blocks internally.
>  A sector is the smallest addressable unit on the physical device.
>  Changing this is useful only when using
>  .Nm
> @@ -227,14 +234,19 @@ created (for example on a write-once dis
>  Note that changing this
>  from its default will make it impossible for
>  .Xr fsck 8
> -to find the alternate superblocks if the standard superblock is
> -lost.
> +to find the alternate superblocks automatically if the standard
> +superblock is lost.
>  .It Fl s Ar size
> -The size of the file system in sectors.
> -This value is multiplied by the number of 512\-byte blocks in a sector
> -to yield the size of the file system in 512\-byte blocks, which is the value
> -used by the kernel.
> -The maximum size of an FFS file system is 2,147,483,647 (2^31 \- 1) of these
> +The size of the file system in sectors (see
> +.Fl S ) .
> +Alternatively
> +.Ar size
> +may instead use a multiplier, as documented in
> +.Xr scan_scaled 3 ,
> +to specify size in bytes; in this case
> +.Ar size
> +is rounded up to the next sector boundary.
> +The maximum size of an FFS file system is 2,147,483,647 (2^31 \- 1) of
>  512\-byte blocks, slightly less than 1 TB.
>  FFS2 file systems can be as large as 64 PB.
>  Note however that for
> Index: newfs.c
> ===================================================================
> RCS file: /cvs/src/sbin/newfs/newfs.c,v
> retrieving revision 1.89
> diff -u -p -r1.89 newfs.c
> --- newfs.c   26 Apr 2011 14:02:14 -0000      1.89
> +++ newfs.c   23 May 2011 09:32:45 -0000
> @@ -114,7 +114,7 @@ int       mfs;                    /* run as the memory 
> based fi
>  int  Nflag;                  /* run without writing file system */
>  int  Oflag = 1;              /* 0 = 4.3BSD ffs, 1 = 4.4BSD ffs, 2 = ffs2 */
>  daddr64_t    fssize;                 /* file system size */
> -int  sectorsize;             /* bytes/sector */
> +long long    sectorsize;             /* bytes/sector */
>  int  fsize = 0;              /* fragment size */
>  int  bsize = 0;              /* block size */
>  int  maxfrgspercg = INT_MAX; /* maximum fragments per cylinder group */
> @@ -169,6 +169,8 @@ main(int argc, char *argv[])
>       char **saveargv = argv;
>       int ffsflag = 1;
>       const char *errstr;
> +     long long fssize_input = 0;
> +     int fssize_usebytes = 0;
>  
>       if (strstr(__progname, "mfs"))
>               mfs = Nflag = quiet = 1;
> @@ -192,9 +194,9 @@ main(int argc, char *argv[])
>                       oflagset = 1;
>                       break;
>               case 'S':
> -                     sectorsize = strtonum(optarg, 1, INT_MAX, &errstr);
> -                     if (errstr)
> -                             fatal("sector size is %s: %s", errstr, optarg);
> +                     if (scan_scaled(optarg, &sectorsize) == -1 ||
> +                         sectorsize <= 0 || (sectorsize % DEV_BSIZE))
> +                             fatal("sector size invalid: %s", optarg);
>                       break;
>               case 'T':
>                       disktype = optarg;
> @@ -265,10 +267,15 @@ main(int argc, char *argv[])
>                       quiet = 1;
>                       break;
>               case 's':
> -                     fssize = strtonum(optarg, 1, LLONG_MAX, &errstr);
> -                     if (errstr)
> -                             fatal("file system size is %s: %s",
> -                                 errstr, optarg);
> +                     if (scan_scaled(optarg, &fssize_input) == -1 ||
> +                         fssize_input <= 0)
> +                             fatal("file system size invalid: %s", optarg);
> +                     fssize_usebytes = 0;    /* in case of multiple -s */
> +                     for (s1 = optarg; *s1 != '\0'; s1++)
> +                             if (isalpha(*s1)) {
> +                                     fssize_usebytes = 1;
> +                                     break;
> +                             }
>                       break;
>               case 't':
>                       fstype = optarg;
> @@ -414,17 +421,25 @@ main(int argc, char *argv[])
>                             argv[0], *cp);
>       }
>  havelabel:
> -     if (fssize == 0)
> -             fssize = DL_GETPSIZE(pp);
> -     if (fssize > DL_GETPSIZE(pp) && !mfs)
> -            fatal("%s: maximum file system size on the `%c' partition is 
> %lld",
> -                     argv[0], *cp, DL_GETPSIZE(pp));
> -
>       if (sectorsize == 0) {
>               sectorsize = lp->d_secsize;
>               if (sectorsize <= 0)
>                       fatal("%s: no default sector size", argv[0]);
>       }
> +
> +     if (fssize_usebytes) {
> +             fssize = (daddr64_t)fssize_input / (daddr64_t)sectorsize;
> +             if ((daddr64_t)fssize_input % (daddr64_t)sectorsize != 0)
> +                     fssize++;
> +     } else if (fssize_input == 0)
> +             fssize = DL_GETPSIZE(pp);
> +     else
> +             fssize = (daddr64_t)fssize_input;
> +
> +     if (fssize > DL_GETPSIZE(pp) && !mfs)
> +            fatal("%s: maximum file system size on the `%c' partition is "
> +                "%lld sectors", argv[0], *cp, DL_GETPSIZE(pp));
> +
>       fssize *= sectorsize / DEV_BSIZE;
>       if (oflagset == 0 && fssize >= INT_MAX)
>               Oflag = 2;      /* FFS2 */
> 

-- 
internetlabbet.se     / work:   +46 8 551 124 80      / "Words must
Benny LC6fgren        /  mobile: +46 70 718 11 90     /   be weighed,
                    /   fax:    +46 8 551 124 89    /    not counted."
                   /    email:  benny -at- internetlabbet.se

Reply via email to