[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [PATCH 5/7] Backport the RHEL5 DriverDisc functionality



On 12/17/2009 10:47 AM, Martin Sivak wrote:
> And adapt it to use glib and better string handling functions
> ---
>  loader/driverdisk.c |  328 +++++++++++++++++++++++++++++++++++++++++++++------
>  loader/driverdisk.h |   12 ++
>  2 files changed, 302 insertions(+), 38 deletions(-)
> 
> diff --git a/loader/driverdisk.c b/loader/driverdisk.c
> index 5f8d43c..95f2592 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,187 @@
>  /* boot flags */
>  extern uint64_t flags;
>  
> -static char * driverDiskFiles[] = { "modinfo", "modules.dep", 
> -                                    "modules.cgz", "modules.alias", NULL };
> +/* modprobe DD mode */
> +int modprobeDDmode(void)
> +{
> +    FILE *f = fopen("/etc/depmod.d/ddmode.conf", "w");
> +    if (f) {
> +        struct utsname unamedata;
> +
> +        if (uname(&unamedata)) fprintf(f, " pblacklist /lib/modules\n");
> +        else fprintf(f, " pblacklist /lib/modules/%s\n", unamedata.release);

Please don't format conditionals this way. Newlines are your friends here.

> +        fclose(f);
> +    }
> +
> +    return f==NULL;
> +}
> +
> +int modprobeNormalmode(void)
> +{
> +    /* remove depmod overrides */
> +    if (unlink("/etc/depmod.d/ddmode.conf")) {
> +        logMessage(ERROR, "removing ddmode.conf failed");
> +        return -1;
> +    }
> +
> +    /* run depmod to refresh modules db */
> +    if (system("depmod -a")) {
> +        logMessage(ERROR, "depmod -a failed");
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +/*
> + * check if the RPM in question provides
> + * Provides: userptr
> + * we use it to check kernel-modules-<kernelversion>
> + */
> +int dlabelProvides(const char* dep, void *userptr)
> +{
> +    char *kernelver = (char*)userptr;
> +
> +    logMessage(DEBUGLVL, "Provides: %s\n", dep);
> +
> +    return strcmp(dep, kernelver);
> +}
> +
> +/*
> + * during cpio extraction, only extract files we need
> + * eg. module .ko files and firmware directory
> + */
> +int dlabelFilter(const char* name, const 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;

Same as above - newlines make things more readable.

> +    l-=3;
> +
> +    /* and we want only .ko files */
> +    if (strcmp(".ko", name+l)) return 1;

Again with the newlines.

> +
> +    /* TODO we are unpacking kernel module, read it's description */
> +
> +    return 0;
> +}
> +
> +char* moduleDescription(const char* modulePath)
> +{
> +    char *command = NULL;
> +    FILE *f = NULL;
> +    char *description = NULL;
> +    int size;
> +
> +    checked_asprintf(&command, "modinfo --description '%s'", modulePath);
> +    f = popen(command, "r");
> +    free(command);
> +
> +    if (f==NULL) return NULL;

Again with the newlines.

> +
> +    description = malloc(sizeof(char)*256);
> +    if (!description) return NULL;

Again with the newlines.

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

1)  "if (size == 0) {" please.
2) As I said in my first review of this code:

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);
> +
> +    return description;
> +}
> +
> +int globErrFunc(const char *epath, int eerrno)
> +{
> +    /* TODO check fatal errors */
> +
> +    return 0;
> +}

This still seems like the wrong thing to do.  From my original review:

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)) {
> +        logMessage(ERROR, _("We weren't able to CWD to %s"), destination);
> +        free(oldcwd);
> +        return 1;
> +    }

This shouldn't be translated. Also, the conditional is wrong. It'd probably
also be better as something like:

oldcwd = getcwd(NULL, 0);
if (!oldcwd) {
    logMessage(ERROR, "getcwd() failed: %m");
    return 1;
}

if (chdir(destination) < 0) {
    logMessage(ERROR, "Unable to change directory to \"%s\": %m", destination);
    free(oldcwd);
    return 1;
}
    

> +
> +    /* get running kernel version */
> +    rc = uname(&unamedata);
> +    checked_asprintf(&kernelver, "kernel-modules-%s",
> +            rc ? "unknown" : unamedata.release);
> +    logMessage(DEBUGLVL, "Kernel version: %s\n", kernelver);
> +
> +    checked_asprintf(&globpattern, "%s/*.rpm", rpmdir);
> +    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, NULL, kernelver);
> +        }

Style/formatting is pretty ugly here.

> +        globfree(&globres);
> +        /* end of iteration */
> +    }
> +    free(globpattern);
> +
> +    /* restore CWD */
> +    if (chdir(oldcwd)) {
> +        logMessage(WARNING, _("We weren't able to restore CWD to %s"), oldcwd);
> +    }

This shouldn't be translated or have braces around it.  And an error message
more like what I've got above would be better.

> +
> +    /* 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++) {
> +        snprintf(file, 200, "%s/%s/%s", mntpt, getProductArch(), *fnPtr);
> +        if (access(file, R_OK)) {
> +            logMessage(ERROR, "cannot find %s, bad driver disk", file);
> +            return LOADER_BACK;
> +        }
> +    }

Again, from my original review of this code:

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 ;)

>  
>      return LOADER_OK;
>  }
> @@ -101,25 +259,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,24 +284,39 @@ 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_RPMDIR_TEMPLATE, 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, DD_MODULES);
> +
> +    sprintf(dest, DD_RPMDIR_TEMPLATE, 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)) {
> +      /* fatal error, log this and jump to exception handler */
> +      logMessage(ERROR, _("Error unpacking RPMs from driver disc no.%d"),
> +              disknum);
> +      goto loadDriverDiscException;
> +    }

