[svn] Rewrote the logging code.

Published at <sxs1ywrf300.fsf@florida.arsdigita.de>.

configure

@@ -2150,7 +2150,7 @@ else
 fi
 done
 
-for ac_func in strerror vsnprintf select signal symlink access isatty
+for ac_func in strerror snprintf vsnprintf select signal symlink access isatty
 do
 echo $ac_n "checking for $ac_func""... $ac_c" 1>&6
 echo "configure:2157: checking for $ac_func" >&5

configure.in

@@ -162,7 +162,7 @@ dnl
 AC_FUNC_ALLOCA
 AC_CHECK_FUNCS(strdup strstr strcasecmp strncasecmp)
 AC_CHECK_FUNCS(gettimeofday mktime strptime)
-AC_CHECK_FUNCS(strerror vsnprintf select signal symlink access isatty)
+AC_CHECK_FUNCS(strerror snprintf vsnprintf select signal symlink access isatty)
 AC_CHECK_FUNCS(uname gethostname)
 
 AC_CHECK_FUNCS(gethostbyname, [], [

src/ChangeLog

@@ -1,3 +1,26 @@
+2000-11-05  Hrvoje Niksic  <hniksic@arsdigita.com>
+
+	* log.c (logvprintf): Use vsnprintf() in all cases.  If necessary,
+	resize the buffer to fit the formated message.  That way, messages
+	of arbitrary size may be printed.
+	(logvprintf): Use saved_append() to optionally log the last
+	several lines of output.
+	(logputs): Ditto.
+	(log_close): Adapt to new data structures.
+	(log_dump): Ditto.
+	(redirect_output): Print messages to stderr, not to stdout.
+
+	* log.c (saved_append_1): New function.  Replaces the old logging
+	system ("log all output until 10M characters") with a new, much
+	more reasonable one ("log last screenful of text").
+	(saved_append): New function; call saved_append_1.
+	(free_log_line): New function.
+
+2000-11-05  Hrvoje Niksic  <hniksic@arsdigita.com>
+
+	* url.c (construct): Fix comment.
+	(find_last_char): Document.
+
 2000-11-04  Hrvoje Niksic  <hniksic@arsdigita.com>
 
 	* snprintf.c: New file.

src/config.h.in

@@ -119,6 +119,9 @@ char *alloca ();
 /* Define if you have the strerror function.  */
 #undef HAVE_STRERROR
 
+/* Define if you have the snprintf function.  */
+#undef HAVE_SNPRINTF
+
 /* Define if you have the vsnprintf function.  */
 #undef HAVE_VSNPRINTF
 

src/log.c

@@ -44,26 +44,182 @@ Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.  */
 #ifndef errno
 extern int errno;
 #endif
-
+
-/* We expect no message passed to logprintf() to be bigger than this.
+/* The file descriptor used for logging. */
-   Before a message is printed, we make sure that at least this much
-   room is left for printing it.  */
-#define SAVED_LOG_MAXMSG 32768
-
-/* Maximum allowed growing size.  */
-#define SAVED_LOG_MAXSIZE (10 * 1L << 20)
-
-static char *saved_log;
-/* Size of the current log.  */
-static long saved_log_size;
-/* Offset into the log where we are about to print (size of the
-   used-up part of SAVED_LOG).  */
-static long saved_log_offset;
-/* Whether logging is saved at all.  */
-int save_log_p;
 
 static FILE *logfp;
 
+/* Whether logging is saved at all.  */
+int save_log_p;
+
+/* In the event of a hang-up, and if its output was on a TTY, Wget
+   redirects its output to `wget-log'.
+
+   For the convenience of reading this newly-created log, we store the
+   last several lines ("screenful", hence the choice of 24) of Wget
+   output, and dump them as context when the time comes.  */
+#define SAVED_LOG_LINES 24
+
+/* log_lines is a circular buffer that stores SAVED_LOG_LINES lines of
+   output.  log_line_current always points to the position in the
+   buffer that will be written to next.  When log_line_current reaches
+   SAVED_LOG_LINES, it is reset to zero.
+
+   The problem here is that we'd have to either (re)allocate and free
+   strings all the time, or limit the lines to an arbitrary number of
+   characters.  Instead of settling for either of these, we do both:
+   if the line is smaller than a certain "usual" line length (80 chars
+   by default), a preallocated memory is used.  The rare lines that
+   are longer than 80 characters are malloc'ed and freed separately.
+   This gives good performance with minimum memory consumption and
+   fragmentation.  */
+
+#define STATIC_LENGTH 80
+
+static struct log_ln {
+  char static_line[STATIC_LENGTH + 1]; /* statically allocated
+                                          line. */
+  char *malloced_line;		/* malloc'ed line, for lines of output
+                                   larger than 80 characters. */
+  char *content;		/* this points either to malloced_line
+                                   or to the appropriate static_line.
+                                   If this is NULL, it means the line
+                                   has not yet been used. */
+} log_lines[SAVED_LOG_LINES];
+
+/* The current position in the ring. */
+static int log_line_current = -1;
+
+/* Whether the most recently written line was "trailing", i.e. did not
+   finish with \n.  This is an important piece of information because
+   the code is always careful to append data to trailing lines, rather
+   than create new ones.  */
+static int trailing_line;
+
+
+#define ROT_ADVANCE(num) do {			\
+  if (++num >= SAVED_LOG_LINES)			\
+    num = 0;					\
+} while (0)
+
+/* Free the log line index with NUM.  This calls free on
+   ln->malloced_line if it's non-NULL, and it also resets
+   ln->malloced_line and ln->content to NULL.  */
+
+static void
+free_log_line (int num)
+{
+  struct log_ln *ln = log_lines + num;
+  if (ln->malloced_line)
+    {
+      free (ln->malloced_line);
+      ln->malloced_line = NULL;
+    }
+  ln->content = NULL;
+}
+
+/* Append bytes in the range [start, end) to one line in the log.  The
+   region is not supposed to contain newlines, except for the last
+   character (at end[-1]).  */
+
+static void
+saved_append_1 (const char *start, const char *end)
+{
+  int len = end - start;
+  if (!len)
+    return;
+
+  /* First, check whether we need to append to an existing line or to
+     create a new one.  */
+  if (!trailing_line)
+    {
+      /* Create a new line. */
+      struct log_ln *ln;
+
+      if (log_line_current == -1)
+	log_line_current = 0;
+      else
+	free_log_line (log_line_current);
+      ln = log_lines + log_line_current;
+      if (len > STATIC_LENGTH)
+	{
+	  ln->malloced_line = strdupdelim (start, end);
+	  ln->content = ln->malloced_line;
+	}
+      else
+	{
+	  memcpy (ln->static_line, start, len);
+	  ln->static_line[len] = '\0';
+	  ln->content = ln->static_line;
+	}
+    }
+  else
+    {
+      /* Append to the last line.  If the line is malloc'ed, we just
+         call realloc and append the new string.  If the line is
+         static, we have to check whether appending the new string
+         would make it exceed STATIC_LENGTH characters, and if so,
+         convert it to malloc(). */
+      struct log_ln *ln = log_lines + log_line_current;
+      if (ln->malloced_line)
+	{
+	  /* Resize malloc'ed line and append. */
+	  int old_len = strlen (ln->malloced_line);
+	  ln->malloced_line = xrealloc (ln->malloced_line, old_len + len + 1);
+	  memcpy (ln->malloced_line + old_len, start, len);
+	  ln->malloced_line[old_len + len] = '\0';
+	  /* might have changed due to realloc */
+	  ln->content = ln->malloced_line;
+	}
+      else
+	{
+	  int old_len = strlen (ln->static_line);
+	  if (old_len + len > STATIC_LENGTH)
+	    {
+	      /* Allocate memory and concatenate the old and the new
+                 contents. */
+	      ln->malloced_line = xmalloc (old_len + len + 1);
+	      memcpy (ln->malloced_line, ln->static_line,
+		      old_len);
+	      memcpy (ln->malloced_line + old_len, start, len);
+	      ln->malloced_line[old_len + len] = '\0';
+	      ln->content = ln->malloced_line;
+	    }
+	  else
+	    {
+	      /* Just append to the old, statically allocated
+                 contents.  */
+	      memcpy (ln->static_line + old_len, start, len);
+	      ln->static_line[old_len + len] = '\0';
+	      ln->content = ln->static_line;
+	    }
+	}
+    }
+  trailing_line = !(end[-1] == '\n');
+  if (!trailing_line)
+    ROT_ADVANCE (log_line_current);
+}
+
+/* Log the contents of S, as explained above.  If S consists of
+   multiple lines, they are logged separately.  If S does not end with
+   a newline, it will form a "trailing" line, to which things will get
+   appended the next time this function is called.  */
+
+static void
+saved_append (const char *s)
+{
+  while (*s)
+    {
+      const char *end = strchr (s, '\n');
+      if (!end)
+	end = s + strlen (s);
+      else
+	++end;
+      saved_append_1 (s, end);
+      s = end;
+    }
+}
+
 /* Check X against opt.verbose and opt.quiet.  The semantics is as
    follows:
 
@@ -96,11 +252,15 @@ static FILE *logfp;
   if (logfp == stdin)				\
     return;					\
   else if (!logfp)				\
-    /* #### Should this ever happen?  */	\
+    /* This might happen if somebody calls a */	\
+    /* log* function before log_init(). */	\
     logfp = stderr;				\
 } while (0)
 
 
