On 2023/05/28 18:09:00 +0200, Marc Espie <marc.espie.open...@gmail.com> wrote:
> Here's a slightly more specific diff avoiding useless stat(2)

looks fine to me, and in ports it produces a nice error message:

        % chmod 600 Makefile
        % make
        [...]
        make: don't know how to make do-extract
        Stop in .
        Makefile(s) that couldn't be read:
                Makefile: Permission denied

so fwiw ok op@

one comment and a minor style nit below

> [...]
> Index: main.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/make/main.c,v
> retrieving revision 1.129
> diff -u -p -r1.129 main.c
> --- main.c    17 Jan 2023 13:03:22 -0000      1.129
> +++ main.c    28 May 2023 16:07:45 -0000
> [...]
> +static FILE *
> +open_makefile(const char *fname)
> +{
> +     FILE *stream;
> +     struct unreadable *u;
> +
> +     stream = fopen(fname, "r");
> +     if (stream != NULL)
> +             return stream;
> +
> +     if (errno != ENOENT) {
> +             u = emalloc(sizeof *u);
> +             u->fname = strdup(fname);

it's unlikely, but strdup can fail here clobbering errno and leaving
fname NULL.  estrdup() should be used instead.

> +             u->errcode = errno;
> +             Lst_AtEnd(&unreadable, u);
> +     }
> +
> +     return NULL;
> +}
> [...]
> @@ -866,7 +910,14 @@ ReadMakefile(void *p, void *q)
>               name = Dir_FindFile(fname, userIncludePath);
>               if (!name)
>                       name = Dir_FindFile(fname, systemIncludePath);
> -             if (!name || !(stream = fopen(name, "r")))
> +             if (!name)
> +                     return false;
> +             /* do not try to open a file we already have, so that
> +              * dump_unreadable() yields non-confusing results.
> +              */

other multiline comments are spelled as

                /*
                 * do not try to open a file we already have, so that
                 * dump_unreadable() yields non-confusing results.
                 */

and btw thanks for adding this comment, figuring out why this was
needed wouldn't have been straightforward to me.

> +             if (strcmp(name, fname) == 0)
> +                     return false;
> +             if ((stream = open_makefile(name)) == NULL)
>                       return false;
>               fname = name;
>               /*

Reply via email to