On 2/5/21, Simon J. Gerraty <[email protected]> wrote:
> Mateusz Guzik <[email protected]> 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>