Randomly switching to two-space indents is bad.  Also this shouldn't be
translated.

>  
> -    checked_asprintf(&location->path, "/tmp/DD-%d/modules.cgz", disknum);
> -    checked_asprintf(&fwdir, "/tmp/DD-%d/firmware", disknum);
> +    /* run depmod to refresh modules db */
> +    if (system("depmod -a")) {
> +      /* this is not really fatal error, it might still work, log it */
> +      logMessage(ERROR, _("Error running depmod -a for driverdisc no.%d"));
> +    }

Same here.

>  
> +    checked_asprintf(&fwdir, DD_FIRMWARE);
>      if (!access(fwdir, R_OK|X_OK)) {
>          add_fw_search_dir(loaderData, fwdir);
>          stop_fw_loader(loaderData);
> @@ -156,8 +324,13 @@ 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);
> +     */
> +
> +loadDriverDiscException:
>  
>      if (!FL_CMDLINE(flags))
>          newtPopWindow();
> @@ -248,6 +421,12 @@ int loadDriverFromMedia(int class, struct loaderData_s *loaderData,
>              char ** part_list = getPartitionsList(device);
>              int nump = 0, num = 0;
>  
> +            /* Do not crash if the device disappeared */
> +            if (!part_list) {
> +              stage = DEV_DEVICE;
> +              break;
> +            }
> +
>              if (part != NULL) free(part);

Again with the formatting (and I realize the last line isn't from this patch,
but fixing it anyway wouldn't be the worst choice ever...)

>  
>              if ((nump = lenPartitionsList(part_list)) == 0) {
> @@ -317,7 +496,7 @@ int loadDriverFromMedia(int class, struct loaderData_s *loaderData,
>          }
>  
>          case DEV_LOADFILE: {
> -            if(ddfile == NULL) {
> +            if (ddfile == NULL) {

Good job.

>                  logMessage(DEBUGLVL, "trying to load dd from NULL");
>                  stage = DEV_CHOOSEFILE;
>                  break;
> @@ -623,3 +802,76 @@ static void getDDFromDev(struct loaderData_s * loaderData, char * dev) {
>      umount("/tmp/drivers");
>      unlink("/tmp/drivers");
>  }
> +
> +
> +/*
> + * Look for partition with specific label (part of #316481)
> + */
> +GSList* findDriverDiskByLabel(void)
> +{
> +    char *ddLabel = "OEMDRV";
> +    GSList *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;
> +    }
> +
> +    if ((ddDevice = g_slist_alloc())==NULL) {
> +      logMessage(ERROR, _("Cannot allocate space for list of devices"));
> +      return NULL;
> +    }

More two-space indenting.

> +
> +    bIter = blkid_dev_iterate_begin(bCache);
> +    blkid_dev_set_search(bIter, "LABEL", ddLabel);
> +    while ((res = blkid_dev_next(bIter, &bDev))!=0) {

Add spaces.

> +        bDev = blkid_verify(bCache, bDev);

... and we're back to four

> +        if (!bDev)
> +          continue;

... and back to two

> +        logMessage(DEBUGLVL, _("Adding driver disc %s to the list of available DDs."), blkid_dev_devname(bDev));

Newlines would be good here, and also don't translate this string.

> +        ddDevice = g_slist_prepend(ddDevice, (gpointer)blkid_dev_devname(bDev));
> +        /*blkid_free_dev(bDev); -- probably taken care of by the put cache call.. it is not exposed in the API */

Splitting lines here would be good.  Or even just:

/* Freeing bDev is taken care of by the put cache call */

> +    }
> +    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;

Five space indents now, I see.  And a translated log message.

> +    }
> +
> +    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;
> +    }

Back to two-space indents... (as well as a translated log message)

> +
> +    rc = loadDriverDisk(loaderData, "/tmp/drivers");
> +    umount("/tmp/drivers");
> +    if (rc == LOADER_BACK) {
> +	return -3;
> +    }

Here we have the rare one-space indent...

> +
> +    return 0;
> +}
> +
> diff --git a/loader/driverdisk.h b/loader/driverdisk.h
> index e6e919d..98bfd4a 100644
> --- a/loader/driverdisk.h
> +++ b/loader/driverdisk.h
> @@ -24,6 +24,11 @@
>  #include "modules.h"
>  #include "moduleinfo.h"
>  
> +#define DD_RPMDIR_TEMPLATE "/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,11 @@ void useKickstartDD(struct loaderData_s * loaderData, int argc,
>  
>  void getDDFromSource(struct loaderData_s * loaderData, char * src);
>  
> +int loadDriverDiskFromPartition(struct loaderData_s *loaderData, char* device);
> +
> +GSList* findDriverDiskByLabel(void);
> +
> +int modprobeNormalmode();
> +int modprobeDDmode();
> +
>  #endif

I'm okay with this bit.

-- 
        Peter

I'd like to start a religion. That's where the money is.
		-- L. Ron Hubbard to Lloyd Eshbach, in 1949;
			quoted by Eshbach in _Over My Shoulder_.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]