On Wed, 12 Nov 2014 09:40:17 +0100, Martin Natano wrote:
Looks good in general but I have a few comments inline.
- todd
> Index: common/main.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/vi/common/main.c,v
> retrieving revision 1.23
> diff -u -r1.23 main.c
> --- common/main.c 12 Nov 2014 04:28:41 -0000 1.23
> +++ common/main.c 12 Nov 2014 06:36:52 -0000
> @@ -348,13 +348,11 @@
> if (sp->frp != NULL) {
> size_t l;
The 'l' variable is now unused.
> /* Cheat -- we know we have an extra argv slot. */
> - l = strlen(sp->frp->name) + 1;
> - MALLOC_NOMSG(sp, *--argv, char *, l);
> + *--argv = strdup(sp->frp->name);
> if (*argv == NULL) {
> v_estr(gp->progname, errno, NULL);
> goto err;
> }
> - (void)strlcpy(*argv, sp->frp->name, l);
> }
> sp->argv = sp->cargv = argv;
> F_SET(sp, SC_ARGNOFREE);
> @@ -554,14 +552,8 @@
> if (argv[0] == NULL)
> goto nomem;
> } else {
> - p = argv[0];
> - len = strlen(argv[0]);
> - MALLOC_NOMSG(NULL, argv[0], char *, len + 2);
> - if (argv[0] == NULL)
> + if (asprintf(&argv[0], "-c%s", argv[0] + 1) ==
> -1)
After this change variables p and len are now unused.
> goto nomem;
> - argv[0][0] = '-';
> - argv[0][1] = 'c';
> - (void)strlcpy(argv[0] + 2, p + 1, len);
> }
> } else if (argv[0][0] == '-') {
> if (argv[0][1] == '\0') {
> Index: common/screen.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/vi/common/screen.c,v
> retrieving revision 1.11
> diff -u -r1.11 screen.c
> --- common/screen.c 12 Nov 2014 04:28:41 -0000 1.11
> +++ common/screen.c 12 Nov 2014 07:20:51 -0000
> @@ -39,7 +39,7 @@
> size_t len;
>
> *spp = NULL;
> - CALLOC_RET(orig, sp, SCR *, 1, sizeof(SCR));
> + CALLOC_RET(orig, sp, 1, sizeof(SCR));
> *spp = sp;
>
> /* INITIALIZED AT SCREEN CREATE. */
> @@ -84,16 +84,16 @@
> goto mem;
> sp->subre_len = orig->subre_len;
> if (orig->repl != NULL && (sp->repl =
> - v_strdup(sp, orig->repl, orig->repl_len)) == NULL)
> - goto mem;
> + v_strdup(sp, orig->repl, orig->repl_len)) == NULL) {
> +mem: msgq(orig, M_SYSERR, NULL);
> + goto err;
> + }
> sp->repl_len = orig->repl_len;
> if (orig->newl_len) {
> len = orig->newl_len * sizeof(size_t);
> - MALLOC(sp, sp->newl, size_t *, len);
> - if (sp->newl == NULL) {
> -mem: msgq(orig, M_SYSERR, NULL);
> + MALLOC(sp, sp->newl, len);
> + if (sp->newl == NULL)
> goto err;
> - }
I think this is better left as-is with the MALLOC converted to
malloc(). Otherwise, msgq() is called with sp instead of orig.
> sp->newl_len = orig->newl_len;
> sp->newl_cnt = orig->newl_cnt;
> memcpy(sp->newl, orig->newl, len);
