distressingly obscure error message on failing to open a file

[Martin Dorey]

This change in magic.c, introduced between 4.23 and 4.24, caused a backup job of ours to produce a large number of mysterious messages saying "couldn't open file".  It took quite some digging to work out (from find /usr/lib -type f | xargs strings | grep) what was producing such a message.

@@ -296,10 +297,12 @@
         errno = 0;
         if ((fd = open(inname, flags)) < 0) {
 #ifdef __CYGWIN__
+            /* FIXME: Do this with EXEEXT from autotools */
             char *tmp = alloca(strlen(inname) + 5);
             (void)strcat(strcpy(tmp, inname), ".exe");
             if ((fd = open(tmp, flags)) < 0) {
 #endif
+                fprintf(stderr, "couldn't open file\n");
                 if (info_from_stat(ms, sb.st_mode) == -1)
                     goto done;
                 rv = 0;

I wanted to submit a patch, so I changed the fprintf line to:

fprintf(stderr, "couldn't open file `%s': %s\n", inname, strerror(errno));

But then I tripped over a problem introduced between the same versions in funcs.c.  The file_printf(ms, f, va) line - the one with the unusual indentation - is passing a va_list to a printf-style function rather than a vprintf-style function.  That crashes for me.

@@ -108,21 +81,17 @@
 file_error_core(struct magic_set *ms, int error, const char *f, va_list va,
     uint32_t lineno)
 {
-	size_t len;
 	/* Only the first error is ok */
 	if (ms->haderr)
 		return;
-	len = 0;
 	if (lineno != 0) {
-		(void)snprintf(ms->o.buf, ms->o.size, "line %u: ", lineno);
-		len = strlen(ms->o.buf);
-	}
-	(void)vsnprintf(ms->o.buf + len, ms->o.size - len, f, va);
-	if (error > 0) {
-		len = strlen(ms->o.buf);
-		(void)snprintf(ms->o.buf + len, ms->o.size - len, " (%s)",
-		    strerror(error));
-	}
+		free(ms->o.buf);
+		ms->o.buf = NULL;
+		file_printf(ms, "line %u: ", lineno);
+	}
+        file_printf(ms, f, va);
+	if (error > 0)
+		file_printf(ms, " (%s)", strerror(error));
 	ms->haderr++;
 	ms->error = error;
 }

Perhaps file_printf should be renamed file_vprintf and given a va_list argument, something like this:

--- funcs.c.old 2008-05-15 15:15:31.000000000 -0700
+++ funcs.c 2008-05-15 15:28:28.000000000 -0700
@@ -45,18 +45,15 @@
  * Like printf, only we append to a buffer.
  */
 protected int
-file_printf(struct magic_set *ms, const char *fmt, ...)
+file_vprintf(struct magic_set *ms, const char *fmt, va_list ap)
 {
-    va_list ap;
     size_t size;
     int len;
     char *buf, *newstr;
 
-    va_start(ap, fmt);
     len = vasprintf(&buf, fmt, ap);
     if (len < 0)
         goto out;
-    va_end(ap);
 
     if (ms->o.buf != NULL) {
         len = asprintf(&newstr, "%s%s", ms->o.buf, buf);
@@ -73,6 +70,18 @@
     return -1;
 }
 
+protected int
+file_printf(struct magic_set *ms, const char *fmt, ...)
+{
+    va_list ap;
+    int len;
+    
+    va_start(ap, fmt);
+    len = file_vprintf(ms, fmt, ap);
+    va_end(ap);
+    return len;
+}
+
 /*
  * error - print best error message possible
  */
@@ -89,7 +98,7 @@
         ms->o.buf = NULL;
         file_printf(ms, "line %u: ", lineno);
     }
-        vprintf(f, va);
+    file_vprintf(ms, f, va);
     if (error > 0)
         file_printf(ms, " (%s)", strerror(error));
     ms->haderr++;

Make sense?