[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [PATCH] Use glib for getPartitionsList()



 On 09/24/2010 02:13 PM, Brian C. Lane wrote:
 On Fri, Sep 24, 2010 at 12:25:00PM -1000, David Cantrell wrote:
> Use glib functions to simplify the getPartitionsList() function in
> loader. Also use the g_strfreev() and g_strv_length() functions to
> eliminate the need for those equivalent functions in getparts.c.
> ---

 A few minor comments:

 1. More documentation, the previous code wasi easier to figure out
 especially with the lines dealing with skipping the partition
 (the check for "1" == skip extended partition wasn't obvious)
 Maybe an example of /proc/partitions with an extended partition
 included.

Added in some comments to explain things a bit more.

 2. g_strsplit_set() max only needs to be 0, not -1

Changed.

 3. check for disk == NULL at top and bail out early.

We don't want that because getPartitionsList() needs to be able to be called
with NULL and return a list of all partitions.  This line handles filtering:

    if (disk != NULL && !g_str_has_prefix(tokens[3], disk)) {

 4. When checking the length of tokens just look for == 4, if there
 aren't 4 then the following code will fail in weird ways.

This block keeps that from happening:

+            while ((j < g_strv_length(fields)) && (i < 4)) {
+                if (g_strcmp0(fields[j], "")) {
+                    tokens[i++] = fields[j];
+                }
+
+                j++;
+            }

Because of the way g_strsplit_set() works, I do this to filter out the empty
fields and just count the fields with data, but then make sure we stay within
the four field limit.

The case that I thought would not be handled were blank lines, but that is
caught fine. I ran this new version of getPartitionsList() locally against my
/proc/partitions, both passing NULL and things like "sda" and "sdb" and it
returned the expected lists of partitions.

 Looks good other than that.

Updated patch:


[PATCH] Use glib for getPartitionsList()

Use glib functions to simplify the getPartitionsList() function in
loader.  Also use the g_strfreev() and g_strv_length() functions to
eliminate the need for those equivalent functions in getparts.c.
---
 loader/driverdisk.c |    4 +-
loader/getparts.c | 189 +++++++++++++++++++--------------------------------
 loader/getparts.h   |    6 +-
 loader/hdinstall.c  |    4 +-
 loader/loader.c     |    4 +-
 5 files changed, 79 insertions(+), 128 deletions(-)

diff --git a/loader/driverdisk.c b/loader/driverdisk.c
index ada8e81..2688c82 100644
--- a/loader/driverdisk.c
+++ b/loader/driverdisk.c
@@ -424,7 +424,7 @@ int loadDriverFromMedia(int class, struct loaderData_s *loaderData,
             if (part != NULL)
                 free(part);

-            if ((nump = lenPartitionsList(part_list)) == 0) {
+            if ((nump = g_strv_length(part_list)) == 0) {
                 if (dir == -1)
                     stage = DEV_DEVICE;
                 else
@@ -442,7 +442,7 @@ int loadDriverFromMedia(int class, struct loaderData_s *loaderData,
                              _("Back"), NULL);

             if (rc == 2) {
-                freePartitionsList(part_list);
+                g_strfreev(part_list);
                 stage = DEV_DEVICE;
                 dir = -1;
                 break;
diff --git a/loader/getparts.c b/loader/getparts.c
index c4544d7..e08e2f8 100644
--- a/loader/getparts.c
+++ b/loader/getparts.c
@@ -1,7 +1,7 @@
 /*
  * getparts.c - functions associated with getting partitions for a disk
  *
- * Copyright (C) 1997, 1998, 1999, 2000, 2001, 2002, 2003, 2004 Red Hat, Inc.
+ * Copyright (C) 1997-2010  Red Hat, Inc.
  * All rights reserved.
  *
  * This program is free software; you can redistribute it and/or modify
@@ -19,6 +19,7 @@
  *
  * Author(s): Michael Fulbright <msf redhat com>
  *            Jeremy Katz <katzj redhat com>
+ *            David Cantrell <dcantrell redhat com>
  */

 #include <stdio.h>
@@ -28,6 +29,7 @@
 #include <errno.h>
 #include <ctype.h>
 #include <string.h>
+#include <glib.h>

 #include "../pyanaconda/isys/log.h"

@@ -50,131 +52,80 @@ static int isPartitionName(char *pname) {
     }
 }

-/* return NULL terminated array of pointers to names of partitons in
- * /proc/partitions
- */
-char **getPartitionsList(char * disk) {
-    FILE *f;
-    int numfound = 0;
-    char **rc=NULL;
-
-    f = fopen("/proc/partitions", "r");
-    if (!f) {
- logMessage(ERROR, "getPartitionsList: could not open /proc/partitions");
-    return NULL;
+/* Return array of the names of partitons in /proc/partitions */
+gchar **getPartitionsList(gchar *disk) {
+    guint i, j;
+    gchar *contents = NULL;
+    gchar *tokens[] = { NULL, NULL, NULL, NULL };
+    gchar **lines, **iter, **rc;
+    gsize len;
+    GError *e = NULL;
+    GSList *parts = NULL, *list = NULL;
+
+    /* read in /proc/partitions and split in to an array of lines */
+    if (!g_file_get_contents("/proc/partitions", &contents, &len, &e)) {
+        return NULL;
     }

-    /* read through /proc/partitions and parse out partitions */
-    while (1) {
-    char *tmpptr, *pptr;
-    char tmpstr[4096];
-
-    tmpptr = fgets(tmpstr, sizeof(tmpstr), f);
-
-    if (tmpptr) {
-        char *a, *b;
-        int toknum = 0;
-
-        a = tmpstr;
-        while (1) {
-        b = strsep(&a, " \n");
-
-        /* if no fields left abort */
-        if (!b)
-            break;
-
-        /* if field was empty means we hit another delimiter */
-        if (!*b)
-            continue;
-
-        /* make sure this is a valid partition line, should start */
-        /* with a numeral */
-        if (toknum == 0) {
-            if (!isdigit(*b))
-            break;
-        } else if (toknum == 2) {
-            /* if size is exactly 1 then ignore it as an extended */
-            if (!strcmp(b, "1"))
-            break;
-        } else if (toknum == 3) {
-            /* this should be the partition name */
-            /* now we need to see if this is the block device or */
-            /* actually a partition name                         */
-            if (!isPartitionName(b))
-            break;
-
-                    /* make sure that either we don't care about the disk
-                     * or it's this one */
-                    if ((disk != NULL) && (strncmp(disk, b, strlen(disk))))
-                        break;
-
-            /* we found a partition! */
-            pptr = (char *) malloc(strlen(b) + 7);
-            sprintf(pptr, "/dev/%s", b);
-
-            if (!rc) {
-            rc = (char **) malloc(2*sizeof(char *));
-                rc[0] = pptr;
-            rc[1] = NULL;
-            } else {
-            int idx;
-
-            rc = (char **) realloc(rc, (numfound+2)*sizeof(char *));
-            idx = 0;
-            while (idx < numfound) {
-                if (strcmp(pptr, rc[idx]) < 0)
-                break;
-
-                idx++;
-            }
-
-            /* move existing out of way if necessary */
-            if (idx != numfound)
-                memmove(rc+idx+1, rc+idx, (numfound-idx)*sizeof(char *));
-
-            rc[idx] = pptr;
-            rc[numfound+1] = NULL;
-            }
-            numfound++;
-            break;
-        }
-        toknum++;
-        }
-    } else {
-        break;
-    }
+    if (contents == NULL) {
+        return NULL;
     }

-    fclose(f);
-
-    return rc;
-}
-
-/* returns length of partitionlist */
-int lenPartitionsList(char **list) {
-    char **part;
-    int  rc;
-
-    if (!list) return 0;
-    for (rc = 0, part = list; *part; rc++, part++);
-
-    return rc;
-}
+    iter = lines = g_strsplit_set(contents, "\n", 0);
+    g_free(contents);
+
+    /* extract partition names from /proc/partitions lines */
+    while (*iter != NULL) {
+        /* split the line in to fields */
+        gchar **fields = g_strsplit_set(*iter, " ", 0);
+        i = j = 0;
+
+        if (g_strv_length(fields) > 0) {
+            /* if we're on a non-empty line, toss empty fields so we
+             * end up with the major, minor, #blocks, and name fields
+             * in positions 0, 1, 2, and 3
+             */
+            while ((j < g_strv_length(fields)) && (i < 4)) {
+                if (g_strcmp0(fields[j], "")) {
+                    tokens[i++] = fields[j];
+                }
+
+                j++;
+            }
+
+            /* skip lines where:
+             * - the 'major' column is a non-digit
+             * - the '#blocks' column is '1' (indicates extended partition)
+             * - the 'name' column is not a valid partition name
+             */
+            if (isdigit(*tokens[0]) && g_strcmp0(tokens[2], "1") &&
+                isPartitionName(tokens[3])) {
+                /* if disk is specified, only return a list of partitions
+                 * for that device
+                 */
+                if (disk != NULL && !g_str_has_prefix(tokens[3], disk)) {
+                    g_strfreev(fields);
+                    iter++;
+                    continue;
+                }
+
+                parts = g_slist_prepend(parts, g_strdup(tokens[3]));
+            }
+        }

-/* frees partition list */
-void freePartitionsList(char **list) {
-    char **part;
+        g_strfreev(fields);
+        iter++;
+    }

-    if (!list)
-        return;
+    i = g_slist_length(parts);
+    rc = g_new(gchar *, i + 1);
+    rc[i] = NULL;

-    for (part = list; *part; part++) {
-    if (*part) {
-            free(*part);
-            *part = NULL;
-        }
+    for (list = parts; list != NULL; list = list->next) {
+        rc[--i] = list->data;
     }

-    free(list);
-    list = NULL;
+    g_strfreev(lines);
+    g_slist_free(parts);
+    return rc;
 }
diff --git a/loader/getparts.h b/loader/getparts.h
index b672a77..52a978c 100644
--- a/loader/getparts.h
+++ b/loader/getparts.h
@@ -20,8 +20,8 @@
 #ifndef GETPARTS_H
 #define GETPARTS_H

-char **getPartitionsList(char * disk);
-int lenPartitionsList(char **list);
-void freePartitionsList(char **list);
+#include <glib.h>
+
+gchar **getPartitionsList(gchar * disk);

 #endif
diff --git a/loader/hdinstall.c b/loader/hdinstall.c
index d52e969..39633d3 100644
--- a/loader/hdinstall.c
+++ b/loader/hdinstall.c
@@ -158,10 +158,10 @@ int promptForHardDrive(struct loaderData_s *loaderData) {
     while (1) {
         /* if we're doing another pass free this up first */
         if (partition_list)
-            freePartitionsList(partition_list);
+            g_strfreev(partition_list);

         partition_list = getPartitionsList(NULL);
-        numPartitions = lenPartitionsList(partition_list);
+        numPartitions = g_strv_length(partition_list);

/* no partitions found, try to load a device driver disk for storage */
         if (!numPartitions) {
diff --git a/loader/loader.c b/loader/loader.c
index 9fccd44..2836593 100644
--- a/loader/loader.c
+++ b/loader/loader.c
@@ -478,7 +478,7 @@ void loadUpdates(struct loaderData_s *loaderData) {
                 part = NULL;
             }

-            if ((nump = lenPartitionsList(part_list)) == 0) {
+            if ((nump = g_strv_length(part_list)) == 0) {
                 if (dir == -1) {
                     stage = UPD_DEVICE;
                 } else {
@@ -499,7 +499,7 @@ void loadUpdates(struct loaderData_s *loaderData) {
                              _("Back"), NULL);

             if (rc == 2) {
-                freePartitionsList(part_list);
+                g_strfreev(part_list);
                 stage = UPD_DEVICE;
                 dir = -1;
                 break;
--
1.7.2.3

--
David Cantrell <dcantrell redhat com>
Red Hat / Honolulu, HI


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]