Re: Making fdisk accept percentages (and modifiers on shell)

[Jean-Yves Migeon <jeanyves.migeon%free.fr@localhost>]

Le 25/12/11 02:42, Julian Djamil Fagir a écrit :
> Hi,
>
> > 1 - keep the parsing (parse_partstr) in the "case 's' " block like
> > before, instead of deferring it at a later step (before change_part() ).
> > The sooner you can error out because of argument errors, the better.
> >
> > 2 - no need to strdup(3) optarg if you can do 1).
> you have to check all options first, because the disk is the only argument.
> Without disk, you cannot determine size, and then you cannot compute
> percentages.

Noted.

> > 3 - missing lower bound check on acc in decimal():
> To be honest: I did not really think about that. I took out the check
> because decimal() also accepts negative sizes, though I don't know why.
> But you're right, for percentages, you can switch this on.
>
> > 4 - documentation does not look right, it's "cyl/mb/gb/tb" (or
> > "CYL/MB/GB/TB"). "c/m/g/t" are not valid according to the various
> > strncasecmp() in the code.
> Because of the 'n' it is - only the length of the modifier of the number is
> taken, so this works.

Indeed. Waow, perhaps dehumanize_number(3) was not available at the time 
fdisk(8) was written.

> > 5 - decimal() and parse_smodifier() have a lot to share, so I am against
> > copy/pasting their code in two different functions. You can merge both,
> > or at least have a common parse_digit() function. This would avoid typo
> > like the one for 3).
> >
> > 6 - you have a copy/paste typo in parse_partstr(), the second "get
> > start" comment should read "get size".
> Fixed that.
>
> Ok, in the attachment, you can see a patch that fixes 3, 5 and 6.
> For 1 and 2, I would rather do it the way I already did. Checking things
> first, saving the values, and then at some point applying them is imho
> confusing.

It's more about applying checks before anything gets executed further. 
You can bail out easily when this gets done early, rather than later 
where you sometimes have to back out changes the program made.

Minor knits I just noticed ((more for the sake of being clean):

- meaningful errors when missing an argument in -s
- check for 'id' validity before converting via strtoll(3)

=>

+       tmpstr = strtok(partstr, "/");
+       if (tmpstr == NULL)
+               errx(1, "Missing id argument to the -s flag.");
+       if (strspn(tmpstr, "1234567890") != strlen(tmpstr))
+               errx(1, "Bad id argument to the -s flag.");
+       *csysid = strtoll(tmpstr, NULL, 10);
+
+       /* Get start. */
+       tmpstr = strtok(NULL, "/");
+       if (tmpstr == NULL)
+               errx(1, "Missing start argument to the -s flag.");
+       tval = parse_smodifier(tmpstr, NULL, DEC_RND);
+       if (tval < 0)
+               errx(1, "Bad start argument to the -s flag.");
+       *cstart = tval;
+
+       /* Get size. */
+       tmpstr = strtok(NULL, "/");
+       if (tmpstr == NULL)
+               errx(1, "Missing size argument to the -s flag.");
+       tval = parse_smodifier(tmpstr, NULL, DEC_RND_DOWN);
+       if (tval < 0)
+               errx(1, "Bad size argument to the -s flag.");
+       *csize = tval;
+

Rest looks ok to me. Thanks!

--
jy