+/* Log a literal string S.  The string is logged as-is, without a
+   newline appended.  */
+
 void
 logputs (enum log_options o, const char *s)
 {
@@ -110,22 +270,13 @@ logputs (enum log_options o, const char *s)
   fputs (s, logfp);
   if (!opt.no_flush)
     fflush (logfp);
-
+  if (save_log_p)
-  if (save_log_p && saved_log_size < SAVED_LOG_MAXSIZE)
+    saved_append (s);
-    {
-      int len = strlen (s);
-
-      /* Increase size of SAVED_LOG exponentially.  */
-      DO_REALLOC (saved_log, saved_log_size,
-		  saved_log_offset + len + 1, char);
-      memcpy (saved_log + saved_log_offset, s, len + 1);
-      saved_log_offset += len;
-    }
 }
 
-/* Print a message to the log file logfp.  If logfp is NULL, print to
+/* Print a message to the log.  A copy of message will be saved to
-   stderr.  If logfp is stdin, don't print at all.  A copy of message
+   saved_log, for later reusal by log_dump().  */
-   will be saved to saved_log, for later reusal by dump_log().  */
+
 static void
 logvprintf (enum log_options o, const char *fmt, va_list args)
 {
@@ -136,25 +287,67 @@ logvprintf (enum log_options o, const char *fmt, va_list args)
      the message needs to be stored with vsprintf().  However, Watcom
      C didn't like ARGS being used twice, so now we first vsprintf()
      the message, and then fwrite() it to LOGFP.  */
-  if (save_log_p && saved_log_size < SAVED_LOG_MAXSIZE)
+
+  if (!save_log_p)
     {
-      int len;
+      /* In the simple case just call vfprintf(), to avoid needless
-      /* Increase size of `saved_log' exponentially.  */
+         allocation and games with vsnprintf(). */
-      DO_REALLOC (saved_log, saved_log_size,
+      vfprintf (logfp, fmt, args);
-		  saved_log_offset + SAVED_LOG_MAXMSG, char);
-      /* Print the message to the log saver...  */
-      vsnprintf (saved_log + saved_log_offset, SAVED_LOG_MAXMSG, fmt, args);
-      /* ...and then dump it to LOGFP.  */
-      len = strlen (saved_log + saved_log_offset);
-      fwrite (saved_log + saved_log_offset, len, 1, logfp);
-      saved_log_offset += len;
-      /* If we ran off the limits and corrupted something, bail out
-	 immediately.  */
-      assert (saved_log_size >= saved_log_offset);
     }
   else
-    vfprintf (logfp, fmt, args);
+    {
+      char smallmsg[128];
+      char *bigmsg = NULL;
+      int available_size = sizeof (smallmsg);
+      char *write_ptr = smallmsg;
 
+      while (1)
+	{
+	  /* The GNU coding standards advise not to rely on the return
+             value of sprintf().  However, vsnprintf() is a relatively
+             new function missing from legacy systems.  Therefore it's
+             safe to assume that its return value is meaningful.  On
+             the systems where vsnprintf() is not available, we use
+             the implementation from snprintf.c which does return the
+             correct value.  */
+	  int numwritten = vsnprintf (write_ptr, available_size, fmt, args);
+
+	  /* vsnprintf() will not step over the limit given by
+             available_size.  If it fails, it will return either -1
+             (POSIX?) or the number of characters that *would have*
+             been written, if there had been enough room.  In the
+             former case, we double the available_size and malloc() to
+             get a larger buffer, and try again.  In the latter case,
+             we use the returned information to build a buffer of the
+             correct size.  */
+
+	  if (numwritten == -1)
+	    {
+	      /* Writing failed, and we don't know the needed size.
+		 Try again with doubled size. */
+	      available_size <<= 1;
+	      bigmsg = xrealloc (bigmsg, available_size);
+	      write_ptr = bigmsg;
+	    }
+	  else if (numwritten >= available_size)
+	    {
+	      /* Writing failed, but we know exactly how much space we
+		 need. */
+	      available_size = numwritten + 1;
+	      bigmsg = xrealloc (bigmsg, available_size);
+	      write_ptr = bigmsg;
+	    }
+	  else
+	    {
+	      /* Writing succeeded. */
+	      break;
+	    }
+	}
+      saved_append (write_ptr);
+      fputs (write_ptr, logfp);
+      if (bigmsg)
+	free (bigmsg);
+    }
   if (!opt.no_flush)
     fflush (logfp);
 }
