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)



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.

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

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

Regards, Julian
--- sbin/fdisk/fdisk.c
+++ sbin/fdisk/fdisk.c
@@ -271,10 +271,12 @@
 static void    get_ptn_alignmemt(void);
 #if defined(USE_DISKLIST)
 static void    get_diskname(const char *, char *, size_t);
 #endif
 static int     change_part(int, int, int, daddr_t, daddr_t, char *);
+static void    parse_partstr(char *, int *, daddr_t *, daddr_t *, char **);
+static int64_t parse_smodifier(const char *, char **, int);
 static void    print_geometry(void);
 static int     first_active(void);
 static void    change_active(int);
 static void    change_bios_geometry(void);
 static void    dos(int, unsigned char *, unsigned char *, unsigned char *);
@@ -291,10 +293,11 @@
 #define DEC_SEC                1               /* asking for a sector number */
 #define        DEC_RND         2               /* round to end of first track 
*/
 #define        DEC_RND_0       4               /* convert 0 to size of a track 
*/
 #define DEC_RND_DOWN   8               /* subtract 1 track */
 #define DEC_RND_DOWN_2 16              /* subtract 2 tracks */
+#define DEC_RND_BNDY   32              /* round _down_ to boundary, instead of 
up. */
 static int     ptn_id(const char *, int *);
 static int     type_match(const void *, const void *);
 static const char *get_type(int);
 static int     get_mapping(int, unsigned int *, unsigned int *, unsigned int 
*, unsigned long *);
 #ifdef BOOTSEL
