On Thu, Jan 13, 2011 at 8:48 AM, Vadim Zhukov <[email protected]> wrote:
> This patch allows specifing K/M/T/... prefixies in newfs(8)
> -S and -s options. Useful for mount_mfs, now you can just say:
>
> mount_mfs -s 50m swap /mnt
>
> and it will do what you want, taking in account sector size. Old
> behavior of -s (specifying count of sectors) is, of course,
> preserved. Hope this'll be useful.
Nice.
> 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),
> + optarg);
I don't think we need this. There aren't many choices, and all of
them are small numbers people know how to type.
> 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);
This is definite strtonum abuse. I'd prefer to see something that
tries a little harder to call the right function.
Also, things get a little slippery because you're leaving this block
of code with only one variable set properly. Get the right value into
fssize before leaving the case statement.
