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

Re: [PATCH 2/2] reIPL support for s390 (#512195).



Hi,

One comment on the C-code (see below) I know it is a bit
of a nitpick, but I couldn't help my self but comment on it.
Otherwise it looks ok.

On 12/22/2009 02:37 AM, David Cantrell wrote:
From: Mark Hamzy<hamzy us ibm com>

Basically this is a back-port of d00ef216099104c03c400f56fb21e19089df4a7d.
For those not capable of reading huge hashes, the previous sentence
means this patch brings the RHEL 5.5 reIPL support in line with what we
currently have upstream in both Fedora and RHEL-6 for rebooting (err, I
mean re-"IPL"ing) s390x VMs after installation.
---

<snip>

diff --git a/loader2/loader.c b/loader2/loader.c
index 4299ab3..5ef268b 100644
--- a/loader2/loader.c
+++ b/loader2/loader.c
@@ -118,6 +118,8 @@ uint64_t flags = LOADER_FLAGS_SELINUX | LOADER_FLAGS_NOFB;
  int num_link_checks = 5;
  int post_link_sleep = 0;

+static pid_t init_pid = 1;
+
  static struct installMethod installMethods[] = {
      { N_("Local CDROM"), "cdrom", 0, CLASS_CDROM, mountCdromImage },
      { N_("Hard drive"), "hd", 0, CLASS_HD, mountHardDrive },
@@ -1384,6 +1386,11 @@ void loaderSegvHandler(int signum) {
      exit(1);
  }

+void loaderUsrXHandler(int signum) {
+    logMessage(INFO, "Sending signal %d to process %d\n", signum, init_pid);
+    kill(init_pid, signum);
+}
+
  static int anaconda_trace_init(void) {
  #if 0
      int fd;
@@ -1455,6 +1462,26 @@ int main(int argc, char ** argv) {
          { 0, 0, 0, 0, 0, 0, 0 }
      };

+    /* get init PID if we have it */
+    if ((f = fopen("/var/run/init.pid", "r")) != NULL) {
+        char linebuf[256];
+        memset(linebuf, '\0', sizeof(linebuf));
+

There is no need for this memset fgets will 0 terminate.

+        while (fgets(linebuf, sizeof(linebuf) - 1, f) != NULL) {

fgets will write max "size - 1" bytes and then 0 terminate, iow, there
is no need for the " - 1" in "sizeof(linebuf) - 1"

+            errno = 0;
+            init_pid = strtol((const char *)&linebuf, NULL, 10);

&linebuf is a bit double, as linebuf without the & will already give
the array address, also there is no need for the cast to (const char *).

So please replace "(const char *)&linebuf" with just "linebuf", also making
the linebuf use consistent with the fgets call, thanks.

Do we have this in master too ? If so could you clean it up there too please?

+            if (errno == EINVAL || errno == ERANGE) {
+                logMessage(ERROR, "%s (%d): %m", __func__, __LINE__);
+                init_pid = 1;
+            }
+        }
+
+        fclose(f);
+    }
+
+    signal(SIGUSR1, loaderUsrXHandler);
+    signal(SIGUSR2, loaderUsrXHandler);
+
      /* Make sure sort order is right. */
      setenv ("LC_COLLATE", "C", 1);	

@@ -1952,23 +1979,6 @@ int main(int argc, char ** argv) {
              waitpid(pid,&status, 0);
          }

-#if defined(__s390__) || defined(__s390x__)
-        /* FIXME: we have to send a signal to linuxrc on s390 so that shutdown
-         * can happen.  this is ugly */
-        FILE * f;
-        f = fopen("/var/run/init.pid", "r");
-        if (!f) {
-            logMessage(WARNING, "can't find init.pid, guessing that init is pid 1");
-            pid = 1;
-        } else {
-            char * buf = malloc(256);
-            char *ret;
-
-            ret = fgets(buf, 256, f);
-            pid = atoi(buf);
-        }
-        kill(pid, SIGUSR2);
-#endif
          stop_fw_loader(&loaderData);
          return rc;
      }

<snip>

Regards,

Hans


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