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

Re: [PATCH] Allow DriverDisc to contain vendor provided tools (#659790)



I'm ok with the patch, except for some style comments...

On 08/03/2011 09:02 AM, Martin Sivak wrote:
This extracts all files from /(s)bin, /usr/(s)bin, /lib(64) and
/usr/lib(64) into /tmp/DD hierarchy and adds those dirs to
PATH and LD_LIBRARY_PATH (in init).

The package has to Provide: installer-enhancement which version
matches the running anaconda's version to be considered for this
feature.

We implemented this feature to enable vendors to add their
tools for initializing special hardware. Our binaries and libraries
have higher priority, so in case of conflicts, anaconda wins.
This should prevent breakages to the installer environment.
---
  loader/driverdisk.c |   80 ++++++++++++++++++++++++++++++++++++++------------
  loader/init.c       |    7 ++--
  loader/rpmextract.c |   12 +++----
  loader/rpmextract.h |   12 ++++++-
  4 files changed, 80 insertions(+), 31 deletions(-)

diff --git a/loader/driverdisk.c b/loader/driverdisk.c
index e6a37a5..96f734b 100644
--- a/loader/driverdisk.c
+++ b/loader/driverdisk.c
@@ -63,6 +63,15 @@
  /* boot flags */
  extern uint64_t flags;

+/* DD extract flags */
+enum {
+    dup_nothing = 0,
+    dup_modules = 1,
+    dup_firmwares = 2,
+    dup_binaries = 4,
+    dup_libraries = 8
+} _dup_extract;
+
  /*
   * check if the RPM in question provides
   * Provides:<dep>  =<version>
@@ -71,50 +80,83 @@ extern uint64_t flags;
  int dlabelProvides(const char* dep, const char* version, uint32_t sense, void *userptr)
  {
      char *kernelver = (char*)userptr;
+    int packageflags = 0;

      logMessage(DEBUGLVL, "Provides: %s = %s", dep, version);

      if (version == NULL)
-        return -1;
+        return 0;

-    /* not a modules package */
-    if (strcmp(dep, "kernel-modules")) return 1;
+    /* is it a modules package? */
+    if (!strcmp(dep, "kernel-modules")) {

-    /*
-     * exception for 6.0 and 6.1 DUDs, we changed the logic a bit and need to maintain compatibility.
-     */
-    if (!strncmp(version, "2.6.32-131", 10) || !strncmp(version, "2.6.32-71", 9)) return 0;
+        /*
+         * exception for 6.0 and 6.1 DUDs, we changed the logic a bit and need to maintain compatibility.
+         */
+        if ((!strncmp(version, "2.6.32-131", 10)) || (!strncmp(version, "2.6.32-71", 9))) packageflags |= dup_modules | dup_firmwares;

I'm not a fan of long if statement lines like this. Please follow the style we do for the vast majority of the code:

    if (TEST)
        STATEMENT;

or:

    if (TEST) {
        STATEMENT;
        STATEMENT;
        STATEMENT;
    }

-    /*
-     * Use this package only if the version match string is true for this kernel version
-     */
-    return !matchVersions(kernelver, sense, version);
+        /*
+         * Use this package only if the version match string is true for this kernel version
+         */
+        if (!matchVersions(kernelver, sense, version)) packageflags |= dup_modules | dup_firmwares;

if statement again.

+    }
+
+    /* is it an app package? */
+    if (!strcmp(dep, "installer-enhancement")) {
+
+        /*
+         * If the version string matches anaconda version, unpack binaries to /tmp/DD
+         */
+        if (!matchVersions(VERSION, sense, version)) packageflags |= dup_binaries | dup_libraries;

if statement again.

+    }
+
+    return packageflags;
  }

  /*
   * 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 dlabelFilter(const char* name, const struct stat *fstat, int packageflags, void *userptr)
  {
      int l = strlen(name);

-    logMessage(DEBUGLVL, "Unpacking %s", name);
+    logMessage(DEBUGLVL, "Unpacking %s with flags %02x", name, packageflags);
+
+    /* unpack bin and sbin if the package was marked as installer-enhancement */
+    if ((packageflags&  dup_binaries)) {
+        if(!strncmp("bin/", name, 4)) return 1;
+        else if (!strncmp("sbin/", name, 5)) return 1;
+        else if (!strncmp("usr/bin/", name, 8)) return 1;
+        else if (!strncmp("usr/sbin/", name, 9)) return 1;

if statements again.

+    }
+
+    /* unpack lib and lib64 if the package was marked as installer-enhancement */
+    if ((packageflags&  dup_libraries)) {
+        if(!strncmp("lib/", name, 4)) return 1;
+        else if (!strncmp("lib64/", name, 6)) return 1;
+        else if (!strncmp("usr/lib/", name, 8)) return 1;
+        else if (!strncmp("usr/lib64/", name, 10)) return 1;

if statements again.

+    }

      /* we want firmware files */
-    if (!strncmp("lib/firmware/", name, 13)) return 0;
+    if ((packageflags&  dup_firmwares)&&  !strncmp("lib/firmware/", name, 13)) return 1;