@@ -329,18 +332,19 @@
 {
        struct stat sb;
        int ch;
        size_t len;
        char *cp;
+       static char *partstr;
        int n;
 #ifdef BOOTSEL
        daddr_t default_ptn;            /* start sector of default ptn */
        char *cbootmenu = 0;
 #endif
 
        int csysid;     /* For the s_flag. */
-       unsigned int cstart, csize;
+       daddr_t cstart, csize;
        a_flag = u_flag = sh_flag = f_flag = s_flag = b_flag = 0;
        i_flag = B_flag = 0;
        v_flag = 0;
        E_flag = 0;
        csysid = cstart = csize = 0;
@@ -398,22 +402,13 @@
                case 'v':       /* Be verbose */
                        v_flag++;
                        break;
                case 's':       /* Partition details */
                        s_flag = 1;
-                       if (sscanf(optarg, "%d/%u/%u%n", &csysid, &cstart,
-                           &csize, &n) == 3) {
-                               if (optarg[n] == 0)
-                                       break;
-#ifdef BOOTSEL
-                               if (optarg[n] == '/') {
-                                       cbootmenu = optarg + n + 1;
-                                       break;
-                               }
-#endif
-                       }
-                       errx(1, "Bad argument to the -s flag.");
+                       partstr = strdup(optarg);
+                       if (!partstr)
+                               err(1, "Could not allocate string.");
                        break;
                case 'b':       /* BIOS geometry */
                        b_flag = 1;
                        if (sscanf(optarg, "%d/%d/%d%n", &b_cyl, &b_head,
                            &b_sec, &n) != 3 || optarg[n] != 0)
@@ -528,13 +523,21 @@
        /* Do the update stuff! */
        if (u_flag) {
                if (!f_flag && !b_flag)
                        change_bios_geometry();
 
-               if (s_flag)
+               if (s_flag) {
+                       parse_partstr(partstr, &csysid, &cstart, &csize, 
&cbootmenu);
+#ifndef BOOTSEL
+                       if (strlen(cbootmenu))
+                               errx(1, "Bad argument to the -s flag: Too many 
slashes.");
+#endif
+                       free(partstr);
+
                        change_part(E_flag, partition, csysid, cstart, csize,
                                cbootmenu);
+               }
                else {
                        int part = partition, chg_ext = E_flag, prompt = 1;
                        do {
                                if (prompt) {
                                        printf("\n");
@@ -1955,10 +1958,122 @@
                }
        }
        return 0;
 }
 
+/* This function has the same flags as the other round-determining stuff, but 
+ * one with a special meaning: DEC_RND_DOWN rounds also to the lower alignment
+ * boundary, instead of the upper one.
+ * We need this to have a total value of 100%. */
+static int64_t
+parse_smodifier(const char *sizestr, char **pos, int flags)
+{
+       char *cp;
+       int64_t acc;
+       int valid;
+       int len;
+
+       if (!isdigit((unsigned char)*sizestr) && *sizestr != '-')
+               return -1;
+
+       acc = strtoll(sizestr, &cp, 10);
+       len = strcspn(cp, " \t\n");
+       valid = 0;
+       if (len != 0) {
+               if (!strncasecmp(cp, "tb", len)) {
+                       acc *= 1024;
+                       valid = 1;
+               }
+               if (valid || !strncasecmp(cp, "gb", len)) {
+                       acc *= 1024;
+                       valid = 1;
+               }
+               if (valid || !strncasecmp(cp, "mb", len)) {
+                       acc *= SEC_IN_1M;
+                       /* round to whole number of cylinders */
+                       acc += ptn_alignment / 2;
+                       acc /= ptn_alignment;
+                       valid = 1;
+               } else if (!strncmp(cp, "%", len)) {
+                       if (acc < 0 || acc > 100)
+                               return -1;
+                       acc = disksectors / 100 * acc;
+                       /* round to whole number of cylinders */
+                       if (acc <= ptn_alignment / 2)
+                               acc += ptn_alignment / 2;
+                       else if (flags & DEC_RND_DOWN)
+                               acc -= ptn_alignment / 2;
+                       else
+                               acc += ptn_alignment / 2;
+                       acc /= ptn_alignment;
+                       valid = 1;
+               }
+               if (valid || !strncasecmp(cp, "cyl", len)) {
+                       acc *= ptn_alignment;
+                       /* adjustments for cylinder boundary */
+                       if (acc == 0 && flags & DEC_RND_0)
+                               acc += ptn_0_offset;
+                       if (flags & DEC_RND)
+                               acc += ptn_0_offset;
+                       if (flags & DEC_RND_DOWN)
+                               acc -= ptn_0_offset;
+                       if (flags & DEC_RND_DOWN_2)
+                               acc -= ptn_0_offset;
+                       cp += len;
+               }
+       }
+       if (pos != NULL)
+               *pos = cp;
+
+       return acc;
+}
+
+static void
+parse_partstr(char *partstr, int *csysid, daddr_t *cstart,
+       daddr_t *csize, char **cbootmenu)
+{
+       int64_t tval;
+       char *tmpstr;
+
+       /* Get sysid. */
+       tmpstr = strtok(partstr, "/");
+       if (tmpstr == NULL)
+               errx(1, "Bad argument to the -s flag.");
+       *csysid = strtoll(tmpstr, NULL, 10);
+       if (strspn(tmpstr, "1234567890") != strlen(tmpstr))
+               errx(1, "Bad id argument to the -s flag.");
+
+       /* Get start. */
+       tmpstr = strtok(NULL, "/");
+       if (tmpstr == NULL)
+               errx(1, "Bad 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, "Bad 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;
+
+#ifdef BOOTSEL
+       /* Get bootmenu. */
+       tmpstr = strtok(NULL, "/");
+       if (tmpstr != NULL)
+               *cbootmenu = strdup(tmpstr);
+       else
+               *cbootmenu = NULL;
+#endif
+
+       return;
+}
+
 static int
 change_part(int extended, int part, int sysid, daddr_t start, daddr_t size,
        char *bootmenu)
 {
        struct mbr_partition *partp;
@@ -2761,44 +2876,15 @@
                        return dflt;
 
                if (cp[0] == '$' && cp[1] == '\n')
                        return maxval;
 
-               if (isdigit((unsigned char)*cp) || *cp == '-') {
-                       acc = strtoll(lbuf, &cp, 10);
-                       len = strcspn(cp, " \t\n");
-                       valid = 0;
-                       if (len != 0 && (flags & DEC_SEC)) {
-                               if (!strncasecmp(cp, "gb", len)) {
-                                       acc *= 1024;
-                                       valid = 1;
-                               }
-                               if (valid || !strncasecmp(cp, "mb", len)) {
-                                       acc *= SEC_IN_1M;
-                                       /* round to whole number of cylinders */
-                                       acc += ptn_alignment / 2;
-                                       acc /= ptn_alignment;
-                                       valid = 1;
-                               }
-                               if (valid || !strncasecmp(cp, "cyl", len)) {
-                                       acc *= ptn_alignment;
-                                       /* adjustments for cylinder boundary */
-                                       if (acc == 0 && flags & DEC_RND_0)
-                                               acc += ptn_0_offset;
-                                       if (flags & DEC_RND)
-                                               acc += ptn_0_offset;
-                                       if (flags & DEC_RND_DOWN)
-                                               acc -= ptn_0_offset;
-                                       if (flags & DEC_RND_DOWN_2)
-                                               acc -= ptn_0_offset;
-                                       cp += len;
-                               }
-                       }
-               }
+               if (flags & DEC_SEC)
+                       acc = parse_smodifier(cp, &cp, flags);
 
                cp += strspn(cp, " \t");
-               if (*cp != '\n') {
+               if (*cp != '\n' || acc == -1) {
                        lbuf[strlen(lbuf) - 1] = 0;
                        printf("%s is not a valid %s number.\n", lbuf,
                            flags & DEC_SEC ? "sector" : "decimal");
                        continue;
                }

Attachment: signature.asc
Description: PGP signature



Home | Main Index | Thread Index | Old Index