Re: [patch] cleanup vi
2016-01-23 14:08 GMT+03:00 Theo Buehler :
> On Sat, Jan 23, 2016 at 11:03:39AM +0100, Martijn van Duren wrote:
>> Here's a small update:
>> On 01/22/16 21:18, Martijn van Duren wrote:
>> >3) 3_vi_remove_progname.diff: Don't keep a copy of the progname in
>> >memory, use getprogname instead.
>> Attached without the setprogname. The reason I included setprogname was
>> because getprogname in the manpage doesn't mention that it always returns a
>> stripped version and it's a strict requirement of vi.
>> >4) 4_vi_remove_tail.diff: Use basename instead of tail
>> The second basename is inside #define DEBUG, so I missed it in the compile
>> check.
>> Furthermore it's only used for msgq, which uses it it vsnprint and I can't
>> imagine it can hurt in there.
>> As for the input, it's used with a variable file and __FILE__, so it
>> shouldn't cause any more harm than tail.
>
> patches 1-4 and 6 are fine with me now, modulo s/baneame/basename in
> vi/vs_refresh.c:477 (I think that's what zhuk@ meant).
>
> I must say I liked the previous version of 5 better.
>
> I haven't checked all of 5, but the addition of these msgq(..) calls in
> common/{screen.c,seq.c} looks wrong to me. The error paths goto mem*
> already contain such a call to msgq.
I absolutely agree that those calls look like wrong and could do bad
things in case of ENOMEM. My point is that we should make them visible
first, by doing a simple, reviewable change like unmacrosing the C
module files, and only then remove/rework stuff.
--
WBR,
Vadim Zhukov
Re: [patch] cleanup vi
On Sat, Jan 23, 2016 at 11:03:39AM +0100, Martijn van Duren wrote:
> Here's a small update:
> On 01/22/16 21:18, Martijn van Duren wrote:
> >3) 3_vi_remove_progname.diff: Don't keep a copy of the progname in
> >memory, use getprogname instead.
> Attached without the setprogname. The reason I included setprogname was
> because getprogname in the manpage doesn't mention that it always returns a
> stripped version and it's a strict requirement of vi.
> >4) 4_vi_remove_tail.diff: Use basename instead of tail
> The second basename is inside #define DEBUG, so I missed it in the compile
> check.
> Furthermore it's only used for msgq, which uses it it vsnprint and I can't
> imagine it can hurt in there.
> As for the input, it's used with a variable file and __FILE__, so it
> shouldn't cause any more harm than tail.
patches 1-4 and 6 are fine with me now, modulo s/baneame/basename in
vi/vs_refresh.c:477 (I think that's what zhuk@ meant).
I must say I liked the previous version of 5 better.
I haven't checked all of 5, but the addition of these msgq(..) calls in
common/{screen.c,seq.c} looks wrong to me. The error paths goto mem*
already contain such a call to msgq.
Re: [patch] cleanup vi
Here's a small update:
On 01/22/16 21:18, Martijn van Duren wrote:
3) 3_vi_remove_progname.diff: Don't keep a copy of the progname in
memory, use getprogname instead.
Attached without the setprogname. The reason I included setprogname was
because getprogname in the manpage doesn't mention that it always
returns a stripped version and it's a strict requirement of vi.
4) 4_vi_remove_tail.diff: Use basename instead of tail
The second basename is inside #define DEBUG, so I missed it in the
compile check.
Furthermore it's only used for msgq, which uses it it vsnprint and I
can't imagine it can hurt in there.
As for the input, it's used with a variable file and __FILE__, so it
shouldn't cause any more harm than tail.
5) 5_vi_remove_v_strdup.diff: Use strdup instead of v_strdup.
Attached an updated patch that returns the msgq calls. I don't feel
comfortable towards these, since msgq calls GET_SPACE_GOTO, which might
call REALLOC if the buffer isn't big enough, which in turn calls msgq
again, which ...
diff --git a/cl/cl_main.c b/cl/cl_main.c
index 11bdabb..34e9548 100644
--- a/cl/cl_main.c
+++ b/cl/cl_main.c
@@ -36,7 +36,7 @@ sigset_t __sigblockset; /*
GLOBAL: Blocked signals. */
static void cl_func_std(GS *);
static CL_PRIVATE *cl_init(GS *);
-static GS*gs_init(char *);
+static GS*gs_init(void);
static intsetsig(int, struct sigaction *, void (*)(int));
static void sig_end(GS *);
static void term_init(char *);
@@ -60,7 +60,7 @@ main(int argc, char *argv[])
abort();
/* Create and initialize the global structure. */
- __global_list = gp = gs_init(argv[0]);
+ __global_list = gp = gs_init();
/* Create and initialize the CL_PRIVATE structure. */
clp = cl_init(gp);
@@ -141,22 +141,15 @@ main(int argc, char *argv[])
* Create and partially initialize the GS structure.
*/
static GS *
-gs_init(char *name)
+gs_init(void)
{
GS *gp;
- char *p;
-
- /* Figure out what our name is. */
- if ((p = strrchr(name, '/')) != NULL)
- name = p + 1;
/* Allocate the global structure. */
CALLOC_NOMSG(NULL, gp, 1, sizeof(GS));
if (gp == NULL)
err(1, NULL);
-
- gp->progname = name;
return (gp);
}
diff --git a/common/gs.h b/common/gs.h
index d1e94de..c47173f 100644
--- a/common/gs.h
+++ b/common/gs.h
@@ -55,8 +55,6 @@ typedef enum { KEY_VEOF, KEY_VERASE, KEY_VKILL, KEY_VWERASE }
scr_keyval_t;
* Structure that describes global state of the running program.
*/
struct _gs {
- char*progname; /* Programe name. */
-
int id;/* Last allocated screen id. */
TAILQ_HEAD(_dqh, _scr) dq; /* Displayed screens. */
TAILQ_HEAD(_hqh, _scr) hq; /* Hidden screens. */
diff --git a/common/main.c b/common/main.c
index 25decbf..028e1c7 100644
--- a/common/main.c
+++ b/common/main.c
@@ -95,12 +95,12 @@ editor(GS *gp, int argc, char *argv[])
/* Set initial screen type and mode based on the program name. */
readonly = 0;
- if (!strcmp(gp->progname, "ex") || !strcmp(gp->progname, "nex"))
+ if (!strcmp(getprogname(), "ex") || !strcmp(getprogname(), "nex"))
LF_INIT(SC_EX);
else {
/* Nview, view are readonly. */
- if (!strcmp(gp->progname, "nview") ||
- !strcmp(gp->progname, "view"))
+ if (!strcmp(getprogname(), "nview") ||
+ !strcmp(getprogname(), "view"))
readonly = 1;
/* Vi is the default. */
@@ -121,11 +121,11 @@ editor(GS *gp, int argc, char *argv[])
F_SET(gp, G_SNAPSHOT);
pmode = MODE_EX;
- if (!strcmp(gp->progname, "ex"))
+ if (!strcmp(getprogname(), "ex"))
pmode = MODE_EX;
- else if (!strcmp(gp->progname, "vi"))
+ else if (!strcmp(getprogname(), "vi"))
pmode = MODE_VI;
- else if (!strcmp(gp->progname, "view"))
+ else if (!strcmp(getprogname(), "view"))
pmode = MODE_VIEW;
while ((ch = getopt(argc, argv, optstr[pmode])) != -1)
diff --git a/common/recover.c b/common/recover.c
index 6d6f1e1..c7f9796 100644
--- a/common/recover.c
+++ b/common/recover.c
@@ -397,8 +397,8 @@ rcv_mailfile(SCR *sp, int issync, char *cp_path)
" was editing a file named ", t, " on the machine ",
host, ", when it was saved for recovery. ",
"You can recover most, if not all, of the changes ",
- "to this file using the -r option to ", gp->progname, ":\n\n\t",
- gp->progname, " -r ", t);
+ "to this file using the -r option to ", getprogname(), ":\n\n\t",
+ getprogname(), " -r ", t);
if (len > sizeof(buf) - 1) {
lerr: msgq(sp, M_ERR, "Recovery file buffer overrun");
Re: [patch] cleanup vi
On Sat, Jan 23, 2016 at 01:45:05AM -0500, Michael McConville wrote:
> Vadim Zhukov wrote:
> > 2016-01-22 23:18 GMT+03:00 Martijn van Duren
> > :
> > > 5) 5_vi_remove_v_strdup.diff: Use strdup instead of v_strdup.
> >
> > Not okay: MALLOC macro prints information message to user, while
> > patched version isn't. I think refactoring shouldn't have such visible
> > impact on functionality, to make sure nothing breaks.
>
> On the subject of the allocation macros:
>
> o maybe we should wrap them in do { ... } while (0) for safety's sake
>
> o I got the *_NOMSG removals upstreamed. Is the below diff ok? No
>binary change.
I already gave you my ok for this early December... I think it is still
ok, but maybe you could wait until martijn's diffs are reviewed and in?
This is enough of a mouthful already.
Re: [patch] cleanup vi
Vadim Zhukov wrote:
> 2016-01-22 23:18 GMT+03:00 Martijn van Duren
> :
> > 5) 5_vi_remove_v_strdup.diff: Use strdup instead of v_strdup.
>
> Not okay: MALLOC macro prints information message to user, while
> patched version isn't. I think refactoring shouldn't have such visible
> impact on functionality, to make sure nothing breaks.
On the subject of the allocation macros:
o maybe we should wrap them in do { ... } while (0) for safety's sake
o I got the *_NOMSG removals upstreamed. Is the below diff ok? No
binary change.
Index: cl/cl_main.c
===
RCS file: /cvs/src/usr.bin/vi/cl/cl_main.c,v
retrieving revision 1.28
diff -u -p -r1.28 cl_main.c
--- cl/cl_main.c28 Dec 2015 19:24:01 - 1.28
+++ cl/cl_main.c23 Jan 2016 06:40:49 -
@@ -151,7 +151,7 @@ gs_init(char *name)
name = p + 1;
/* Allocate the global structure. */
- CALLOC_NOMSG(NULL, gp, 1, sizeof(GS));
+ gp = calloc(1, sizeof(GS));
if (gp == NULL)
err(1, NULL);
@@ -171,7 +171,7 @@ cl_init(GS *gp)
int fd;
/* Allocate the CL private structure. */
- CALLOC_NOMSG(NULL, clp, 1, sizeof(CL_PRIVATE));
+ clp = calloc(1, sizeof(CL_PRIVATE));
if (clp == NULL)
err(1, NULL);
gp->cl_private = clp;
Index: common/main.c
===
RCS file: /cvs/src/usr.bin/vi/common/main.c,v
retrieving revision 1.33
diff -u -p -r1.33 main.c
--- common/main.c 9 Jan 2016 16:13:26 - 1.33
+++ common/main.c 23 Jan 2016 06:40:49 -
@@ -361,7 +361,7 @@ editor(GS *gp, int argc, char *argv[])
size_t l;
/* Cheat -- we know we have an extra argv slot. */
l = strlen(sp->frp->name) + 1;
- MALLOC_NOMSG(sp, *--argv, l);
+ *--argv = malloc(l);
if (*argv == NULL) {
v_estr(gp->progname, errno, NULL);
goto err;
@@ -561,7 +561,7 @@ v_obsolete(char *name, char *argv[])
} else {
p = argv[0];
len = strlen(argv[0]);
- MALLOC_NOMSG(NULL, argv[0], len + 2);
+ argv[0] = malloc(len + 2);
if (argv[0] == NULL)
goto nomem;
argv[0][0] = '-';
Index: common/mem.h
===
RCS file: /cvs/src/usr.bin/vi/common/mem.h,v
retrieving revision 1.7
diff -u -p -r1.7 mem.h
--- common/mem.h7 Dec 2015 20:39:19 - 1.7
+++ common/mem.h23 Jan 2016 06:40:49 -
@@ -120,9 +120,6 @@
if (((p) = calloc((nmemb), (size))) == NULL)\
goto alloc_err; \
}
-#defineCALLOC_NOMSG(sp, p, nmemb, size) {
\
- (p) = calloc((nmemb), (size)); \
-}
#defineCALLOC_RET(sp, p, nmemb, size) {
\
if (((p) = calloc((nmemb), (size))) == NULL) { \
msgq((sp), M_SYSERR, NULL); \
@@ -137,9 +134,6 @@
#defineMALLOC_GOTO(sp, p, size) {
\
if (((p) = malloc(size)) == NULL) \
goto alloc_err; \
-}
-#defineMALLOC_NOMSG(sp, p, size) {
\
- (p) = malloc(size); \
}
#defineMALLOC_RET(sp, p, size) {
\
if (((p) = malloc(size)) == NULL) { \
Re: [patch] cleanup vi
from part 3: > diff --git cl/cl_main.c cl/cl_main.c > index ed4abda..cc48ad3 100644 > --- cl/cl_main.c > +++ cl/cl_main.c > @@ -19,6 +19,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -36,7 +37,7 @@ sigset_t __sigblockset; /* > GLOBAL: Blocked signals. */ > > static void cl_func_std(GS *); > static CL_PRIVATE *cl_init(GS *); > -static GS *gs_init(char *); > +static GS *gs_init(void); > static int setsig(int, struct sigaction *, void (*)(int)); > static void sig_end(GS *); > static void term_init(char *); > @@ -59,8 +60,10 @@ main(int argc, char *argv[]) > if (reenter++) > abort(); > > + /* Set progname for compatibility reasons */ > + setprogname(basename(argv[0])); I'm a bit confused about this. First, the basename call is unnecessary since setprogname() internally strips everything up to and including the last slash. Second, why is setprogname() necessary at all? The result of getprogname() is the string stored in __progname, which in turn is what follows the last slash in argv[0], see lib/csu/crt0.c:91. So this seems to be a no-op. What am I missing?
Re: [patch] cleanup vi
2016-01-22 23:18 GMT+03:00 Martijn van Duren : > Hello tech@, > > Attached are 6 patches to do some initial cleanup in vi (these complement > the perr patch that got comitted earlier). They are intended to be applied > in sequential order, because there may be slight overlap, but they (almost) > all do more or less the same thing: Remove custom code with their libc > counterpart. > > 1) 1_vi_prefer_errx.diff: don't use fprintf+exit if we can call errx Okay. > 2) 2_vi_remove_v_estr.diff: v_estr is practically warn and warnx combined. Okay. > 3) 3_vi_remove_progname.diff: Don't keep a copy of the progname in memory, > use getprogname instead. Okay. > 4) 4_vi_remove_tail.diff: Use basename instead of tail The basename(3) returns a pointer to internal buffer, which could be overwritten by subsequent calls. Also, basename(3) could return ".", it compacts slashes and so on... Did you check all those differences? Also, it looks like you have a typo in vi/vs_refresh.c. > 5) 5_vi_remove_v_strdup.diff: Use strdup instead of v_strdup. Not okay: MALLOC macro prints information message to user, while patched version isn't. I think refactoring shouldn't have such visible impact on functionality, to make sure nothing breaks. > 6) 6_vi_use_tmp.diff: /var/tmp is dead, long live /tmp Okay. > The first four patches have also been accepted by the nvi2 project. > I haven't offered the fifth patch, since they also included a v_wstrdup > function which takes CHAR_T, which in their code can be an u_char or a > wchar_t depending on the compile options and I don't want to get entangled > in there. > The 6th patch should speak for itself. Cool, thank you for work! -- WBR, Vadim Zhukov
[patch] cleanup vi
Hello tech@,
Attached are 6 patches to do some initial cleanup in vi (these
complement the perr patch that got comitted earlier). They are intended
to be applied in sequential order, because there may be slight overlap,
but they (almost) all do more or less the same thing: Remove custom code
with their libc counterpart.
1) 1_vi_prefer_errx.diff: don't use fprintf+exit if we can call errx
2) 2_vi_remove_v_estr.diff: v_estr is practically warn and warnx combined.
3) 3_vi_remove_progname.diff: Don't keep a copy of the progname in
memory, use getprogname instead.
4) 4_vi_remove_tail.diff: Use basename instead of tail
5) 5_vi_remove_v_strdup.diff: Use strdup instead of v_strdup.
6) 6_vi_use_tmp.diff: /var/tmp is dead, long live /tmp
The first four patches have also been accepted by the nvi2 project.
I haven't offered the fifth patch, since they also included a v_wstrdup
function which takes CHAR_T, which in their code can be an u_char or a
wchar_t depending on the compile options and I don't want to get
entangled in there.
The 6th patch should speak for itself.
Martijn
diff --git cl/cl_main.c cl/cl_main.c
index 8cf9045..ed4abda 100644
--- cl/cl_main.c
+++ cl/cl_main.c
@@ -39,7 +39,7 @@ static CL_PRIVATE *cl_init(GS *);
static GS*gs_init(char *);
static intsetsig(int, struct sigaction *, void (*)(int));
static void sig_end(GS *);
-static void term_init(char *, char *);
+static void term_init(char *);
/*
* main --
@@ -73,7 +73,7 @@ main(int argc, char *argv[])
*/
if ((ttype = getenv("TERM")) == NULL)
ttype = "unknown";
- term_init(gp->progname, ttype);
+ term_init(ttype);
/* Add the terminal type to the global structure. */
if ((OG_D_STR(gp, GO_TERM) =
@@ -211,7 +211,7 @@ tcfail: err(1, "tcgetattr");
* Initialize terminal information.
*/
static void
-term_init(char *name, char *ttype)
+term_init(char *ttype)
{
int err;
@@ -219,13 +219,10 @@ term_init(char *name, char *ttype)
setupterm(ttype, STDOUT_FILENO, &err);
switch (err) {
case -1:
- (void)fprintf(stderr,
- "%s: No terminal database found\n", name);
+ errx(1, "No terminal database found\n");
exit (1);
case 0:
- (void)fprintf(stderr,
- "%s: %s: unknown terminal type\n", name, ttype);
- exit (1);
+ errx(1, "%s: unknown terminal type\n", ttype);
}
}
diff --git common/main.c common/main.c
index f0e2093..25decbf 100644
--- common/main.c
+++ common/main.c
@@ -17,6 +17,7 @@
#include
#include
+#include
#include
#include
#include
@@ -32,8 +33,7 @@
#ifdef DEBUG
static void attach(GS *);
#endif
-static void v_estr(char *, int, char *);
-static int v_obsolete(char *, char *[]);
+static int v_obsolete(char *[]);
/*
* editor --
@@ -108,7 +108,7 @@ editor(GS *gp, int argc, char *argv[])
}
/* Convert old-style arguments into new-style ones. */
- if (v_obsolete(gp->progname, argv))
+ if (v_obsolete(argv))
return (1);
/* Parse the arguments. */
@@ -136,8 +136,7 @@ editor(GS *gp, int argc, char *argv[])
* We should support multiple -c options.
*/
if (gp->c_option != NULL) {
- v_estr(gp->progname, 0,
- "only one -c command may be specified.");
+ warnx("only one -c command may be specified.");
return (1);
}
gp->c_option = optarg;
@@ -152,8 +151,7 @@ editor(GS *gp, int argc, char *argv[])
attach(gp);
break;
default:
- v_estr(gp->progname, 0,
- "-D requires s or w argument.");
+ warnx("-D requires s or w argument.");
return (1);
}
break;
@@ -173,7 +171,7 @@ editor(GS *gp, int argc, char *argv[])
break;
case 'r': /* Recover. */
if (flagchk == 't') {
- v_estr(gp->progname, 0,
+ warnx(
"only one of -r and -t may be specified.");
return (1);
}
@@ -188,7 +186,7 @@ editor(GS *gp, int argc, char *argv[])
#ifdef DEBUG
case 'T': /* Trace. */
if ((gp->tracefp = fopen(optarg, "w")) == NULL) {
- v_estr(gp->progname, errno, optarg);
+ warn("%
