"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\n");
> + }
> + else{
> + fprintf(f, " pblacklist /lib/modules/%s\n", 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\n", 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\n", 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)\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;
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\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);
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] = '\0';
> 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
