Re: [PATCH] Use module reloading in driver disc operations (#586905)

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

 

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Ack for functionality, but I have some comments on coding style cleanup.

On Thu, 6 May 2010, Martin Sivak wrote:


We have to load all drivers to get access to devices containing driver discs. But when we copy the DD content into RAM, we have to reinitialize those drivers to pick up updated versions.
---
loader/driverdisk.c   |   68 +++++++++---------------------
loader/driverdisk.h   |    9 +---
loader/driverselect.c |    4 +-
loader/hdinstall.c    |    4 +-
loader/loader.c       |   51 +++++++++++++----------
loader/modules.c      |  110 ++++++++++++++++++++++++++++++++++++++++++++++++-
loader/modules.h      |    4 ++
scripts/upd-instroot  |    2 +-
8 files changed, 171 insertions(+), 81 deletions(-)

diff --git a/loader/driverdisk.c b/loader/driverdisk.c
index 192beb0..bfebc06 100644
--- a/loader/driverdisk.c
+++ b/loader/driverdisk.c
@@ -63,40 +63,6 @@
/* boot flags */
extern uint64_t flags;

-/* modprobe DD mode */
-int modprobeDDmode(void)
-{
-    FILE *f = fopen("/etc/depmod.d/ddmode.conf", "w");
-    if (f) {
-        struct utsname unamedata;
-
-        if (uname(&unamedata))
-            fprintf(f, " pblacklist /lib/modules\n");
-        else
-            fprintf(f, " pblacklist /lib/modules/%s\n", unamedata.release);
-        fclose(f);
-    }
-
-    return f==NULL;
-}
-
-int modprobeNormalmode(void)
-{
-    /* remove depmod overrides */
-    if (unlink("/etc/depmod.d/ddmode.conf")) {
-        logMessage(ERROR, "removing ddmode.conf failed");
-        return -1;
-    }
-
-    /* run depmod to refresh modules db */
-    if (system("depmod -a")) {
-        logMessage(ERROR, "depmod -a failed");
-        return -1;
-    }
-
-    return 0;
-}
-
/*
 * check if the RPM in question provides
 * Provides: userptr
@@ -362,6 +328,7 @@ int getRemovableDevices(char *** devNames) {
    devs = getDevices(DEVICE_DISK | DEVICE_CDROM);

    if(devs) for (i = 0; devs[i] ; i++) {
+            logMessage(DEBUGLVL, "Considering device %s (isremovable: %d)", devs[i]->device, devs[i]->priv.removable);
        if (devs[i]->priv.removable) {
            *devNames = realloc(*devNames, (numDevices + 2) * sizeof(char *));
            (*devNames)[numDevices] = strdup(devs[i]->device);
@@ -383,7 +350,7 @@ int getRemovableDevices(char *** devNames) {
 * usecancel: if 1, use cancel instead of back
 */
int loadDriverFromMedia(int class, struct loaderData_s *loaderData,
-                        int usecancel, int noprobe) {
+                        int usecancel, int noprobe, GTree *moduleState) {
    char * device = NULL, * part = NULL, * ddfile = NULL;
    char ** devNames = NULL;
    enum { DEV_DEVICE, DEV_PART, DEV_CHOOSEFILE, DEV_LOADFILE,
@@ -609,6 +576,9 @@ int loadDriverFromMedia(int class, struct loaderData_s *loaderData,
                break;
            }

+            /* Unload all devices and load them again to use the updated modules */
+            logMessage(INFO, "Trying to refresh loaded drivers");
+            mlRestoreModuleState(moduleState);
            busProbe(0);

            devices = getDevices(class);
@@ -659,7 +629,7 @@ int loadDriverFromMedia(int class, struct loaderData_s *loaderData,


/* looping way to load driver disks */
-int loadDriverDisks(int class, struct loaderData_s *loaderData) {
+int loadDriverDisks(int class, struct loaderData_s *loaderData, GTree *moduleState) {
    int rc;

    rc = newtWinChoice(_("Driver disk"), _("Yes"), _("No"),
@@ -667,7 +637,7 @@ int loadDriverDisks(int class, struct loaderData_s *loaderData) {
    if (rc != 1)
        return LOADER_OK;

-    rc = loadDriverFromMedia(DEVICE_ANY, loaderData, 1, 0);
+    rc = loadDriverFromMedia(DEVICE_ANY, loaderData, 1, 0, moduleState);
    if (rc == LOADER_BACK)
        return LOADER_OK;

@@ -676,23 +646,27 @@ int loadDriverDisks(int class, struct loaderData_s *loaderData) {
                           _("Do you wish to load any more driver disks?"));
        if (rc != 1)
            break;
-        loadDriverFromMedia(DEVICE_ANY, loaderData, 0, 0);
+        loadDriverFromMedia(DEVICE_ANY, loaderData, 0, 0, moduleState);
    } while (1);

    return LOADER_OK;
}

-static void loadFromLocation(struct loaderData_s * loaderData, char * dir) {
+static void loadFromLocation(struct loaderData_s * loaderData, char * dir, GTree *moduleState) {
    if (verifyDriverDisk(dir) == LOADER_BACK) {
        logMessage(ERROR, "not a valid driver disk");
        return;
    }

    loadDriverDisk(loaderData, dir);
+
+    /* Unload all devices and load them again to use the updated modules */
+    logMessage(INFO, "Trying to refresh loaded drivers");
+    mlRestoreModuleState(moduleState);
    busProbe(0);
}

-void getDDFromSource(struct loaderData_s * loaderData, char * src) {
+void getDDFromSource(struct loaderData_s * loaderData, char * src, GTree *moduleState) {
    char *path = "/tmp/dd.img";
    int unlinkf = 0;

@@ -712,7 +686,7 @@ void getDDFromSource(struct loaderData_s * loaderData, char * src) {
     * scsi cdrom drives */
#if !defined(__s390__) && !defined(__s390x__)
    } else if (!strncmp(src, "cdrom", 5)) {
-        loadDriverDisks(DEVICE_ANY, loaderData);
+        loadDriverDisks(DEVICE_ANY, loaderData, moduleState);
        return;
#endif
    } else if (!strncmp(src, "path:", 5)) {
@@ -724,7 +698,7 @@ void getDDFromSource(struct loaderData_s * loaderData, char * src) {
    }

    if (!mountLoopback(path, "/tmp/drivers", "/dev/loop6")) {
-        loadFromLocation(loaderData, "/tmp/drivers");
+        loadFromLocation(loaderData, "/tmp/drivers", moduleState);
        umountLoopback("/tmp/drivers", "/dev/loop6");
        unlink("/tmp/drivers");
        if (unlinkf) unlink(path);
@@ -732,7 +706,7 @@ void getDDFromSource(struct loaderData_s * loaderData, char * src) {

}

-static void getDDFromDev(struct loaderData_s * loaderData, char * dev);
+static void getDDFromDev(struct loaderData_s * loaderData, char * dev, GTree *moduleState);

void useKickstartDD(struct loaderData_s * loaderData,
                    int argc, char ** argv) {
@@ -796,22 +770,22 @@ void useKickstartDD(struct loaderData_s * loaderData,
    }

    if (dev) {
-        getDDFromDev(loaderData, dev);
+        getDDFromDev(loaderData, dev, NULL);
    } else {
-        getDDFromSource(loaderData, src);
+        getDDFromSource(loaderData, src, NULL);
    }

    g_strfreev(remaining);
    return;
}

-static void getDDFromDev(struct loaderData_s * loaderData, char * dev) {
+static void getDDFromDev(struct loaderData_s * loaderData, char * dev, GTree* moduleState) {
    if (doPwMount(dev, "/tmp/drivers", "auto", "ro", NULL)) {
        logMessage(ERROR, "unable to mount driver disk %s", dev);
        return;
    }

-    loadFromLocation(loaderData, "/tmp/drivers");
+    loadFromLocation(loaderData, "/tmp/drivers", moduleState);
    umount("/tmp/drivers");
    unlink("/tmp/drivers");
}
diff --git a/loader/driverdisk.h b/loader/driverdisk.h
index 98bfd4a..4dc8685 100644
--- a/loader/driverdisk.h
+++ b/loader/driverdisk.h
@@ -32,9 +32,9 @@
extern char *ddFsTypes[];

int loadDriverFromMedia(int class, struct loaderData_s *loaderData,
-                        int usecancel, int noprobe);
+                        int usecancel, int noprobe, GTree *moduleState);

-int loadDriverDisks(int class, struct loaderData_s *loaderData);
+int loadDriverDisks(int class, struct loaderData_s *loaderData, GTree *moduleState);

int getRemovableDevices(char *** devNames);

@@ -42,13 +42,10 @@ int chooseManualDriver(int class, struct loaderData_s *loaderData);
void useKickstartDD(struct loaderData_s * loaderData, int argc,
                    char ** argv);

-void getDDFromSource(struct loaderData_s * loaderData, char * src);
+void getDDFromSource(struct loaderData_s * loaderData, char * src, GTree *moduleState);

int loadDriverDiskFromPartition(struct loaderData_s *loaderData, char* device);

GSList* findDriverDiskByLabel(void);

-int modprobeNormalmode();
-int modprobeDDmode();
-
#endif
diff --git a/loader/driverselect.c b/loader/driverselect.c
index f9379f0..14d05e7 100644
--- a/loader/driverselect.c
+++ b/loader/driverselect.c
@@ -155,7 +155,7 @@ int chooseManualDriver(int class, struct loaderData_s *loaderData) {
            if (i != 1)
                return LOADER_BACK;

-            loadDriverFromMedia(class, loaderData, 1, 1);
+            loadDriverFromMedia(class, loaderData, 1, 1, NULL);
            continue;
        } else {
            break;
@@ -235,7 +235,7 @@ int chooseManualDriver(int class, struct loaderData_s *loaderData) {
    if (done == -1)
        return LOADER_BACK;
    if (done == -2) {
-        loadDriverFromMedia(class, loaderData, 1, 1);
+        loadDriverFromMedia(class, loaderData, 1, 1, NULL);
        return chooseManualDriver(class, loaderData);
    }

diff --git a/loader/hdinstall.c b/loader/hdinstall.c
index e05be0c..54a5fd9 100644
--- a/loader/hdinstall.c
+++ b/loader/hdinstall.c
@@ -218,7 +218,7 @@ char * mountHardDrive(struct installMethod * method,
                return NULL;
            }

-            rc = loadDriverFromMedia(DEVICE_DISK, loaderData, 0, 0);
+            rc = loadDriverFromMedia(DEVICE_DISK, loaderData, 0, 0, NULL);
            continue;
        }

@@ -303,7 +303,7 @@ char * mountHardDrive(struct installMethod * method,
            loaderData->stage2Data = NULL;
            return NULL;
        } else if (es.reason == NEWT_EXIT_HOTKEY && es.u.key == NEWT_KEY_F2) {
-            rc = loadDriverFromMedia(DEVICE_DISK, loaderData, 0, 0);
+            rc = loadDriverFromMedia(DEVICE_DISK, loaderData, 0, 0, NULL);
            continue;
        }

diff --git a/loader/loader.c b/loader/loader.c
index c828b96..91abdab 100644
--- a/loader/loader.c
+++ b/loader/loader.c
@@ -1379,7 +1379,7 @@ static char *doLoaderMain(struct loaderData_s *loaderData,
                    break;
                }

-                rc = loadDriverFromMedia(class, loaderData, 0, 0);
+                rc = loadDriverFromMedia(class, loaderData, 0, 0, NULL);
                if (rc == LOADER_BACK) {
                    step = STEP_DRIVER;
                    dir = -1;
@@ -1799,6 +1799,7 @@ int main(int argc, char ** argv) {

    char *path, *fmt;
    GSList *dd, *dditer;
+    GTree *moduleState;

    gchar *cmdLine = NULL, *ksFile = NULL, *virtpcon = NULL;
    gboolean mediacheck = FALSE;
@@ -1963,9 +1964,6 @@ int main(int argc, char ** argv) {
    /* FIXME: this is a bit of a hack */
    loaderData.modInfo = modInfo;

-    /* Setup depmod & modprobe so we can load multiple DDs */
-    modprobeDDmode();
-
    /* If there is /.rundepmod file present, rerun depmod */
    if (!access("/.rundepmod", R_OK)){
        if (system("depmod -a")) {
@@ -1974,6 +1972,25 @@ int main(int argc, char ** argv) {
        }
    }

+    /* this allows us to do an early load of modules specified on the
+     * command line to allow automating the load order of modules so that
+     * eg, certain scsi controllers are definitely first.
+     * FIXME: this syntax is likely to change in a future release
+     *        but is done as a quick hack for the present.
+     */
+    if (!mlInitModuleConfig()) {
+        logMessage(ERROR, "unable to initialize kernel module loading");
+        abort();
+    }
+
+    earlyModuleLoad(0);
+
+    /* Save list of preloaded modules so we can restore the state */
+    moduleState = mlSaveModuleState();
+
+    /* Load all known devices */
+    busProbe(FL_NOPROBE(flags));
+
    if (FL_AUTOMODDISK(flags)) {
        /* Load all autodetected DDs */
        logMessage(INFO, "Trying to detect vendor driver discs");
@@ -1986,6 +2003,10 @@ int main(int argc, char ** argv) {
            }
            else {
                logMessage(INFO, "Automatic driver disk loader succeeded for %s.", (char*)(dditer->data));
+
+                /* Unload all devices and load them again to use the updated modules */
+                mlRestoreModuleState(moduleState);
+                busProbe(0);
            }

            /* clean the device record */
@@ -2000,30 +2021,16 @@ int main(int argc, char ** argv) {

    if (FL_MODDISK(flags)) {
        startNewt();
-        loadDriverDisks(DEVICE_ANY, &loaderData);
+        loadDriverDisks(DEVICE_ANY, &loaderData, moduleState);
    }

    if (!access("/dd.img", R_OK)) {
        logMessage(INFO, "found /dd.img, loading drivers");
-        getDDFromSource(&loaderData, "path:/dd.img");
+        getDDFromSource(&loaderData, "path:/dd.img", moduleState);
    }

    /* Reset depmod & modprobe to normal mode and get the rest of drivers*/
-    modprobeNormalmode();
-
-    /* this allows us to do an early load of modules specified on the
-     * command line to allow automating the load order of modules so that
-     * eg, certain scsi controllers are definitely first.
-     * FIXME: this syntax is likely to change in a future release
-     *        but is done as a quick hack for the present.
-     */
-    if (!mlInitModuleConfig()) {
-        logMessage(ERROR, "unable to initialize kernel module loading");
-        abort();
-    }
-
-    earlyModuleLoad(0);
-
+    mlFreeModuleState(moduleState);
    busProbe(FL_NOPROBE(flags));

    /* HAL daemon */
@@ -2055,7 +2062,7 @@ int main(int argc, char ** argv) {
     * we won't have network devices available (and that's the only thing
     * we support with this right now */
    if (loaderData.ddsrc != NULL) {
-        getDDFromSource(&loaderData, loaderData.ddsrc);
+        getDDFromSource(&loaderData, loaderData.ddsrc, NULL);
    }

    /* JKFIXME: loaderData->ksFile is set to the arg from the command line,
diff --git a/loader/modules.c b/loader/modules.c
index 07f781a..b7608bf 100644
--- a/loader/modules.c
+++ b/loader/modules.c
@@ -24,7 +24,6 @@
 *            Jeremy Katz <katzj@xxxxxxxxxx>
 *            David Cantrell <dcantrell@xxxxxxxxxx>
 */
-
#include <ctype.h>
#include <errno.h>
#include <fcntl.h>
@@ -39,6 +38,8 @@
#include <sys/wait.h>
#include <unistd.h>
#include <glib.h>
+#include <glib/gprintf.h>
+#include <assert.h>

#include "loader.h"
#include "log.h"
@@ -408,3 +409,110 @@ void loadKickstartModule(struct loaderData_s * loaderData,
    g_strfreev(args);
    return;
}
+
+inline gint gcmp(gconstpointer a, gconstpointer b, gpointer userptr)
+{
+    return g_strcmp0(a, b);
+}
+
+int processModuleLines(GTree *data, int (*f)(gchar**, GTree*)){
+    char *line = NULL;
+    size_t linesize = 0;
+    gchar** lineparts = NULL;
+    int count = 0;
+
+    FILE *file = fopen("/proc/modules", "r");
+    if (file == NULL) return -1;


We don't (or at least try to avoid) if blocks like this.  I would prefer:

    if (file == NULL)
        return -1;

or:

    if (file == NULL) {
        return -1;
    }


+
+    while (1) {
+        if (getline(&line, &linesize, file) < 0) break;
+        assert(*line);
+
+        lineparts = g_strsplit_set(line, " ", 4);
+	assert(lineparts);


This line uses a tab to indent, but should use spaces.


+
+        free(line);
+        line = NULL;
+        int ret = f(lineparts, data);
+        g_strfreev(lineparts);
+
+        if (ret < 0) break;


See if block comment above.


+        count+=ret;
+    }
+
+    fclose(file);
+    return count;
+}
+
+inline int cb_savestate(gchar** parts, GTree *data)
+{
+    logMessage(DEBUGLVL, "Saving module %s", parts[0]);
+    g_tree_insert(data, g_strdup(parts[0]), (gchar*)1);
+    return 1;
+}
+
+GTree* mlSaveModuleState()
+{
+    GTree *state = NULL;
+
+    state = g_tree_new_full(gcmp, NULL, g_free, NULL);
+    if(!state) return NULL;


See if block comment above.


+
+    processModuleLines(state, cb_savestate);
+
+    return state;
+}
+
+inline int cb_restorestate(gchar** parts, GTree *data)
+{
+    pid_t pid;
+    int status;
+
+    /* this module has to stay loaded */
+    if (g_tree_lookup(data, parts[0])){
+        return 0;
+    }
+
+    /* this module is still required */
+    if (parts[2][0]!='0'){


Easier to read and more consistent with other code in the project:

    if (parts[2][0] != '0') {


+	return 0;


Another tab for indentation, please convert to spaces.


+    }
+
+    /* rmmod parts[0] */
+    pid = fork();
+    if (pid == 0) {
+        execl("/sbin/rmmod", "-f", parts[0], NULL);
+        _exit(1);
+    }
+    else if (pid < 0) {
+        logMessage(ERROR, "Module %s removal FAILED", parts[0]);
+        return 0;
+    }
+
+    waitpid(pid, &status, 0);
+    if (WEXITSTATUS(status)) {
+      logMessage(DEBUGLVL, "Module %s was NOT removed", parts[0]);
+      return 0;
+    }
+    else{
+      logMessage(DEBUGLVL, "Module %s was removed", parts[0]);
+      return 1;
+    }


Inconsistent block indentation with the rest of the code.  Should be 4 spaces.


+
+    return 0;
+}
+
+void mlRestoreModuleState(GTree *state)
+{
+    if(!state) return;


See if block comment above.


+
+    logMessage(INFO, "Restoring module state...");
+    while(processModuleLines(state, cb_restorestate)>0);


Could use some spaces in this line to improve readability:

    while (processModuleLines(state, cb_restorestate) > 0);


+}
+
+void mlFreeModuleState(GTree *state)
+{
+    if(!state) return;


See if block comment above.


+
+    g_tree_destroy(state);
+}
diff --git a/loader/modules.h b/loader/modules.h
index 88fa25f..605d01e 100644
--- a/loader/modules.h
+++ b/loader/modules.h
@@ -40,4 +40,8 @@ gboolean mlAddBlacklist(gchar *);
gboolean mlRemoveBlacklist(gchar *);
void loadKickstartModule(struct loaderData_s *, int, char **);

+GTree* mlSaveModuleState();
+void mlRestoreModuleState(GTree *state);
+void mlFreeModuleState(GTree *state);
+
#endif
diff --git a/scripts/upd-instroot b/scripts/upd-instroot
index c97326a..23cac11 100755
--- a/scripts/upd-instroot
+++ b/scripts/upd-instroot
@@ -477,6 +477,7 @@ sbin/reiserfsck
sbin/reiserfstune
sbin/resize_reiserfs
sbin/resize2fs
+sbin/rmmod
sbin/route
sbin/setfiles
sbin/sfdisk
@@ -826,7 +827,6 @@ sbin/depmod
sbin/netstat
sbin/restore
sbin/rrestore
-sbin/rmmod
sbin/route
sbin/mount.cifs
sbin/umount.cifs
- -- David Cantrell <dcantrell@xxxxxxxxxx> 
Red Hat / Honolulu, HI

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iEYEARECAAYFAkvi3xwACgkQ5hsjjIy1VkkMGgCglvlvaGw6zb6/+Ur1xN8nm/w7
cYoAniMddvUYKHOyV6u5vriIJKuDpKOK
=NFMD
-----END PGP SIGNATURE-----

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
[Index of Archives]     [Kickstart]     [Fedora Users]     [Fedora Legacy List]     [Fedora Maintainers]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [Yosemite Photos]     [KDE Users]     [Fedora Tools]
  Powered by Linux