two append bugs?

two append bugs?

[Clint Adams]

% chmod 0 t

% echo bla >> t || echo failed
zsh: permission denied: t

% /bin/echo bla >> t || echo failed
zsh: permission denied: t   
failed


This happens with other builtins as well.  I assume that this inconsistency
is the first bug.


Now, on an NFS read-only mount, I get:

zsh: no such file or directory: t

whereas 3.0.7 more accurately responds

zsh: read-only file system: t

which I assume to be the other bug.

Re: two append bugs?

[Bart Schaefer]

On Sep 27,  3:46pm, Clint Adams wrote:
} Subject: two append bugs?
}
} % chmod 0 t
} 
} % echo bla >> t || echo failed
} zsh: permission denied: t
} 
} % /bin/echo bla >> t || echo failed
} zsh: permission denied: t   
} failed
} 
} This happens with other builtins as well.  I assume that this inconsistency
} is the first bug.

It's not exactly a bug.  In neither case does zsh execute the "echo"; it
just happens that in the case of the external command, it forks before it
attempts to open the file for redirection.  (It has nothing to do with
whether or not the redirect is an append, so it's not an "append bug".)

It might be possible to get zsh to behave "as if" it had forked in this
circumstance.

} Now, on an NFS read-only mount, I get:
} 
} zsh: no such file or directory: t
} 
} whereas 3.0.7 more accurately responds
} 
} zsh: read-only file system: t
} 
} which I assume to be the other bug.

This isn't a bug either; it depends on the setting of the CLOBBER option.
You'll get "no such file" if you attempt to append-without-create, and
"read-only file system" if you attempt to append-with-create.  This is
true in both 3.0.[78] and 3.1.9; watch what happens when you do

	/bin/echo bla >>| t

to force append-with-create.  Conversely, try just ">>" on a nonexistent
name with "unsetopt CLOBBER" in effect, even on a writable file system.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Use of zerr() vs. zwarn()

[Bart Schaefer]

On Sep 28,  3:35am, Bart Schaefer wrote:
} Subject: Re: two append bugs?
}
} On Sep 27,  3:46pm, Clint Adams wrote:
} } 
} } % echo bla >> t || echo failed
} } zsh: permission denied: t
} } 
} } % /bin/echo bla >> t || echo failed
} } zsh: permission denied: t   
} } failed
} 
} It might be possible to get zsh to behave "as if" it had forked in this
} circumstance.

This would appear to be (as I suspected) a simple case of changing some
calls to zerr() to be calls to zwarn() instead.  However, there are a
number of other places where zerr() is called that would have to change
in order to generalize "builtins behave as if forked".

There also seem to be a number of inconsistencies in whether zerr() or
zwarn() is used for a particular condition.  The following is an audit
of places where I think the usage *might* need revision.

-----------------------------------------------------------------------

Modules/parameter.c:199:	zwarn("restricted: %s", value, 0);
  Should assignment to the `commands' parameter use zerr() when the
  shell is restricted?  (See builtin.c:1605, below)

Zle/computil.c:255:	zwarnnam(nam, "can only be called from completion function", NULL, 0);
Zle/computil.c:1727:	zwarnnam(nam, "can only be called from completion function", NULL, 0);
Zle/computil.c:2408:	zwarnnam(nam, "can only be called from completion function", NULL, 0);
Zle/computil.c:2591:	zwarnnam(nam, "can only be called from completion function", NULL, 0);
Zle/computil.c:2737:	zwarnnam(nam, "can only be called from completion function", NULL, 0);
Zle/computil.c:2865:	zwarnnam(nam, "can only be called from completion function", NULL, 0);
Zle/computil.c:3555:	zwarnnam(nam, "can only be called from completion function", NULL, 0);
Zle/computil.c:3652:	zwarnnam(nam, "can only be called from completion function", NULL, 0);
init.c:1122:    zwarnnam(name, "option valid only in functions called from completion",
  Should these use zerrnam(), as do completion-only conditions within
  [[ ... ]] ?

Zle/zle_main.c:761:	zwarnnam(name, "ZLE cannot be used recursively (yet)", NULL, 0);
  Hmm, zerrnam()?

Zle/zle_thingy.c:400:	zwarnnam(name, "can only be called from widget function", NULL, 0);
Zle/zle_thingy.c:444:	zwarnnam(name, "can only be called from widget function", NULL, 0);
Zle/zle_thingy.c:460:	zwarnnam(name, "can only be called from widget function", NULL, 0);
Zle/zle_thingy.c:611:	zwarnnam(name, "widgets can only be called when ZLE is active",
  Same situation as completion-only ...

builtin.c:294:		zerr("bad option: -%c", NULL, *arg);
  Why is passing a bad option to a builtin a fatal error?  Just zwarn()?

builtin.c:1510:	zerr("bad assignment", NULL, 0);
  This comes from e.g. `typeset \=foo'.  Why a fatal error?

