-----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\n");
+ }
+ else{
+ fprintf(f, " pblacklist /lib/modules/%s\n", 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\n", 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\n", 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)\n", 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@xxxxxxxxxx */
#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.\n");
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\n");
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\n", e->message);
g_error_free(e);
if (errors) {
fprintf(stderr, "modinfo error: %s\n", errors);
}
return EXIT_FAILURE;
}
/* display the module description */
if (output) {
/* we do not want trailing newlines */
output = g_strchomp(output);
printf("%s\n", 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\n", 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] = '\0';
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@xxxxxxxxxx>
Red Hat / Honolulu, HI
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
iEYEARECAAYFAksdZfsACgkQ5hsjjIy1VkkSmQCePhyqQ0MioUiUz5tmdI1X2TGC
DwgAoKPhf1qCcR+ROTjptAVZnjdGPW9J
=W8vv
-----END PGP SIGNATURE-----
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
