Re: mg: dirname, basename
Comments? Looks quite fine, but I noticed there are no NULL checks for the newly allocated strings. Aborting a command gracefully might be better than crashing, should anyone ever be unfortunate enough to hit this. Also: @@ -415,8 +421,11 @@ ewprintf(Directory name too long); return (FALSE); } - if ((bufp = eread(Rename %s to: , toname, - sizeof(toname), EFDEF | EFNEW | EFCR, basename(frname))) == NULL) + s = xbasename(frname); + bufp = eread(Rename %s to: , toname, + sizeof(toname), EFDEF | EFNEW | EFCR, basename(frname));
Re: mg: dirname, basename
Thus, I propose the following respin: implement xdirname and xbasename
using a strlcat/strlcpy semantic. This is more natural in most calls anyway.
Good idea. Being used to the strlfoo functions, it looks a bit strange
to make xplen the middlemost argument, but I'm just being me now. :-)
+ if (*dp dp[0] == '/' dp[1] == '\0') {
Is the *dp check necessary? I realize it's like this in the old code
too, but it caught my attention now.
In any case, the diff looks good, and I found no problems while
testing it.
Re: mg: dirname, basename
Looks quite fine, but I noticed there are no NULL checks for the
newly allocated strings. Aborting a command gracefully might be
better than crashing, should anyone ever be unfortunate enough
to hit this.
Absolutely true. And in thinking about adding such checks, I realize
they just ugify the code. In fact, we have taken the approach to
avoid such allocating calls in favor of static-sized buffers throughout
mg.
Thus, I propose the following respin: implement xdirname and xbasename
using a strlcat/strlcpy semantic. This is more natural in most calls anyway.
-kj
Index: buffer.c
===
RCS file: /cvs/src/usr.bin/mg/buffer.c,v
retrieving revision 1.75
diff -u -r1.75 buffer.c
--- buffer.c18 Jan 2011 17:35:42 - 1.75
+++ buffer.c19 Jan 2011 20:35:51 -
@@ -666,8 +666,7 @@
int count;
size_t remain, len;
- len = strlcpy(bn, basename(fn), bs);
- if (len = bs)
+ if ((len = xbasename(bn, bs, fn)) = bs)
return (FALSE);
remain = bs - len;
Index: def.h
===
RCS file: /cvs/src/usr.bin/mg/def.h,v
retrieving revision 1.115
diff -u -r1.115 def.h
--- def.h 18 Jan 2011 16:25:40 - 1.115
+++ def.h 19 Jan 2011 20:35:52 -
@@ -357,6 +357,7 @@
int makebkfile(int, int);
int writeout(struct buffer *, char *);
voidupmodes(struct buffer *);
+size_t xbasename(char *, size_t, const char *);
/* line.c X */
struct line*lalloc(int);
Index: dired.c
===
RCS file: /cvs/src/usr.bin/mg/dired.c,v
retrieving revision 1.47
diff -u -r1.47 dired.c
--- dired.c 18 Jan 2011 17:35:42 - 1.47
+++ dired.c 19 Jan 2011 20:35:52 -
@@ -335,7 +335,7 @@
d_expunge(int f, int n)
{
struct line *lp, *nlp;
- char fname[NFILEN];
+ char fname[NFILEN], sname[NFILEN];
for (lp = bfirstlp(curbp); lp != curbp-b_headp; lp = nlp) {
nlp = lforw(lp);
@@ -346,15 +346,16 @@
return (FALSE);
case FALSE:
if (unlink(fname) 0) {
- ewprintf(Could not delete '%s',
- basename(fname));
+ (void)xbasename(sname, NFILEN, fname);
+ ewprintf(Could not delete '%s',
sname);
return (FALSE);
}
break;
case TRUE:
if (rmdir(fname) 0) {
- ewprintf(Could not delete directory
'%s',
- basename(fname));
+ (void)xbasename(sname, NFILEN, fname);
+ ewprintf(Could not delete directory
+ '%s', sname);
return (FALSE);
}
break;
@@ -371,7 +372,7 @@
int
d_copy(int f, int n)
{
- charfrname[NFILEN], toname[NFILEN], *bufp;
+ charfrname[NFILEN], toname[NFILEN], sname[NFILEN], *bufp;
int ret;
size_t off;
struct buffer *bp;
@@ -385,8 +386,10 @@
ewprintf(Directory name too long);
return (FALSE);
}
- if ((bufp = eread(Copy %s to: , toname, sizeof(toname),
- EFDEF | EFNEW | EFCR, basename(frname))) == NULL)
+ (void)xbasename(sname, NFILEN, frname);
+ bufp = eread(Copy %s to: , toname, sizeof(toname),
+ EFDEF | EFNEW | EFCR, sname);
+ if (bufp == NULL)
return (ABORT);
else if (bufp[0] == '\0')
return (FALSE);
@@ -405,6 +408,7 @@
int ret;
size_t off;
struct buffer *bp;
+ char sname[NFILEN];
if (d_makename(curwp-w_dotp, frname, sizeof(frname)) != FALSE) {
ewprintf(Not a file);
@@ -415,8 +419,10 @@
ewprintf(Directory name too long);
return (FALSE);
}
- if ((bufp = eread(Rename %s to: , toname,
- sizeof(toname), EFDEF | EFNEW | EFCR, basename(frname))) == NULL)
+ (void)xbasename(sname, NFILEN, frname);
+ bufp = eread(Rename %s to: , toname,
+ sizeof(toname), EFDEF | EFNEW | EFCR, sname);
+ if (bufp == NULL)
return (ABORT);
else if (bufp[0] == '\0')
return (FALSE);
@@ -452,6 +458,7 @@
struct buffer *bp;
struct mgwin*wp;
FILE*fin;
+ char
mg: dirname, basename
And this is the other frequently encountered issue for people
trying to port our mg changes elsewhere. dirname and basename
are usually stupid on other platforms, modifying their input strings,
and other such silliness.
This fixes this issue by wrapping everything in allocating
routines. It requires a couple of extra malloc/frees, but is
infinitely harder to get wrong.
I thought about passing a buffer and length to the basename wrapper,
but this is easier, consistent with xdirname, and, did I mention,
easier to get right?
It looks like xdirname was added to avoid this bug on such platforms,
but as written, it fails to do so. Probably my bad. It is now fixed.
Also, all uses of basename are moved to a consistent xbasename.
Comments?
Index: buffer.c
===
RCS file: /cvs/src/usr.bin/mg/buffer.c,v
retrieving revision 1.75
diff -u -r1.75 buffer.c
--- buffer.c18 Jan 2011 17:35:42 - 1.75
+++ buffer.c19 Jan 2011 03:51:36 -
@@ -665,8 +666,11 @@
{
int count;
size_t remain, len;
+ char*s;
- len = strlcpy(bn, basename(fn), bs);
+ s = xbasename(fn);
+ len = strlcpy(bn, s, bs);
+ free(s);
if (len = bs)
return (FALSE);
Index: def.h
===
RCS file: /cvs/src/usr.bin/mg/def.h,v
retrieving revision 1.115
diff -u -r1.115 def.h
--- def.h 18 Jan 2011 16:25:40 - 1.115
+++ def.h 19 Jan 2011 03:51:36 -
@@ -357,6 +358,7 @@
int makebkfile(int, int);
int writeout(struct buffer *, char *);
voidupmodes(struct buffer *);
+char *xbasename(const char *);
/* line.c X */
struct line*lalloc(int);
Index: dired.c
===
RCS file: /cvs/src/usr.bin/mg/dired.c,v
retrieving revision 1.47
diff -u -r1.47 dired.c
--- dired.c 18 Jan 2011 17:35:42 - 1.47
+++ dired.c 19 Jan 2011 03:51:36 -
@@ -336,6 +336,7 @@
{
struct line *lp, *nlp;
char fname[NFILEN];
+ char*s;
for (lp = bfirstlp(curbp); lp != curbp-b_headp; lp = nlp) {
nlp = lforw(lp);
@@ -346,15 +347,17 @@
return (FALSE);
case FALSE:
if (unlink(fname) 0) {
- ewprintf(Could not delete '%s',
- basename(fname));
+ s = xbasename(fname);
+ ewprintf(Could not delete '%s', s);
+ free(s);
return (FALSE);
}
break;
case TRUE:
if (rmdir(fname) 0) {
- ewprintf(Could not delete directory
'%s',
- basename(fname));
+ s = xbasename(fname);
+ ewprintf(Could not delete directory
'%s', s);
+ free(s);
return (FALSE);
}
break;
@@ -371,7 +374,7 @@
int
d_copy(int f, int n)
{
- charfrname[NFILEN], toname[NFILEN], *bufp;
+ charfrname[NFILEN], toname[NFILEN], *bufp, *s;
int ret;
size_t off;
struct buffer *bp;
@@ -385,8 +388,11 @@
ewprintf(Directory name too long);
return (FALSE);
}
- if ((bufp = eread(Copy %s to: , toname, sizeof(toname),
- EFDEF | EFNEW | EFCR, basename(frname))) == NULL)
+ s = xbasename(frname);
+ bufp = eread(Copy %s to: , toname, sizeof(toname),
+ EFDEF | EFNEW | EFCR, s);
+ free(s);
+ if (bufp == NULL)
return (ABORT);
else if (bufp[0] == '\0')
return (FALSE);
@@ -401,7 +407,7 @@
int
d_rename(int f, int n)
{
- char frname[NFILEN], toname[NFILEN], *bufp;
+ char frname[NFILEN], toname[NFILEN], *bufp, *s;
int ret;
size_t off;
struct buffer *bp;
@@ -415,8 +421,11 @@
ewprintf(Directory name too long);
return (FALSE);
}
- if ((bufp = eread(Rename %s to: , toname,
- sizeof(toname), EFDEF | EFNEW | EFCR, basename(frname))) == NULL)
+ s = xbasename(frname);
+ bufp = eread(Rename %s to: , toname,
+ sizeof(toname), EFDEF | EFNEW | EFCR, basename(frname));
+ free(s);
+ if (bufp == NULL)
return (ABORT);
else if (bufp[0] == '\0')