builtin.c:1584:	zerrnam(cname, "%s: can't change type of a special parameter",
builtin.c:1886:	    zerrnam(name, "can't tie a variable to itself", NULL, 0);
builtin.c:1939:	zerrnam(name, "use unset to remove tied variables", NULL, 0);
builtin.c:1998:	    zerr("not an identifier: %s", asg->name, 0);
builtin.c:2240:		zerrnam(name, "%s: invalid parameter name", s, 0);
builtin.c:2264:		zerrnam(name, "%s: invalid element for unset", s, 0);
  Similarly, it doesn't seem these would need to be fatal.  (Other typeset
  errors are not, but other parameter errors are, e.g. almost anything but
  autoloading in params.c.)

builtin.c:1605:	    zerrnam(cname, "%s: restricted", pname, 0);
builtin.c:2252:	    zerrnam(name, "%s: restricted", pm->nam, 0);
  See Modules/parameter.c:199 above

builtin.c:3081:	    zerr(*p == '?' ? "bad option: -%c" :
exec.c:2290:			zerr("write error: %e", NULL, errno);
glob.c:576:				zerr("%e: %s", fn, errno);
signals.c:544:		zerr("timeout", NULL, 0);
utils.c:2408:		    zerr("can't set tty pgrp: %e", NULL, errno);
  Why call zerr() and then reset errflag = 0, rather than call zwarn()?

exec.c:1454:	    zerr("file mode mismatch on fd %d", NULL, fd1);
exec.c:1827:		    zerr("no such builtin: %s", cmdarg, 0);
exec.c:2088:		zerr("writing redirection not allowed in restricted mode", NULL, 0);
exec.c:2100:			zerr("%e", NULL, errno);
exec.c:2155:		    zerr("%s: %e", fn->fd2 == -2 ? "coprocess" : fdstr, errno);
exec.c:2876:    zerr("doesn't look like your system supports FIFOs.", NULL, 0);
exec.c:2917:	zerr("can't open %s: %e", pnam, errno);
  These are all in the "builtin should behave like external command" set.

exec.c:2678:	    zerr("%e: %s", s, errno);
  This is getoutput().  Not sure what should happen here.

exec.c:2784:	zerr("oops.", NULL, 0);
  How helpful.

exec.c:3238:	zerr("%s: function definition file not found", shf->nam, 0);
exec.c:3261:		zerr("%s: function not defined by file", n, 0);
exec.c:3365:	    zerr("%s: function not defined by file", name, 0);
  These occur during autoloading.  Should they behave like "command not
  found" for externals, and therefore only be warnings?

jobs.c:1214:	    zwarnnam(name, "-Z is restricted", NULL, 0);
  Another case where restricted shells might use zerrnam().

signals.c:699:        zerr("can't trap SIG%s in interactive shells", sigs[sig], 0);
  Doesn't seem serious enough to be a fatal error.


-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: Use of zerr() vs. zwarn()

[Bart Schaefer]

On Sep 29,  4:58pm, Bart Schaefer wrote:
} Subject: Use of zerr() vs. zwarn()
}
} init.c:1122:    zwarnnam(name, "option valid only in functions called from completion",
}   Should these use zerrnam(), as do completion-only conditions within
}   [[ ... ]] ?

Judging from the indentation on the line following the call to zwarnnam(),
I suspect that this originally *was* a call to zerrnam() and was changed
to zwarnnam() at some point in the past, but I can't find a zsh-workers
article in the archive to confirm.  (Maybe it was in a uuencoded patch;
it predates my first local import of 3.1.x, which I believe was 3.1.4.)

This makes me inclined to leave this whole sub-category alone, at least
until there's been some feedback.

} exec.c:1454:	    zerr("file mode mismatch on fd %d", NULL, fd1);

This one could be problematic because it's buried in addfd(), so only
errflag propagates the error ... but in fact it happens early enough that
zsh hasn't forked yet, so both builtins and external commands behave the
same.  And since it only occurs for multios, there's no issue with sh/ksh
emulations.  So it can stay as is.

} exec.c:2100:			zerr("%e", NULL, errno);

This one happens when zsh fails to process a here-document.  This should
never happen, really, but could if e.g. [[ ! -w $TMPPREFIX:h ]].  It's
probably harmless to change this to zwarn(), but I wanted to point it out
in particular because it's such an unusual event.

} exec.c:2876:    zerr("doesn't look like your system supports FIFOs.", NULL, 0);
} exec.c:2917:	zerr("can't open %s: %e", pnam, errno);
} exec.c:2678:	    zerr("%e: %s", s, errno);

