Read-only file system causes suboptimal error message

Bugs
1

A power outage between a VM and its storage caused Linux to remount-ro everything. A subsequent automated bk changes failed with:

_bktmp(changes.c, 1603) failedbk: stdio/fopen.c:62: bk_fopen: Assertion `file != ((void *)0)' failed.

I understand that it’s not practical to continue when you can’t create a temporary file. I would have appreciated an explicit mention of:

/usr/include/asm-generic/errno-base.h:#define EROFS 30 /* Read-only file system */

… but I suppose errno has gone by this point and C doesn’t make it as easy as, say, C++ would to divert any earlier strerror reporting into a temporary std::ostringstream, where it could be discarded if a later member of tmpdirs worked. But perhaps we should exit rather than plowing on? I looked at where the later assertion happens and see that it’s quitting safely, so there’s no need to exit in this situation but, fixing the missing newline, that’s got to be uncontroversial:

--- 1.28/src/gettemp.c	2016-02-25 14:20:36 -08:00
+++ edited/src/gettemp.c	2017-06-30 09:58:15 -07:00
@@ -117,7 +117,7 @@ _bktmp
 	for (i = 0; i < tmpdirs_len; i++) {
 		unless (getTempfile(tmpdirs[i], file, line, &buf)) return (buf);
 	}
-	fprintf(stderr, "_bktmp(%s, %d) failed", file, line);
+	fprintf(stderr, "_bktmp(%s, %d) failed\n", file, line);
 	return (0);
 }

In the hope of getting the ball rolling by sending a pull request or something similarly constructive, I went to http://blo-gs.bkbits.net/signup but was eventually rebuffed by:

Content-Type: text/html
error reading "sock1d48940": socket is not connected
    while executing
"smtp::sendmessage(tok, originator: from, recipients: to,
	    servers: smtp_hostname, username: smtp_username,
	    password: smtp_password)"
    (procedure "send_mail" line 17)
    invoked from within
"send_mail(email, "Verify your email address", body)"
    (procedure "signup_action" line 101)
    invoked from within
"signup_action"
    ("uplevel" body line 1)
    invoked from within
"uplevel("#0", "${action}_action")"
    (procedure "route_request" line 90)
    invoked from within
"route_request()"
    (procedure "12%l_toplevel" line 36)
    invoked from within
"12%l_toplevel"
    invoked from within
"L --lineadj=-1"
    (file "/home/bkbits/www/app/route.l" line 1)
    invoked from within
"source(appInit)"

Now I’ve reported two infelicities in one place. Wrong!

2

Responding to some of the problems in this message

bktmp() error message missing newline

Yes indeed and your patch looks pretty good.

assertion doesn’t mention errno EROFS

Yeah, we would need to change the code like this:

--- 1.207/src/changes.c	2016-09-30 10:58:23 -04:00
+++ edited/src/changes.c	2017-07-01 16:30:09 -04:00
@@ -1600,7 +1600,10 @@ send_part1_msg(remote *r, char **av)
 	char	*cmdf, *probef = 0;	/* tmpfiles */
 	char	buf[MAXLINE];
 
-	cmdf = bktmp(0);
+	unless (cmdf = bktmp(0)) {
+		perror("bktmp");
+		return (1);
+	}
 	f = fopen(cmdf, "w");
 	assert(f);
 	sendEnv(f, 0, r, ((opts.remote || opts.local) ? 0 : SENDENV_NOREPO));

Our perror() includes the bk file and line number were the error occurred.

Also, you can set TMPDIR=/tmp in the environment to have bk stop trying to use BitKeeper/tmp in the current repository.

bkbits signup is broken

Someone at Bitmover will have to look into that. I will ping them.

3

You’re suggesting the errno from the last mkstemp will survive for long enough.

…

Reading the man page for fprintf and free, OK, I agree. If at least one caller tries to cope with _bktmp failing, then it’s odd for _bktmp to report an error. If no-one tries to cope, then why not exit there? Because perhaps callers clean up? Changing changes.c does seem like the lower risk choice.

4

Lots of other callers of bktmp() do try to recover from errors so adding an exit() is probably too big of a change. Perhaps just adding the output of strerror() to the error message in _bktmp() is enough.

5

I pushed a simple change to address part of this to the dev repo. (bk://bkbits.net/bk/dev)

http://repos.bkbits.net/bk/dev/?PAGE=patch&REV=599ed2647hPN6wb2kzI_dIAETFw3yA