Thanks to both, I will fix the coding style. The assert was really ment as assert, the value should never be NULL. And when it is we are screwed anyways. I'm well aware of -NDEBUG, this is still experimental code.. The split should use only spaces to split data, I read only one line from /proc/modules and unless kernel API changes, it has always 5 fields with one space between them. I need first three. Martin -- Martin Sivák msivak@xxxxxxxxxx Red Hat Czech Anaconda team / Brno, CZ ----- "David Cantrell" <dcantrell@xxxxxxxxxx> wrote: > -----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 _______________________________________________ Anaconda-devel-list mailing list Anaconda-devel-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/anaconda-devel-list