Re: bmake inefficiencies

[Mateusz Guzik <mjguzik%gmail.com@localhost>]

On 2/5/21, Simon J. Gerraty <sjg%juniper.net@localhost> wrote:
> 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.
>

Ok, I did not get this bit.

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

Why keep calling eunlink instead of unlink?

More importantly though, folding all of this into one routine will
facilitate using O_TMPFILE. It's a Linux-specific (for now) flag to
create a file without having to supply a random pattern (and having to
unlink it later, like you do here).

> 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!");
>      }
>


-- 
Mateusz Guzik <mjguzik gmail.com>