[stgt] [PATCH] fix leak of shared memory
Doron Shoham
dorons at Voltaire.COM
Mon Sep 29 08:44:15 CEST 2008
>>> You should use the following command:
>>>
>>> tgtadm --op delete --mode system
>> I wasn't aware of this option.
>> I couldn't find any info about "--mode system".
>> It's not written in tgtadm help nor in any man/readme file.
>
> We have lots of undocumented features (they are mentioned only on the
> mailing list). Documents are always welcome.
>
>
>> Anyway, using this command doesn't solve the memory leakage.
>
> Yeah, I know. I didn't say that 'tgtadm --op delete --mode system'
> removes the shared memory.
>
Ok, this patch will solve this issue as well.
> tgtd creates shared memory at startup and exits without cleaning up
> it. So unless you start and stop tgtd again and again, the leak is not
> an issue at all.
That's right, but we still want to solve this problem.
I have combined my patch and yours (if you don't mind) and update it for
the current git.
Now, if you exit tgtd with tgtadm --op delete --mode system or kill it with
SIG_TERM,SIG_INT or SIG_KILL it will clear the shared memory.
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 | 187 +++++++++++++++++++++++++++++++++++++++---------------------
usr/log.h | 7 +-
usr/tgtd.c | 36 +++++++++--
3 files changed, 155 insertions(+), 75 deletions(-)
diff --git a/usr/log.c b/usr/log.c
index 076c770..e25729e 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,29 +81,42 @@ 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;
+
+ shmctl(shmid, IPC_RMID, NULL);
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);
+ shmctl(shmid, IPC_RMID, NULL);
+
la->empty = 1;
la->end = la->start + size;
la->head = la->start;
@@ -82,43 +124,36 @@ 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;
+
+ shmctl(shmid, IPC_RMID, NULL);
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 +279,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 +287,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 +333,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 +341,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 62aaa04..747a58f 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"
@@ -40,6 +41,8 @@
unsigned long pagesize, pageshift, pagemask;
+static int is_daemon = 1;
+static pid_t log_pid = 0;
int system_active = 1;
static int ep_fd;
static char program_name[] = "tgtd";
@@ -71,8 +74,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)
@@ -276,11 +293,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);
@@ -332,9 +349,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();
@@ -354,7 +376,7 @@ int main(int argc, char **argv)
ipc_exit();
- log_close();
+ log_close(log_pid);
return 0;
}
--
1.5.3.8
--
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