FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Fedora Games

 
 
LinkBack Thread Tools
 
Old 12-02-2009, 03:27 PM
Martin Sivak
 
Default Backport the driver disc functionality from rhel5

- updates the code to agree with current anaconda API
- adds dlabel functionality
- also adds some stub calls to set modprobe behaviour
---
loader/driverdisk.c | 333 +++++++++++++++++++++++++++++++++++++++++++++------
loader/driverdisk.h | 20 +++
loader/loader.c | 37 ++++++
loader/loader.h | 2 +
4 files changed, 355 insertions(+), 37 deletions(-)

diff --git a/loader/driverdisk.c b/loader/driverdisk.c
index 5f8d43c..f718684 100644
--- a/loader/driverdisk.c
+++ b/loader/driverdisk.c
@@ -30,6 +30,12 @@
#include <unistd.h>
#include <glib.h>

+#include <blkid/blkid.h>
+
+#include <glob.h>
+#include <rpm/rpmlib.h>
+#include <sys/utsname.h>
+
#include "copy.h"
#include "loader.h"
#include "log.h"
@@ -48,6 +54,8 @@
#include "nfsinstall.h"
#include "urlinstall.h"

+#include "rpmextract.h"
+
#include "../isys/isys.h"
#include "../isys/imount.h"
#include "../isys/eddsupport.h"
@@ -55,37 +63,175 @@
/* boot flags */
extern uint64_t flags;

