On Fri, Aug 19, 2022 at 10:26:32AM +0000, Klemens Nanni wrote:
> It is parsed in three steps in this order:
> alphanumeric ones: 1C
> numerical ones: 0,1,5
> alphabetic ones: C,c
>
> Wrt. numerical ones, the strlen(3) check could be omitted, but we do
> need it to guard against '-c Cfoo', etc.
>
> However, deferring it past the strnum(3) check results in better error
> messages for the '-c 10' case:
>
> (wrong usage, forgot "-l" for "vnd0")
> # bioctl -c 10 vnd0 softraid0
> bioctl: Invalid RAID level
> # ./obj/bioctl -c 10 vnd0 softraid0
> usage: bioctl [-hiqv] [-a alarm-function] [-b channel:target[.lun]]
> ...
>
> (correct usage but unsupported RAID level)
> # bioctl -c 10 -l vnd0a softraid0
> bioctl: Invalid RAID level
> # ./obj/bioctl -c 10 -l vnd0a softraid0
> bioctl: unsupported RAID level
>
> Uppercase the abbreviation while here.
>
> Feedback? OK?
>
>
> Index: bioctl.c
> ===================================================================
> RCS file: /cvs/src/sbin/bioctl/bioctl.c,v
> retrieving revision 1.147
> diff -u -p -U4 -r1.147 bioctl.c
> --- bioctl.c 8 Feb 2021 19:05:05 -0000 1.147
> +++ bioctl.c 19 Aug 2022 10:18:13 -0000
> @@ -132,14 +132,14 @@ main(int argc, char *argv[])
> case 'c': /* create */
> func |= BIOC_CREATERAID;
> if (strcmp(optarg, "1C") == 0) {
> cr_level = 0x1C;
> - } else if (strlen(optarg) != 1) {
> - errx(1, "Invalid RAID level");
> } else if (isdigit((unsigned char)*optarg)) {
> cr_level = strtonum(optarg, 0, 10, &errstr);
> if (errstr != NULL)
> errx(1, "Invalid RAID level");
any reason not to use the errx(1, "RAID level %s: %s", errstr, optarg);
idiom?
> + } else if (strlen(optarg) != 1) {
> + errx(1, "Invalid RAID level");
> } else
maybe flip the logic around and do
} else if (strlen(optarg) == 1) {
cr_level = *optarg;
} else {
errx(1, "Invalid RAID level");
}
so that we treat the (potentially) valid cases first and then error out
once they're exhausted. As indicated, I wouldn't mind consistent use of
braces.
> cr_level = *optarg;
> break;
> case 'd':
> @@ -862,9 +862,9 @@ bio_createraid(u_int16_t level, char *de
> case 'c':
> min_disks = 1;
> break;
> default:
> - errx(1, "unsupported raid level");
> + errx(1, "unsupported RAID level");
> }
>
> if (no_dev < min_disks)
> errx(1, "not enough disks");
>
