[Ocfs2-tools-devel] Review of the o2hbmonitor sem patch

[Sunil Mushran]

Srini,

I don't have the email for the o2hbmonitor sem patch. So I reviewed
the last version I saw on ocfs2-tools-devel.

http://oss.oracle.com/pipermail/ocfs2-tools-devel/2010-November/003619.html

+	if ((ret = getlock())) {
+	       if (ret == EAGAIN)
+		       syslog(LOG_WARNING, "o2hbmonitor already running\n");
+	       else
+		       syslog(LOG_WARNING, "o2hbmonitor failed to start\n");
+	       closelog();
+	       return ret;
+	}

main() should not care about the details. As in, encapsulate the locking in
a function whose job is to detect whether another instance is running or not.

Also, if we cannot determine its status, launch o2hbmonitor after printing
a message to stderr. Not syslog. All of this should be upfront. Not after
we open syslog.

Like...

+	ret = is_already_running();
+	switch (ret) {
+	case -1:
+		fprintf(stderr, "Another instance of %s is already running. "
+			"Aborting.\n", progname);
+		goto bail;
+	case 1:
+		fprintf(stderr, "Unable to determine if %s is already "
+			"running. Starting a new instance.\n", progname);
+	case 0:
+	default:
+		break;
+	}


Naked inits are not readable.

+	int semid, vals[]={0};
+	struct sembuf trylock[] = {{0, 0, SEM_UNDO|IPC_NOWAIT},
+				   {0, 1, SEM_UNDO|IPC_NOWAIT}};

See similar code extracted from libo2cb/o2cb_abi.c:

  968         struct sembuf sops[2] = {
  969                 { .sem_num = 0, .sem_op = 0, .sem_flg = SEM_UNDO },
  970                 { .sem_num = 0, .sem_op = 1, .sem_flg = SEM_UNDO }
  971         };


Thanks
Sunil