Re: bmake inefficiencies

["Simon J. Gerraty" <sjg%juniper.net@localhost>]

Mateusz Guzik <mjguzik%gmail.com@localhost> wrote:
> >> is the signal stuff really necessary?
> >
> > It avoids the need to loop dealing with interupts.
> > The name seems missleading should be block not lock.
> >
> 
> I know what it does, it popped up on truss.
> 
> My point is that there are no callers of mkTempFile from a signal
> handler (and would argue if any were present, they should be fixed)
> and the DEBUG stuff seems to only be set at startup time. iow i don't
> see any relevance of signal handling to this code.

The open in mkstemp can fail due to EINTR.
That's the only reason I see for blocking signals.

The patch below eliminates the strdup in mkTempFile
and adds a Job_TempFile so meta.c can get the signal blocking too when
needed.

diff -r 19c080e3f296 bmake/job.c
--- a/bmake/job.c	Mon Feb 01 10:44:48 2021 -0800
+++ b/bmake/job.c	Fri Feb 05 10:16:20 2021 -0800
@@ -1586,8 +1586,7 @@
 	 * are put. It is removed before the child shell is executed,
 	 * unless DEBUG(SCRIPT) is set.
 	 */
-	char *tfile;
-	sigset_t mask;
+	char tfile[MAXPATHLEN];
 	int tfd;		/* File descriptor to the temp file */
 
 	/*
@@ -1599,11 +1598,9 @@
 		DieHorribly();
 	}
 
-	JobSigLock(&mask);
-	tfd = mkTempFile(TMPPAT, &tfile);
+	tfd = Job_TempFile(TMPPAT, tfile, sizeof tfile);
 	if (!DEBUG(SCRIPT))
-		(void)eunlink(tfile);
-	JobSigUnlock(&mask);
+	    eunlink(tfile);
 
 	job->cmdFILE = fdopen(tfd, "w+");
 	if (job->cmdFILE == NULL)
@@ -1620,8 +1617,6 @@
 #endif
 
 	*out_run = JobPrintCommands(job);
-
-	free(tfile);
 }
 
 /*
@@ -2796,6 +2791,20 @@
 		continue;
 }
 
+/* Get a temp file */
+int
+Job_TempFile(const char *pattern, char *tfile, size_t tfile_sz)
+{
+	int fd;
+	sigset_t mask;
+
+	JobSigLock(&mask);
+	fd = mkTempFile(pattern, tfile, tfile_sz);
+	JobSigUnlock(&mask);
+
+	return fd;
+}
+
 /* Prep the job token pipe in the root make process. */
 void
 Job_ServerStart(int max_tokens, int jp_0, int jp_1)
diff -r 19c080e3f296 bmake/job.h
--- a/bmake/job.h	Mon Feb 01 10:44:48 2021 -0800
+++ b/bmake/job.h	Fri Feb 05 10:16:20 2021 -0800
@@ -205,5 +205,6 @@
 void Job_SetPrefix(void);
 Boolean Job_RunTarget(const char *, const char *);
 void Job_FlagsToString(const Job *, char *, size_t);
+int Job_TempFile(const char *, char *, size_t);
 
 #endif /* MAKE_JOB_H */
diff -r 19c080e3f296 bmake/main.c
--- a/bmake/main.c	Mon Feb 01 10:44:48 2021 -0800
+++ b/bmake/main.c	Fri Feb 05 10:16:20 2021 -0800
@@ -2245,27 +2245,29 @@
  * Otherwise unlink the file once open.
  */
 int
-mkTempFile(const char *pattern, char **out_fname)
+mkTempFile(const char *pattern, char *tfile, size_t tfile_sz)
 {
 	static char *tmpdir = NULL;
-	char tfile[MAXPATHLEN];
+	char tbuf[MAXPATHLEN];
 	int fd;
 
 	if (pattern == NULL)
 		pattern = TMPPAT;
 	if (tmpdir == NULL)
 		tmpdir = getTmpdir();
+	if (tfile == NULL) {
+	    tfile = tbuf;
+	    tfile_sz = sizeof tbuf;
+	}
 	if (pattern[0] == '/') {
-		snprintf(tfile, sizeof tfile, "%s", pattern);
+		snprintf(tfile, tfile_sz, "%s", pattern);
 	} else {
-		snprintf(tfile, sizeof tfile, "%s%s", tmpdir, pattern);
+		snprintf(tfile, tfile_sz, "%s%s", tmpdir, pattern);
 	}
 	if ((fd = mkstemp(tfile)) < 0)
 		Punt("Could not create temporary file %s: %s", tfile,
 		    strerror(errno));
-	if (out_fname != NULL) {
-		*out_fname = bmake_strdup(tfile);
-	} else {
+	if (tfile == tbuf) {
 		unlink(tfile);	/* we just want the descriptor */
 	}
 	return fd;
diff -r 19c080e3f296 bmake/make.h
--- a/bmake/make.h	Mon Feb 01 10:44:48 2021 -0800
+++ b/bmake/make.h	Fri Feb 05 10:16:20 2021 -0800
@@ -727,7 +727,7 @@
 void PrintOnError(GNode *, const char *);
 void Main_ExportMAKEFLAGS(Boolean);
 Boolean Main_SetObjdir(Boolean, const char *, ...) MAKE_ATTR_PRINTFLIKE(2, 3);
-int mkTempFile(const char *, char **);
+int mkTempFile(const char *, char *, size_t);
 int str2Lst_Append(StringList *, char *);
 int getInt(const char *, int);
 void GNode_FprintDetails(FILE *, const char *, const GNode *, const char *);
diff -r 19c080e3f296 bmake/meta.c
--- a/bmake/meta.c	Mon Feb 01 10:44:48 2021 -0800
+++ b/bmake/meta.c	Fri Feb 05 10:16:20 2021 -0800
@@ -144,7 +144,10 @@
      * cwd causing getcwd to do a lot more work.
      * We only care about the descriptor.
      */
-    pbm->mon_fd = mkTempFile("filemon.XXXXXX", NULL);
+    if (!opts.compatMake)
+	pbm->mon_fd = Job_TempFile("filemon.XXXXXX", NULL, 0);
+    else
+	pbm->mon_fd = mkTempFile("filemon.XXXXXX", NULL, 0);
     if ((dupfd = dup(pbm->mon_fd)) == -1) {
 	err(1, "Could not dup filemon output!");
     }