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.
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.
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)
