[stgt] [PATCH] fix leak of shared memory
Doron Shoham
dorons at Voltaire.COM
Mon Sep 15 16:55:04 CEST 2008
fix problem of shared memory (used for logging) not released when tgtd shutdown.
this patch fixes the shared memory leak when tgtd is killed.
add error messages when log initialization fails.
Signed-off-by: Doron Shoham <dorons at voltaire.com>
---
usr/log.c | 181 ++++++++++++++++++++++++++++++++++++++---------------------
usr/log.h | 7 +-
usr/tgtd.c | 37 ++++++++++--
3 files changed, 150 insertions(+), 75 deletions(-)
diff --git a/usr/log.c b/usr/log.c
index 076c770..38365ab 100644
--- a/usr/log.c
+++ b/usr/log.c
@@ -24,6 +24,7 @@
#include <unistd.h>
#include <syslog.h>
#include <signal.h>
+#include <errno.h>
#include <sys/shm.h>
#include <sys/ipc.h>
#include <sys/types.h>
@@ -43,7 +44,35 @@
static struct logarea *la;
static char *log_name;
static int is_debug;
-static pid_t pid;
+static int log_daemon = 0;
+static int log_stop_daemon = 0;
+
+static void free_logarea (void)
+{
+ int shmid;
+
+ if (!la)
+ return;
+
+ if (la->semid != -1)
+ semctl(la->semid, 0, IPC_RMID, la->semarg);
+ if (la->buff) {
+ shmdt(la->buff);
+ shmctl(la->shmid_buff, IPC_RMID, NULL);
+ la->buff = NULL;
+ la->shmid_buff = -1;
+ }
+ if (la->start) {
+ shmdt(la->start);
+ shmctl(la->shmid_msg, IPC_RMID, NULL);
+ la->start = NULL;
+ la->shmid_msg = -1;
+ }
+ shmid = la->shmid;
+ shmdt(la);
+ shmctl(shmid, IPC_RMID, NULL);
+ la = NULL;
+}
static int logarea_init (int size)
{
@@ -52,25 +81,34 @@ static int logarea_init (int size)
logdbg(stderr,"enter logarea_init\n");
if ((shmid = shmget(IPC_PRIVATE, sizeof(struct logarea),
- 0644 | IPC_CREAT | IPC_EXCL)) == -1)
+ 0644 | IPC_CREAT | IPC_EXCL)) == -1) {
+ syslog(LOG_ERR, "shmget logarea failed %d", errno);
return 1;
+ }
la = shmat(shmid, NULL, 0);
- if (!la)
+ if (!la) {
+ syslog(LOG_ERR, "shmat logarea failed %d", errno);
+ shmctl(shmid, IPC_RMID, NULL);
return 1;
+ }
+ la->shmid = shmid;
if (size < MAX_MSG_SIZE)
size = LOG_SPACE_SIZE;
if ((shmid = shmget(IPC_PRIVATE, size,
0644 | IPC_CREAT | IPC_EXCL)) == -1) {
- shmdt(la);
+ syslog(LOG_ERR, "shmget msg failed %d", errno);
+ free_logarea();
return 1;
}
+ la->shmid_msg = shmid;
la->start = shmat(shmid, NULL, 0);
if (!la->start) {
- shmdt(la);
+ syslog(LOG_ERR, "shmat msg failed %d", errno);
+ free_logarea();
return 1;
}
memset(la->start, 0, size);
@@ -82,43 +120,34 @@ static int logarea_init (int size)
if ((shmid = shmget(IPC_PRIVATE, MAX_MSG_SIZE + sizeof(struct logmsg),
0644 | IPC_CREAT | IPC_EXCL)) == -1) {
- shmdt(la->start);
- shmdt(la);
+ syslog(LOG_ERR, "shmget logmsg failed %d", errno);
+ free_logarea();
return 1;
}
la->buff = shmat(shmid, NULL, 0);
if (!la->buff) {
- shmdt(la->start);
- shmdt(la);
+ syslog(LOG_ERR, "shmat logmsgfailed %d", errno);
+ free_logarea();
return 1;
}
+ la->shmid_buff = shmid;
if ((la->semid = semget(SEMKEY, 1, 0666 | IPC_CREAT)) < 0) {
- shmdt(la->buff);
- shmdt(la->start);
- shmdt(la);
+ syslog(LOG_ERR, "semget failed %d", errno);
+ free_logarea();
return 1;
}
la->semarg.val=1;
if (semctl(la->semid, 0, SETVAL, la->semarg) < 0) {
- shmdt(la->buff);
- shmdt(la->start);
- shmdt(la);
+ syslog(LOG_ERR, "semctl failed %d", errno);
+ free_logarea();
return 1;
}
return 0;
}
-static void free_logarea (void)
-{
- semctl(la->semid, 0, IPC_RMID, la->semarg);
- shmdt(la->buff);
- shmdt(la->start);
- shmdt(la);
-}
-
#if LOGDBG
static void dump_logarea (void)
{
@@ -244,7 +273,7 @@ static void dolog(int prio, const char *fmt, va_list ap)
ops.sem_flg = 0;
ops.sem_op = -1;
if (semtimedop(la->semid, &ops, 1, &ts) < 0) {
- syslog(LOG_ERR, "semop up failed");
+ syslog(LOG_ERR, "semop down failed %d", errno);
return;
}
@@ -252,7 +281,7 @@ static void dolog(int prio, const char *fmt, va_list ap)
ops.sem_op = 1;
if (semop(la->semid, &ops, 1) < 0) {
- syslog(LOG_ERR, "semop down failed");
+ syslog(LOG_ERR, "semop up failed %d", errno);
return;
}
} else {
@@ -298,7 +327,7 @@ static void log_flush(void)
ops.sem_flg = 0;
ops.sem_op = -1;
if (semop(la->semid, &ops, 1) < 0) {
- syslog(LOG_ERR, "semop up failed");
+ syslog(LOG_ERR, "semop down failed %d", errno);
exit(1);
}
@@ -306,67 +335,89 @@ static void log_flush(void)
ops.sem_op = 1;
if (semop(la->semid, &ops, 1) < 0) {
- syslog(LOG_ERR, "semop down failed");
+ syslog(LOG_ERR, "semop up failed %d", errno);
exit(1);
}
log_syslog(la->buff);
}
}
+static void log_catch_signal(int signo)
+{
+ switch (signo) {
+ case SIGSEGV:
+ log_flush();
+ break;
+ case SIGTERM:
+ log_stop_daemon = 1;
+ break;
+ }
+}
+
+static void __log_close(void)
+{
+ if (log_daemon) {
+ log_flush();
+ free_logarea();
+ closelog();
+ }
+}
+
int log_init(char *program_name, int size, int daemon, int debug)
{
+ struct sigaction sa_old;
+ struct sigaction sa_new;
+ pid_t pid;
+
is_debug = debug;
+ log_daemon = daemon;
logdbg(stderr,"enter log_init\n");
log_name = program_name;
- if (daemon) {
- struct sigaction sa_old;
- struct sigaction sa_new;
-
- openlog(log_name, 0, LOG_DAEMON);
- setlogmask (LOG_UPTO (LOG_DEBUG));
+ openlog(log_name, 0, LOG_DAEMON);
+ setlogmask (LOG_UPTO (LOG_DEBUG));
- if (logarea_init(size)) {
- syslog(LOG_ERR, "failed to initialize the logger\n");
- return 1;
- }
-
- la->active = 1;
- pid = fork();
- if (pid < 0) {
- syslog(LOG_ERR, "fail to fork the logger\n");
- return 1;
- } else if (pid) {
- syslog(LOG_WARNING,
- "Target daemon logger with pid=%d started!\n", pid);
- return 0;
- }
+ if (logarea_init(size)) {
+ syslog(LOG_ERR, "failed to initialize logarea\n");
+ return -1;
+ }
- /* flush on daemon's crash */
- sa_new.sa_handler = (void*)log_flush;
- sigemptyset(&sa_new.sa_mask);
- sa_new.sa_flags = 0;
- sigaction(SIGSEGV, &sa_new, &sa_old );
+ pid = fork();
+ if (pid < 0) {
+ syslog(LOG_ERR, "fail to fork the logger\n");
+ return -1;
+ } else if (pid) {
+ syslog(LOG_WARNING,
+ "Target daemon logger with pid=%d started!\n", pid);
+ return pid;
+ }
- while (la->active) {
- log_flush();
- sleep(1);
+ /* flush on daemon's crash */
+ sa_new.sa_handler = log_catch_signal;
+ sigemptyset(&sa_new.sa_mask);
+ sa_new.sa_flags = 0;
+ sigaction(SIGSEGV, &sa_new, &sa_old );
+ sigaction(SIGTERM, &sa_new, &sa_old );
+
+ while (1) {
+ log_flush();
+ sleep(1);
+ if (log_stop_daemon) {
+ break;
}
-
- exit(0);
}
- return 0;
+ __log_close();
+ exit(0);
}
-void log_close(void)
+void log_close(pid_t pid)
{
- if (la) {
- la->active = 0;
- waitpid(pid, NULL, 0);
+ int status;
- closelog();
- free_logarea();
+ if (log_daemon && pid > 0) {
+ kill(pid, SIGTERM);
+ waitpid(pid, &status, 0);
}
}
diff --git a/usr/log.h b/usr/log.h
index b84f6d6..a923494 100644
--- a/usr/log.h
+++ b/usr/log.h
@@ -38,7 +38,6 @@ union semun {
#define LOG_SPACE_SIZE 16384
#define MAX_MSG_SIZE 256
-extern int log_daemon;
extern int log_level;
struct logmsg {
@@ -48,8 +47,10 @@ struct logmsg {
};
struct logarea {
+ int shmid;
+ int shmid_msg;
+ int shmid_buff;
int empty;
- int active;
void *head;
void *tail;
void *start;
@@ -60,7 +61,7 @@ struct logarea {
};
extern int log_init (char * progname, int size, int daemon, int debug);
-extern void log_close (void);
+extern void log_close (pid_t pid);
extern void dump_logmsg (void *);
extern void log_warning(const char *fmt, ...)
__attribute__ ((format (printf, 1, 2)));
diff --git a/usr/tgtd.c b/usr/tgtd.c
index 0b1cb4c..efa96b7 100644
--- a/usr/tgtd.c
+++ b/usr/tgtd.c
@@ -31,6 +31,7 @@
#include <string.h>
#include <unistd.h>
#include <sys/epoll.h>
+#include <syslog.h>
#include "list.h"
#include "tgtd.h"
@@ -38,6 +39,9 @@
#include "work.h"
#include "util.h"
+static int is_daemon = 1;
+static pid_t log_pid = 0;
+
struct tgt_event {
union {
event_handler_t *handler;
@@ -84,8 +88,22 @@ Target framework daemon.\n\
}
exit(status);
}
+static void tgtd_shutdown(void)
+{
+ syslog(LOG_ERR, "tgtd shutting down");
+ system_active = 0;
+}
-static void signal_catch(int signo) {
+static void main_catch_signal(int signo)
+{
+ switch (signo) {
+ case SIGINT:
+ case SIGTERM:
+ tgtd_shutdown();
+ break;
+ default:
+ break;
+ }
}
static int oom_adjust(void)
@@ -294,11 +312,11 @@ int main(int argc, char **argv)
struct sigaction sa_old;
struct sigaction sa_new;
int err, ch, longindex, nr_lld = 0;
- int is_daemon = 1, is_debug = 0;
+ int is_debug = 0;
int use_kernel = 0;
/* do not allow ctrl-c for now... */
- sa_new.sa_handler = signal_catch;
+ sa_new.sa_handler = main_catch_signal;
sigemptyset(&sa_new.sa_mask);
sa_new.sa_flags = 0;
sigaction(SIGINT, &sa_new, &sa_old);
@@ -350,9 +368,14 @@ int main(int argc, char **argv)
if (err)
exit(1);
- err = log_init(program_name, LOG_SPACE_SIZE, is_daemon, is_debug);
- if (err)
- exit(1);
+ /* initialize logger */
+ if (is_daemon) {
+ log_pid = log_init(program_name, LOG_SPACE_SIZE, is_daemon, is_debug);
+ if (log_pid < 0) {
+ syslog(LOG_ERR, "failed to initialize the logger\n");
+ exit(1);
+ }
+ }
if (use_kernel) {
err = kreq_init();
@@ -372,7 +395,7 @@ int main(int argc, char **argv)
ipc_exit();
- log_close();
+ log_close(log_pid);
return 0;
}
--
1.5.3.8
Hi,
I have seen that the logging mechanism of iscsid and tgtd are similar.
In iscsid those bugs were fixed but but on tgtd they were not.
This patch should fix them.
Thanks,
Doron
--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
More information about the stgt
mailing list