These three all happen during prefork() and therefore presumably already
are the same for builtins and externals.  I don't think it would hurt to
change them to zwarn(), but I'd like other opinions.

} exec.c:2784:	zerr("oops.", NULL, 0);
}   How helpful.

This is in parsecmd().  Does somebody know what the actual error is?  It
looks like some kind of parse problem -- either a missing close paren or
too many open parens -- but apparently that should be impossible?

} exec.c:3365:	    zerr("%s: function not defined by file", name, 0);

This one is a bit odd because it can be called in a context where there's
no `lastval' returned.  This means functions like chpwd and precmd, or
zftp_progress, or various functions called from old compctls.  Presumably
in those cases we *do* want errflag set?

} jobs.c:1214:	    zwarnnam(name, "-Z is restricted", NULL, 0);
}   Another case where restricted shells might use zerrnam().

I'm now of the mind to leave this one alone, simply because it makes no
difference whether "jobs -Z" succeeds.
 
} signals.c:699:        zerr("can't trap SIG%s in interactive shells", sigs[sig], 0);
}   Doesn't seem serious enough to be a fatal error.

This one also brings up the question of whether enableshfuncnode() (in
hashtable.c) should avoid sigtrapped[signum] |= ZSIG_FUNC in the event
that settrap() should fail.  In any case, the fact that this is called
from places other than builtin commands makes me wonder whether (1) we
rely on errflag becoming set (it doesn't appear so, but ...) or (2) the
tests this is doing that lead to that error, ought to be done somewhere
else instead.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: Use of zerr() vs. zwarn()

[Clint Adams]

> This would appear to be (as I suspected) a simple case of changing some
> calls to zerr() to be calls to zwarn() instead.  However, there are a
> number of other places where zerr() is called that would have to change
> in order to generalize "builtins behave as if forked".
> 
> There also seem to be a number of inconsistencies in whether zerr() or
> zwarn() is used for a particular condition.  The following is an audit
> of places where I think the usage *might* need revision.

With current CVS and exec.c:2178 changed to zwarn, the failed redirection
from a builtin behaves as the failed redirection from a forked command, or
however that should be expressed in English.

I can't see any value to not doing it this way except perhaps in interactive
contexts, and even then it's a bit counterintuitive.  I would expect something
like

echo blah blah blah >>logfile || echo log problem | mailx pagesomeone

to do the same thing as

/bin/echo blah blah blah >>logfile || echo log problem | mailx pagesomeone

PATCH: First pass at zerr/zwarn sanity

[Bart Schaefer]

On Oct 1,  8:18pm, Clint Adams wrote:
} Subject: Re: Use of zerr() vs. zwarn()
}
} I can't see any value to not doing it this way except perhaps in interactive
} contexts, and even then it's a bit counterintuitive.

This patch changes only zerr --> zwarn, and none the other way 'round.  It
is possible that a script or function relies on the exit-if-redirect-fails
behavior of builtins, which after this patch need an explicit `|| return'
or the like; but otherwise nothing in this patch should affect a working
script or function.  (Some non-working ones might begin running farther
than they previously had.)

See zsh-workers/12867 for discussion of other places that still might be
changable.