@@ -167,7 +360,8 @@ logflush (void)
   fflush (logfp);
 }
 
-/* Portability makes these two functions look like @#%#@$@#$.  */
+/* Portability with pre-ANSI compilers makes these two functions look
+   like @#%#@$@#$.  */
 
 #ifdef WGET_USE_STDARG
 void
@@ -242,7 +436,14 @@ log_init (const char *file, int appendp)
     }
   else
     {
+      /* The log goes to stderr to avoid collisions with the output if
+         the user specifies `-O -'.  #### Francois Pinard suggests
+         that it's a better idea to print to stdout by default, and to
+         stderr only if the user actually specifies `-O -'.  He says
+         this inconsistency is harder to document, but is overall
+         easier on the user.  */
       logfp = stderr;
+
       /* If the output is a TTY, enable logging, which will make Wget
          remember all the printed messages, to be able to dump them to
          a log file in case SIGHUP or SIGUSR1 is received (or
@@ -263,30 +464,45 @@ log_init (const char *file, int appendp)
 void
 log_close (void)
 {
-  fclose (logfp);
+  int i;
+
+  if (logfp != stdin)
+    fclose (logfp);
   save_log_p = 0;
-  FREE_MAYBE (saved_log);
+  for (i = 0; i < SAVED_LOG_LINES; i++)
-  saved_log = NULL;
+    free_log_line (i);
-  saved_log_size = saved_log_offset = 0;
+  log_line_current = -1;
+  trailing_line = 0;
 }
 
-/* Dump SAVED_LOG using logprintf(), but quit further logging to memory.
+/* Dump saved lines to logfp. */
-   Also, free the memory occupied by saved_log.  */
 static void
 log_dump (void)
 {
-  save_log_p = 0;
+  int num = log_line_current;
-  if (!saved_log)
+  FILE *fp = logfp;
+
+  if (num == -1)
     return;
-  logputs (LOG_ALWAYS, saved_log);
+  if (trailing_line)
-  free (saved_log);
+    ROT_ADVANCE (num);
-  saved_log = NULL;
+  do
-  saved_log_size = saved_log_offset = 0;
+    {
+      struct log_ln *ln = log_lines + num;
+      if (ln->content)
+	fputs (ln->content, fp);
+      ROT_ADVANCE (num);
+    }
+  while (num != log_line_current);
+  if (trailing_line)
+    if (log_lines[log_line_current].content)
+      fputs (log_lines[log_line_current].content, fp);
+  fflush (fp);
 }
 
-/* Redirect output to `wget-log' if opt.lfile is stdout.  MESSIJ is
+/* Redirect output to `wget-log'.  MESSIJ is printed on stdout, and
-   printed on stdout, and should contain *exactly one* `%s', which
+   should contain *exactly one* `%s', which will be replaced by the
-   will be replaced by the log file name.
+   log file name.
 
    If logging was not enabled, MESSIJ will not be printed.  */
 void
@@ -301,12 +517,15 @@ redirect_output (const char *messij)
   logfp = fopen (logfile, "w");
   if (!logfp)
     {
-      printf ("%s: %s: %s\n", exec_name, logfile, strerror (errno));
+      /* Eek!  Opening the alternate log file has failed.  Nothing we
-      /* `stdin' is magic to not print anything.  */
+         can do but disable printing completely. */
+      fprintf (stderr, "%s: %s: %s\n", exec_name, logfile, strerror (errno));
+      /* `stdin' is magic to not print anything, ever.  */
       logfp = stdin;
     }
-  printf (messij, logfile);
+  fprintf (stderr, messij, logfile);
   free (logfile);
-  /* Dump all the previous messages to LOGFILE.  */
+  /* Dump the previous screenful of output to LOGFILE.  */
   log_dump ();
+  save_log_p = 0;
 }

src/url.c

@@ -1311,6 +1311,10 @@ urlpath_length (const char *url)
   return strlen (url);
 }
 
+/* Find the last occurrence of character C in the range [b, e), or
+   NULL, if none are present.  This is almost completely equivalent to
+   { *e = '\0'; return strrchr(b); }, except that it doesn't change
+   the contents of the string.  */
 static const char *
 find_last_char (const char *b, const char *e, char c)
 {
@@ -1320,9 +1324,10 @@ find_last_char (const char *b, const char *e, char c)
   return NULL;
 }
 
-/* Construct an absolute URL, given a (possibly) relative one.  This
+/* Construct a URL by concatenating an absolute URL and a path, which
-   gets tricky if you want to cover all the "reasonable" cases, but
+   may or may not be absolute.  This tries to behave "reasonably" in
-   I'm satisfied with the result.  */
+   all foreseeable cases.  It employs little specific knowledge about
+   protocols or URL-specific stuff -- it just works on strings.  */
 static char *
 construct (const char *url, const char *sub, int subsize, int no_proto)
 {