[Ocfs2-tools-devel] Review of the o2hbmonitor sem patch
Srinivas Eeda
srinivas.eeda at oracle.com
Wed Dec 8 23:02:50 PST 2010
we have to take semaphore only after the process is demonized. This is why I preferred to print errors to syslog rather than stderr.
I'll implement your other review comments.
thanks,
--Srini.
----- Original Message -----
From: sunil.mushran at oracle.com
To: srinivas.eeda at oracle.com, ocfs2-tools-devel at oss.oracle.com
Sent: Thursday, December 9, 2010 8:28:30 AM GMT +05:30 Chennai, Kolkata, Mumbai, New Delhi
Subject: Review of the o2hbmonitor sem patch
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
More information about the Ocfs2-tools-devel
mailing list