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
