Received: (from majordom@localhost) by hyperreal.org (8.8.5/8.8.5) id RAA24335; Sat, 4 Oct 1997 17:51:29 -0700 (PDT) Received: from twinlark.arctic.org (twinlark.arctic.org [204.62.130.91]) by hyperreal.org (8.8.5/8.8.5) with SMTP id RAA24331 for ; Sat, 4 Oct 1997 17:51:26 -0700 (PDT) Received: (qmail 6758 invoked by uid 500); 5 Oct 1997 00:51:48 -0000 Date: Sat, 4 Oct 1997 17:51:48 -0700 (PDT) From: Dean Gaudet To: new-httpd@apache.org Subject: Re: [PATCH] Cleaning up FILE,LINE,errno in aplog_error() In-Reply-To: Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: new-httpd-owner@apache.org Precedence: bulk Reply-To: new-httpd@apache.org Status: O X-Status: Can I get some votes on this please? I want to clean this up today. If people support this then I'll go stick in the APLOG_NOERRNO all over the place Dean On Wed, 1 Oct 1997, Dean Gaudet wrote: > On Wed, 1 Oct 1997, Rodent of Unusual Size wrote: > > > I spent a little time on this to-day to try to address the issues of > > filename, line number, and errno information showing up in the error > > log. > > Good start ... but here's my approach. > > Stick an APLOG_NOERRNO bit into the level paramter, this saves one > parameter on each call (and the level and errno are constants so it > actually comes for free). > > When server_conf is NULL use stderr. Necessary to do some things during > startup and other odd cases (i.e. SEGV). > > Do the null filename thing like Ken did. > > Do it all with a lot less string copying -- use snprintf incrementally to > build up the entire buffer. > > Fix a buffer overflow with syslog stuff using sprintf(). > > I tweaked a bunch of stuff in http_main.c as well to show how you'd use > this. > > Dean > > Index: main/http_log.c > =================================================================== > RCS file: /export/home/cvs/apachen/src/main/http_log.c,v > retrieving revision 1.34 > diff -u -r1.34 http_log.c > --- http_log.c 1997/09/12 20:37:33 1.34 > +++ http_log.c 1997/10/02 00:23:05 > @@ -253,48 +253,56 @@ > va_list args; > char errstr[MAX_STRING_LEN]; > static TRANS *pname = priorities; > - > + size_t len; > + int save_errno = errno; > + FILE *logf; > > - if (level > s->loglevel) > + if (s && (level & APLOG_LEVELMASK) > s->loglevel) > return; > > - switch (s->loglevel) { > - case APLOG_DEBUG: > - ap_snprintf(errstr, sizeof(errstr), "[%s] %d: %s: %s: %d: ", > - pname[level].t_name, errno, strerror(errno), file, line); > - break; > - case APLOG_EMERG: > - case APLOG_CRIT: > - case APLOG_ALERT: > - ap_snprintf(errstr, sizeof(errstr), "[%s] %d: %s: ", > - pname[level].t_name, errno, strerror(errno)); > - break; > - case APLOG_INFO: > - case APLOG_ERR: > - case APLOG_WARNING: > - case APLOG_NOTICE: > - ap_snprintf(errstr, sizeof(errstr), "[%s] ", pname[level].t_name); > - break; > + if (!s) { > + logf = stderr; > + } else if (s && s->error_log) { > + logf = s->error_log; > + } else { > + logf = NULL; > + } > + > + if (logf) { > + len = ap_snprintf(errstr, sizeof(errstr), "[%s] ", get_time()); > + } else { > + len = 0; > + } > + > + len += ap_snprintf(errstr + len, sizeof(errstr) - len, > + "[%s] ", pname[level & APLOG_LEVELMASK].t_name); > + > + if (!(level & APLOG_NOERRNO)) { > + len += ap_snprintf(errstr + len, sizeof(errstr) - len, > + "%d: %s: ", save_errno, strerror(save_errno)); > } > - > + if (file && (level & APLOG_LEVELMASK) == APLOG_DEBUG) { > + len += ap_snprintf(errstr + len, sizeof(errstr) - len, > + "%s: %d: ", file, line); > + } > + > va_start(args, fmt); > + len += ap_vsnprintf(errstr + len, sizeof(errstr) - len, fmt, args); > + va_end(args); > > /* NULL if we are logging to syslog */ > - if (s->error_log) { > - fprintf(s->error_log, "[%s] %s", get_time(), errstr); > - vfprintf(s->error_log, fmt, args); > - fprintf(s->error_log, "\n"); > - fflush(s->error_log); > + if (logf) { > + fputs(errstr, logf); > + fputc('\n', logf); > + fflush(logf); > } > #ifdef HAVE_SYSLOG > else { > - vsprintf(errstr + strlen(errstr), fmt, args); > - syslog(level, "%s", errstr); > + syslog(level & APLOG_LEVELMASK, "%s", errstr); > } > #endif > - > - va_end(args); > } > + > > void log_pid (pool *p, char *pid_fname) > { > Index: main/http_log.h > =================================================================== > RCS file: /export/home/cvs/apachen/src/main/http_log.h,v > retrieving revision 1.13 > diff -u -r1.13 http_log.h > --- http_log.h 1997/09/12 20:13:08 1.13 > +++ http_log.h 1997/10/02 00:23:05 > @@ -59,6 +59,8 @@ > #define APLOG_INFO 6 /* informational */ > #define APLOG_DEBUG 7 /* debug-level messages */ > #define DEFAULT_LOGLEVEL APLOG_ERR > +#define APLOG_LEVELMASK 15 /* mask off the level value */ > +#define APLOG_NOERRNO 16 > #define APLOG_MARK __FILE__,__LINE__ > > typedef struct _trans { > Index: main/http_main.c > =================================================================== > RCS file: /export/home/cvs/apachen/src/main/http_main.c,v > retrieving revision 1.226 > diff -u -r1.226 http_main.c > --- http_main.c 1997/09/25 01:03:22 1.226 > +++ http_main.c 1997/10/02 00:23:08 > @@ -715,7 +715,8 @@ > } > > if (!current_conn->keptalive) > - aplog_error(APLOG_MARK, APLOG_DEBUG, current_conn->server, errstr); > + aplog_error(APLOG_MARK, APLOG_NOERRNO|APLOG_DEBUG, > + current_conn->server, errstr); > > if (timeout_req) { > /* Someone has asked for this transaction to just be aborted > @@ -1331,7 +1332,6 @@ > > static void setup_shared_mem(void) > { > - char errstr[MAX_STRING_LEN]; > struct shmid_ds shmbuf; > #ifdef MOVEBREAK > char *obrk; > @@ -1340,18 +1340,18 @@ > if ((shmid = shmget(shmkey, SCOREBOARD_SIZE, IPC_CREAT | SHM_R | SHM_W)) == -1) { > #ifdef LINUX > if (errno == ENOSYS) { > - fprintf(stderr, > + aplog_error(APLOG_MARK, APLOG_NOERRNO|APLOG_EMERG, server_conf, > "httpd: Your kernel was built without CONFIG_SYSVIPC\n" > "httpd: please consult the Apache FAQ for details\n"); > } > #endif > - perror("shmget"); > - fprintf(stderr, "httpd: Could not call shmget\n"); > + aplog_error(APLOG_MARK, APLOG_EMERG, server_conf, > + "could not call shmget"); > exit(1); > } > > - ap_snprintf(errstr, sizeof(errstr), "created shared memory segment #%d", shmid); > - aplog_error(APLOG_MARK, APLOG_INFO, server_conf, errstr); > + aplog_error(APLOG_MARK, APLOG_NOERRNO|APLOG_INFO, server_conf, > + "created shared memory segment #%d", shmid); > > #ifdef MOVEBREAK > /* > @@ -1364,30 +1364,29 @@ > * attach the segment and then move break back down. Ugly > */ > if ((obrk = sbrk(MOVEBREAK)) == (char *) -1) { > - perror("sbrk"); > - fprintf(stderr, "httpd: Could not move break\n"); > + aplog_error(APLOG_MARK, APLOG_ERR, server_conf, > + "sbrk() could not move break"); > } > #endif > > #define BADSHMAT ((scoreboard *)(-1)) > if ((scoreboard_image = (scoreboard *) shmat(shmid, 0, 0)) == BADSHMAT) { > - perror("shmat"); > - fprintf(stderr, "httpd: Could not call shmat\n"); > + aplog_error(APLOG_MARK, APLOG_EMERG, server_conf, "shmat error"); > /* > * We exit below, after we try to remove the segment > */ > } > else { /* only worry about permissions if we attached the segment */ > if (shmctl(shmid, IPC_STAT, &shmbuf) != 0) { > - perror("shmctl"); > - fprintf(stderr, "httpd: Could not stat segment #%d\n", shmid); > + aplog_error(APLOG_MARK, APLOG_ERR, server_conf, > + "shmctl() could not stat segment #%d", shmid); > } > else { > shmbuf.shm_perm.uid = user_id; > shmbuf.shm_perm.gid = group_id; > if (shmctl(shmid, IPC_SET, &shmbuf) != 0) { > - perror("shmctl"); > - fprintf(stderr, "httpd: Could not set segment #%d\n", shmid); > + aplog_error(APLOG_MARK, APLOG_ERR, server_conf, > + "shmctl() could not set segment #%d", shmid); > } > } > } > @@ -1396,12 +1395,9 @@ > * (small) tables. > */ > if (shmctl(shmid, IPC_RMID, NULL) != 0) { > - perror("shmctl"); > - fprintf(stderr, "httpd: Could not delete segment #%d\n", shmid); > - ap_snprintf(errstr, sizeof(errstr), > - "could not remove shared memory segment #%d", shmid); > aplog_error(APLOG_MARK, APLOG_WARNING, server_conf, > - "shmctl: IPC_RMID: %s", errstr); > + "shmctl: IPC_RMID: could not remove shared memory segment #%d", > + shmid); > } > if (scoreboard_image == BADSHMAT) /* now bailout */ > exit(1); > @@ -1410,8 +1406,8 @@ > if (obrk == (char *) -1) > return; /* nothing else to do */ > if (sbrk(-(MOVEBREAK)) == (char *) -1) { > - perror("sbrk"); > - fprintf(stderr, "httpd: Could not move break back\n"); > + aplog_error(APLOG_MARK, APLOG_ERR, server_conf, > + "sbrk() could not move break back"); > } > #endif > scoreboard_image->global.exit_generation = 0; > @@ -1752,21 +1748,21 @@ > switch (tries) { > case 1: > /* perhaps it missed the SIGHUP, lets try again */ > - aplog_error(APLOG_MARK, APLOG_ERR, server_conf, > + aplog_error(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, server_conf, > "child process %d did not exit, sending another SIGHUP", > pid); > kill(pid, SIGHUP); > break; > case 2: > /* ok, now it's being annoying */ > - aplog_error(APLOG_MARK, APLOG_ERR, server_conf, > + aplog_error(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, server_conf, > "child process %d still did not exit, sending a SIGTERM", > pid); > kill(pid, SIGTERM); > break; > case 3: > /* die child scum */ > - aplog_error(APLOG_MARK, APLOG_ERR, server_conf, > + aplog_error(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, server_conf, > "child process %d still did not exit, sending a SIGKILL", > pid); > kill(pid, SIGKILL); > @@ -1777,7 +1773,7 @@ > * exited, we will likely fail to bind to the port > * after the restart. > */ > - aplog_error(APLOG_MARK, APLOG_ERR, server_conf, > + aplog_error(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, server_conf, > "could not make child process %d exit, " > "attempting to continue anyway", pid); > break; > @@ -1914,7 +1910,8 @@ > ap_snprintf(emsg, sizeof(emsg), > "httpd: caught SIGBUS, attempting to dump core in %s", > coredump_dir); > - aplog_error(APLOG_MARK, APLOG_INFO, server_conf, emsg); > + /* don't use any specific server record, no idea if it's been messed up */ > + aplog_error(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, NULL, emsg); > chdir(coredump_dir); > abort(); > exit(1); > @@ -1927,7 +1924,8 @@ > ap_snprintf(emsg, sizeof(emsg), > "httpd: caught SIGSEGV, attempting to dump core in %s", > coredump_dir); > - aplog_error(APLOG_MARK, APLOG_INFO, server_conf, emsg); > + /* don't use any specific server record, no idea if it's been messed up */ > + aplog_error(APLOG_MARK, APLOG_NOERRNO|APLOG_INFO, NULL, emsg); > chdir(coredump_dir); > abort(); > exit(1); > @@ -2266,7 +2264,7 @@ > } > p += ap_snprintf(p, sizeof(buf) - (p - buf), " %ux%u", > total, count[VHASH_TABLE_SIZE - 1]); > - aplog_error(APLOG_MARK, APLOG_DEBUG, server_conf, buf); > + aplog_error(APLOG_MARK, APLOG_NOERRNO|APLOG_DEBUG, server_conf, buf); > } > #endif > > @@ -2871,7 +2869,7 @@ > #ifdef LINUX > if (errno == EFAULT) { > aplog_error(APLOG_MARK, APLOG_ERR, server_conf, > - "select: (listen) fatal, exiting"); > + "select: (listen) fatal, child exiting"); > child_exit_modules(pconf, server_conf); > destroy_pool(pconf); > exit(1); > @@ -3250,7 +3248,7 @@ > static int reported = 0; > > if (!reported) { > - aplog_error(APLOG_MARK, APLOG_ERR, server_conf, > + aplog_error(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, server_conf, > "server reached MaxClients setting, consider" > " raising the MaxClients setting"); > reported = 1; > @@ -3259,7 +3257,7 @@ > } > else { > if (idle_spawn_rate >= 4) { > - aplog_error(APLOG_MARK, APLOG_ERR, server_conf, > + aplog_error(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, server_conf, > "server seems busy, spawning %d children (you may need " > "to increase StartServers, or Min/MaxSpareServers)", > idle_spawn_rate); > @@ -3364,11 +3362,11 @@ > hold_off_on_exponential_spawning = 10; > } > > - aplog_error(APLOG_MARK, APLOG_INFO, server_conf, > + aplog_error(APLOG_MARK, APLOG_NOERRNO|APLOG_INFO, server_conf, > "Apache HTTP Server version: %s", SERVER_VERSION); > - aplog_error(APLOG_MARK, APLOG_INFO, server_conf, > + aplog_error(APLOG_MARK, APLOG_NOERRNO|APLOG_INFO, server_conf, > "Server built: %s", SERVER_BUILT); > - aplog_error(APLOG_MARK, APLOG_INFO, server_conf, > + aplog_error(APLOG_MARK, APLOG_NOERRNO|APLOG_INFO, server_conf, > "Server configured -- resuming normal operations"); > restart_pending = shutdown_pending = 0; > > @@ -3408,7 +3406,7 @@ > * scoreboard. Somehow we don't know about this > * child. > */ > - aplog_error(APLOG_MARK, APLOG_WARNING, server_conf, > + aplog_error(APLOG_MARK, APLOG_NOERRNO|APLOG_WARNING, server_conf, > "long lost child came home! (pid %d)", pid); > } > /* Don't perform idle maintenance when a child dies, > @@ -3443,7 +3441,7 @@ > aplog_error(APLOG_MARK, APLOG_WARNING, server_conf, "killpg SIGTERM"); > } > reclaim_child_processes(2); /* Start with SIGTERM */ > - aplog_error(APLOG_MARK, APLOG_NOTICE, server_conf, > + aplog_error(APLOG_MARK, APLOG_NOERRNO|APLOG_NOTICE, server_conf, > "httpd: caught SIGTERM, shutting down"); > > /* Clear the pool - including any registered cleanups */ > @@ -3473,7 +3471,7 @@ > scoreboard_image->global.exit_generation = generation; > update_scoreboard_global(); > > - aplog_error(APLOG_MARK, APLOG_NOTICE, server_conf, > + aplog_error(APLOG_MARK, APLOG_NOERRNO|APLOG_NOTICE, server_conf, > "SIGUSR1 received. Doing graceful restart"); > > /* kill off the idle ones */ > @@ -3500,7 +3498,7 @@ > aplog_error(APLOG_MARK, APLOG_WARNING, server_conf, "killpg SIGHUP"); > } > reclaim_child_processes(1); /* Not when just starting up */ > - aplog_error(APLOG_MARK, APLOG_NOTICE, server_conf, > + aplog_error(APLOG_MARK, APLOG_NOERRNO|APLOG_NOTICE, server_conf, > "SIGHUP received. Attempting to restart"); > } > ++generation; > >