if statement again and strange operator spacing (no space before & or &&).


+    /* we do not want kernel files */
+    if(!(packageflags&  dup_modules)) return 0;

if statement again.

and "if (" instead of "if(", please.

+
+    /* check if the file has at least four chars eg X.SS */
      if (l<3)
-        return 1;
+        return 0;
      l-=3;

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

-    /* TODO we are unpacking kernel module, read it's description */
+    /* we are unpacking kernel module.. */

-    return 0;
+    return 1;
  }

  char* moduleDescription(const char* modulePath)
diff --git a/loader/init.c b/loader/init.c
index 0504f8b..b04d872 100644
--- a/loader/init.c
+++ b/loader/init.c
@@ -85,14 +85,15 @@
  char * env[] = {
      "PATH=/usr/bin:/bin:/sbin:/usr/sbin:/mnt/sysimage/bin:"
      "/mnt/sysimage/usr/bin:/mnt/sysimage/usr/sbin:/mnt/sysimage/sbin:"
-    "/mnt/sysimage/usr/X11R6/bin",
+    "/mnt/sysimage/usr/X11R6/bin:"
+    "/tmp/DD/bin:/tmp/DD/sbin:/tmp/DD/usr/bin:/tmp/DD/usr/bin", /* for tools provided by DUPs from vendors*/

      /* we set a nicer ld library path specifically for bash -- a full
         one makes anaconda unhappy */
  #if defined(__x86_64__) || defined(__s390x__) || defined(__powerpc64__)
-    "LD_LIBRARY_PATH=/lib64:/usr/lib64:/lib:/usr/lib",
+    "LD_LIBRARY_PATH=/lib64:/usr/lib64:/lib:/usr/lib:/tmp/DD/lib64:/tmp/DD/usr/lib64:/tmp/DD/lib:/tmp/DD/usr/lib",
  #else
-    "LD_LIBRARY_PATH=/lib:/usr/lib",
+    "LD_LIBRARY_PATH=/lib:/usr/lib:/tmp/DD/lib:/tmp/DD/usr/lib",
  #endif
      "HOME=/",
      "TERM=linux",
diff --git a/loader/rpmextract.c b/loader/rpmextract.c
index 241b7e3..4325c6d 100644
--- a/loader/rpmextract.c
+++ b/loader/rpmextract.c
@@ -123,6 +123,8 @@ int explodeRPM(const char *source,
      rpmVSFlags vsflags;
      const char *compr;

+    int packageflags = 0;
+
      if (strcmp(source, "-") == 0)
          fdi = fdDup(STDIN_FILENO);
      else
@@ -219,8 +221,6 @@ int explodeRPM(const char *source,
          const char *depversion;
          uint32_t depsense;

-        int found = 0;
-
          if (!headerGet(h, RPMTAG_PROVIDES,&tddep, HEADERGET_MINMEM))
              break;

@@ -239,16 +239,14 @@ int explodeRPM(const char *source,
          while ((depname = rpmtdNextString(&tddep))) {
              depversion = rpmtdNextString(&tdver);
              depsense = *(rpmtdNextUint32(&tdsense));
-            if (!provides(depname, depversion, depsense, userptr)) {
-                found++;
-            }
+            packageflags |= provides(depname, depversion, depsense, userptr);
          }

          rpmtdFreeData(&tddep);
          rpmtdFreeData(&tdver);
          rpmtdFreeData(&tdsense);

-        if (found<=0){
+        if (packageflags == 0){

") {" instead of "){", please.

              Fclose(fdi);
              return EXIT_BADDEPS;
          }
@@ -319,7 +317,7 @@ int explodeRPM(const char *source,
          else
              towrite = 2;

-        if (filter&&  filter(filename+offset, fstat, userptr)) {
+        if (filter&&  (!filter(filename+offset, fstat, packageflags, userptr))) {
              /* filter this file */
              towrite = 0;
          }
diff --git a/loader/rpmextract.h b/loader/rpmextract.h
index 32774da..d8ae67f 100644
--- a/loader/rpmextract.h
+++ b/loader/rpmextract.h
@@ -30,8 +30,16 @@
  #define EXIT_BADDEPS 4
  #define BUFFERSIZE 1024

-/* both filter functions return 0 - match, 1 - match not found */
-typedef int (*filterfunc)(const char* name, const struct stat *fstat, void *userptr);
+/*
+  filter function returns 1 - extract file, 0 - skip
+  flags is binary accumulated result from provides callbacks
+ */
+typedef int (*filterfunc)(const char* name, const struct stat *fstat, int flags, void *userptr);
+
+/*
+  returns nonzero if the dependency/provides means we should unpack the package
+  moreover the results from provides checks are binary orred (|) and passed as flags to filter function
+*/
  typedef int (*dependencyfunc)(const char* depname, const char* depversion, const uint32_t sense, void *userptr);

  int explodeRPM(const char* file,

--
David Cantrell <dcantrell redhat com>
Supervisor, Installer Engineering Team
Red Hat, Inc. | Westford, MA | EST5EDT


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