-static char * driverDiskFiles[] = { "modinfo", "modules.dep",
- "modules.cgz", "modules.alias", NULL };
+/* modprobe DD mode */
+int modprobeDDmode()
+{
+ FILE *f = fopen("/etc/depmod.d/ddmode.conf", "w");
+ if(f){
+ struct utsname unamedata;
+ if(uname(&unamedata)){
+ fprintf(f, " pblacklist /lib/modules
");
+ }
+ else{
+ fprintf(f, " pblacklist /lib/modules/%s
", unamedata.release);
+ }
+
+ fclose(f);
+ }
+
+ return f==NULL;
+}
+
+int modprobeNormalmode()
+{
+ unlink("/etc/depmod.d/ddmode.conf");
+
+ /* run depmod to refresh modules db */
+ if(system("depmod -a")){
+ /* FIXME: depmod didn't run */
+ }
+
+ return 0;
+}
+
+/* RPM extraction dependency checks */
+int dlabelDeps(const char* depends, void *userptr)
+{
+ logMessage(DEBUGLVL, "Depends on: %s
", depends);
+ return 0;
+}
+
+int dlabelProvides(const char* dep, void *userptr)
+{
+ char *kernelver = (char*)userptr;
+
+ logMessage(DEBUGLVL, "Provides: %s
", dep);
+
+ return strcmp(dep, kernelver);
+}
+
+int dlabelFilter(const char* name, struct stat *fstat, void *userptr)
+{
+ int l = strlen(name);
+
+ logMessage(DEBUGLVL, "Unpacking %s (%lluB)
", name, fstat->st_size);
+
+ /* we want firmware files */
+ if(!strncmp("lib/firmware/", name, 13)) return 0;
+
+ if(l<3) return 1;
+ l-=3;
+
+ /* and we want only .ko files */
+ if(strncmp(".ko", name+l, 3)) return 1;
+
+ /* TODO we are unpacking kernel module, read it's description */
+
+ return 0;
+}
+
+char* moduleDescription(const char* modulePath)
+{
+ char *command = rstrscat(NULL, "modinfo --description '", modulePath, "'", NULL);
+ FILE *f = popen(command, "r");
+ if(f==NULL) return NULL;
+
+ char *description = malloc(sizeof(char)*256);
+ int size = fread(description, 1, 255, f);
+ if(size==0){
+ free(description);
+ return NULL;
+ }
+ description[size-1]=0; /* strip the trailing newline */
+ pclose(f);
+
+ free(command);
+ return description;
+}
+
+int globErrFunc(const char *epath, int eerrno)
+{
+ return 0;
+}
+
+int dlabelUnpackRPMDir(char* rpmdir, char* destination)
+{
+ char *kernelver;
+ struct utsname unamedata;
+ char *oldcwd;
+ char *globpattern;
+ int rc;
+
+ /* get current working directory */
+ oldcwd = getcwd(NULL, 0);
+
+ /* set the cwd to destination */
+ if(chdir(destination)){
+ free(oldcwd);
+ return 1;
+ }
+
+ /* get running kernel version */
+ if(uname(&unamedata)){
+ kernelver = rstrscat(NULL, "kernel-modules-", "unknown", NULL);
+ }
+ else{
+ kernelver = rstrscat(NULL, "kernel-modules-", unamedata.release, NULL);
+ }
+ logMessage(DEBUGLVL, "Kernel version: %s
", kernelver);
+
+ globpattern = rstrscat(NULL, rpmdir, "/*.rpm", NULL);
+ glob_t globres;
+ char** globitem;
+ if(!glob(globpattern, GLOB_NOSORT|GLOB_NOESCAPE, globErrFunc, &globres)){
+ /* iterate over all rpm files */
+ globitem = globres.gl_pathv;
+ while(globres.gl_pathc>0 && globitem!=NULL){
+ explodeRPM(*globitem, dlabelFilter, dlabelProvides, dlabelDeps, kernelver);
+ }
+ globfree(&globres);
+ /* end of iteration */
+ }
+ free(globpattern);
+
+ /* restore CWD */
+ if(chdir(oldcwd)){
+ /* FIXME: too bad.. */
+ }
+
+ /* cleanup */
+ free(kernelver);
+ free(oldcwd);
+ return rc;
+}
+
+
+static char * driverDiskFiles[] = { "repodata", NULL };

static int verifyDriverDisk(char *mntpt) {
char ** fnPtr;
char file[200];
struct stat sb;

- for (fnPtr = driverDiskFiles; *fnPtr; fnPtr++) {
- sprintf(file, "%s/%s", mntpt, *fnPtr);
- if (access(file, R_OK)) {
- logMessage(ERROR, "cannot find %s, bad driver disk", file);
- return LOADER_BACK;
- }
- }
-
- /* check for both versions */
+ /* check for dd descriptor */
sprintf(file, "%s/rhdd", mntpt);
if (access(file, R_OK)) {
- logMessage(DEBUGLVL, "not a new format driver disk, checking for old");
- sprintf(file, "%s/rhdd-6.1", mntpt);
- if (access(file, R_OK)) {
- logMessage(ERROR, "can't find either driver disk identifier, bad "
- "driver disk");
- }
+ logMessage(ERROR, "can't find driver disk identifier, bad "
+ "driver disk");
+ return LOADER_BACK;
}

/* side effect: file is still mntpt/ddident */
stat(file, &sb);
if (!sb.st_size)
return LOADER_BACK;
+ for (fnPtr = driverDiskFiles; *fnPtr; fnPtr++) {
+ sprintf(file, "%s/%s/%s", mntpt, getProductArch(), *fnPtr);
+ if (access(file, R_OK)) {
+ logMessage(ERROR, "cannot find %s, bad driver disk", file);
+ return LOADER_BACK;
+ }
+ }

return LOADER_OK;
}
@@ -101,25 +247,20 @@ static void copyErrorFn (char *msg) {
/* this copies the contents of the driver disk to a ramdisk and loads
* the moduleinfo, etc. assumes a "valid" driver disk mounted at mntpt */
static int loadDriverDisk(struct loaderData_s *loaderData, char *mntpt) {
- moduleInfoSet modInfo = loaderData->modInfo;
- char file[200], dest[200];
+ /* FIXME moduleInfoSet modInfo = loaderData->modInfo; */
+ char file[200], dest[200], src[200];
char *title;
char *fwdir = NULL;
struct moduleBallLocation * location;
struct stat sb;
static int disknum = 0;
- int version = 1;
int fd, ret;

- /* check for both versions */
- sprintf(file, "%s/rhdd", mntpt);
+ /* check for new version */
+ sprintf(file, "%s/rhdd3", mntpt);
if (access(file, R_OK)) {
- version = 0;
- sprintf(file, "%s/rhdd-6.1", mntpt);
- if (access(file, R_OK)) {
- /* this can't happen, we already verified it! */
- return LOADER_BACK;
- }
+ /* this can't happen, we already verified it! */
+ return LOADER_BACK;
}
stat(file, &sb);
title = malloc(sb.st_size + 1);
@@ -131,23 +272,35 @@ static int loadDriverDisk(struct loaderData_s *loaderData, char *mntpt) {
title[sb.st_size] = '';
close(fd);

- sprintf(file, "/tmp/DD-%d", disknum);
+ sprintf(file, DD_RPMS, disknum);
mkdirChain(file);
+ mkdirChain(DD_MODULES);
+ mkdirChain(DD_FIRMWARE);

if (!FL_CMDLINE(flags)) {
startNewt();
winStatus(40, 3, _("Loading"), _("Reading driver disk"));
}

- sprintf(dest, "/tmp/DD-%d", disknum);
- copyDirectory(mntpt, dest, copyWarnFn, copyErrorFn);
-
location = malloc(sizeof(struct moduleBallLocation));
location->title = strdup(title);
- location->version = version;

- checked_asprintf(&location->path, "/tmp/DD-%d/modules.cgz", disknum);
- checked_asprintf(&fwdir, "/tmp/DD-%d/firmware", disknum);
+ checked_asprintf(&location->path, DD_MODULES);
+ checked_asprintf(&fwdir, DD_FIRMWARE);
+
+ sprintf(dest, DD_RPMS, disknum);
+ sprintf(src, "%s/rpms/%s", mntpt, getProductArch());
+ copyDirectory(src, dest, copyWarnFn, copyErrorFn);
+
+ /* unpack packages from dest into location->path */
+ if(dlabelUnpackRPMDir(dest, DD_EXTRACTED)){
+ /* TODO error handler */
+ }
+
+ /* run depmod to refresh modules db */
+ if(system("depmod -a")){
+ /* FIXME: depmod didn't run */
+ }

if (!access(fwdir, R_OK|X_OK)) {
add_fw_search_dir(loaderData, fwdir);
@@ -156,8 +309,11 @@ static int loadDriverDisk(struct loaderData_s *loaderData, char *mntpt) {
}
free(fwdir);

- sprintf(file, "%s/modinfo", mntpt);
- readModuleInfo(file, modInfo, location, 1);
+ /* TODO generate and read module info
+ *
+ * sprintf(file, "%s/modinfo", mntpt);
+ * readModuleInfo(file, modInfo, location, 1);
+ */

if (!FL_CMDLINE(flags))
newtPopWindow();
@@ -623,3 +779,106 @@ static void getDDFromDev(struct loaderData_s * loaderData, char * dev) {
umount("/tmp/drivers");
unlink("/tmp/drivers");
}
+
+/*
+ * Utility functions to maintain linked-list of device names
+ * */
+
+struct ddlist* ddlist_add(struct ddlist *list, const char* device)
+{
+ struct ddlist* item;
+
+ item = (struct ddlist*)malloc(sizeof(struct ddlist));
+ if(item==NULL){
+ return list;
+ }
+
+ item->device = strdup(device);
+ item->next = list;
+
+ return item;
+}
+
+int ddlist_free(struct ddlist *list)
+{
+ struct ddlist *next;
+ int count = 0;
+
+ while(list!=NULL){
+ next = list->next;
+ free(list->device);
+ free(list);
+ list = next;
+ count++;
+ }
+
+ return count;
+}
+
+
+/*
+ * Look for partition with specific label (part of #316481)
+ */
+struct ddlist* findDriverDiskByLabel(void)
+{
+ char *ddLabel = "OEMDRV";
+ struct ddlist *ddDevice = NULL;
+ blkid_cache bCache;
+
+ int res;
+ blkid_dev_iterate bIter;
+ blkid_dev bDev;
+
+ if(blkid_get_cache(&bCache, NULL)<0){
+ logMessage(ERROR, _("Cannot initialize cache instance for blkid"));
+ return NULL;
+ }
+ if((res = blkid_probe_all(bCache))<0){
+ logMessage(ERROR, _("Cannot probe devices in blkid: %d"), res);
+ return NULL;
+ }
+
+ bIter = blkid_dev_iterate_begin(bCache);
+ blkid_dev_set_search(bIter, "LABEL", ddLabel);
+ while((res = blkid_dev_next(bIter, &bDev))!=0){
+ bDev = blkid_verify(bCache, bDev);
+ if(!bDev)
+ continue;
+ logMessage(DEBUGLVL, _("Adding driver disc %s to the list of available DDs."), blkid_dev_devname(bDev));
+ ddDevice = ddlist_add(ddDevice, blkid_dev_devname(bDev));
+ /*blkid_free_dev(bDev); -- probably taken care of by the put cache call.. it is not exposed in the API */
+ }
+ blkid_dev_iterate_end(bIter);
+
+ blkid_put_cache(bCache);
+
+ return ddDevice;
+}
+
+int loadDriverDiskFromPartition(struct loaderData_s *loaderData, char* device)
+{
+ int rc;
+
+ logMessage(INFO, "trying to mount %s", device);
+ if (doPwMount(device, "/tmp/drivers", "auto", "ro", NULL)) {
+ logMessage(ERROR, _("Failed to mount driver disk."));
+ return -1;
+ }
+
+ rc = verifyDriverDisk("/tmp/drivers");
+ if (rc == LOADER_BACK) {
+ logMessage(ERROR, _("Driver disk is invalid for this "
+ "release of %s."), getProductName());
+ umount("/tmp/drivers");
+ return -2;
+ }
+
+ rc = loadDriverDisk(loaderData, "/tmp/drivers");
+ umount("/tmp/drivers");
+ if (rc == LOADER_BACK) {
+ return -3;
+ }
+
+ return 0;
+}
+
diff --git a/loader/driverdisk.h b/loader/driverdisk.h
index e6e919d..3e442fb 100644
--- a/loader/driverdisk.h
+++ b/loader/driverdisk.h
@@ -24,6 +24,11 @@
#include "modules.h"
#include "moduleinfo.h"

+#define DD_RPMS "/tmp/DD-%d"
+#define DD_EXTRACTED "/tmp/DD"
+#define DD_MODULES "/tmp/DD/lib/modules"
+#define DD_FIRMWARE "/tmp/DD/lib/firmware"
+
extern char *ddFsTypes[];

int loadDriverFromMedia(int class, struct loaderData_s *loaderData,
@@ -39,4 +44,19 @@ void useKickstartDD(struct loaderData_s * loaderData, int argc,

void getDDFromSource(struct loaderData_s * loaderData, char * src);

+int loadDriverDiskFromPartition(struct loaderData_s *loaderData, char* device);
+
+struct ddlist {
+ char* device;
+ struct ddlist* next;
+};
+
+struct ddlist* ddlist_add(struct ddlist *list, const char* device);
+int ddlist_free(struct ddlist *list);
+
+struct ddlist* findDriverDiskByLabel(void);
+
+int modprobeNormalmode();
+int modprobeDDmode();
+
#endif
diff --git a/loader/loader.c b/loader/loader.c
index cd3e178..3159513 100644
--- a/loader/loader.c
+++ b/loader/loader.c
@@ -959,6 +959,10 @@ static void parseCmdLineFlags(struct loaderData_s * loaderData,
else if (!strcasecmp(argv[i], "dd") ||
!strcasecmp(argv[i], "driverdisk"))
flags |= LOADER_FLAGS_MODDISK;
+ else if (!strcasecmp(argv[i], "dlabel=on"))
+ flags |= LOADER_FLAGS_AUTOMODDISK;
+ else if (!strcasecmp(argv[i], "dlabel=off"))
+ flags &= ~LOADER_FLAGS_AUTOMODDISK;
else if (!strcasecmp(argv[i], "rescue"))
flags |= LOADER_FLAGS_RESCUE;
else if (!strcasecmp(argv[i], "nopass"))
@@ -1785,6 +1789,7 @@ int main(int argc, char ** argv) {
struct loaderData_s loaderData;

char *path;
+ struct ddlist *dd, *dditer;

gchar *cmdLine = NULL, *ksFile = NULL, *virtpcon = NULL;
gboolean testing = FALSE, mediacheck = FALSE;
@@ -1882,6 +1887,12 @@ int main(int argc, char ** argv) {
flags |= LOADER_FLAGS_NOSHELL;
#endif

+ /* XXX if RHEL, enable the AUTODD feature by default,
+ * but we should come with more general way how to control this */
+ if(!strncmp(getProductName(), "Red Hat", 7)){
+ flags |= LOADER_FLAGS_AUTOMODDISK;
+ }
+
openLog(FL_TESTING(flags));
if (!FL_TESTING(flags))
openlog("loader", 0, LOG_LOCAL0);
@@ -1946,6 +1957,26 @@ 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(FL_AUTOMODDISK(flags)){
+ /* Load all autodetected DDs */
+ logMessage(INFO, "Trying to detect vendor driver discs");
+ dd = findDriverDiskByLabel();
+ dditer = dd;
+ while(dditer){
+ if(loadDriverDiskFromPartition(&loaderData, dditer->device)){
+ logMessage(ERROR, "Automatic driver disk loader failed for %s.", dditer->device);
+ }
+ else{
+ logMessage(INFO, "Automatic driver disk loader succeeded for %s.", dditer->device);
+ }
+ dditer = dditer->next;
+ }
+ ddlist_free(dd);
+ }
+
if (FL_MODDISK(flags)) {
startNewt();
loadDriverDisks(DEVICE_ANY, &loaderData);
@@ -1955,6 +1986,9 @@ int main(int argc, char ** argv) {
logMessage(INFO, "found /dd.img, loading drivers");
getDDFromSource(&loaderData, "path:/dd.img");
}
+
+ /* 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
@@ -2139,6 +2173,9 @@ int main(int argc, char ** argv) {
tmparg++;
}

+ if (FL_AUTOMODDISK(flags))
+ *argptr++ = "--dlabel";
+
if (FL_NOIPV4(flags))
*argptr++ = "--noipv4";

diff --git a/loader/loader.h b/loader/loader.h
index ebf3766..ca6404f 100644
--- a/loader/loader.h
+++ b/loader/loader.h
@@ -70,6 +70,7 @@
#define LOADER_FLAGS_HAVE_CMSCONF (((uint64_t) 1) << 37)
#define LOADER_FLAGS_NOKILL (((uint64_t) 1) << 38)
#define LOADER_FLAGS_KICKSTART_SEND_SERIAL (((uint64_t) 1) << 39)
+#define LOADER_FLAGS_AUTOMODDISK (((uint64_t) 1) << 40)

#define FL_TESTING(a) ((a) & LOADER_FLAGS_TESTING)
#define FL_TEXT(a) ((a) & LOADER_FLAGS_TEXT)
@@ -107,6 +108,7 @@
#define FL_HAVE_CMSCONF(a) ((a) & LOADER_FLAGS_HAVE_CMSCONF)
#define FL_NOKILL(a) ((a) & LOADER_FLAGS_NOKILL)
#define FL_KICKSTART_SEND_SERIAL(a) ((a) & LOADER_FLAGS_KICKSTART_SEND_SERIAL)
+#define FL_AUTOMODDISK(a) ((a) & LOADER_FLAGS_AUTOMODDISK)

void startNewt(void);
void stopNewt(void);
--
1.6.4.4

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 12-02-2009, 06:16 PM
Peter Jones
 
Default Backport the driver disc functionality from rhel5

"git config --global sendemail.chainreplyto no" might be a good thing to do

On 12/02/2009 11:27 AM, Martin Sivak wrote:

> +/* modprobe DD mode */
> +int modprobeDDmode()
> +{
> + FILE *f = fopen("/etc/depmod.d/ddmode.conf", "w");
> + if(f){
> + struct utsname unamedata;
> + if(uname(&unamedata)){
> + fprintf(f, " pblacklist /lib/modules
");
> + }
> + else{
> + fprintf(f, " pblacklist /lib/modules/%s
", unamedata.release);
> + }
> +
> + fclose(f);
> + }

What's with the funky coding style? It doesn't match the rest of the file.

> +
> + return f==NULL;
> +}
> +
> +int modprobeNormalmode()
> +{
> + unlink("/etc/depmod.d/ddmode.conf");
> +
> + /* run depmod to refresh modules db */
> + if(system("depmod -a")){
> + /* FIXME: depmod didn't run */
> + }

Seems like there's nothing you can really do here except log it.

> +
> + return 0;
> +}

What's the point of this not being a void function?

> +
> +/* RPM extraction dependency checks */
> +int dlabelDeps(const char* depends, void *userptr)
> +{
> + logMessage(DEBUGLVL, "Depends on: %s
", depends);
> + return 0;
> +}

What's the point of this at all?

> +
> +int dlabelProvides(const char* dep, void *userptr)
> +{
> + char *kernelver = (char*)userptr;
> +
> + logMessage(DEBUGLVL, "Provides: %s
", dep);
> +
> + return strcmp(dep, kernelver);
> +}

A better function name and some comments would be nice here. Upon reading it,
I'm not at all sure what it's supposed to do.

> +
> +int dlabelFilter(const char* name, struct stat *fstat, void *userptr)
> +{
> + int l = strlen(name);
> +
> + logMessage(DEBUGLVL, "Unpacking %s (%lluB)
", name, fstat->st_size);
> +
> + /* we want firmware files */
> + if(!strncmp("lib/firmware/", name, 13)) return 0;
> +
> + if(l<3) return 1;
> + l-=3;
> +
> + /* and we want only .ko files */
> + if(strncmp(".ko", name+l, 3)) return 1;

Better to use strcmp() here.

> +
> + /* TODO we are unpacking kernel module, read it's description */
> +
> + return 0;
> +}


> +
> +char* moduleDescription(const char* modulePath)
> +{
> + char *command = rstrscat(NULL, "modinfo --description '", modulePath, "'", NULL);
> + FILE *f = popen(command, "r");
> + if(f==NULL) return NULL;
> +
> + char *description = malloc(sizeof(char)*256);
> + int size = fread(description, 1, 255, f);

Couple of things here:
1) All the code/decleration mixing here isn't very easy to read.
2) allocations need to be checked
3) There's a pretty good case for using checked_asprintf() instead of using
*anything* provided by rpmlib. Or implement a stack-allocating sprintfa(),
which would be even better since it'll cut down on free()s in the return
paths. You can borrow this code from nash in mkinitrd (git is at
ssh://git.fedoraproject.org/git/hosted/mkinitrd) if you like. I think I
called it asprintfa() there, but whatever.

> + if(size==0){
> + free(description);
> + return NULL;
> + }
> + description[size-1]=0; /* strip the trailing newline */

I'm pretty sure this should be using strrchr() to find the newline rather than
expecting all description strings to be exactly 254 bytes plus a newline. (a
read/realloc loop also isn't out of the question...)

> + pclose(f);
> +
> + free(command);

Why not free this immediately after the popen?

> + return description;
> +}
> +
> +int globErrFunc(const char *epath, int eerrno)
> +{
> + return 0;
> +}

Seems like the wrong thing to do - you're effectively telling glob() to ignore
all errors and try to continue processing, but you're not doing anything about
the errors while doing so. I think "return immediately" (i.e. NULL or a
globErrFunc() that returns nonzero) makes more sense if we're not doing any
actual handling of the errors.

> +int dlabelUnpackRPMDir(char* rpmdir, char* destination)
> +{
> + char *kernelver;
> + struct utsname unamedata;
> + char *oldcwd;
> + char *globpattern;
> + int rc;
> +
> + /* get current working directory */
> + oldcwd = getcwd(NULL, 0);
> +
> + /* set the cwd to destination */
> + if(chdir(destination)){
> + free(oldcwd);
> + return 1;
> + }
> +
> + /* get running kernel version */
> + if(uname(&unamedata)){
> + kernelver = rstrscat(NULL, "kernel-modules-", "unknown", NULL);
> + }
> + else{
> + kernelver = rstrscat(NULL, "kernel-modules-", unamedata.release, NULL);
> + }

rc = uname(&unamedata);
checked_asprintf(&kernelver, "kernel-modules-%s", rc ? "unknown" : unamedata.release);

> + logMessage(DEBUGLVL, "Kernel version: %s
", kernelver);
> +
> + globpattern = rstrscat(NULL, rpmdir, "/*.rpm", NULL);
> + glob_t globres;
> + char** globitem;
> + if(!glob(globpattern, GLOB_NOSORT|GLOB_NOESCAPE, globErrFunc, &globres)){
> + /* iterate over all rpm files */
> + globitem = globres.gl_pathv;
> + while(globres.gl_pathc>0 && globitem!=NULL){
> + explodeRPM(*globitem, dlabelFilter, dlabelProvides, dlabelDeps, kernelver);

Where does explodeRPM come from? Ah, it's in a later patch. I usually find it
better to organize a patchset like this as:

1) description of the overall goal
2) cleanup and removal of old code
3) addition of helper functions
4) addition of the new stuff that uses the helpers.

> + }
> + globfree(&globres);
> + /* end of iteration */
> + }
> + free(globpattern);
> +
> + /* restore CWD */
> + if(chdir(oldcwd)){
> + /* FIXME: too bad.. */
> + }
> +
> + /* cleanup */
> + free(kernelver);
> + free(oldcwd);
> + return rc;
> +}
> +
> +
> +static char * driverDiskFiles[] = { "repodata", NULL };
>
> static int verifyDriverDisk(char *mntpt) {
> char ** fnPtr;
> char file[200];
> struct stat sb;
>
> - for (fnPtr = driverDiskFiles; *fnPtr; fnPtr++) {
> - sprintf(file, "%s/%s", mntpt, *fnPtr);
> - if (access(file, R_OK)) {
> - logMessage(ERROR, "cannot find %s, bad driver disk", file);
> - return LOADER_BACK;
> - }
> - }
> -
> - /* check for both versions */
> + /* check for dd descriptor */
> sprintf(file, "%s/rhdd", mntpt);
> if (access(file, R_OK)) {
> - logMessage(DEBUGLVL, "not a new format driver disk, checking for old");
> - sprintf(file, "%s/rhdd-6.1", mntpt);
> - if (access(file, R_OK)) {
> - logMessage(ERROR, "can't find either driver disk identifier, bad "
> - "driver disk");
> - }
> + logMessage(ERROR, "can't find driver disk identifier, bad "
> + "driver disk");
> + return LOADER_BACK;
> }
>
> /* side effect: file is still mntpt/ddident */
> stat(file, &sb);
> if (!sb.st_size)
> return LOADER_BACK;
> + for (fnPtr = driverDiskFiles; *fnPtr; fnPtr++) {
> + sprintf(file, "%s/%s/%s", mntpt, getProductArch(), *fnPtr);

I realize mntpt and getProductArch's return are unlikely to be very large, but
these should still probably be snprintf() or checked_asprintf() (... or sprintfa()
if it were to suddenly come into existence

> + if (access(file, R_OK)) {
> + logMessage(ERROR, "cannot find %s, bad driver disk", file);
> + return LOADER_BACK;
> + }
> + }
>
> return LOADER_OK;
> }
> @@ -101,25 +247,20 @@ static void copyErrorFn (char *msg) {
> /* this copies the contents of the driver disk to a ramdisk and loads
> * the moduleinfo, etc. assumes a "valid" driver disk mounted at mntpt */
> static int loadDriverDisk(struct loaderData_s *loaderData, char *mntpt) {
> - moduleInfoSet modInfo = loaderData->modInfo;
> - char file[200], dest[200];
> + /* FIXME moduleInfoSet modInfo = loaderData->modInfo; */
> + char file[200], dest[200], src[200];
> char *title;
> char *fwdir = NULL;
> struct moduleBallLocation * location;
> struct stat sb;
> static int disknum = 0;
> - int version = 1;
> int fd, ret;
>
> - /* check for both versions */
> - sprintf(file, "%s/rhdd", mntpt);
> + /* check for new version */
> + sprintf(file, "%s/rhdd3", mntpt);
> if (access(file, R_OK)) {
> - version = 0;
> - sprintf(file, "%s/rhdd-6.1", mntpt);
> - if (access(file, R_OK)) {
> - /* this can't happen, we already verified it! */
> - return LOADER_BACK;
> - }
> + /* this can't happen, we already verified it! */
> + return LOADER_BACK;
> }
> stat(file, &sb);
> title = malloc(sb.st_size + 1);
> @@ -131,23 +272,35 @@ static int loadDriverDisk(struct loaderData_s *loaderData, char *mntpt) {
> title[sb.st_size] = '';
> close(fd);
>
> - sprintf(file, "/tmp/DD-%d", disknum);
> + sprintf(file, DD_RPMS, disknum);

I'm really not a big fan of hiding the format specifier behind a macro, but
I do see the case for re-use here. Given that, it probably ought to be
defined somewhere very near to this code, not in a separate header file.

Also maybe name it something like DD_RPMDIR_TEMPLATE ?

> mkdirChain(file);
> + mkdirChain(DD_MODULES);
> + mkdirChain(DD_FIRMWARE);
>
> if (!FL_CMDLINE(flags)) {
> startNewt();
> winStatus(40, 3, _("Loading"), _("Reading driver disk"));
> }
>
> - sprintf(dest, "/tmp/DD-%d", disknum);
> - copyDirectory(mntpt, dest, copyWarnFn, copyErrorFn);
> -
> location = malloc(sizeof(struct moduleBallLocation));
> location->title = strdup(title);
> - location->version = version;
>
> - checked_asprintf(&location->path, "/tmp/DD-%d/modules.cgz", disknum);
> - checked_asprintf(&fwdir, "/tmp/DD-%d/firmware", disknum);
> + checked_asprintf(&location->path, DD_MODULES);
> + checked_asprintf(&fwdir, DD_FIRMWARE);
> +
> + sprintf(dest, DD_RPMS, disknum);
> + sprintf(src, "%s/rpms/%s", mntpt, getProductArch());
> + copyDirectory(src, dest, copyWarnFn, copyErrorFn);
> +
> + /* unpack packages from dest into location->path */
> + if(dlabelUnpackRPMDir(dest, DD_EXTRACTED)){
> + /* TODO error handler */
> + }

There's no way to recover, really - log and don't use this DD?

> +
> + /* run depmod to refresh modules db */
> + if(system("depmod -a")){
> + /* FIXME: depmod didn't run */

Again, there's no graceful way out here - log it and abort using this driver disk?

> + }
>
> if (!access(fwdir, R_OK|X_OK)) {
> add_fw_search_dir(loaderData, fwdir);
> @@ -156,8 +309,11 @@ static int loadDriverDisk(struct loaderData_s *loaderData, char *mntpt) {
> }
> free(fwdir);
>
> - sprintf(file, "%s/modinfo", mntpt);
> - readModuleInfo(file, modInfo, location, 1);
> + /* TODO generate and read module info
> + *
> + * sprintf(file, "%s/modinfo", mntpt);
> + * readModuleInfo(file, modInfo, location, 1);
> + */
>
> if (!FL_CMDLINE(flags))
> newtPopWindow();
> @@ -623,3 +779,106 @@ static void getDDFromDev(struct loaderData_s * loaderData, char * dev) {
> umount("/tmp/drivers");
> unlink("/tmp/drivers");
> }
> +
> +/*
> + * Utility functions to maintain linked-list of device names
> + * */
> +
> +struct ddlist* ddlist_add(struct ddlist *list, const char* device)
> +{
> + struct ddlist* item;
> +
> + item = (struct ddlist*)malloc(sizeof(struct ddlist));
> + if(item==NULL){
> + return list;
> + }
> +
> + item->device = strdup(device);
> + item->next = list;
> +
> + return item;
> +}
> +
> +int ddlist_free(struct ddlist *list)
> +{
> + struct ddlist *next;
> + int count = 0;
> +
> + while(list!=NULL){
> + next = list->next;
> + free(list->device);
> + free(list);
> + list = next;
> + count++;
> + }
> +
> + return count;
> +}

Seems like using glib's linked lists is better than adding our own.

> +
> +
> +/*
> + * Look for partition with specific label (part of #316481)
> + */
> +struct ddlist* findDriverDiskByLabel(void)
> +{
> + char *ddLabel = "OEMDRV";
> + struct ddlist *ddDevice = NULL;
> + blkid_cache bCache;
> +
> + int res;
> + blkid_dev_iterate bIter;
> + blkid_dev bDev;
> +
> + if(blkid_get_cache(&bCache, NULL)<0){
> + logMessage(ERROR, _("Cannot initialize cache instance for blkid"));
> + return NULL;
> + }
> + if((res = blkid_probe_all(bCache))<0){
> + logMessage(ERROR, _("Cannot probe devices in blkid: %d"), res);
> + return NULL;
> + }
> +
> + bIter = blkid_dev_iterate_begin(bCache);
> + blkid_dev_set_search(bIter, "LABEL", ddLabel);
> + while((res = blkid_dev_next(bIter, &bDev))!=0){
> + bDev = blkid_verify(bCache, bDev);
> + if(!bDev)
> + continue;
> + logMessage(DEBUGLVL, _("Adding driver disc %s to the list of available DDs."), blkid_dev_devname(bDev));
> + ddDevice = ddlist_add(ddDevice, blkid_dev_devname(bDev));
> + /*blkid_free_dev(bDev); -- probably taken care of by the put cache call.. it is not exposed in the API */
> + }
> + blkid_dev_iterate_end(bIter);
> +
> + blkid_put_cache(bCache);
> +
> + return ddDevice;
> +}
> +
> +int loadDriverDiskFromPartition(struct loaderData_s *loaderData, char* device)
> +{
> + int rc;
> +
> + logMessage(INFO, "trying to mount %s", device);
> + if (doPwMount(device, "/tmp/drivers", "auto", "ro", NULL)) {
> + logMessage(ERROR, _("Failed to mount driver disk."));
> + return -1;
> + }
> +
> + rc = verifyDriverDisk("/tmp/drivers");
> + if (rc == LOADER_BACK) {
> + logMessage(ERROR, _("Driver disk is invalid for this "
> + "release of %s."), getProductName());
> + umount("/tmp/drivers");
> + return -2;
> + }
> +
> + rc = loadDriverDisk(loaderData, "/tmp/drivers");
> + umount("/tmp/drivers");
> + if (rc == LOADER_BACK) {
> + return -3;
> + }
> +
> + return 0;
> +}
> +
> diff --git a/loader/driverdisk.h b/loader/driverdisk.h
> index e6e919d..3e442fb 100644
> --- a/loader/driverdisk.h
> +++ b/loader/driverdisk.h
> @@ -24,6 +24,11 @@
> #include "modules.h"
> #include "moduleinfo.h"
>
> +#define DD_RPMS "/tmp/DD-%d"
> +#define DD_EXTRACTED "/tmp/DD"
> +#define DD_MODULES "/tmp/DD/lib/modules"
> +#define DD_FIRMWARE "/tmp/DD/lib/firmware"
> +
> extern char *ddFsTypes[];
>
> int loadDriverFromMedia(int class, struct loaderData_s *loaderData,
> @@ -39,4 +44,19 @@ void useKickstartDD(struct loaderData_s * loaderData, int argc,
>
> void getDDFromSource(struct loaderData_s * loaderData, char * src);
>
> +int loadDriverDiskFromPartition(struct loaderData_s *loaderData, char* device);
> +
> +struct ddlist {
> + char* device;
> + struct ddlist* next;
> +};
> +
> +struct ddlist* ddlist_add(struct ddlist *list, const char* device);
> +int ddlist_free(struct ddlist *list);
> +
> +struct ddlist* findDriverDiskByLabel(void);
> +
> +int modprobeNormalmode();
> +int modprobeDDmode();
> +
> #endif
> diff --git a/loader/loader.c b/loader/loader.c
> index cd3e178..3159513 100644
> --- a/loader/loader.c
> +++ b/loader/loader.c
> @@ -959,6 +959,10 @@ static void parseCmdLineFlags(struct loaderData_s * loaderData,
> else if (!strcasecmp(argv[i], "dd") ||
> !strcasecmp(argv[i], "driverdisk"))
> flags |= LOADER_FLAGS_MODDISK;
> + else if (!strcasecmp(argv[i], "dlabel=on"))
> + flags |= LOADER_FLAGS_AUTOMODDISK;
> + else if (!strcasecmp(argv[i], "dlabel=off"))
> + flags &= ~LOADER_FLAGS_AUTOMODDISK;
> else if (!strcasecmp(argv[i], "rescue"))
> flags |= LOADER_FLAGS_RESCUE;
> else if (!strcasecmp(argv[i], "nopass"))
> @@ -1785,6 +1789,7 @@ int main(int argc, char ** argv) {
> struct loaderData_s loaderData;
>
> char *path;
> + struct ddlist *dd, *dditer;
>
> gchar *cmdLine = NULL, *ksFile = NULL, *virtpcon = NULL;
> gboolean testing = FALSE, mediacheck = FALSE;
> @@ -1882,6 +1887,12 @@ int main(int argc, char ** argv) {
> flags |= LOADER_FLAGS_NOSHELL;
> #endif
>
> + /* XXX if RHEL, enable the AUTODD feature by default,
> + * but we should come with more general way how to control this */
> + if(!strncmp(getProductName(), "Red Hat", 7)){
> + flags |= LOADER_FLAGS_AUTOMODDISK;
> + }
> +

You probably ought to split this checking out to its own function somewhere.

> openLog(FL_TESTING(flags));
> if (!FL_TESTING(flags))
> openlog("loader", 0, LOG_LOCAL0);
> @@ -1946,6 +1957,26 @@ 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(FL_AUTOMODDISK(flags)){
> + /* Load all autodetected DDs */
> + logMessage(INFO, "Trying to detect vendor driver discs");
> + dd = findDriverDiskByLabel();
> + dditer = dd;
> + while(dditer){
> + if(loadDriverDiskFromPartition(&loaderData, dditer->device)){
> + logMessage(ERROR, "Automatic driver disk loader failed for %s.", dditer->device);
> + }
> + else{
> + logMessage(INFO, "Automatic driver disk loader succeeded for %s.", dditer->device);
> + }
> + dditer = dditer->next;
> + }
> + ddlist_free(dd);
> + }
> +
> if (FL_MODDISK(flags)) {
> startNewt();
> loadDriverDisks(DEVICE_ANY, &loaderData);
> @@ -1955,6 +1986,9 @@ int main(int argc, char ** argv) {
> logMessage(INFO, "found /dd.img, loading drivers");
> getDDFromSource(&loaderData, "path:/dd.img");
> }
> +
> + /* 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
> @@ -2139,6 +2173,9 @@ int main(int argc, char ** argv) {
> tmparg++;
> }
>
> + if (FL_AUTOMODDISK(flags))
> + *argptr++ = "--dlabel";
> +
> if (FL_NOIPV4(flags))
> *argptr++ = "--noipv4";
>
> diff --git a/loader/loader.h b/loader/loader.h
> index ebf3766..ca6404f 100644
> --- a/loader/loader.h
> +++ b/loader/loader.h
> @@ -70,6 +70,7 @@
> #define LOADER_FLAGS_HAVE_CMSCONF (((uint64_t) 1) << 37)
> #define LOADER_FLAGS_NOKILL (((uint64_t) 1) << 38)
> #define LOADER_FLAGS_KICKSTART_SEND_SERIAL (((uint64_t) 1) << 39)
> +#define LOADER_FLAGS_AUTOMODDISK (((uint64_t) 1) << 40)
>
> #define FL_TESTING(a) ((a) & LOADER_FLAGS_TESTING)
> #define FL_TEXT(a) ((a) & LOADER_FLAGS_TEXT)
> @@ -107,6 +108,7 @@
> #define FL_HAVE_CMSCONF(a) ((a) & LOADER_FLAGS_HAVE_CMSCONF)
> #define FL_NOKILL(a) ((a) & LOADER_FLAGS_NOKILL)
> #define FL_KICKSTART_SEND_SERIAL(a) ((a) & LOADER_FLAGS_KICKSTART_SEND_SERIAL)
> +#define FL_AUTOMODDISK(a) ((a) & LOADER_FLAGS_AUTOMODDISK)
>
> void startNewt(void);
> void stopNewt(void);

The flags code in general looks pretty good.

--
Peter

Growth for the sake of growth is the ideology of the cancer cell.

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 12-03-2009, 08:58 AM
Martin Sivak
 
Default Backport the driver disc functionality from rhel5

Hi, thanks for comments.

> "git config --global sendemail.chainreplyto no" might be a good thing
> to do

Right, thanks

> On 12/02/2009 11:27 AM, Martin Sivak wrote:
>
> > +/* modprobe DD mode */
> > +int modprobeDDmode()
> > +{
> > + FILE *f = fopen("/etc/depmod.d/ddmode.conf", "w");
> > + if(f){
> > + struct utsname unamedata;
> > + if(uname(&unamedata)){
> > + fprintf(f, " pblacklist /lib/modules
");
> > + }
> > + else{
> > + fprintf(f, " pblacklist /lib/modules/%s
",
> unamedata.release);
> > + }
> > +
> > + fclose(f);
> > + }
>
> What's with the funky coding style? It doesn't match the rest of the
> file.

Well the code used to have more lines in the if/else clauses, but I can't see anything else...

> > +
> > + return f==NULL;
> > +}
> > +
> > +int modprobeNormalmode()
> > +{
> > + unlink("/etc/depmod.d/ddmode.conf");
> > +
> > + /* run depmod to refresh modules db */
> > + if(system("depmod -a")){
> > + /* FIXME: depmod didn't run */
> > + }
>
> Seems like there's nothing you can really do here except log it.
>

and return error code in case we want to check it in the loader.c

> > +
> > +/* RPM extraction dependency checks */
> > +int dlabelDeps(const char* depends, void *userptr)
> > +{
> > + logMessage(DEBUGLVL, "Depends on: %s
", depends);
> > + return 0;
> > +}
>
> What's the point of this at all?

I wanted to have a complete set for dependency checks as I hope to sell this to RPM guys some day, so we do not have to maintain it ourselves. Right now, it is just an empty callback, which I can remove.

>
> > +
> > +int dlabelProvides(const char* dep, void *userptr)
> > +{
> > + char *kernelver = (char*)userptr;
> > +
> > + logMessage(DEBUGLVL, "Provides: %s
", dep);
> > +
> > + return strcmp(dep, kernelver);
> > +}
>
> A better function name and some comments would be nice here. Upon
> reading it,
> I'm not at all sure what it's supposed to do.

I'm adding a comment. This checks if the RPM provides symbol name passed through userptr

> > +
> > +char* moduleDescription(const char* modulePath)
> > +{
> > + char *command = rstrscat(NULL, "modinfo --description '",
> modulePath, "'", NULL);
> > + FILE *f = popen(command, "r");
> > + if(f==NULL) return NULL;
> > +
> > + char *description = malloc(sizeof(char)*256);
> > + int size = fread(description, 1, 255, f);
>
> Couple of things here:
> 1) All the code/decleration mixing here isn't very easy to read.

Ok

> 2) allocations need to be checked

Ok

> 3) There's a pretty good case for using checked_asprintf() instead of
> using
> *anything* provided by rpmlib. Or implement a stack-allocating
> sprintfa(),
> which would be even better since it'll cut down on free()s in the
> return
> paths. You can borrow this code from nash in mkinitrd (git is at
> ssh://git.fedoraproject.org/git/hosted/mkinitrd) if you like. I
> think I
> called it asprintfa() there, but whatever.

I'll take a look

> > + if(size==0){
> > + free(description);
> > + return NULL;
> > + }
> > + description[size-1]=0; /* strip the trailing newline */
>
> I'm pretty sure this should be using strrchr() to find the newline
> rather than
> expecting all description strings to be exactly 254 bytes plus a
> newline.

size contains number of actually read blocks, block size is set to 1, number of blocks to max 255. So [size-1] is the last character and it doesn't have to be the 255th. And honestly, I do not care about shortening the description to max 255 chars, since we can't show that in the interface anyways..

> > + pclose(f);
> > +
> > + free(command);
>
> Why not free this immediately after the popen?
>

You found a possible memory leak, thanks.

> > + return description;
> > +}
> > +
> > +int globErrFunc(const char *epath, int eerrno)
> > +{
> > + return 0;
> > +}
>
> Seems like the wrong thing to do - you're effectively telling glob()
> to ignore
> all errors and try to continue processing

Actually, that is what I want, I want to copy/see everything possible. If one file fails, the rest is still usable.
I might add a check for the fatal errors though...


> rc = uname(&unamedata);
> checked_asprintf(&kernelver, "kernel-modules-%s", rc ? "unknown" :
> unamedata.release);
>

That looks much better, thanks


> I'm really not a big fan of hiding the format specifier behind a
> macro, but
> I do see the case for re-use here. Given that, it probably ought to
> be
> defined somewhere very near to this code, not in a separate header
> file.
>
> Also maybe name it something like DD_RPMDIR_TEMPLATE ?

TEMPLATE in the name seems like good idea.


>
> Seems like using glib's linked lists is better than adding our own.
>

Yep, it was backport from rhel5 code. I'll change it (I was thinking about this too, but I have forgotten to do it)

> > + if(!strncmp(getProductName(), "Red Hat", 7)){
> > + flags |= LOADER_FLAGS_AUTOMODDISK;
> > + }
> > +
>
> You probably ought to split this checking out to its own function
> somewhere.
>

What I would like to do is to have some "flags" in the treeinfo file, which would control this. But that is a topic for a longer separate discussion.


Martin

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 12-07-2009, 07:30 PM
David Cantrell
 
Default Backport the driver disc functionality from rhel5

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

Apologies for duplicate comments, I'm just running through everything.

On Wed, 2 Dec 2009, Martin Sivak wrote:


- updates the code to agree with current anaconda API
- adds dlabel functionality
- also adds some stub calls to set modprobe behaviour


This comment improves things, but I'd really like to see more. A patch like
this with 355 insertions deserves more of a writeup. I would like to see a
detailed comment starting with a short one line description (preferably <=
76 chars) including the bug number, then paragraphs explaining what the patch
is doing, what was changed, etc.

Since multiple files were changed and multiple functions touched (and new
ones added), I'd prefer to see explanations for that.


---
loader/driverdisk.c | 333 +++++++++++++++++++++++++++++++++++++++++++++------
loader/driverdisk.h | 20 +++
loader/loader.c | 37 ++++++
loader/loader.h | 2 +
4 files changed, 355 insertions(+), 37 deletions(-)

diff --git a/loader/driverdisk.c b/loader/driverdisk.c
index 5f8d43c..f718684 100644
--- a/loader/driverdisk.c
+++ b/loader/driverdisk.c
@@ -30,6 +30,12 @@
#include <unistd.h>
#include <glib.h>

+#include <blkid/blkid.h>
+
+#include <glob.h>
+#include <rpm/rpmlib.h>
+#include <sys/utsname.h>
+
#include "copy.h"
#include "loader.h"
#include "log.h"
@@ -48,6 +54,8 @@
#include "nfsinstall.h"
#include "urlinstall.h"

+#include "rpmextract.h"
+
#include "../isys/isys.h"
#include "../isys/imount.h"
#include "../isys/eddsupport.h"
@@ -55,37 +63,175 @@
/* boot flags */
extern uint64_t flags;

-static char * driverDiskFiles[] = { "modinfo", "modules.dep",
- "modules.cgz", "modules.alias", NULL };
+/* modprobe DD mode */
+int modprobeDDmode()
+{
+ FILE *f = fopen("/etc/depmod.d/ddmode.conf", "w");
+ if(f){
+ struct utsname unamedata;
+ if(uname(&unamedata)){
+ fprintf(f, " pblacklist /lib/modules
");
+ }
+ else{
+ fprintf(f, " pblacklist /lib/modules/%s
", unamedata.release);
+ }
+
+ fclose(f);
+ }
+
+ return f==NULL;
+}


I think Peter commented on the coding style. The issues here that I have a
problem with are indentation, spacing, and the function declaration. We
generally use 4 space indents, you've done 2. We also do stuff like:

if (f) {

and not:

if(f){

For the function declaration, I don't like functionName(). I'd prefer
functionName(void).

These are nits, I know, but this is all new code and I would much prefer that
we maintain coding style consistency. A good guide is here:

http://www.x.org/wiki/CodingStyle

I'd say with the exception of function naming, we do most of the other stuff.
I'm ok with function_names_like_this or functionNamesLikeThis.


+int modprobeNormalmode()
+{
+ unlink("/etc/depmod.d/ddmode.conf");
+
+ /* run depmod to refresh modules db */
+ if(system("depmod -a")){
+ /* FIXME: depmod didn't run */
+ }
+
+ return 0;
+}


If unlink() fails or system() fails, that would be nice to know and why.


+/* RPM extraction dependency checks */
+int dlabelDeps(const char* depends, void *userptr)
+{
+ logMessage(DEBUGLVL, "Depends on: %s
", depends);
+ return 0;
+}


I assume this is a stub and will contain more later?


+int dlabelProvides(const char* dep, void *userptr)
+{
+ char *kernelver = (char*)userptr;
+
+ logMessage(DEBUGLVL, "Provides: %s
", dep);
+
+ return strcmp(dep, kernelver);
+}


Will this contain more at some point?


+int dlabelFilter(const char* name, struct stat *fstat, void *userptr)
+{
+ int l = strlen(name);
+
+ logMessage(DEBUGLVL, "Unpacking %s (%lluB)
", name, fstat->st_size);
+
+ /* we want firmware files */
+ if(!strncmp("lib/firmware/", name, 13)) return 0;
+
+ if(l<3) return 1;
+ l-=3;
+
+ /* and we want only .ko files */
+ if(strncmp(".ko", name+l, 3)) return 1;
+
+ /* TODO we are unpacking kernel module, read it's description */
+
+ return 0;
+}
+
+char* moduleDescription(const char* modulePath)
+{
+ char *command = rstrscat(NULL, "modinfo --description '", modulePath, "'", NULL);
+ FILE *f = popen(command, "r");
+ if(f==NULL) return NULL;
+
+ char *description = malloc(sizeof(char)*256);
+ int size = fread(description, 1, 255, f);
+ if(size==0){
+ free(description);
+ return NULL;
+ }
+ description[size-1]=0; /* strip the trailing newline */
+ pclose(f);
+
+ free(command);
+ return description;
+}


Rather than the malloc() and fread() here, I'd like to see us make use of more
convenience functions from glib because we don't then have to deal with our own
memory management or static buffers. The g_spawn_sync() function would work
well here. It can run modinfo and capture stdout and stderr. Here is a
verbose standalone example showing how to get the module description via
modinfo with glib functions (this is also meant to demonstrate some of the
other stuff that glib can do):

- ------
/*
* use modinfo to get kernel module description, report errors
* overly verbose example demonstrating glib convenience functions
*/

/* gcc -O0 -g -Wall $(pkg-config --cflags glib-2.0) getModDesc.c
* $(pkg-config --libs glib-2.0)
*/

/* dcantrell@redhat.com */

#include <stdio.h>
#include <stdlib.h>
#include <glib.h>

int main(int argc, char **argv) {
GString *tmp = g_string_new("modinfo -F description ");
gchar **modinfoArgs = NULL;
GSpawnFlags flags = G_SPAWN_SEARCH_PATH;
gchar *output = NULL;
gchar *errors = NULL;
gint status = 0;
GError *e = NULL;

if (argc != 2) {
fprintf(stderr, "Please specify one module file by full path.
");
return EXIT_FAILURE;
}

/* append the module path to our command, split in to argument vector */
g_string_append_printf(tmp, argv[1]);
modinfoArgs = g_strsplit(tmp->str, " ", 0);
g_string_free(tmp, TRUE);

if (!modinfoArgs) {
fprintf(stderr, "modinfoArgs split failure
");
return EXIT_FAILURE;
}

/* run modinfo, get description, report errors on failure */
if (!g_spawn_sync(NULL, modinfoArgs, NULL, flags, NULL, NULL,
&output, &errors, &status, &e)) {
fprintf(stderr, "Error getting module description: %s
", e->message);
g_error_free(e);

if (errors) {
fprintf(stderr, "modinfo error: %s
", errors);
}

return EXIT_FAILURE;
}

/* display the module description */
if (output) {
/* we do not want trailing newlines */
output = g_strchomp(output);

printf("%s
", output);
}

return EXIT_SUCCESS;
}
- ------

I would like to avoid static buffers when we can.


+int globErrFunc(const char *epath, int eerrno)
+{
+ return 0;
+}


I see you're passing this to glob to toss errors, but according to the glob(3)
man page, you could pass NULL to glob() and get the same effect.


+int dlabelUnpackRPMDir(char* rpmdir, char* destination)
+{
+ char *kernelver;
+ struct utsname unamedata;
+ char *oldcwd;
+ char *globpattern;
+ int rc;
+
+ /* get current working directory */
+ oldcwd = getcwd(NULL, 0);
+
+ /* set the cwd to destination */
+ if(chdir(destination)){
+ free(oldcwd);
+ return 1;
+ }
+
+ /* get running kernel version */
+ if(uname(&unamedata)){
+ kernelver = rstrscat(NULL, "kernel-modules-", "unknown", NULL);
+ }
+ else{
+ kernelver = rstrscat(NULL, "kernel-modules-", unamedata.release, NULL);
+ }
+ logMessage(DEBUGLVL, "Kernel version: %s
", kernelver);
+
+ globpattern = rstrscat(NULL, rpmdir, "/*.rpm", NULL);
+ glob_t globres;
+ char** globitem;
+ if(!glob(globpattern, GLOB_NOSORT|GLOB_NOESCAPE, globErrFunc, &globres)){
+ /* iterate over all rpm files */
+ globitem = globres.gl_pathv;
+ while(globres.gl_pathc>0 && globitem!=NULL){
+ explodeRPM(*globitem, dlabelFilter, dlabelProvides, dlabelDeps, kernelver);
+ }
+ globfree(&globres);
+ /* end of iteration */
+ }
+ free(globpattern);
+
+ /* restore CWD */
+ if(chdir(oldcwd)){
+ /* FIXME: too bad.. */
+ }
+
+ /* cleanup */
+ free(kernelver);
+ free(oldcwd);
+ return rc;
+}


See email reply to your first message in this thread for my rpm handling
comments.


+static char * driverDiskFiles[] = { "repodata", NULL };

static int verifyDriverDisk(char *mntpt) {
char ** fnPtr;
char file[200];
struct stat sb;

- for (fnPtr = driverDiskFiles; *fnPtr; fnPtr++) {
- sprintf(file, "%s/%s", mntpt, *fnPtr);
- if (access(file, R_OK)) {
- logMessage(ERROR, "cannot find %s, bad driver disk", file);
- return LOADER_BACK;
- }
- }
-
- /* check for both versions */
+ /* check for dd descriptor */
sprintf(file, "%s/rhdd", mntpt);
if (access(file, R_OK)) {
- logMessage(DEBUGLVL, "not a new format driver disk, checking for old");
- sprintf(file, "%s/rhdd-6.1", mntpt);
- if (access(file, R_OK)) {
- logMessage(ERROR, "can't find either driver disk identifier, bad "
- "driver disk");
- }
+ logMessage(ERROR, "can't find driver disk identifier, bad "
+ "driver disk");
+ return LOADER_BACK;
}

/* side effect: file is still mntpt/ddident */
stat(file, &sb);
if (!sb.st_size)
return LOADER_BACK;
+ for (fnPtr = driverDiskFiles; *fnPtr; fnPtr++) {
+ sprintf(file, "%s/%s/%s", mntpt, getProductArch(), *fnPtr);
+ if (access(file, R_OK)) {
+ logMessage(ERROR, "cannot find %s, bad driver disk", file);
+ return LOADER_BACK;
+ }
+ }

return LOADER_OK;
}
@@ -101,25 +247,20 @@ static void copyErrorFn (char *msg) {
/* this copies the contents of the driver disk to a ramdisk and loads
* the moduleinfo, etc. assumes a "valid" driver disk mounted at mntpt */
static int loadDriverDisk(struct loaderData_s *loaderData, char *mntpt) {
- moduleInfoSet modInfo = loaderData->modInfo;
- char file[200], dest[200];
+ /* FIXME moduleInfoSet modInfo = loaderData->modInfo; */
+ char file[200], dest[200], src[200];
char *title;
char *fwdir = NULL;
struct moduleBallLocation * location;
struct stat sb;
static int disknum = 0;
- int version = 1;
int fd, ret;

- /* check for both versions */
- sprintf(file, "%s/rhdd", mntpt);
+ /* check for new version */
+ sprintf(file, "%s/rhdd3", mntpt);
if (access(file, R_OK)) {
- version = 0;
- sprintf(file, "%s/rhdd-6.1", mntpt);
- if (access(file, R_OK)) {
- /* this can't happen, we already verified it! */
- return LOADER_BACK;
- }
+ /* this can't happen, we already verified it! */
+ return LOADER_BACK;
}
stat(file, &sb);
title = malloc(sb.st_size + 1);
@@ -131,23 +272,35 @@ static int loadDriverDisk(struct loaderData_s *loaderData, char *mntpt) {
title[sb.st_size] = '';
close(fd);

- sprintf(file, "/tmp/DD-%d", disknum);
+ sprintf(file, DD_RPMS, disknum);
mkdirChain(file);
+ mkdirChain(DD_MODULES);
+ mkdirChain(DD_FIRMWARE);

if (!FL_CMDLINE(flags)) {
startNewt();
winStatus(40, 3, _("Loading"), _("Reading driver disk"));
}

- sprintf(dest, "/tmp/DD-%d", disknum);
- copyDirectory(mntpt, dest, copyWarnFn, copyErrorFn);
-
location = malloc(sizeof(struct moduleBallLocation));
location->title = strdup(title);
- location->version = version;

- checked_asprintf(&location->path, "/tmp/DD-%d/modules.cgz", disknum);
- checked_asprintf(&fwdir, "/tmp/DD-%d/firmware", disknum);
+ checked_asprintf(&location->path, DD_MODULES);
+ checked_asprintf(&fwdir, DD_FIRMWARE);
+
+ sprintf(dest, DD_RPMS, disknum);
+ sprintf(src, "%s/rpms/%s", mntpt, getProductArch());
+ copyDirectory(src, dest, copyWarnFn, copyErrorFn);
+
+ /* unpack packages from dest into location->path */
+ if(dlabelUnpackRPMDir(dest, DD_EXTRACTED)){
+ /* TODO error handler */
+ }
+
+ /* run depmod to refresh modules db */
+ if(system("depmod -a")){
+ /* FIXME: depmod didn't run */
+ }

if (!access(fwdir, R_OK|X_OK)) {
add_fw_search_dir(loaderData, fwdir);
@@ -156,8 +309,11 @@ static int loadDriverDisk(struct loaderData_s *loaderData, char *mntpt) {
}
free(fwdir);

- sprintf(file, "%s/modinfo", mntpt);
- readModuleInfo(file, modInfo, location, 1);
+ /* TODO generate and read module info
+ *
+ * sprintf(file, "%s/modinfo", mntpt);
+ * readModuleInfo(file, modInfo, location, 1);
+ */

if (!FL_CMDLINE(flags))
newtPopWindow();
@@ -623,3 +779,106 @@ static void getDDFromDev(struct loaderData_s * loaderData, char * dev) {
umount("/tmp/drivers");
unlink("/tmp/drivers");
}


Given the opportunity, I'd like to see older code using static buffers and
sprintf() replaced with checked_asprintf().


+
+/*
+ * Utility functions to maintain linked-list of device names
+ * */
+
+struct ddlist* ddlist_add(struct ddlist *list, const char* device)
+{
+ struct ddlist* item;
+
+ item = (struct ddlist*)malloc(sizeof(struct ddlist));
+ if(item==NULL){
+ return list;
+ }
+
+ item->device = strdup(device);
+ item->next = list;
+
+ return item;
+}
+
+int ddlist_free(struct ddlist *list)
+{
+ struct ddlist *next;
+ int count = 0;
+
+ while(list!=NULL){
+ next = list->next;
+ free(list->device);
+ free(list);
+ list = next;
+ count++;
+ }
+
+ return count;
+}


Let's use glib's linked list routines. The GSList is usable here.
http://library.gnome.org/devel/glib/stable/glib-Singly-Linked-Lists.html


+/*
+ * Look for partition with specific label (part of #316481)
+ */
+struct ddlist* findDriverDiskByLabel(void)
+{
+ char *ddLabel = "OEMDRV";
+ struct ddlist *ddDevice = NULL;
+ blkid_cache bCache;
+
+ int res;
+ blkid_dev_iterate bIter;
+ blkid_dev bDev;
+
+ if(blkid_get_cache(&bCache, NULL)<0){
+ logMessage(ERROR, _("Cannot initialize cache instance for blkid"));
+ return NULL;
+ }
+ if((res = blkid_probe_all(bCache))<0){
+ logMessage(ERROR, _("Cannot probe devices in blkid: %d"), res);
+ return NULL;
+ }
+
+ bIter = blkid_dev_iterate_begin(bCache);
+ blkid_dev_set_search(bIter, "LABEL", ddLabel);
+ while((res = blkid_dev_next(bIter, &bDev))!=0){
+ bDev = blkid_verify(bCache, bDev);
+ if(!bDev)
+ continue;
+ logMessage(DEBUGLVL, _("Adding driver disc %s to the list of available DDs."), blkid_dev_devname(bDev));
+ ddDevice = ddlist_add(ddDevice, blkid_dev_devname(bDev));
+ /*blkid_free_dev(bDev); -- probably taken care of by the put cache call.. it is not exposed in the API */
+ }
+ blkid_dev_iterate_end(bIter);
+
+ blkid_put_cache(bCache);
+
+ return ddDevice;
+}
+
+int loadDriverDiskFromPartition(struct loaderData_s *loaderData, char* device)
+{
+ int rc;
+
+ logMessage(INFO, "trying to mount %s", device);
+ if (doPwMount(device, "/tmp/drivers", "auto", "ro", NULL)) {
+ logMessage(ERROR, _("Failed to mount driver disk."));
+ return -1;
+ }
+
+ rc = verifyDriverDisk("/tmp/drivers");
+ if (rc == LOADER_BACK) {
+ logMessage(ERROR, _("Driver disk is invalid for this "
+ "release of %s."), getProductName());
+ umount("/tmp/drivers");
+ return -2;
+ }
+
+ rc = loadDriverDisk(loaderData, "/tmp/drivers");
+ umount("/tmp/drivers");
+ if (rc == LOADER_BACK) {
+ return -3;
+ }
+
+ return 0;
+}
+
diff --git a/loader/driverdisk.h b/loader/driverdisk.h
index e6e919d..3e442fb 100644
--- a/loader/driverdisk.h
+++ b/loader/driverdisk.h
@@ -24,6 +24,11 @@
#include "modules.h"
#include "moduleinfo.h"

+#define DD_RPMS "/tmp/DD-%d"
+#define DD_EXTRACTED "/tmp/DD"
+#define DD_MODULES "/tmp/DD/lib/modules"
+#define DD_FIRMWARE "/tmp/DD/lib/firmware"
+
extern char *ddFsTypes[];

int loadDriverFromMedia(int class, struct loaderData_s *loaderData,
@@ -39,4 +44,19 @@ void useKickstartDD(struct loaderData_s * loaderData, int argc,

void getDDFromSource(struct loaderData_s * loaderData, char * src);

+int loadDriverDiskFromPartition(struct loaderData_s *loaderData, char* device);
+
+struct ddlist {
+ char* device;
+ struct ddlist* next;
+};
+
+struct ddlist* ddlist_add(struct ddlist *list, const char* device);
+int ddlist_free(struct ddlist *list);
+
+struct ddlist* findDriverDiskByLabel(void);
+
+int modprobeNormalmode();
+int modprobeDDmode();
+
#endif
diff --git a/loader/loader.c b/loader/loader.c
index cd3e178..3159513 100644
--- a/loader/loader.c
+++ b/loader/loader.c
@@ -959,6 +959,10 @@ static void parseCmdLineFlags(struct loaderData_s * loaderData,
else if (!strcasecmp(argv[i], "dd") ||
!strcasecmp(argv[i], "driverdisk"))
flags |= LOADER_FLAGS_MODDISK;
+ else if (!strcasecmp(argv[i], "dlabel=on"))
+ flags |= LOADER_FLAGS_AUTOMODDISK;
+ else if (!strcasecmp(argv[i], "dlabel=off"))
+ flags &= ~LOADER_FLAGS_AUTOMODDISK;
else if (!strcasecmp(argv[i], "rescue"))
flags |= LOADER_FLAGS_RESCUE;
else if (!strcasecmp(argv[i], "nopass"))
@@ -1785,6 +1789,7 @@ int main(int argc, char ** argv) {
struct loaderData_s loaderData;

char *path;
+ struct ddlist *dd, *dditer;

gchar *cmdLine = NULL, *ksFile = NULL, *virtpcon = NULL;
gboolean testing = FALSE, mediacheck = FALSE;
@@ -1882,6 +1887,12 @@ int main(int argc, char ** argv) {
flags |= LOADER_FLAGS_NOSHELL;
#endif

+ /* XXX if RHEL, enable the AUTODD feature by default,
+ * but we should come with more general way how to control this */
+ if(!strncmp(getProductName(), "Red Hat", 7)){
+ flags |= LOADER_FLAGS_AUTOMODDISK;
+ }
+
openLog(FL_TESTING(flags));
if (!FL_TESTING(flags))
openlog("loader", 0, LOG_LOCAL0);
@@ -1946,6 +1957,26 @@ 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(FL_AUTOMODDISK(flags)){
+ /* Load all autodetected DDs */
+ logMessage(INFO, "Trying to detect vendor driver discs");
+ dd = findDriverDiskByLabel();
+ dditer = dd;
+ while(dditer){
+ if(loadDriverDiskFromPartition(&loaderData, dditer->device)){
+ logMessage(ERROR, "Automatic driver disk loader failed for %s.", dditer->device);
+ }
+ else{
+ logMessage(INFO, "Automatic driver disk loader succeeded for %s.", dditer->device);
+ }
+ dditer = dditer->next;
+ }
+ ddlist_free(dd);
+ }
+
if (FL_MODDISK(flags)) {
startNewt();
loadDriverDisks(DEVICE_ANY, &loaderData);
@@ -1955,6 +1986,9 @@ int main(int argc, char ** argv) {
logMessage(INFO, "found /dd.img, loading drivers");
getDDFromSource(&loaderData, "path:/dd.img");
}
+
+ /* 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
@@ -2139,6 +2173,9 @@ int main(int argc, char ** argv) {
tmparg++;
}

+ if (FL_AUTOMODDISK(flags))
+ *argptr++ = "--dlabel";
+
if (FL_NOIPV4(flags))
*argptr++ = "--noipv4";

diff --git a/loader/loader.h b/loader/loader.h
index ebf3766..ca6404f 100644
--- a/loader/loader.h
+++ b/loader/loader.h
@@ -70,6 +70,7 @@
#define LOADER_FLAGS_HAVE_CMSCONF (((uint64_t) 1) << 37)
#define LOADER_FLAGS_NOKILL (((uint64_t) 1) << 38)
#define LOADER_FLAGS_KICKSTART_SEND_SERIAL (((uint64_t) 1) << 39)
+#define LOADER_FLAGS_AUTOMODDISK (((uint64_t) 1) << 40)

#define FL_TESTING(a) ((a) & LOADER_FLAGS_TESTING)
#define FL_TEXT(a) ((a) & LOADER_FLAGS_TEXT)
@@ -107,6 +108,7 @@
#define FL_HAVE_CMSCONF(a) ((a) & LOADER_FLAGS_HAVE_CMSCONF)
#define FL_NOKILL(a) ((a) & LOADER_FLAGS_NOKILL)
#define FL_KICKSTART_SEND_SERIAL(a) ((a) & LOADER_FLAGS_KICKSTART_SEND_SERIAL)
+#define FL_AUTOMODDISK(a) ((a) & LOADER_FLAGS_AUTOMODDISK)

void startNewt(void);
void stopNewt(void);



Large items:

- - Fix the coding style.
- - Use glib's linked list rather than creating a new one.
- - Avoid the use of static buffers. This is one thing I like about the
convenience functions in glib. Almost all will handle dynamic memory
allocation. I see no reason to not make use of that in loader.

I've got comments for rpm handling in another email.

- --
David Cantrell <dcantrell@redhat.com>

Red Hat / Honolulu, HI

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

iEYEARECAAYFAksdZfsACgkQ5hsjjIy1VkkSmQCePhyqQ0MioU iUz5tmdI1X2TGC
DwgAoKPhf1qCcR+ROTjptAVZnjdGPW9J
=W8vv
-----END PGP SIGNATURE-----

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 12-08-2009, 07:55 AM
Martin Sivak
 
Default Backport the driver disc functionality from rhel5

Well the bugnumbers.. all those patches fix pieces and bits from the following list of bugs (all product and releases combined and relevant for this work):

466754, 469709, 485060, 489314, 500743, 504155, 508242, 515437, 522400, 523666, 538590,
+ couple of bugs closed with RHEL5.3 and RHEL5.4

I asked for comments for the feature mentioned in 538591.. and I ment it.. so far no response
We are not doing 538597, rpm does this just fine

Also bear in mind, that most of the functionality IS present and was reviewed in RHEL5. The algorithm description was described in the leading email. The goals are in the documentation I added in another patch.

Martin

----- "David Cantrell" <dcantrell@redhat.com> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Apologies for duplicate comments, I'm just running through
> everything.
>
> On Wed, 2 Dec 2009, Martin Sivak wrote:
>
> > - updates the code to agree with current anaconda API
> > - adds dlabel functionality
> > - also adds some stub calls to set modprobe behaviour
>
> This comment improves things, but I'd really like to see more. A
> patch like
> this with 355 insertions deserves more of a writeup. I would like to
> see a
> detailed comment starting with a short one line description
> (preferably <=
> 76 chars) including the bug number, then paragraphs explaining what
> the patch
> is doing, what was changed, etc.
>
> Since multiple files were changed and multiple functions touched (and
> new
> ones added), I'd prefer to see explanations for that.
>
> > ---
> > loader/driverdisk.c | 333
> +++++++++++++++++++++++++++++++++++++++++++++------
> > loader/driverdisk.h | 20 +++
> > loader/loader.c | 37 ++++++
> > loader/loader.h | 2 +
> > 4 files changed, 355 insertions(+), 37 deletions(-)
> >
> > diff --git a/loader/driverdisk.c b/loader/driverdisk.c
> > index 5f8d43c..f718684 100644
> > --- a/loader/driverdisk.c
> > +++ b/loader/driverdisk.c
> > @@ -30,6 +30,12 @@
> > #include <unistd.h>
> > #include <glib.h>
> >
> > +#include <blkid/blkid.h>
> > +
> > +#include <glob.h>
> > +#include <rpm/rpmlib.h>
> > +#include <sys/utsname.h>
> > +
> > #include "copy.h"
> > #include "loader.h"
> > #include "log.h"
> > @@ -48,6 +54,8 @@
> > #include "nfsinstall.h"
> > #include "urlinstall.h"
> >
> > +#include "rpmextract.h"
> > +
> > #include "../isys/isys.h"
> > #include "../isys/imount.h"
> > #include "../isys/eddsupport.h"
> > @@ -55,37 +63,175 @@
> > /* boot flags */
> > extern uint64_t flags;
> >
> > -static char * driverDiskFiles[] = { "modinfo", "modules.dep",
> > - "modules.cgz", "modules.alias",
> NULL };
> > +/* modprobe DD mode */
> > +int modprobeDDmode()
> > +{
> > + FILE *f = fopen("/etc/depmod.d/ddmode.conf", "w");
> > + if(f){
> > + struct utsname unamedata;
> > + if(uname(&unamedata)){
> > + fprintf(f, " pblacklist /lib/modules
");
> > + }
> > + else{
> > + fprintf(f, " pblacklist /lib/modules/%s
",
> unamedata.release);
> > + }
> > +
> > + fclose(f);
> > + }
> > +
> > + return f==NULL;
> > +}
>
> I think Peter commented on the coding style. The issues here that I
> have a
> problem with are indentation, spacing, and the function declaration.
> We
> generally use 4 space indents, you've done 2. We also do stuff like:
>
> if (f) {
>
> and not:
>
> if(f){
>
> For the function declaration, I don't like functionName(). I'd
> prefer
> functionName(void).
>
> These are nits, I know, but this is all new code and I would much
> prefer that
> we maintain coding style consistency. A good guide is here:
>
> http://www.x.org/wiki/CodingStyle
>
> I'd say with the exception of function naming, we do most of the other
> stuff.
> I'm ok with function_names_like_this or functionNamesLikeThis.
>
> > +int modprobeNormalmode()
> > +{
> > + unlink("/etc/depmod.d/ddmode.conf");
> > +
> > + /* run depmod to refresh modules db */
> > + if(system("depmod -a")){
> > + /* FIXME: depmod didn't run */
> > + }
> > +
> > + return 0;
> > +}
>
> If unlink() fails or system() fails, that would be nice to know and
> why.
>
> > +/* RPM extraction dependency checks */
> > +int dlabelDeps(const char* depends, void *userptr)
> > +{
> > + logMessage(DEBUGLVL, "Depends on: %s
", depends);
> > + return 0;
> > +}
>
> I assume this is a stub and will contain more later?
>
> > +int dlabelProvides(const char* dep, void *userptr)
> > +{
> > + char *kernelver = (char*)userptr;
> > +
> > + logMessage(DEBUGLVL, "Provides: %s
", dep);
> > +
> > + return strcmp(dep, kernelver);
> > +}
>
> Will this contain more at some point?
>
> > +int dlabelFilter(const char* name, struct stat *fstat, void
> *userptr)
> > +{
> > + int l = strlen(name);
> > +
> > + logMessage(DEBUGLVL, "Unpacking %s (%lluB)
", name,
> fstat->st_size);
> > +
> > + /* we want firmware files */
> > + if(!strncmp("lib/firmware/", name, 13)) return 0;
> > +
> > + if(l<3) return 1;
> > + l-=3;
> > +
> > + /* and we want only .ko files */
> > + if(strncmp(".ko", name+l, 3)) return 1;
> > +
> > + /* TODO we are unpacking kernel module, read it's description */
> > +
> > + return 0;
> > +}
> > +
> > +char* moduleDescription(const char* modulePath)
> > +{
> > + char *command = rstrscat(NULL, "modinfo --description '",
> modulePath, "'", NULL);
> > + FILE *f = popen(command, "r");
> > + if(f==NULL) return NULL;
> > +
> > + char *description = malloc(sizeof(char)*256);
> > + int size = fread(description, 1, 255, f);
> > + if(size==0){
> > + free(description);
> > + return NULL;
> > + }
> > + description[size-1]=0; /* strip the trailing newline */
> > + pclose(f);
> > +
> > + free(command);
> > + return description;
> > +}
>
> Rather than the malloc() and fread() here, I'd like to see us make use
> of more
> convenience functions from glib because we don't then have to deal
> with our own
> memory management or static buffers. The g_spawn_sync() function
> would work
> well here. It can run modinfo and capture stdout and stderr. Here is
> a
> verbose standalone example showing how to get the module description
> via
> modinfo with glib functions (this is also meant to demonstrate some of
> the
> other stuff that glib can do):
>
> - ------
> /*
> * use modinfo to get kernel module description, report errors
> * overly verbose example demonstrating glib convenience functions
> */
>
> /* gcc -O0 -g -Wall $(pkg-config --cflags glib-2.0) getModDesc.c
> * $(pkg-config --libs glib-2.0)
> */
>
> /* dcantrell@redhat.com */
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <glib.h>
>
> int main(int argc, char **argv) {
> GString *tmp = g_string_new("modinfo -F description ");
> gchar **modinfoArgs = NULL;
> GSpawnFlags flags = G_SPAWN_SEARCH_PATH;
> gchar *output = NULL;
> gchar *errors = NULL;
> gint status = 0;
> GError *e = NULL;
>
> if (argc != 2) {
> fprintf(stderr, "Please specify one module file by full
> path.
");
> return EXIT_FAILURE;
> }
>
> /* append the module path to our command, split in to argument
> vector */
> g_string_append_printf(tmp, argv[1]);
> modinfoArgs = g_strsplit(tmp->str, " ", 0);
> g_string_free(tmp, TRUE);
>
> if (!modinfoArgs) {
> fprintf(stderr, "modinfoArgs split failure
");
> return EXIT_FAILURE;
> }
>
> /* run modinfo, get description, report errors on failure */
> if (!g_spawn_sync(NULL, modinfoArgs, NULL, flags, NULL, NULL,
> &output, &errors, &status, &e)) {
> fprintf(stderr, "Error getting module description: %s
",
> e->message);
> g_error_free(e);
>
> if (errors) {
> fprintf(stderr, "modinfo error: %s
", errors);
> }
>
> return EXIT_FAILURE;
> }
>
> /* display the module description */
> if (output) {
> /* we do not want trailing newlines */
> output = g_strchomp(output);
>
> printf("%s
", output);
> }
>
> return EXIT_SUCCESS;
> }
> - ------
>
> I would like to avoid static buffers when we can.
>
> > +int globErrFunc(const char *epath, int eerrno)
> > +{
> > + return 0;
> > +}
>
> I see you're passing this to glob to toss errors, but according to the
> glob(3)
> man page, you could pass NULL to glob() and get the same effect.
>
> > +int dlabelUnpackRPMDir(char* rpmdir, char* destination)
> > +{
> > + char *kernelver;
> > + struct utsname unamedata;
> > + char *oldcwd;
> > + char *globpattern;
> > + int rc;
> > +
> > + /* get current working directory */
> > + oldcwd = getcwd(NULL, 0);
> > +
> > + /* set the cwd to destination */
> > + if(chdir(destination)){
> > + free(oldcwd);
> > + return 1;
> > + }
> > +
> > + /* get running kernel version */
> > + if(uname(&unamedata)){
> > + kernelver = rstrscat(NULL, "kernel-modules-", "unknown",
> NULL);
> > + }
> > + else{
> > + kernelver = rstrscat(NULL, "kernel-modules-",
> unamedata.release, NULL);
> > + }
> > + logMessage(DEBUGLVL, "Kernel version: %s
", kernelver);
> > +
> > + globpattern = rstrscat(NULL, rpmdir, "/*.rpm", NULL);
> > + glob_t globres;
> > + char** globitem;
> > + if(!glob(globpattern, GLOB_NOSORT|GLOB_NOESCAPE, globErrFunc,
> &globres)){
> > + /* iterate over all rpm files */
> > + globitem = globres.gl_pathv;
> > + while(globres.gl_pathc>0 && globitem!=NULL){
> > + explodeRPM(*globitem, dlabelFilter, dlabelProvides,
> dlabelDeps, kernelver);
> > + }
> > + globfree(&globres);
> > + /* end of iteration */
> > + }
> > + free(globpattern);
> > +
> > + /* restore CWD */
> > + if(chdir(oldcwd)){
> > + /* FIXME: too bad.. */
> > + }
> > +
> > + /* cleanup */
> > + free(kernelver);
> > + free(oldcwd);
> > + return rc;
> > +}
>
> See email reply to your first message in this thread for my rpm
> handling
> comments.
>
> > +static char * driverDiskFiles[] = { "repodata", NULL };
> >
> > static int verifyDriverDisk(char *mntpt) {
> > char ** fnPtr;
> > char file[200];
> > struct stat sb;
> >
> > - for (fnPtr = driverDiskFiles; *fnPtr; fnPtr++) {
> > - sprintf(file, "%s/%s", mntpt, *fnPtr);
> > - if (access(file, R_OK)) {
> > - logMessage(ERROR, "cannot find %s, bad driver disk",
> file);
> > - return LOADER_BACK;
> > - }
> > - }
> > -
> > - /* check for both versions */
> > + /* check for dd descriptor */
> > sprintf(file, "%s/rhdd", mntpt);
> > if (access(file, R_OK)) {
> > - logMessage(DEBUGLVL, "not a new format driver disk,
> checking for old");
> > - sprintf(file, "%s/rhdd-6.1", mntpt);
> > - if (access(file, R_OK)) {
> > - logMessage(ERROR, "can't find either driver disk
> identifier, bad "
> > - "driver disk");
> > - }
> > + logMessage(ERROR, "can't find driver disk identifier, bad
> "
> > + "driver disk");
> > + return LOADER_BACK;
> > }
> >
> > /* side effect: file is still mntpt/ddident */
> > stat(file, &sb);
> > if (!sb.st_size)
> > return LOADER_BACK;
> > + for (fnPtr = driverDiskFiles; *fnPtr; fnPtr++) {
> > + sprintf(file, "%s/%s/%s", mntpt, getProductArch(),
> *fnPtr);
> > + if (access(file, R_OK)) {
> > + logMessage(ERROR, "cannot find %s, bad driver disk",
> file);
> > + return LOADER_BACK;
> > + }
> > + }
> >
> > return LOADER_OK;
> > }
> > @@ -101,25 +247,20 @@ static void copyErrorFn (char *msg) {
> > /* this copies the contents of the driver disk to a ramdisk and
> loads
> > * the moduleinfo, etc. assumes a "valid" driver disk mounted at
> mntpt */
> > static int loadDriverDisk(struct loaderData_s *loaderData, char
> *mntpt) {
> > - moduleInfoSet modInfo = loaderData->modInfo;
> > - char file[200], dest[200];
> > + /* FIXME moduleInfoSet modInfo = loaderData->modInfo; */
> > + char file[200], dest[200], src[200];
> > char *title;
> > char *fwdir = NULL;
> > struct moduleBallLocation * location;
> > struct stat sb;
> > static int disknum = 0;
> > - int version = 1;
> > int fd, ret;
> >
> > - /* check for both versions */
> > - sprintf(file, "%s/rhdd", mntpt);
> > + /* check for new version */
> > + sprintf(file, "%s/rhdd3", mntpt);
> > if (access(file, R_OK)) {
> > - version = 0;
> > - sprintf(file, "%s/rhdd-6.1", mntpt);
> > - if (access(file, R_OK)) {
> > - /* this can't happen, we already verified it! */
> > - return LOADER_BACK;
> > - }
> > + /* this can't happen, we already verified it! */
> > + return LOADER_BACK;
> > }
> > stat(file, &sb);
> > title = malloc(sb.st_size + 1);
> > @@ -131,23 +272,35 @@ static int loadDriverDisk(struct loaderData_s
> *loaderData, char *mntpt) {
> > title[sb.st_size] = '';
> > close(fd);
> >
> > - sprintf(file, "/tmp/DD-%d", disknum);
> > + sprintf(file, DD_RPMS, disknum);
> > mkdirChain(file);
> > + mkdirChain(DD_MODULES);
> > + mkdirChain(DD_FIRMWARE);
> >
> > if (!FL_CMDLINE(flags)) {
> > startNewt();
> > winStatus(40, 3, _("Loading"), _("Reading driver disk"));
> > }
> >
> > - sprintf(dest, "/tmp/DD-%d", disknum);
> > - copyDirectory(mntpt, dest, copyWarnFn, copyErrorFn);
> > -
> > location = malloc(sizeof(struct moduleBallLocation));
> > location->title = strdup(title);
> > - location->version = version;
> >
> > - checked_asprintf(&location->path, "/tmp/DD-%d/modules.cgz",
> disknum);
> > - checked_asprintf(&fwdir, "/tmp/DD-%d/firmware", disknum);
> > + checked_asprintf(&location->path, DD_MODULES);
> > + checked_asprintf(&fwdir, DD_FIRMWARE);
> > +
> > + sprintf(dest, DD_RPMS, disknum);
> > + sprintf(src, "%s/rpms/%s", mntpt, getProductArch());
> > + copyDirectory(src, dest, copyWarnFn, copyErrorFn);
> > +
> > + /* unpack packages from dest into location->path */
> > + if(dlabelUnpackRPMDir(dest, DD_EXTRACTED)){
> > + /* TODO error handler */
> > + }
> > +
> > + /* run depmod to refresh modules db */
> > + if(system("depmod -a")){
> > + /* FIXME: depmod didn't run */
> > + }
> >
> > if (!access(fwdir, R_OK|X_OK)) {
> > add_fw_search_dir(loaderData, fwdir);
> > @@ -156,8 +309,11 @@ static int loadDriverDisk(struct loaderData_s
> *loaderData, char *mntpt) {
> > }
> > free(fwdir);
> >
> > - sprintf(file, "%s/modinfo", mntpt);
> > - readModuleInfo(file, modInfo, location, 1);
> > + /* TODO generate and read module info
> > + *
> > + * sprintf(file, "%s/modinfo", mntpt);
> > + * readModuleInfo(file, modInfo, location, 1);
> > + */
> >
> > if (!FL_CMDLINE(flags))
> > newtPopWindow();
> > @@ -623,3 +779,106 @@ static void getDDFromDev(struct loaderData_s *
> loaderData, char * dev) {
> > umount("/tmp/drivers");
> > unlink("/tmp/drivers");
> > }
>
> Given the opportunity, I'd like to see older code using static buffers
> and
> sprintf() replaced with checked_asprintf().
>
> > +
> > +/*
> > + * Utility functions to maintain linked-list of device names
> > + * */
> > +
> > +struct ddlist* ddlist_add(struct ddlist *list, const char* device)
> > +{
> > + struct ddlist* item;
> > +
> > + item = (struct ddlist*)malloc(sizeof(struct ddlist));
> > + if(item==NULL){
> > + return list;
> > + }
> > +
> > + item->device = strdup(device);
> > + item->next = list;
> > +
> > + return item;
> > +}
> > +
> > +int ddlist_free(struct ddlist *list)
> > +{
> > + struct ddlist *next;
> > + int count = 0;
> > +
> > + while(list!=NULL){
> > + next = list->next;
> > + free(list->device);
> > + free(list);
> > + list = next;
> > + count++;
> > + }
> > +
> > + return count;
> > +}
>
> Let's use glib's linked list routines. The GSList is usable here.
> http://library.gnome.org/devel/glib/stable/glib-Singly-Linked-Lists.html
>
> > +/*
> > + * Look for partition with specific label (part of #316481)
> > + */
> > +struct ddlist* findDriverDiskByLabel(void)
> > +{
> > + char *ddLabel = "OEMDRV";
> > + struct ddlist *ddDevice = NULL;
> > + blkid_cache bCache;
> > +
> > + int res;
> > + blkid_dev_iterate bIter;
> > + blkid_dev bDev;
> > +
> > + if(blkid_get_cache(&bCache, NULL)<0){
> > + logMessage(ERROR, _("Cannot initialize cache instance for
> blkid"));
> > + return NULL;
> > + }
> > + if((res = blkid_probe_all(bCache))<0){
> > + logMessage(ERROR, _("Cannot probe devices in blkid: %d"),
> res);
> > + return NULL;
> > + }
> > +
> > + bIter = blkid_dev_iterate_begin(bCache);
> > + blkid_dev_set_search(bIter, "LABEL", ddLabel);
> > + while((res = blkid_dev_next(bIter, &bDev))!=0){
> > + bDev = blkid_verify(bCache, bDev);
> > + if(!bDev)
> > + continue;
> > + logMessage(DEBUGLVL, _("Adding driver disc %s to the list
> of available DDs."), blkid_dev_devname(bDev));
> > + ddDevice = ddlist_add(ddDevice, blkid_dev_devname(bDev));
> > + /*blkid_free_dev(bDev); -- probably taken care of by the
> put cache call.. it is not exposed in the API */
> > + }
> > + blkid_dev_iterate_end(bIter);
> > +
> > + blkid_put_cache(bCache);
> > +
> > + return ddDevice;
> > +}
> > +
> > +int loadDriverDiskFromPartition(struct loaderData_s *loaderData,
> char* device)
> > +{
> > + int rc;
> > +
> > + logMessage(INFO, "trying to mount %s", device);
> > + if (doPwMount(device, "/tmp/drivers", "auto", "ro", NULL)) {
> > + logMessage(ERROR, _("Failed to mount driver disk."));
> > + return -1;
> > + }
> > +
> > + rc = verifyDriverDisk("/tmp/drivers");
> > + if (rc == LOADER_BACK) {
> > + logMessage(ERROR, _("Driver disk is invalid for this "
> > + "release of %s."), getProductName());
> > + umount("/tmp/drivers");
> > + return -2;
> > + }
> > +
> > + rc = loadDriverDisk(loaderData, "/tmp/drivers");
> > + umount("/tmp/drivers");
> > + if (rc == LOADER_BACK) {
> > + return -3;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > diff --git a/loader/driverdisk.h b/loader/driverdisk.h
> > index e6e919d..3e442fb 100644
> > --- a/loader/driverdisk.h
> > +++ b/loader/driverdisk.h
> > @@ -24,6 +24,11 @@
> > #include "modules.h"
> > #include "moduleinfo.h"
> >
> > +#define DD_RPMS "/tmp/DD-%d"
> > +#define DD_EXTRACTED "/tmp/DD"
> > +#define DD_MODULES "/tmp/DD/lib/modules"
> > +#define DD_FIRMWARE "/tmp/DD/lib/firmware"
> > +
> > extern char *ddFsTypes[];
> >
> > int loadDriverFromMedia(int class, struct loaderData_s *loaderData,
> > @@ -39,4 +44,19 @@ void useKickstartDD(struct loaderData_s *
> loaderData, int argc,
> >
> > void getDDFromSource(struct loaderData_s * loaderData, char * src);
> >
> > +int loadDriverDiskFromPartition(struct loaderData_s *loaderData,
> char* device);
> > +
> > +struct ddlist {
> > + char* device;
> > + struct ddlist* next;
> > +};
> > +
> > +struct ddlist* ddlist_add(struct ddlist *list, const char*
> device);
> > +int ddlist_free(struct ddlist *list);
> > +
> > +struct ddlist* findDriverDiskByLabel(void);
> > +
> > +int modprobeNormalmode();
> > +int modprobeDDmode();
> > +
> > #endif
> > diff --git a/loader/loader.c b/loader/loader.c
> > index cd3e178..3159513 100644
> > --- a/loader/loader.c
> > +++ b/loader/loader.c
> > @@ -959,6 +959,10 @@ static void parseCmdLineFlags(struct
> loaderData_s * loaderData,
> > else if (!strcasecmp(argv[i], "dd") ||
> > !strcasecmp(argv[i], "driverdisk"))
> > flags |= LOADER_FLAGS_MODDISK;
> > + else if (!strcasecmp(argv[i], "dlabel=on"))
> > + flags |= LOADER_FLAGS_AUTOMODDISK;
> > + else if (!strcasecmp(argv[i], "dlabel=off"))
> > + flags &= ~LOADER_FLAGS_AUTOMODDISK;
> > else if (!strcasecmp(argv[i], "rescue"))
> > flags |= LOADER_FLAGS_RESCUE;
> > else if (!strcasecmp(argv[i], "nopass"))
> > @@ -1785,6 +1789,7 @@ int main(int argc, char ** argv) {
> > struct loaderData_s loaderData;
> >
> > char *path;
> > + struct ddlist *dd, *dditer;
> >
> > gchar *cmdLine = NULL, *ksFile = NULL, *virtpcon = NULL;
> > gboolean testing = FALSE, mediacheck = FALSE;
> > @@ -1882,6 +1887,12 @@ int main(int argc, char ** argv) {
> > flags |= LOADER_FLAGS_NOSHELL;
> > #endif
> >
> > + /* XXX if RHEL, enable the AUTODD feature by default,
> > + * but we should come with more general way how to control this
> */
> > + if(!strncmp(getProductName(), "Red Hat", 7)){
> > + flags |= LOADER_FLAGS_AUTOMODDISK;
> > + }
> > +
> > openLog(FL_TESTING(flags));
> > if (!FL_TESTING(flags))
> > openlog("loader", 0, LOG_LOCAL0);
> > @@ -1946,6 +1957,26 @@ 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(FL_AUTOMODDISK(flags)){
> > + /* Load all autodetected DDs */
> > + logMessage(INFO, "Trying to detect vendor driver discs");
> > + dd = findDriverDiskByLabel();
> > + dditer = dd;
> > + while(dditer){
> > + if(loadDriverDiskFromPartition(&loaderData,
> dditer->device)){
> > + logMessage(ERROR, "Automatic driver disk loader
> failed for %s.", dditer->device);
> > + }
> > + else{
> > + logMessage(INFO, "Automatic driver disk loader
> succeeded for %s.", dditer->device);
> > + }
> > + dditer = dditer->next;
> > + }
> > + ddlist_free(dd);
> > + }
> > +
> > if (FL_MODDISK(flags)) {
> > startNewt();
> > loadDriverDisks(DEVICE_ANY, &loaderData);
> > @@ -1955,6 +1986,9 @@ int main(int argc, char ** argv) {
> > logMessage(INFO, "found /dd.img, loading drivers");
> > getDDFromSource(&loaderData, "path:/dd.img");
> > }
> > +
> > + /* 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
> > @@ -2139,6 +2173,9 @@ int main(int argc, char ** argv) {
> > tmparg++;
> > }
> >
> > + if (FL_AUTOMODDISK(flags))
> > + *argptr++ = "--dlabel";
> > +
> > if (FL_NOIPV4(flags))
> > *argptr++ = "--noipv4";
> >
> > diff --git a/loader/loader.h b/loader/loader.h
> > index ebf3766..ca6404f 100644
> > --- a/loader/loader.h
> > +++ b/loader/loader.h
> > @@ -70,6 +70,7 @@
> > #define LOADER_FLAGS_HAVE_CMSCONF (((uint64_t) 1) << 37)
> > #define LOADER_FLAGS_NOKILL (((uint64_t) 1) << 38)
> > #define LOADER_FLAGS_KICKSTART_SEND_SERIAL (((uint64_t) 1) << 39)
> > +#define LOADER_FLAGS_AUTOMODDISK (((uint64_t) 1) << 40)
> >
> > #define FL_TESTING(a) ((a) & LOADER_FLAGS_TESTING)
> > #define FL_TEXT(a) ((a) & LOADER_FLAGS_TEXT)
> > @@ -107,6 +108,7 @@
> > #define FL_HAVE_CMSCONF(a) ((a) & LOADER_FLAGS_HAVE_CMSCONF)
> > #define FL_NOKILL(a) ((a) & LOADER_FLAGS_NOKILL)
> > #define FL_KICKSTART_SEND_SERIAL(a) ((a) &
> LOADER_FLAGS_KICKSTART_SEND_SERIAL)
> > +#define FL_AUTOMODDISK(a) ((a) & LOADER_FLAGS_AUTOMODDISK)
> >
> > void startNewt(void);
> > void stopNewt(void);
> >
>
> Large items:
>
> - - Fix the coding style.
> - - Use glib's linked list rather than creating a new one.
> - - Avoid the use of static buffers. This is one thing I like about
> the
> convenience functions in glib. Almost all will handle dynamic
> memory
> allocation. I see no reason to not make use of that in loader.
>
> I've got comments for rpm handling in another email.
>
> - --
> David Cantrell <dcantrell@redhat.com>
> Red Hat / Honolulu, HI
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
>
> iEYEARECAAYFAksdZfsACgkQ5hsjjIy1VkkSmQCePhyqQ0MioU iUz5tmdI1X2TGC
> DwgAoKPhf1qCcR+ROTjptAVZnjdGPW9J
> =W8vv
> -----END PGP SIGNATURE-----
>
> _______________________________________________
> Anaconda-devel-list mailing list
> Anaconda-devel-list@redhat.com
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 12-08-2009, 08:25 AM
Martin Sivak
 
Default Backport the driver disc functionality from rhel5

And to the code comments. You kind of overlap with Peter so..

- dependency check function was removed
- string construction use checked_asprintf where possible
- linked lists use glib now

- the rest I will look at

But this is first time I see some of the codestyle rules. Anaconda has lot of codestyles in it's codebase and nobody ever told us which is the correct one. Indentation was my mistake, because I'm used to different setup and restored my old .vimrc.

Using glib for string handling is the same situation.. you have to start talking about this stuff _before_ we post patches or document it somewhere. All the rewrites delay testing a lot...

Martin

----- "David Cantrell" <dcantrell@redhat.com> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Apologies for duplicate comments, I'm just running through
> everything.
>
> On Wed, 2 Dec 2009, Martin Sivak wrote:
>
> > - updates the code to agree with current anaconda API
> > - adds dlabel functionality
> > - also adds some stub calls to set modprobe behaviour
>
> This comment improves things, but I'd really like to see more. A
> patch like
> this with 355 insertions deserves more of a writeup. I would like to
> see a
> detailed comment starting with a short one line description
> (preferably <=
> 76 chars) including the bug number, then paragraphs explaining what
> the patch
> is doing, what was changed, etc.
>
> Since multiple files were changed and multiple functions touched (and
> new
> ones added), I'd prefer to see explanations for that.
>
> > ---
> > loader/driverdisk.c | 333
> +++++++++++++++++++++++++++++++++++++++++++++------
> > loader/driverdisk.h | 20 +++
> > loader/loader.c | 37 ++++++
> > loader/loader.h | 2 +
> > 4 files changed, 355 insertions(+), 37 deletions(-)
> >
> > diff --git a/loader/driverdisk.c b/loader/driverdisk.c
> > index 5f8d43c..f718684 100644
> > --- a/loader/driverdisk.c
> > +++ b/loader/driverdisk.c
> > @@ -30,6 +30,12 @@
> > #include <unistd.h>
> > #include <glib.h>
> >
> > +#include <blkid/blkid.h>
> > +
> > +#include <glob.h>
> > +#include <rpm/rpmlib.h>
> > +#include <sys/utsname.h>
> > +
> > #include "copy.h"
> > #include "loader.h"
> > #include "log.h"
> > @@ -48,6 +54,8 @@
> > #include "nfsinstall.h"
> > #include "urlinstall.h"
> >
> > +#include "rpmextract.h"
> > +
> > #include "../isys/isys.h"
> > #include "../isys/imount.h"
> > #include "../isys/eddsupport.h"
> > @@ -55,37 +63,175 @@
> > /* boot flags */
> > extern uint64_t flags;
> >
> > -static char * driverDiskFiles[] = { "modinfo", "modules.dep",
> > - "modules.cgz", "modules.alias",
> NULL };
> > +/* modprobe DD mode */
> > +int modprobeDDmode()
> > +{
> > + FILE *f = fopen("/etc/depmod.d/ddmode.conf", "w");
> > + if(f){
> > + struct utsname unamedata;
> > + if(uname(&unamedata)){
> > + fprintf(f, " pblacklist /lib/modules
");
> > + }
> > + else{
> > + fprintf(f, " pblacklist /lib/modules/%s
",
> unamedata.release);
> > + }
> > +
> > + fclose(f);
> > + }
> > +
> > + return f==NULL;
> > +}
>
> I think Peter commented on the coding style. The issues here that I
> have a
> problem with are indentation, spacing, and the function declaration.
> We
> generally use 4 space indents, you've done 2. We also do stuff like:
>
> if (f) {
>
> and not:
>
> if(f){
>
> For the function declaration, I don't like functionName(). I'd
> prefer
> functionName(void).
>
> These are nits, I know, but this is all new code and I would much
> prefer that
> we maintain coding style consistency. A good guide is here:
>
> http://www.x.org/wiki/CodingStyle
>
> I'd say with the exception of function naming, we do most of the other
> stuff.
> I'm ok with function_names_like_this or functionNamesLikeThis.
>
> > +int modprobeNormalmode()
> > +{
> > + unlink("/etc/depmod.d/ddmode.conf");
> > +
> > + /* run depmod to refresh modules db */
> > + if(system("depmod -a")){
> > + /* FIXME: depmod didn't run */
> > + }
> > +
> > + return 0;
> > +}
>
> If unlink() fails or system() fails, that would be nice to know and
> why.
>
> > +/* RPM extraction dependency checks */
> > +int dlabelDeps(const char* depends, void *userptr)
> > +{
> > + logMessage(DEBUGLVL, "Depends on: %s
", depends);
> > + return 0;
> > +}
>
> I assume this is a stub and will contain more later?
>
> > +int dlabelProvides(const char* dep, void *userptr)
> > +{
> > + char *kernelver = (char*)userptr;
> > +
> > + logMessage(DEBUGLVL, "Provides: %s
", dep);
> > +
> > + return strcmp(dep, kernelver);
> > +}
>
> Will this contain more at some point?
>
> > +int dlabelFilter(const char* name, struct stat *fstat, void
> *userptr)
> > +{
> > + int l = strlen(name);
> > +
> > + logMessage(DEBUGLVL, "Unpacking %s (%lluB)
", name,
> fstat->st_size);
> > +
> > + /* we want firmware files */
> > + if(!strncmp("lib/firmware/", name, 13)) return 0;
> > +
> > + if(l<3) return 1;
> > + l-=3;
> > +
> > + /* and we want only .ko files */
> > + if(strncmp(".ko", name+l, 3)) return 1;
> > +
> > + /* TODO we are unpacking kernel module, read it's description */
> > +
> > + return 0;
> > +}
> > +
> > +char* moduleDescription(const char* modulePath)
> > +{
> > + char *command = rstrscat(NULL, "modinfo --description '",
> modulePath, "'", NULL);
> > + FILE *f = popen(command, "r");
> > + if(f==NULL) return NULL;
> > +
> > + char *description = malloc(sizeof(char)*256);
> > + int size = fread(description, 1, 255, f);
> > + if(size==0){
> > + free(description);
> > + return NULL;
> > + }
> > + description[size-1]=0; /* strip the trailing newline */
> > + pclose(f);
> > +
> > + free(command);
> > + return description;
> > +}
>
> Rather than the malloc() and fread() here, I'd like to see us make use
> of more
> convenience functions from glib because we don't then have to deal
> with our own
> memory management or static buffers. The g_spawn_sync() function
> would work
> well here. It can run modinfo and capture stdout and stderr. Here is
> a
> verbose standalone example showing how to get the module description
> via
> modinfo with glib functions (this is also meant to demonstrate some of
> the
> other stuff that glib can do):
>
> - ------
> /*
> * use modinfo to get kernel module description, report errors
> * overly verbose example demonstrating glib convenience functions
> */
>
> /* gcc -O0 -g -Wall $(pkg-config --cflags glib-2.0) getModDesc.c
> * $(pkg-config --libs glib-2.0)
> */
>
> /* dcantrell@redhat.com */
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <glib.h>
>
> int main(int argc, char **argv) {
> GString *tmp = g_string_new("modinfo -F description ");
> gchar **modinfoArgs = NULL;
> GSpawnFlags flags = G_SPAWN_SEARCH_PATH;
> gchar *output = NULL;
> gchar *errors = NULL;
> gint status = 0;
> GError *e = NULL;
>
> if (argc != 2) {
> fprintf(stderr, "Please specify one module file by full
> path.
");
> return EXIT_FAILURE;
> }
>
> /* append the module path to our command, split in to argument
> vector */
> g_string_append_printf(tmp, argv[1]);
> modinfoArgs = g_strsplit(tmp->str, " ", 0);
> g_string_free(tmp, TRUE);
>
> if (!modinfoArgs) {
> fprintf(stderr, "modinfoArgs split failure
");
> return EXIT_FAILURE;
> }
>
> /* run modinfo, get description, report errors on failure */
> if (!g_spawn_sync(NULL, modinfoArgs, NULL, flags, NULL, NULL,
> &output, &errors, &status, &e)) {
> fprintf(stderr, "Error getting module description: %s
",
> e->message);
> g_error_free(e);
>
> if (errors) {
> fprintf(stderr, "modinfo error: %s
", errors);
> }
>
> return EXIT_FAILURE;
> }
>
> /* display the module description */
> if (output) {
> /* we do not want trailing newlines */
> output = g_strchomp(output);
>
> printf("%s
", output);
> }
>
> return EXIT_SUCCESS;
> }
> - ------
>
> I would like to avoid static buffers when we can.
>
> > +int globErrFunc(const char *epath, int eerrno)
> > +{
> > + return 0;
> > +}
>
> I see you're passing this to glob to toss errors, but according to the
> glob(3)
> man page, you could pass NULL to glob() and get the same effect.
>
> > +int dlabelUnpackRPMDir(char* rpmdir, char* destination)
> > +{
> > + char *kernelver;
> > + struct utsname unamedata;
> > + char *oldcwd;
> > + char *globpattern;
> > + int rc;
> > +
> > + /* get current working directory */
> > + oldcwd = getcwd(NULL, 0);
> > +
> > + /* set the cwd to destination */
> > + if(chdir(destination)){
> > + free(oldcwd);
> > + return 1;
> > + }
> > +
> > + /* get running kernel version */
> > + if(uname(&unamedata)){
> > + kernelver = rstrscat(NULL, "kernel-modules-", "unknown",
> NULL);
> > + }
> > + else{
> > + kernelver = rstrscat(NULL, "kernel-modules-",
> unamedata.release, NULL);
> > + }
> > + logMessage(DEBUGLVL, "Kernel version: %s
", kernelver);
> > +
> > + globpattern = rstrscat(NULL, rpmdir, "/*.rpm", NULL);
> > + glob_t globres;
> > + char** globitem;
> > + if(!glob(globpattern, GLOB_NOSORT|GLOB_NOESCAPE, globErrFunc,
> &globres)){
> > + /* iterate over all rpm files */
> > + globitem = globres.gl_pathv;
> > + while(globres.gl_pathc>0 && globitem!=NULL){
> > + explodeRPM(*globitem, dlabelFilter, dlabelProvides,
> dlabelDeps, kernelver);
> > + }
> > + globfree(&globres);
> > + /* end of iteration */
> > + }
> > + free(globpattern);
> > +
> > + /* restore CWD */
> > + if(chdir(oldcwd)){
> > + /* FIXME: too bad.. */
> > + }
> > +
> > + /* cleanup */
> > + free(kernelver);
> > + free(oldcwd);
> > + return rc;
> > +}
>
> See email reply to your first message in this thread for my rpm
> handling
> comments.
>
> > +static char * driverDiskFiles[] = { "repodata", NULL };
> >
> > static int verifyDriverDisk(char *mntpt) {
> > char ** fnPtr;
> > char file[200];
> > struct stat sb;
> >
> > - for (fnPtr = driverDiskFiles; *fnPtr; fnPtr++) {
> > - sprintf(file, "%s/%s", mntpt, *fnPtr);
> > - if (access(file, R_OK)) {
> > - logMessage(ERROR, "cannot find %s, bad driver disk",
> file);
> > - return LOADER_BACK;
> > - }
> > - }
> > -
> > - /* check for both versions */
> > + /* check for dd descriptor */
> > sprintf(file, "%s/rhdd", mntpt);
> > if (access(file, R_OK)) {
> > - logMessage(DEBUGLVL, "not a new format driver disk,
> checking for old");
> > - sprintf(file, "%s/rhdd-6.1", mntpt);
> > - if (access(file, R_OK)) {
> > - logMessage(ERROR, "can't find either driver disk
> identifier, bad "
> > - "driver disk");
> > - }
> > + logMessage(ERROR, "can't find driver disk identifier, bad
> "
> > + "driver disk");
> > + return LOADER_BACK;
> > }
> >
> > /* side effect: file is still mntpt/ddident */
> > stat(file, &sb);
> > if (!sb.st_size)
> > return LOADER_BACK;
> > + for (fnPtr = driverDiskFiles; *fnPtr; fnPtr++) {
> > + sprintf(file, "%s/%s/%s", mntpt, getProductArch(),
> *fnPtr);
> > + if (access(file, R_OK)) {
> > + logMessage(ERROR, "cannot find %s, bad driver disk",
> file);
> > + return LOADER_BACK;
> > + }
> > + }
> >
> > return LOADER_OK;
> > }
> > @@ -101,25 +247,20 @@ static void copyErrorFn (char *msg) {
> > /* this copies the contents of the driver disk to a ramdisk and
> loads
> > * the moduleinfo, etc. assumes a "valid" driver disk mounted at
> mntpt */
> > static int loadDriverDisk(struct loaderData_s *loaderData, char
> *mntpt) {
> > - moduleInfoSet modInfo = loaderData->modInfo;
> > - char file[200], dest[200];
> > + /* FIXME moduleInfoSet modInfo = loaderData->modInfo; */
> > + char file[200], dest[200], src[200];
> > char *title;
> > char *fwdir = NULL;
> > struct moduleBallLocation * location;
> > struct stat sb;
> > static int disknum = 0;
> > - int version = 1;
> > int fd, ret;
> >
> > - /* check for both versions */
> > - sprintf(file, "%s/rhdd", mntpt);
> > + /* check for new version */
> > + sprintf(file, "%s/rhdd3", mntpt);
> > if (access(file, R_OK)) {
> > - version = 0;
> > - sprintf(file, "%s/rhdd-6.1", mntpt);
> > - if (access(file, R_OK)) {
> > - /* this can't happen, we already verified it! */
> > - return LOADER_BACK;
> > - }
> > + /* this can't happen, we already verified it! */
> > + return LOADER_BACK;
> > }
> > stat(file, &sb);
> > title = malloc(sb.st_size + 1);
> > @@ -131,23 +272,35 @@ static int loadDriverDisk(struct loaderData_s
> *loaderData, char *mntpt) {
> > title[sb.st_size] = '';
> > close(fd);
> >
> > - sprintf(file, "/tmp/DD-%d", disknum);
> > + sprintf(file, DD_RPMS, disknum);
> > mkdirChain(file);
> > + mkdirChain(DD_MODULES);
> > + mkdirChain(DD_FIRMWARE);
> >
> > if (!FL_CMDLINE(flags)) {
> > startNewt();
> > winStatus(40, 3, _("Loading"), _("Reading driver disk"));
> > }
> >
> > - sprintf(dest, "/tmp/DD-%d", disknum);
> > - copyDirectory(mntpt, dest, copyWarnFn, copyErrorFn);
> > -
> > location = malloc(sizeof(struct moduleBallLocation));
> > location->title = strdup(title);
> > - location->version = version;
> >
> > - checked_asprintf(&location->path, "/tmp/DD-%d/modules.cgz",
> disknum);
> > - checked_asprintf(&fwdir, "/tmp/DD-%d/firmware", disknum);
> > + checked_asprintf(&location->path, DD_MODULES);
> > + checked_asprintf(&fwdir, DD_FIRMWARE);
> > +
> > + sprintf(dest, DD_RPMS, disknum);
> > + sprintf(src, "%s/rpms/%s", mntpt, getProductArch());
> > + copyDirectory(src, dest, copyWarnFn, copyErrorFn);
> > +
> > + /* unpack packages from dest into location->path */
> > + if(dlabelUnpackRPMDir(dest, DD_EXTRACTED)){
> > + /* TODO error handler */
> > + }
> > +
> > + /* run depmod to refresh modules db */
> > + if(system("depmod -a")){
> > + /* FIXME: depmod didn't run */
> > + }
> >
> > if (!access(fwdir, R_OK|X_OK)) {
> > add_fw_search_dir(loaderData, fwdir);
> > @@ -156,8 +309,11 @@ static int loadDriverDisk(struct loaderData_s
> *loaderData, char *mntpt) {
> > }
> > free(fwdir);
> >
> > - sprintf(file, "%s/modinfo", mntpt);
> > - readModuleInfo(file, modInfo, location, 1);
> > + /* TODO generate and read module info
> > + *
> > + * sprintf(file, "%s/modinfo", mntpt);
> > + * readModuleInfo(file, modInfo, location, 1);
> > + */
> >
> > if (!FL_CMDLINE(flags))
> > newtPopWindow();
> > @@ -623,3 +779,106 @@ static void getDDFromDev(struct loaderData_s *
> loaderData, char * dev) {
> > umount("/tmp/drivers");
> > unlink("/tmp/drivers");
> > }
>
> Given the opportunity, I'd like to see older code using static buffers
> and
> sprintf() replaced with checked_asprintf().
>
> > +
> > +/*
> > + * Utility functions to maintain linked-list of device names
> > + * */
> > +
> > +struct ddlist* ddlist_add(struct ddlist *list, const char* device)
> > +{
> > + struct ddlist* item;
> > +
> > + item = (struct ddlist*)malloc(sizeof(struct ddlist));
> > + if(item==NULL){
> > + return list;
> > + }
> > +
> > + item->device = strdup(device);
> > + item->next = list;
> > +
> > + return item;
> > +}
> > +
> > +int ddlist_free(struct ddlist *list)
> > +{
> > + struct ddlist *next;
> > + int count = 0;
> > +
> > + while(list!=NULL){
> > + next = list->next;
> > + free(list->device);
> > + free(list);
> > + list = next;
> > + count++;
> > + }
> > +
> > + return count;
> > +}
>
> Let's use glib's linked list routines. The GSList is usable here.
> http://library.gnome.org/devel/glib/stable/glib-Singly-Linked-Lists.html
>
> > +/*
> > + * Look for partition with specific label (part of #316481)
> > + */
> > +struct ddlist* findDriverDiskByLabel(void)
> > +{
> > + char *ddLabel = "OEMDRV";
> > + struct ddlist *ddDevice = NULL;
> > + blkid_cache bCache;
> > +
> > + int res;
> > + blkid_dev_iterate bIter;
> > + blkid_dev bDev;
> > +
> > + if(blkid_get_cache(&bCache, NULL)<0){
> > + logMessage(ERROR, _("Cannot initialize cache instance for
> blkid"));
> > + return NULL;
> > + }
> > + if((res = blkid_probe_all(bCache))<0){
> > + logMessage(ERROR, _("Cannot probe devices in blkid: %d"),
> res);
> > + return NULL;
> > + }
> > +
> > + bIter = blkid_dev_iterate_begin(bCache);
> > + blkid_dev_set_search(bIter, "LABEL", ddLabel);
> > + while((res = blkid_dev_next(bIter, &bDev))!=0){
> > + bDev = blkid_verify(bCache, bDev);
> > + if(!bDev)
> > + continue;
> > + logMessage(DEBUGLVL, _("Adding driver disc %s to the list
> of available DDs."), blkid_dev_devname(bDev));
> > + ddDevice = ddlist_add(ddDevice, blkid_dev_devname(bDev));
> > + /*blkid_free_dev(bDev); -- probably taken care of by the
> put cache call.. it is not exposed in the API */
> > + }
> > + blkid_dev_iterate_end(bIter);
> > +
> > + blkid_put_cache(bCache);
> > +
> > + return ddDevice;
> > +}
> > +
> > +int loadDriverDiskFromPartition(struct loaderData_s *loaderData,
> char* device)
> > +{
> > + int rc;
> > +
> > + logMessage(INFO, "trying to mount %s", device);
> > + if (doPwMount(device, "/tmp/drivers", "auto", "ro", NULL)) {
> > + logMessage(ERROR, _("Failed to mount driver disk."));
> > + return -1;
> > + }
> > +
> > + rc = verifyDriverDisk("/tmp/drivers");
> > + if (rc == LOADER_BACK) {
> > + logMessage(ERROR, _("Driver disk is invalid for this "
> > + "release of %s."), getProductName());
> > + umount("/tmp/drivers");
> > + return -2;
> > + }
> > +
> > + rc = loadDriverDisk(loaderData, "/tmp/drivers");
> > + umount("/tmp/drivers");
> > + if (rc == LOADER_BACK) {
> > + return -3;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > diff --git a/loader/driverdisk.h b/loader/driverdisk.h
> > index e6e919d..3e442fb 100644
> > --- a/loader/driverdisk.h
> > +++ b/loader/driverdisk.h
> > @@ -24,6 +24,11 @@
> > #include "modules.h"
> > #include "moduleinfo.h"
> >
> > +#define DD_RPMS "/tmp/DD-%d"
> > +#define DD_EXTRACTED "/tmp/DD"
> > +#define DD_MODULES "/tmp/DD/lib/modules"
> > +#define DD_FIRMWARE "/tmp/DD/lib/firmware"
> > +
> > extern char *ddFsTypes[];
> >
> > int loadDriverFromMedia(int class, struct loaderData_s *loaderData,
> > @@ -39,4 +44,19 @@ void useKickstartDD(struct loaderData_s *
> loaderData, int argc,
> >
> > void getDDFromSource(struct loaderData_s * loaderData, char * src);
> >
> > +int loadDriverDiskFromPartition(struct loaderData_s *loaderData,
> char* device);
> > +
> > +struct ddlist {
> > + char* device;
> > + struct ddlist* next;
> > +};
> > +
> > +struct ddlist* ddlist_add(struct ddlist *list, const char*
> device);
> > +int ddlist_free(struct ddlist *list);
> > +
> > +struct ddlist* findDriverDiskByLabel(void);
> > +
> > +int modprobeNormalmode();
> > +int modprobeDDmode();
> > +
> > #endif
> > diff --git a/loader/loader.c b/loader/loader.c
> > index cd3e178..3159513 100644
> > --- a/loader/loader.c
> > +++ b/loader/loader.c
> > @@ -959,6 +959,10 @@ static void parseCmdLineFlags(struct
> loaderData_s * loaderData,
> > else if (!strcasecmp(argv[i], "dd") ||
> > !strcasecmp(argv[i], "driverdisk"))
> > flags |= LOADER_FLAGS_MODDISK;
> > + else if (!strcasecmp(argv[i], "dlabel=on"))
> > + flags |= LOADER_FLAGS_AUTOMODDISK;
> > + else if (!strcasecmp(argv[i], "dlabel=off"))
> > + flags &= ~LOADER_FLAGS_AUTOMODDISK;
> > else if (!strcasecmp(argv[i], "rescue"))
> > flags |= LOADER_FLAGS_RESCUE;
> > else if (!strcasecmp(argv[i], "nopass"))
> > @@ -1785,6 +1789,7 @@ int main(int argc, char ** argv) {
> > struct loaderData_s loaderData;
> >
> > char *path;
> > + struct ddlist *dd, *dditer;
> >
> > gchar *cmdLine = NULL, *ksFile = NULL, *virtpcon = NULL;
> > gboolean testing = FALSE, mediacheck = FALSE;
> > @@ -1882,6 +1887,12 @@ int main(int argc, char ** argv) {
> > flags |= LOADER_FLAGS_NOSHELL;
> > #endif
> >
> > + /* XXX if RHEL, enable the AUTODD feature by default,
> > + * but we should come with more general way how to control this
> */
> > + if(!strncmp(getProductName(), "Red Hat", 7)){
> > + flags |= LOADER_FLAGS_AUTOMODDISK;
> > + }
> > +
> > openLog(FL_TESTING(flags));
> > if (!FL_TESTING(flags))
> > openlog("loader", 0, LOG_LOCAL0);
> > @@ -1946,6 +1957,26 @@ 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(FL_AUTOMODDISK(flags)){
> > + /* Load all autodetected DDs */
> > + logMessage(INFO, "Trying to detect vendor driver discs");
> > + dd = findDriverDiskByLabel();
> > + dditer = dd;
> > + while(dditer){
> > + if(loadDriverDiskFromPartition(&loaderData,
> dditer->device)){
> > + logMessage(ERROR, "Automatic driver disk loader
> failed for %s.", dditer->device);
> > + }
> > + else{
> > + logMessage(INFO, "Automatic driver disk loader
> succeeded for %s.", dditer->device);
> > + }
> > + dditer = dditer->next;
> > + }
> > + ddlist_free(dd);
> > + }
> > +
> > if (FL_MODDISK(flags)) {
> > startNewt();
> > loadDriverDisks(DEVICE_ANY, &loaderData);
> > @@ -1955,6 +1986,9 @@ int main(int argc, char ** argv) {
> > logMessage(INFO, "found /dd.img, loading drivers");
> > getDDFromSource(&loaderData, "path:/dd.img");
> > }
> > +
> > + /* 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
> > @@ -2139,6 +2173,9 @@ int main(int argc, char ** argv) {
> > tmparg++;
> > }
> >
> > + if (FL_AUTOMODDISK(flags))
> > + *argptr++ = "--dlabel";
> > +
> > if (FL_NOIPV4(flags))
> > *argptr++ = "--noipv4";
> >
> > diff --git a/loader/loader.h b/loader/loader.h
> > index ebf3766..ca6404f 100644
> > --- a/loader/loader.h
> > +++ b/loader/loader.h
> > @@ -70,6 +70,7 @@
> > #define LOADER_FLAGS_HAVE_CMSCONF (((uint64_t) 1) << 37)
> > #define LOADER_FLAGS_NOKILL (((uint64_t) 1) << 38)
> > #define LOADER_FLAGS_KICKSTART_SEND_SERIAL (((uint64_t) 1) << 39)
> > +#define LOADER_FLAGS_AUTOMODDISK (((uint64_t) 1) << 40)
> >
> > #define FL_TESTING(a) ((a) & LOADER_FLAGS_TESTING)
> > #define FL_TEXT(a) ((a) & LOADER_FLAGS_TEXT)
> > @@ -107,6 +108,7 @@
> > #define FL_HAVE_CMSCONF(a) ((a) & LOADER_FLAGS_HAVE_CMSCONF)
> > #define FL_NOKILL(a) ((a) & LOADER_FLAGS_NOKILL)
> > #define FL_KICKSTART_SEND_SERIAL(a) ((a) &
> LOADER_FLAGS_KICKSTART_SEND_SERIAL)
> > +#define FL_AUTOMODDISK(a) ((a) & LOADER_FLAGS_AUTOMODDISK)
> >
> > void startNewt(void);
> > void stopNewt(void);
> >
>
> Large items:
>
> - - Fix the coding style.
> - - Use glib's linked list rather than creating a new one.
> - - Avoid the use of static buffers. This is one thing I like about
> the
> convenience functions in glib. Almost all will handle dynamic
> memory
> allocation. I see no reason to not make use of that in loader.
>
> I've got comments for rpm handling in another email.
>
> - --
> David Cantrell <dcantrell@redhat.com>
> Red Hat / Honolulu, HI
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
>
> iEYEARECAAYFAksdZfsACgkQ5hsjjIy1VkkSmQCePhyqQ0MioU iUz5tmdI1X2TGC
> DwgAoKPhf1qCcR+ROTjptAVZnjdGPW9J
> =W8vv
> -----END PGP SIGNATURE-----
>
> _______________________________________________
> Anaconda-devel-list mailing list
> Anaconda-devel-list@redhat.com
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 12-08-2009, 04:48 PM
David Cantrell
 
Default Backport the driver disc functionality from rhel5

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

On Tue, 8 Dec 2009, Martin Sivak wrote:


And to the code comments. You kind of overlap with Peter so..

- dependency check function was removed
- string construction use checked_asprintf where possible
- linked lists use glib now

- the rest I will look at

But this is first time I see some of the codestyle rules. Anaconda has lot of codestyles in it's codebase and nobody ever told us which is the correct one. Indentation was my mistake, because I'm used to different setup and restored my old .vimrc.


We don't really have a rule other than keep it similar to what's already
there.


Using glib for string handling is the same situation.. you have to start talking about this stuff _before_ we post patches or document it somewhere. All the rewrites delay testing a lot...


I would bring up those things if large items like the driver disk were
discussed on the list while in their planning stages (correct me if I'm wrong,
but I can't find any threads related to this feature that were sent out within
the past couple of months except for the patches to review). If you post
patches for comments after you've written stuff on your own, this will
probably happen. It happens to me a lot, but that's sort of the nature of
working on this stuff.

Regarding glib specifically, I figured people would have seen my recent
patches to loader as picked up on those as examples of how to improve our
string handling. When it comes down to it, I don't really care if we use glib
or not. What I really want to do away with are static buffers and things like
linked list code. glib just happens to be the one that I like to use because
I think the API is pretty simple and the online API reference is easy to read
through (also, we already have glib in the initrd). But most importantly:

- I want us to avoid static buffers.
- I want us to use existing libraries when we can and not reinvent things.



Martin

----- "David Cantrell" <dcantrell@redhat.com> wrote:


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

Apologies for duplicate comments, I'm just running through
everything.

On Wed, 2 Dec 2009, Martin Sivak wrote:


- updates the code to agree with current anaconda API
- adds dlabel functionality
- also adds some stub calls to set modprobe behaviour


This comment improves things, but I'd really like to see more. A
patch like
this with 355 insertions deserves more of a writeup. I would like to
see a
detailed comment starting with a short one line description
(preferably <=
76 chars) including the bug number, then paragraphs explaining what
the patch
is doing, what was changed, etc.

Since multiple files were changed and multiple functions touched (and
new
ones added), I'd prefer to see explanations for that.


---
loader/driverdisk.c | 333

+++++++++++++++++++++++++++++++++++++++++++++------

loader/driverdisk.h | 20 +++
loader/loader.c | 37 ++++++
loader/loader.h | 2 +
4 files changed, 355 insertions(+), 37 deletions(-)

diff --git a/loader/driverdisk.c b/loader/driverdisk.c
index 5f8d43c..f718684 100644
--- a/loader/driverdisk.c
+++ b/loader/driverdisk.c
@@ -30,6 +30,12 @@
#include <unistd.h>
#include <glib.h>

+#include <blkid/blkid.h>
+
+#include <glob.h>
+#include <rpm/rpmlib.h>
+#include <sys/utsname.h>
+
#include "copy.h"
#include "loader.h"
#include "log.h"
@@ -48,6 +54,8 @@
#include "nfsinstall.h"
#include "urlinstall.h"

+#include "rpmextract.h"
+
#include "../isys/isys.h"
#include "../isys/imount.h"
#include "../isys/eddsupport.h"
@@ -55,37 +63,175 @@
/* boot flags */
extern uint64_t flags;

-static char * driverDiskFiles[] = { "modinfo", "modules.dep",
- "modules.cgz", "modules.alias",

NULL };

+/* modprobe DD mode */
+int modprobeDDmode()
+{
+ FILE *f = fopen("/etc/depmod.d/ddmode.conf", "w");
+ if(f){
+ struct utsname unamedata;
+ if(uname(&unamedata)){
+ fprintf(f, " pblacklist /lib/modules
");
+ }
+ else{
+ fprintf(f, " pblacklist /lib/modules/%s
",

unamedata.release);

+ }
+
+ fclose(f);
+ }
+
+ return f==NULL;
+}


I think Peter commented on the coding style. The issues here that I
have a
problem with are indentation, spacing, and the function declaration.
We
generally use 4 space indents, you've done 2. We also do stuff like:

if (f) {

and not:

if(f){

For the function declaration, I don't like functionName(). I'd
prefer
functionName(void).

These are nits, I know, but this is all new code and I would much
prefer that
we maintain coding style consistency. A good guide is here:

http://www.x.org/wiki/CodingStyle

I'd say with the exception of function naming, we do most of the other
stuff.
I'm ok with function_names_like_this or functionNamesLikeThis.


+int modprobeNormalmode()
+{
+ unlink("/etc/depmod.d/ddmode.conf");
+
+ /* run depmod to refresh modules db */
+ if(system("depmod -a")){
+ /* FIXME: depmod didn't run */
+ }
+
+ return 0;
+}


If unlink() fails or system() fails, that would be nice to know and
why.


+/* RPM extraction dependency checks */
+int dlabelDeps(const char* depends, void *userptr)
+{
+ logMessage(DEBUGLVL, "Depends on: %s
", depends);
+ return 0;
+}


I assume this is a stub and will contain more later?


+int dlabelProvides(const char* dep, void *userptr)
+{
+ char *kernelver = (char*)userptr;
+
+ logMessage(DEBUGLVL, "Provides: %s
", dep);
+
+ return strcmp(dep, kernelver);
+}


Will this contain more at some point?


+int dlabelFilter(const char* name, struct stat *fstat, void

*userptr)

+{
+ int l = strlen(name);
+
+ logMessage(DEBUGLVL, "Unpacking %s (%lluB)
", name,

fstat->st_size);

+
+ /* we want firmware files */
+ if(!strncmp("lib/firmware/", name, 13)) return 0;
+
+ if(l<3) return 1;
+ l-=3;
+
+ /* and we want only .ko files */
+ if(strncmp(".ko", name+l, 3)) return 1;
+
+ /* TODO we are unpacking kernel module, read it's description */
+
+ return 0;
+}
+
+char* moduleDescription(const char* modulePath)
+{
+ char *command = rstrscat(NULL, "modinfo --description '",

modulePath, "'", NULL);

+ FILE *f = popen(command, "r");
+ if(f==NULL) return NULL;
+
+ char *description = malloc(sizeof(char)*256);
+ int size = fread(description, 1, 255, f);
+ if(size==0){
+ free(description);
+ return NULL;
+ }
+ description[size-1]=0; /* strip the trailing newline */
+ pclose(f);
+
+ free(command);
+ return description;
+}


Rather than the malloc() and fread() here, I'd like to see us make use
of more
convenience functions from glib because we don't then have to deal
with our own
memory management or static buffers. The g_spawn_sync() function
would work
well here. It can run modinfo and capture stdout and stderr. Here is
a
verbose standalone example showing how to get the module description
via
modinfo with glib functions (this is also meant to demonstrate some of
the
other stuff that glib can do):

- ------
/*
* use modinfo to get kernel module description, report errors
* overly verbose example demonstrating glib convenience functions
*/

/* gcc -O0 -g -Wall $(pkg-config --cflags glib-2.0) getModDesc.c
* $(pkg-config --libs glib-2.0)
*/

/* dcantrell@redhat.com */

#include <stdio.h>
#include <stdlib.h>
#include <glib.h>

int main(int argc, char **argv) {
GString *tmp = g_string_new("modinfo -F description ");
gchar **modinfoArgs = NULL;
GSpawnFlags flags = G_SPAWN_SEARCH_PATH;
gchar *output = NULL;
gchar *errors = NULL;
gint status = 0;
GError *e = NULL;

if (argc != 2) {
fprintf(stderr, "Please specify one module file by full
path.
");
return EXIT_FAILURE;
}

/* append the module path to our command, split in to argument
vector */
g_string_append_printf(tmp, argv[1]);
modinfoArgs = g_strsplit(tmp->str, " ", 0);
g_string_free(tmp, TRUE);

if (!modinfoArgs) {
fprintf(stderr, "modinfoArgs split failure
");
return EXIT_FAILURE;
}

/* run modinfo, get description, report errors on failure */
if (!g_spawn_sync(NULL, modinfoArgs, NULL, flags, NULL, NULL,
&output, &errors, &status, &e)) {
fprintf(stderr, "Error getting module description: %s
",
e->message);
g_error_free(e);

if (errors) {
fprintf(stderr, "modinfo error: %s
", errors);
}

return EXIT_FAILURE;
}

/* display the module description */
if (output) {
/* we do not want trailing newlines */
output = g_strchomp(output);

printf("%s
", output);
}

return EXIT_SUCCESS;
}
- ------

I would like to avoid static buffers when we can.


+int globErrFunc(const char *epath, int eerrno)
+{
+ return 0;
+}


I see you're passing this to glob to toss errors, but according to the
glob(3)
man page, you could pass NULL to glob() and get the same effect.


+int dlabelUnpackRPMDir(char* rpmdir, char* destination)
+{
+ char *kernelver;
+ struct utsname unamedata;
+ char *oldcwd;
+ char *globpattern;
+ int rc;
+
+ /* get current working directory */
+ oldcwd = getcwd(NULL, 0);
+
+ /* set the cwd to destination */
+ if(chdir(destination)){
+ free(oldcwd);
+ return 1;
+ }
+
+ /* get running kernel version */
+ if(uname(&unamedata)){
+ kernelver = rstrscat(NULL, "kernel-modules-", "unknown",

NULL);

+ }
+ else{
+ kernelver = rstrscat(NULL, "kernel-modules-",

unamedata.release, NULL);

+ }
+ logMessage(DEBUGLVL, "Kernel version: %s
", kernelver);
+
+ globpattern = rstrscat(NULL, rpmdir, "/*.rpm", NULL);
+ glob_t globres;
+ char** globitem;
+ if(!glob(globpattern, GLOB_NOSORT|GLOB_NOESCAPE, globErrFunc,

&globres)){

+ /* iterate over all rpm files */
+ globitem = globres.gl_pathv;
+ while(globres.gl_pathc>0 && globitem!=NULL){
+ explodeRPM(*globitem, dlabelFilter, dlabelProvides,

dlabelDeps, kernelver);

+ }
+ globfree(&globres);
+ /* end of iteration */
+ }
+ free(globpattern);
+
+ /* restore CWD */
+ if(chdir(oldcwd)){
+ /* FIXME: too bad.. */
+ }
+
+ /* cleanup */
+ free(kernelver);
+ free(oldcwd);
+ return rc;
+}


See email reply to your first message in this thread for my rpm
handling
comments.


+static char * driverDiskFiles[] = { "repodata", NULL };

static int verifyDriverDisk(char *mntpt) {
char ** fnPtr;
char file[200];
struct stat sb;

- for (fnPtr = driverDiskFiles; *fnPtr; fnPtr++) {
- sprintf(file, "%s/%s", mntpt, *fnPtr);
- if (access(file, R_OK)) {
- logMessage(ERROR, "cannot find %s, bad driver disk",

file);

- return LOADER_BACK;
- }
- }
-
- /* check for both versions */
+ /* check for dd descriptor */
sprintf(file, "%s/rhdd", mntpt);
if (access(file, R_OK)) {
- logMessage(DEBUGLVL, "not a new format driver disk,

checking for old");

- sprintf(file, "%s/rhdd-6.1", mntpt);
- if (access(file, R_OK)) {
- logMessage(ERROR, "can't find either driver disk

identifier, bad "

- "driver disk");
- }
+ logMessage(ERROR, "can't find driver disk identifier, bad

"

+ "driver disk");
+ return LOADER_BACK;
}

/* side effect: file is still mntpt/ddident */
stat(file, &sb);
if (!sb.st_size)
return LOADER_BACK;
+ for (fnPtr = driverDiskFiles; *fnPtr; fnPtr++) {
+ sprintf(file, "%s/%s/%s", mntpt, getProductArch(),

*fnPtr);

+ if (access(file, R_OK)) {
+ logMessage(ERROR, "cannot find %s, bad driver disk",

file);

+ return LOADER_BACK;
+ }
+ }

return LOADER_OK;
}
@@ -101,25 +247,20 @@ static void copyErrorFn (char *msg) {
/* this copies the contents of the driver disk to a ramdisk and

loads

* the moduleinfo, etc. assumes a "valid" driver disk mounted at

mntpt */

static int loadDriverDisk(struct loaderData_s *loaderData, char

*mntpt) {

- moduleInfoSet modInfo = loaderData->modInfo;
- char file[200], dest[200];
+ /* FIXME moduleInfoSet modInfo = loaderData->modInfo; */
+ char file[200], dest[200], src[200];
char *title;
char *fwdir = NULL;
struct moduleBallLocation * location;
struct stat sb;
static int disknum = 0;
- int version = 1;
int fd, ret;

- /* check for both versions */
- sprintf(file, "%s/rhdd", mntpt);
+ /* check for new version */
+ sprintf(file, "%s/rhdd3", mntpt);
if (access(file, R_OK)) {
- version = 0;
- sprintf(file, "%s/rhdd-6.1", mntpt);
- if (access(file, R_OK)) {
- /* this can't happen, we already verified it! */
- return LOADER_BACK;
- }
+ /* this can't happen, we already verified it! */
+ return LOADER_BACK;
}
stat(file, &sb);
title = malloc(sb.st_size + 1);
@@ -131,23 +272,35 @@ static int loadDriverDisk(struct loaderData_s

*loaderData, char *mntpt) {

title[sb.st_size] = '';
close(fd);

- sprintf(file, "/tmp/DD-%d", disknum);
+ sprintf(file, DD_RPMS, disknum);
mkdirChain(file);
+ mkdirChain(DD_MODULES);
+ mkdirChain(DD_FIRMWARE);

if (!FL_CMDLINE(flags)) {
startNewt();
winStatus(40, 3, _("Loading"), _("Reading driver disk"));
}

- sprintf(dest, "/tmp/DD-%d", disknum);
- copyDirectory(mntpt, dest, copyWarnFn, copyErrorFn);
-
location = malloc(sizeof(struct moduleBallLocation));
location->title = strdup(title);
- location->version = version;

- checked_asprintf(&location->path, "/tmp/DD-%d/modules.cgz",

disknum);

- checked_asprintf(&fwdir, "/tmp/DD-%d/firmware", disknum);
+ checked_asprintf(&location->path, DD_MODULES);
+ checked_asprintf(&fwdir, DD_FIRMWARE);
+
+ sprintf(dest, DD_RPMS, disknum);
+ sprintf(src, "%s/rpms/%s", mntpt, getProductArch());
+ copyDirectory(src, dest, copyWarnFn, copyErrorFn);
+
+ /* unpack packages from dest into location->path */
+ if(dlabelUnpackRPMDir(dest, DD_EXTRACTED)){
+ /* TODO error handler */
+ }
+
+ /* run depmod to refresh modules db */
+ if(system("depmod -a")){
+ /* FIXME: depmod didn't run */
+ }

if (!access(fwdir, R_OK|X_OK)) {
add_fw_search_dir(loaderData, fwdir);
@@ -156,8 +309,11 @@ static int loadDriverDisk(struct loaderData_s

*loaderData, char *mntpt) {

}
free(fwdir);

- sprintf(file, "%s/modinfo", mntpt);
- readModuleInfo(file, modInfo, location, 1);
+ /* TODO generate and read module info
+ *
+ * sprintf(file, "%s/modinfo", mntpt);
+ * readModuleInfo(file, modInfo, location, 1);
+ */

if (!FL_CMDLINE(flags))
newtPopWindow();
@@ -623,3 +779,106 @@ static void getDDFromDev(struct loaderData_s *

loaderData, char * dev) {

umount("/tmp/drivers");
unlink("/tmp/drivers");
}


Given the opportunity, I'd like to see older code using static buffers
and
sprintf() replaced with checked_asprintf().


+
+/*
+ * Utility functions to maintain linked-list of device names
+ * */
+
+struct ddlist* ddlist_add(struct ddlist *list, const char* device)
+{
+ struct ddlist* item;
+
+ item = (struct ddlist*)malloc(sizeof(struct ddlist));
+ if(item==NULL){
+ return list;
+ }
+
+ item->device = strdup(device);
+ item->next = list;
+
+ return item;
+}
+
+int ddlist_free(struct ddlist *list)
+{
+ struct ddlist *next;
+ int count = 0;
+
+ while(list!=NULL){
+ next = list->next;
+ free(list->device);
+ free(list);
+ list = next;
+ count++;
+ }
+
+ return count;
+}


Let's use glib's linked list routines. The GSList is usable here.
http://library.gnome.org/devel/glib/stable/glib-Singly-Linked-Lists.html


+/*
+ * Look for partition with specific label (part of #316481)
+ */
+struct ddlist* findDriverDiskByLabel(void)
+{
+ char *ddLabel = "OEMDRV";
+ struct ddlist *ddDevice = NULL;
+ blkid_cache bCache;
+
+ int res;
+ blkid_dev_iterate bIter;
+ blkid_dev bDev;
+
+ if(blkid_get_cache(&bCache, NULL)<0){
+ logMessage(ERROR, _("Cannot initialize cache instance for

blkid"));

+ return NULL;
+ }
+ if((res = blkid_probe_all(bCache))<0){
+ logMessage(ERROR, _("Cannot probe devices in blkid: %d"),

res);

+ return NULL;
+ }
+
+ bIter = blkid_dev_iterate_begin(bCache);
+ blkid_dev_set_search(bIter, "LABEL", ddLabel);
+ while((res = blkid_dev_next(bIter, &bDev))!=0){
+ bDev = blkid_verify(bCache, bDev);
+ if(!bDev)
+ continue;
+ logMessage(DEBUGLVL, _("Adding driver disc %s to the list

of available DDs."), blkid_dev_devname(bDev));

+ ddDevice = ddlist_add(ddDevice, blkid_dev_devname(bDev));
+ /*blkid_free_dev(bDev); -- probably taken care of by the

put cache call.. it is not exposed in the API */

+ }
+ blkid_dev_iterate_end(bIter);
+
+ blkid_put_cache(bCache);
+
+ return ddDevice;
+}
+
+int loadDriverDiskFromPartition(struct loaderData_s *loaderData,

char* device)

+{
+ int rc;
+
+ logMessage(INFO, "trying to mount %s", device);
+ if (doPwMount(device, "/tmp/drivers", "auto", "ro", NULL)) {
+ logMessage(ERROR, _("Failed to mount driver disk."));
+ return -1;
+ }
+
+ rc = verifyDriverDisk("/tmp/drivers");
+ if (rc == LOADER_BACK) {
+ logMessage(ERROR, _("Driver disk is invalid for this "
+ "release of %s."), getProductName());
+ umount("/tmp/drivers");
+ return -2;
+ }
+
+ rc = loadDriverDisk(loaderData, "/tmp/drivers");
+ umount("/tmp/drivers");
+ if (rc == LOADER_BACK) {
+ return -3;
+ }
+
+ return 0;
+}
+
diff --git a/loader/driverdisk.h b/loader/driverdisk.h
index e6e919d..3e442fb 100644
--- a/loader/driverdisk.h
+++ b/loader/driverdisk.h
@@ -24,6 +24,11 @@
#include "modules.h"
#include "moduleinfo.h"

+#define DD_RPMS "/tmp/DD-%d"
+#define DD_EXTRACTED "/tmp/DD"
+#define DD_MODULES "/tmp/DD/lib/modules"
+#define DD_FIRMWARE "/tmp/DD/lib/firmware"
+
extern char *ddFsTypes[];

int loadDriverFromMedia(int class, struct loaderData_s *loaderData,
@@ -39,4 +44,19 @@ void useKickstartDD(struct loaderData_s *

loaderData, int argc,


void getDDFromSource(struct loaderData_s * loaderData, char * src);

+int loadDriverDiskFromPartition(struct loaderData_s *loaderData,

char* device);

+
+struct ddlist {
+ char* device;
+ struct ddlist* next;
+};
+
+struct ddlist* ddlist_add(struct ddlist *list, const char*

device);

+int ddlist_free(struct ddlist *list);
+
+struct ddlist* findDriverDiskByLabel(void);
+
+int modprobeNormalmode();
+int modprobeDDmode();
+
#endif
diff --git a/loader/loader.c b/loader/loader.c
index cd3e178..3159513 100644
--- a/loader/loader.c
+++ b/loader/loader.c
@@ -959,6 +959,10 @@ static void parseCmdLineFlags(struct

loaderData_s * loaderData,

else if (!strcasecmp(argv[i], "dd") ||
!strcasecmp(argv[i], "driverdisk"))
flags |= LOADER_FLAGS_MODDISK;
+ else if (!strcasecmp(argv[i], "dlabel=on"))
+ flags |= LOADER_FLAGS_AUTOMODDISK;
+ else if (!strcasecmp(argv[i], "dlabel=off"))
+ flags &= ~LOADER_FLAGS_AUTOMODDISK;
else if (!strcasecmp(argv[i], "rescue"))
flags |= LOADER_FLAGS_RESCUE;
else if (!strcasecmp(argv[i], "nopass"))
@@ -1785,6 +1789,7 @@ int main(int argc, char ** argv) {
struct loaderData_s loaderData;

char *path;
+ struct ddlist *dd, *dditer;

gchar *cmdLine = NULL, *ksFile = NULL, *virtpcon = NULL;
gboolean testing = FALSE, mediacheck = FALSE;
@@ -1882,6 +1887,12 @@ int main(int argc, char ** argv) {
flags |= LOADER_FLAGS_NOSHELL;
#endif

+ /* XXX if RHEL, enable the AUTODD feature by default,
+ * but we should come with more general way how to control this

*/

+ if(!strncmp(getProductName(), "Red Hat", 7)){
+ flags |= LOADER_FLAGS_AUTOMODDISK;
+ }
+
openLog(FL_TESTING(flags));
if (!FL_TESTING(flags))
openlog("loader", 0, LOG_LOCAL0);
@@ -1946,6 +1957,26 @@ 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(FL_AUTOMODDISK(flags)){
+ /* Load all autodetected DDs */
+ logMessage(INFO, "Trying to detect vendor driver discs");
+ dd = findDriverDiskByLabel();
+ dditer = dd;
+ while(dditer){
+ if(loadDriverDiskFromPartition(&loaderData,

dditer->device)){

+ logMessage(ERROR, "Automatic driver disk loader

failed for %s.", dditer->device);

+ }
+ else{
+ logMessage(INFO, "Automatic driver disk loader

succeeded for %s.", dditer->device);

+ }
+ dditer = dditer->next;
+ }
+ ddlist_free(dd);
+ }
+
if (FL_MODDISK(flags)) {
startNewt();
loadDriverDisks(DEVICE_ANY, &loaderData);
@@ -1955,6 +1986,9 @@ int main(int argc, char ** argv) {
logMessage(INFO, "found /dd.img, loading drivers");
getDDFromSource(&loaderData, "path:/dd.img");
}
+
+ /* 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

@@ -2139,6 +2173,9 @@ int main(int argc, char ** argv) {
tmparg++;
}

+ if (FL_AUTOMODDISK(flags))
+ *argptr++ = "--dlabel";
+
if (FL_NOIPV4(flags))
*argptr++ = "--noipv4";

diff --git a/loader/loader.h b/loader/loader.h
index ebf3766..ca6404f 100644
--- a/loader/loader.h
+++ b/loader/loader.h
@@ -70,6 +70,7 @@
#define LOADER_FLAGS_HAVE_CMSCONF (((uint64_t) 1) << 37)
#define LOADER_FLAGS_NOKILL (((uint64_t) 1) << 38)
#define LOADER_FLAGS_KICKSTART_SEND_SERIAL (((uint64_t) 1) << 39)
+#define LOADER_FLAGS_AUTOMODDISK (((uint64_t) 1) << 40)

#define FL_TESTING(a) ((a) & LOADER_FLAGS_TESTING)
#define FL_TEXT(a) ((a) & LOADER_FLAGS_TEXT)
@@ -107,6 +108,7 @@
#define FL_HAVE_CMSCONF(a) ((a) & LOADER_FLAGS_HAVE_CMSCONF)
#define FL_NOKILL(a) ((a) & LOADER_FLAGS_NOKILL)
#define FL_KICKSTART_SEND_SERIAL(a) ((a) &

LOADER_FLAGS_KICKSTART_SEND_SERIAL)

+#define FL_AUTOMODDISK(a) ((a) & LOADER_FLAGS_AUTOMODDISK)

void startNewt(void);
void stopNewt(void);



Large items:

- - Fix the coding style.
- - Use glib's linked list rather than creating a new one.
- - Avoid the use of static buffers. This is one thing I like about
the
convenience functions in glib. Almost all will handle dynamic
memory
allocation. I see no reason to not make use of that in loader.

I've got comments for rpm handling in another email.

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

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

iEYEARECAAYFAksdZfsACgkQ5hsjjIy1VkkSmQCePhyqQ0MioU iUz5tmdI1X2TGC
DwgAoKPhf1qCcR+ROTjptAVZnjdGPW9J
=W8vv
-----END PGP SIGNATURE-----

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list


_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list



- --
David Cantrell <dcantrell@redhat.com>

Red Hat / Honolulu, HI

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

iEYEARECAAYFAksekYYACgkQ5hsjjIy1VkkGfwCgnA7gPArYCy Tc6U7tg8KpfDja
dBAAn28uDG2/zxC6gWz1QyafJ0owGbAG
=PuFV
-----END PGP SIGNATURE-----

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 12-08-2009, 05:51 PM
Jon Masters
 
Default Backport the driver disc functionality from rhel5

On Tue, 2009-12-08 at 07:48 -1000, David Cantrell wrote:

> I would bring up those things if large items like the driver disk were
> discussed on the list while in their planning stages (correct me if I'm wrong,
> but I can't find any threads related to this feature that were sent out within
> the past couple of months except for the patches to review).

Some of the problem here is that we have discussed this as part of
overall planning for <insert RHEL here> but there are parts of the
conversation I won't send to a public mailing list like this one. We
probably can learn to have this conversation differently next time.

FWIW, I agree with Martin that we need his approach for now, which is
isolated, and can rip it out and switch over to a pure RPM-based one
later. Having RPM in the loader is a clear win for anyone who later
decides to use it for something else. It will also allow us to finally
just install the RPMs (as you say) without doing anything differently
and allow for things like assistive scripts in the RPM too.

Jon.


_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 12-10-2009, 03:22 AM
Chris Lumens
 
Default Backport the driver disc functionality from rhel5

> But this is first time I see some of the codestyle rules. Anaconda has
> lot of codestyles in it's codebase and nobody ever told us which is
> the correct one. Indentation was my mistake, because I'm used to
> different setup and restored my old .vimrc.

Yeah we've never really enforced any kind of style rules before, and
it's not just this patch set that's the cause of starting to enforce
them now. anaconda has been a mishmash of everyone's style for a long
time and it really shows. For now I'm interested in people just going
along with whatever's already in the files you're editing. Eventually,
we really do need to suck it up and convert everything to one consistent
style that no one will like.

> Using glib for string handling is the same situation.. you have to
> start talking about this stuff _before_ we post patches or document it
> somewhere. All the rewrites delay testing a lot...

Of course, it's hard to know what to talk about when we can't see things
in progress.

The glib stuff is new and we haven't really made a big push to get
everything using it, but David did have some patches a little while back
to start converting loader. We're probably not like to wholesale
convert loader, but making sure new code uses glib where appropriate is
good. We're linking with this stuff already. We might as well use it.

- Chris

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 12-10-2009, 03:25 AM
Chris Lumens
 
Default Backport the driver disc functionality from rhel5

> FWIW, I agree with Martin that we need his approach for now, which is
> isolated, and can rip it out and switch over to a pure RPM-based one
> later.

There's a trap here. Once we've written and committed something, it's
in. As long as it works, no one will really want to touch it to use
rpm, and everyone will be too busy to really think about it. Add in the
fact that it'll be in RHEL and therefore be much harder to change.

- Chris

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 

Thread Tools




All times are GMT. The time now is 02:41 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org