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_.
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
