On Thu, Mar 30, 2023 at 03:47:24PM +0200, Omar Polo wrote:
> There's this "pattern" in mg where it first access(2) a path, then
> does a buch of other things and finally opens it. It can do better.
>
> Diff below removes the access() calls for conffile handling.
> startupfile() now does an ffropen() (fopen() wrapper) instead of
> access() and returns the file via parameter; callers will pass it to
> load() and then close it.
>
> There's one more subtle fix in here that may not be obvious on first
> look. I've moved the adjustname() call from load() to evalfile() to
> avoid doing a double tilde expansion. All call to load(), except
> evalfile(), have a path that's been constructed by mg itself or it has
> been passed by the user via -b/-u, thus not need to be "adjusted".
> Consider:
> % mkdir \~ ; mv ~/.mg \~/ ; mg -u \~/.mg
> which admittedly is a very corner case but shows the problem in doing
> an extra adjustname().
>
> evalfile() -- aka M-x load -- prompts the user for a path so it's not
> unreasonable to call adjustname() on it.
>
> ok?
I have a nit and a comment below.
>
> Index: def.h
> ===================================================================
> RCS file: /home/cvs/src/usr.bin/mg/def.h,v
> retrieving revision 1.177
> diff -u -p -r1.177 def.h
> --- def.h 20 Oct 2022 18:59:24 -0000 1.177
> +++ def.h 30 Mar 2023 11:04:59 -0000
> @@ -486,7 +486,7 @@ int ffputbuf(FILE *, struct buffer *,
> int ffgetline(FILE *, char *, int, int *);
> int fbackupfile(const char *);
> char *adjustname(const char *, int);
> -char *startupfile(char *, char *);
> +char *startupfile(FILE **, char *, char *);
> int copy(char *, char *);
> struct list *make_file_list(char *);
> int fisdir(const char *);
> @@ -594,7 +594,7 @@ int extend(int, int);
> int evalexpr(int, int);
> int evalbuffer(int, int);
> int evalfile(int, int);
> -int load(const char *);
> +int load(FILE *, const char *);
> int excline(char *, int, int);
> char *skipwhite(char *);
>
> Index: extend.c
> ===================================================================
> RCS file: /home/cvs/src/usr.bin/mg/extend.c,v
> retrieving revision 1.77
> diff -u -p -r1.77 extend.c
> --- extend.c 8 Mar 2023 04:43:11 -0000 1.77
> +++ extend.c 30 Mar 2023 11:04:59 -0000
> @@ -620,37 +620,37 @@ evalbuffer(int f, int n)
> int
> evalfile(int f, int n)
> {
> + FILE *ffp;
> char fname[NFILEN], *bufp;
> + int ret;
>
> if ((bufp = eread("Load file: ", fname, NFILEN,
> EFNEW | EFCR)) == NULL)
> return (ABORT);
> - else if (bufp[0] == '\0')
> + if (bufp[0] == '\0')
> return (FALSE);
> - return (load(fname));
> + if ((bufp = adjustname(fname, TRUE)) == NULL)
> + return (FALSE);
> + ret = ffropen(&ffp, bufp, NULL);
Nit: I'd avoid the nesting (I know that you just moved the code):
if (ret == FIODIR)
(void)ffclose(ffp, NULL);
if (ret != FIOSUC)
return (FALSE);
> + if (ret != FIOSUC) {
> + if (ret == FIODIR)
> + (void)ffclose(ffp, NULL);
> + return (FALSE);
> + }
> + ret = load(ffp, bufp);
> + (void)ffclose(ffp, NULL);
> + return (ret);
> }
>
> /*
> * load - go load the file name we got passed.
> */
> int
> -load(const char *fname)
> +load(FILE *ffp, const char *fname)
> {
> - int s = TRUE, line, ret;
> + int s = TRUE, line;
> int nbytes = 0;
> char excbuf[BUFSIZE], fncpy[NFILEN];
> - FILE *ffp;
> -
> - if ((fname = adjustname(fname, TRUE)) == NULL)
> - /* just to be careful */
> - return (FALSE);
> -
> - ret = ffropen(&ffp, fname, NULL);
> - if (ret != FIOSUC) {
> - if (ret == FIODIR)
> - (void)ffclose(ffp, NULL);
> - return (FALSE);
> - }
>
> /* keep a note of fname in case of errors in loaded file. */
> (void)strlcpy(fncpy, fname, sizeof(fncpy));
> @@ -666,7 +666,6 @@ load(const char *fname)
> break;
> }
> }
> - (void)ffclose(ffp, NULL);
> excbuf[nbytes] = '\0';
> if (s != FIOEOF || (nbytes && excline(excbuf, nbytes, ++line) != TRUE))
> return (FALSE);
> Index: fileio.c
> ===================================================================
> RCS file: /home/cvs/src/usr.bin/mg/fileio.c,v
> retrieving revision 1.110
> diff -u -p -r1.110 fileio.c
> --- fileio.c 30 Mar 2023 07:26:15 -0000 1.110
> +++ fileio.c 30 Mar 2023 11:04:59 -0000
> @@ -329,7 +329,7 @@ adjustname(const char *fn, int slashslas
> * to the startup file name.
> */
> char *
> -startupfile(char *suffix, char *conffile)
> +startupfile(FILE **ffp, char *suffix, char *conffile)
> {
> static char file[NFILEN];
> char *home;
> @@ -350,8 +350,13 @@ startupfile(char *suffix, char *conffile
> return (NULL);
> }
>
> - if (access(file, R_OK) == 0)
> + ret = ffropen(ffp, file, NULL);
> + if (ret == FIOSUC)
> return (file);
> + if (ret == FIODIR)
> + (void)ffclose(*ffp, NULL);
> + *ffp = NULL;
> +
> nohome:
> #ifdef STARTUPFILE
> if (suffix == NULL) {
> @@ -365,8 +370,12 @@ nohome:
> return (NULL);
> }
>
> - if (access(file, R_OK) == 0)
> + ret = ffropen(ffp, file, NULL);
> + if (ret == FIOSUC)
> return (file);
> + if (ret == FIODIR)
> + (void)ffclose(*ffp, NULL);
> + *ffp = NULL;
> #endif /* STARTUPFILE */
> return (NULL);
> }
> Index: main.c
> ===================================================================
> RCS file: /home/cvs/src/usr.bin/mg/main.c,v
> retrieving revision 1.92
> diff -u -p -r1.92 main.c
> --- main.c 30 Mar 2023 08:07:07 -0000 1.92
> +++ main.c 30 Mar 2023 13:26:13 -0000
> @@ -62,6 +62,7 @@ usage()
> int
> main(int argc, char **argv)
> {
> + FILE *ffp;
> char *cp, *conffile = NULL, *init_fcn_name = NULL;
> char *batchfile = NULL;
> PF init_fcn = NULL;
> @@ -107,7 +108,7 @@ main(int argc, char **argv)
> pty_init();
> conffile = batchfile;
> }
> - if (conffile != NULL && access(conffile, R_OK) != 0) {
> + if ((cp = startupfile(&ffp, NULL, conffile)) != NULL && conffile) {
This doesn't look right. I'd have expected
if ((cp = startupfile(&ffp, NULL, conffile)) == NULL && conffile) {
(and indeed, your patch breaks -u).
> fprintf(stderr, "%s: Problem with file: %s\n", __progname,
> conffile);
> exit(1);
> @@ -159,8 +160,10 @@ main(int argc, char **argv)
> update(CMODE);
>
> /* user startup file. */
> - if ((cp = startupfile(NULL, conffile)) != NULL)
> - (void)load(cp);
> + if (cp) {
> + (void)load(ffp, cp);
> + ffclose(ffp, NULL);
> + }
>
> if (batch)
> return (0);
> Index: ttykbd.c
> ===================================================================
> RCS file: /home/cvs/src/usr.bin/mg/ttykbd.c,v
> retrieving revision 1.21
> diff -u -p -r1.21 ttykbd.c
> --- ttykbd.c 30 Mar 2023 08:07:07 -0000 1.21
> +++ ttykbd.c 30 Mar 2023 11:04:59 -0000
> @@ -31,6 +31,7 @@ char *keystrings[] = {NULL};
> void
> ttykeymapinit(void)
> {
> + FILE *ffp;
> char *cp;
>
> /* Bind keypad function keys. */
> @@ -57,10 +58,11 @@ ttykeymapinit(void)
> if (key_dc)
> dobindkey(fundamental_map, "delete-char", key_dc);
>
> - if ((cp = getenv("TERM"))) {
> - if (((cp = startupfile(cp, NULL)) != NULL) &&
> - (load(cp) != TRUE))
> + if ((cp = getenv("TERM")) != NULL &&
> + (cp = startupfile(&ffp, cp, NULL)) != NULL) {
> + if (load(ffp, cp) != TRUE)
> ewprintf("Error reading key initialization file");
> + (void)ffclose(ffp, NULL);
> }
> if (keypad_xmit)
> /* turn on keypad */
>
