tech-userlevel archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

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



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


Home | Main Index | Thread Index | Old Index