On Fri, Aug 19, 2022 at 11:58:20AM +0000, Klemens Nanni wrote:
> On Fri, Aug 19, 2022 at 01:10:32PM +0200, Theo Buehler wrote:
> > 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?
>
> RAID levels are discrete values, not an interval, so
> "RAID level 11: too lage" does not read like an improvement to me.
To my knowledge levels are an interval (0-6), then there are the nested
levels, which we don't support anyway. I don't know where the upper
bound 10 comes from, perhaps an off-by-one for parsing a single digit?
Anyway, I don't care much.
> strtonum(3) is really just a short form of strcmp'ing optarg against all
> valid RAID levels, from which we know at this point that they at least
> start with a digit.
>
> >
> > > + } 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.
>
> Sure, why not. I just kept the diff simple.
I'm ok with the diff.
>
> Index: bioctl.c
> ===================================================================
> RCS file: /cvs/src/sbin/bioctl/bioctl.c,v
> retrieving revision 1.147
> diff -u -p -r1.147 bioctl.c
> --- bioctl.c 8 Feb 2021 19:05:05 -0000 1.147
> +++ bioctl.c 19 Aug 2022 11:55:53 -0000
> @@ -133,14 +133,15 @@ main(int argc, char *argv[])
> 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");
> - } else
> + } else if (strlen(optarg) == 1) {
> cr_level = *optarg;
> + } else {
> + errx(1, "Invalid RAID level");
> + }
> break;
> case 'd':
> /* delete volume */
> @@ -863,7 +864,7 @@ bio_createraid(u_int16_t level, char *de
> min_disks = 1;
> break;
> default:
> - errx(1, "unsupported raid level");
> + errx(1, "unsupported RAID level");
> }
>
> if (no_dev < min_disks)