The only possibly-controversial change here is, I think, the last hunk of
exec.c.  It preserves the zerr behavior when the caller has requested that
`lastval' not change.

Index: Src/builtin.c
===================================================================
@@ -291,7 +291,7 @@
 	    if (*arg) {
 		if(*arg == Meta)
 		    *++arg ^= 32;
-		zerr("bad option: -%c", NULL, *arg);
+		zwarn("bad option: -%c", NULL, *arg);
 		return 1;
 	    }
 	    arg = (char *) ugetnode(args);
@@ -3078,10 +3078,9 @@
 	if(quiet) {
 	    zoptarg = metafy(optbuf, lenoptbuf, META_DUP);
 	} else {
-	    zerr(*p == '?' ? "bad option: -%c" :
-		"argument expected after -%c option", NULL, opch);
+	    zwarn(*p == '?' ? "bad option: -%c" :
+		  "argument expected after -%c option", NULL, opch);
 	    zoptarg=ztrdup("");
-	    errflag = 0;
 	}
 	return 0;
     }
Index: Src/exec.c
===================================================================
@@ -1824,8 +1824,8 @@
 	    }
 	    if (!(hn = builtintab->getnode(builtintab, cmdarg))) {
 		if (cflags & BINF_BUILTIN) {
-		    zerr("no such builtin: %s", cmdarg, 0);
-		    errflag = lastval = 1;
+		    zwarn("no such builtin: %s", cmdarg, 0);
+		    lastval = 1;
 		    return;
 		}
 		break;
@@ -2085,7 +2085,7 @@
 		execerr();
 	    }
 	    if (isset(RESTRICTED) && IS_WRITE_FILE(fn->type)) {
-		zerr("writing redirection not allowed in restricted mode", NULL, 0);
+		zwarn("writing redirection not allowed in restricted mode", NULL, 0);
 		execerr();
 	    }
 	    if (unset(EXECOPT))
@@ -2097,7 +2097,7 @@
 		    closemnodes(mfds);
 		    fixfds(save);
 		    if (errno != EINTR)
-			zerr("%e", NULL, errno);
+			zwarn("%e", NULL, errno);
 		    execerr();
 		}
 		addfd(forked, save, mfds, fn->fd1, fil, 0);
@@ -2113,7 +2113,7 @@
 		    closemnodes(mfds);
 		    fixfds(save);
 		    if (errno != EINTR)
-			zerr("%e: %s", fn->name, errno);
+			zwarn("%e: %s", fn->name, errno);
 		    execerr();
 		}
 		addfd(forked, save, mfds, fn->fd1, fil, 0);
@@ -2152,7 +2152,7 @@
 		    fixfds(save);
 		    if (fn->fd2 != -2)
 		    	sprintf(fdstr, "%d", fn->fd2);
-		    zerr("%s: %e", fn->fd2 == -2 ? "coprocess" : fdstr, errno);
+		    zwarn("%s: %e", fn->fd2 == -2 ? "coprocess" : fdstr, errno);
 		    execerr();
 		}
 		addfd(forked, save, mfds, fn->fd1, fil, fn->type == MERGEOUT);
@@ -2175,7 +2175,7 @@
 		    closemnodes(mfds);
 		    fixfds(save);
 		    if (errno != EINTR)
-			zerr("%e: %s", fn->name, errno);
+			zwarn("%e: %s", fn->name, errno);
 		    execerr();
 		}
 		addfd(forked, save, mfds, fn->fd1, fil, 1);
@@ -2287,9 +2287,8 @@
 		fflush(stdout);
 		if (save[1] == -2) {
 		    if (ferror(stdout)) {
-			zerr("write error: %e", NULL, errno);
+			zwarn("write error: %e", NULL, errno);
 			clearerr(stdout);
-			errflag = 0;
 		    }
 		} else
 		    clearerr(stdout);
@@ -3235,7 +3234,7 @@
     if (prog == &dummy_eprog) {
 	/* We're not actually in the function; decrement locallevel */
 	locallevel--;
-	zerr("%s: function definition file not found", shf->nam, 0);
+	zwarn("%s: function definition file not found", shf->nam, 0);
 	locallevel++;
 	popheap();
 	return NULL;
@@ -3258,7 +3257,7 @@
 	    execode(prog, 1, 0);
 	    shf = (Shfunc) shfunctab->getnode(shfunctab, n);
 	    if (!shf || (shf->flags & PM_UNDEFINED)) {
-		zerr("%s: function not defined by file", n, 0);
+		zwarn("%s: function not defined by file", n, 0);
 		popheap();
 		return NULL;
 	    }
@@ -3362,8 +3361,10 @@
 
 	if (!(shf = (Shfunc) shfunctab->getnode(shfunctab,
 						(name = fname)))) {
-	    zerr("%s: function not defined by file", name, 0);
-	    if (!noreturnval)
+	    zwarn("%s: function not defined by file", name, 0);
+	    if (noreturnval)
+		errflag = 1;
+	    else
 		lastval = 1;
 	    popheap();
 	    scriptname = oldscriptname;
Index: Src/glob.c
===================================================================
@@ -573,8 +573,7 @@
 			if (statfullpath(fn, &buf, !q->follow)) {
 			    if (errno != ENOENT && errno != EINTR &&
 				errno != ENOTDIR && !errflag) {
-				zerr("%e: %s", fn, errno);
-				errflag = 0;
+				zwarn("%e: %s", fn, errno);
 			    }
 			    continue;
 			}
Index: Src/signals.c
===================================================================
@@ -541,8 +541,7 @@
 		alarm(tmout - idle);
 	    else {
 		errflag = noerrs = 0;
-		zerr("timeout", NULL, 0);
-		errflag = 0;
+		zwarn("timeout", NULL, 0);
 		stopmsg = 1;
 		zexit(SIGALRM, 1);
 	    }
Index: Src/utils.c
===================================================================
@@ -2405,12 +2405,11 @@
 	    else {
 		if (errno != ENOTTY)
 		{
-		    zerr("can't set tty pgrp: %e", NULL, errno);
+		    zwarn("can't set tty pgrp: %e", NULL, errno);
 		    fflush(stderr);
 		}
 		opts[MONITOR] = 0;
 		ep = 1;
-		errflag = 0;
 	    }
 	}
     }


-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net