On 01/14/11 03:12, Vadim Zhukov wrote:
> On 14 January 2011 ?. 02:46:49 Alexander Hall wrote:
>> On 01/13/11 14:48, Vadim Zhukov wrote:
>>> @@ -230,10 +232,12 @@ from its default will make it impossible
>>> to find the alternate superblocks 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 size of the file system.
>>> +The argument may contain a multiplier, as documented in
>>> +.Xr scan_scaled 3 .
>>> +If multiplier was not specified then 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 (not bytes!), which is the value used by the kernel.
>>
>> Can someone explain to me why the manpage needs to explain all of this
>> internal mumbo-jumbo? It just confused me. Either it's sector size or
>> it's in [KMGTP].
>
> I was wandering this too... Tried to fix, see below.
>
>>> case 'S':
>>> - sectorsize = strtonum(optarg, 1, INT_MAX, &errstr);
>>> - if (errstr)
>>> - fatal("sector size is %s: %s", errstr, optarg);
>>> + if (scan_scaled(optarg, §orsize) == -1 ||
>>> + sectorsize <= 0 || sectorsize > INT_MAX)
>>> + fatal("sector size: %s: %s", strerror(errno),
>>
>> errno will only be set if scan_scaled() failed.
>
> Yep.
>
>>> + optarg);
>>> break;
>>> case 'T':
>>> disktype = optarg;
>>> @@ -265,10 +267,18 @@ main(int argc, char *argv[])
>>> quiet = 1;
>>> break;
>>> case 's':
>>> + /*
>>> + * We need to save scaled and unscaled value separately
>>> + * because unscaled is not representing bytes.
>>> + */
>>> + fssize_scaled = -1; /* in case of multiple -s */
>>> fssize = strtonum(optarg, 1, LLONG_MAX, &errstr);
>>> - if (errstr)
>>> - fatal("file system size is %s: %s",
>>> - errstr, optarg);
>>> + if (!errstr)
>>> + break;
>>> + if (strcmp(errstr, "invalid") ||
>>> + scan_scaled(optarg, &fssize_scaled) == -1 ||
>>> + fssize_scaled <= 0)
>>> + fatal("file system size is %s: %s", errstr,
>>> optarg);
>>
>> same here
>
> Double-yep.
>
> The fixed patch, implementing Ted's suggestions too, is below. I still
> have to use separate variable, fssize_input, but now it is the one
> always initialized in the argument parsing loop, and fssize is set up
> based on fssize_input and sectorsize - after we could calculate
> sectorsize itself.
>
> --
> Best wishes,
> Vadim Zhukov
>
> A: Because it messes up the order in which people normally read text.
> Q: Why is top-posting such a bad thing?
> A: Top-posting.
> Q: What is the most annoying thing in e-mail?
Yawn...
>
>
> Index: newfs.8
> ===================================================================
> RCS file: /cvs/src/sbin/newfs/newfs.8,v
> retrieving revision 1.68
> diff -u -p -r1.68 newfs.8
> --- newfs.8 21 Mar 2010 07:51:23 -0000 1.68
> +++ newfs.8 14 Jan 2011 02:09:52 -0000
> @@ -218,6 +218,8 @@ 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).
> +The argument may contain a multiplier, as documented in
> +.Xr scan_scaled 3 .
> A sector is the smallest addressable unit on the physical device.
> Changing this is useful only when using
> .Nm
> @@ -230,11 +232,14 @@ from its default will make it impossible
> to find the alternate superblocks 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.
> +The argument may contain a multiplier, as documented in
> +.Xr scan_scaled 3 .
> +If multiplier was not specified then this value is interpreted as number of
> +sectors (see
> +.Fl S ) ,
> +not number of bytes.
I'm not entirely happy with that... Maybe jmc@ can help?
I'd suggest something even simpler:
"If no multiplier is present,
.Ar size
represents the number of sectors (see
.Fl S ) .
> +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.88
> diff -u -p -r1.88 newfs.c
> --- newfs.c 13 Dec 2010 00:02:58 -0000 1.88
> +++ newfs.c 14 Jan 2011 02:09:52 -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;
> + int fssize_usebytes = 0;
>
> if (strstr(__progname, "mfs"))
> mfs = Nflag = quiet = 1;
> @@ -192,9 +194,12 @@ 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, §orsize) == -1)
> + fatal("sector size: %s: %s",
> + strerror(errno), optarg);
> + if (sectorsize <= 0)
> + fatal("sector size is not positive: %s",
> + optarg);
> break;
> case 'T':
> disktype = optarg;
> @@ -265,10 +270,18 @@ main(int argc, char *argv[])
> quiet = 1;
> break;
> case 's':
> - fssize = strtonum(optarg, 1, LLONG_MAX, &errstr);
> - if (errstr)
> + if (scan_scaled(optarg, &fssize_input) == -1)
> fatal("file system size is %s: %s",
> - errstr, optarg);
> + strerror(errno), optarg);
This looks strange;
-s xyz => "file system size is invalid argument: zyx" ?
> + if (fssize_input <= 0)
> + fatal("file system size is not positive: %s",
> + optarg);
Do we really need different ways of just telling us that the size
argument is invalid?
if (scan_scaled(optarg, &fssize_input) == -1 ||
fssize_input <= 0)
fatal("invalid file system size");
(Up for discussion)
> + 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;
> @@ -412,17 +425,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",
Maybe this should be changed into "%lld sectors" and/or the size in bytes?
> + argv[0], *cp, DL_GETPSIZE(pp));
> +
> fssize *= sectorsize / DEV_BSIZE;
> if (oflagset == 0 && fssize >= INT_MAX)
> Oflag = 2; /* FFS2 */
