Re: mg: don't access(conffile), just open it
On 2023/03/30 20:09:21 +0200, Theo Buehler wrote:
> > Index: main.c
> [...]
> > @@ -159,8 +162,10 @@ main(int argc, char **argv)
> > update(CMODE);
> >
> > /* user startup file. */
> > - if ((cp = startupfile(NULL, conffile)) != NULL)
> > - (void)load(cp);
> > + if (ffp) {
>
> Could you check this against NULL
>
> if (ffp != NULL) {
>
> Then it's ok.
Thanks!
load now has an unnecessary copy of the file path for no reason, ok to
drop?
Index: extend.c
===
RCS file: /home/cvs/src/usr.bin/mg/extend.c,v
retrieving revision 1.78
diff -u -p -r1.78 extend.c
--- extend.c30 Mar 2023 19:00:02 - 1.78
+++ extend.c30 Mar 2023 20:22:52 -
@@ -649,10 +649,8 @@ load(FILE *ffp, const char *fname)
{
int s = TRUE, line;
int nbytes = 0;
- char excbuf[BUFSIZE], fncpy[NFILEN];
+ char excbuf[BUFSIZE];
- /* keep a note of fname in case of errors in loaded file. */
- (void)strlcpy(fncpy, fname, sizeof(fncpy));
line = 0;
while ((s = ffgetline(ffp, excbuf, sizeof(excbuf) - 1, ))
== FIOSUC) {
@@ -661,7 +659,7 @@ load(FILE *ffp, const char *fname)
if (excline(excbuf, nbytes, line) != TRUE) {
s = FIOERR;
dobeep();
- ewprintf("Error loading file %s at line %d", fncpy,
line);
+ ewprintf("Error loading file %s at line %d", fname,
line);
break;
}
}
Re: mg: don't access(conffile), just open it
> I've also noticed that since startupfile() returns static memory
> errors in -u would report the wrong path (ttykeymapinit() calls
> startupfile() itself) so I'm changing it to use a caller provided
> buffer for the path. tbf i don't completely like it and it also
> causes some churn in the diff in startupfile().
I'm not offended by it. It reads fine.
> Index: main.c
[...]
> @@ -159,8 +162,10 @@ main(int argc, char **argv)
> update(CMODE);
>
> /* user startup file. */
> - if ((cp = startupfile(NULL, conffile)) != NULL)
> - (void)load(cp);
> + if (ffp) {
Could you check this against NULL
if (ffp != NULL) {
Then it's ok.
> + (void)load(ffp, file);
> + ffclose(ffp, NULL);
> + }
>
> if (batch)
> return (0);
Re: mg: don't access(conffile), just open it
On 2023/03/30 18:11:32 +0200, Theo Buehler wrote:
> On Thu, Mar 30, 2023 at 03:47:24PM +0200, Omar Polo wrote:
> > - else if (bufp[0] == '\0')
> > + if (bufp[0] == '\0')
> > return (FALSE);
> > - return (load(fname));
> > + if ((bufp = adjustname(fname, TRUE)) == NULL)
> > + return (FALSE);
> > + ret = ffropen(, 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);
I prefer without the nesting too, changed.
> [...]
> > - if (conffile != NULL && access(conffile, R_OK) != 0) {
> > + if ((cp = startupfile(, NULL, conffile)) != NULL && conffile) {
>
> This doesn't look right. I'd have expected
>
> if ((cp = startupfile(, NULL, conffile)) == NULL && conffile) {
>
> (and indeed, your patch breaks -u).
Arggg... Initially I had startupfile() right before load(), I've moved
it before sending the mail and not re-tested afterwards, sorry!
I've also noticed that since startupfile() returns static memory
errors in -u would report the wrong path (ttykeymapinit() calls
startupfile() itself) so I'm changing it to use a caller provided
buffer for the path. tbf i don't completely like it and it also
causes some churn in the diff in startupfile().
The alternative would be to call ttykeymapinit() after load() in
main.c, and delaying ttykeymapinit(). It'll make the diff smaller but
it feels a bit hackish to keep a pointer to static memory for that
long, and future hackers may be bitten again by this.
(delaying ttykeymapinit() is something on my todo list as well, it
should be called at least after update(CMODE) for errors to be
printed, but it's a separate issue.)
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 - 1.177
+++ def.h 30 Mar 2023 16:34:07 -
@@ -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 *);
+FILE *startupfile(char *, char *, char *, size_t);
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.c8 Mar 2023 04:43:11 - 1.77
+++ extend.c30 Mar 2023 16:22:52 -
@@ -620,37 +620,36 @@ 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(, bufp, NULL);
+ if (ret == FIODIR)
+ (void)ffclose(ffp, NULL);
+ if (ret != FIOSUC)
+ 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(, 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 +665,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:
Re: mg: don't access(conffile), just open it
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 - 1.177
> +++ def.h 30 Mar 2023 11:04:59 -
> @@ -486,7 +486,7 @@ intffputbuf(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 @@ intextend(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 - 1.77
> +++ extend.c 30 Mar 2023 11:04:59 -
> @@ -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(, 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(, 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 - 1.110
> +++ fileio.c 30 Mar 2023 11:04:59 -
> @@ -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
mg: don't access(conffile), just open it
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?
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 - 1.177
+++ def.h 30 Mar 2023 11:04:59 -
@@ -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.c8 Mar 2023 04:43:11 - 1.77
+++ extend.c30 Mar 2023 11:04:59 -
@@ -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(, bufp, NULL);
+ 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(, 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.c30 Mar 2023 07:26:15 - 1.110
+++ fileio.c30 Mar 2023 11:04:59 -
@@ -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;
+
