unalias should exit with non-zero status on error #910
Conversation
src/cmd/ksh93/bltins/typeset.c
Outdated
| @@ -1238,6 +1241,8 @@ static_fn int unall(int argc, char **argv, Dt_t *troot, Shell_t *shp) { | |||
| nv_delete(np, dp, NV_NOFREE); | |||
| } else if (isfun && !(np->nvalue.rp && np->nvalue.rp->running)) { | |||
| nv_delete(np, troot, 0); | |||
| } else if (type == ALIAS) { | |||
| nv_delete(np, troot, 0); | |||
There was a problem hiding this comment.
This is why I really wonder if the people who were writing this code were smoking crack. You are apparently relying on nv_delete() to longjmp() on error. That is what the debug printf statements I added seem to suggest. I see nothing in the nv_delete() implementation to suggest that is a valid assumption. So I'd like to see a comment added here, and in the nv_delete() definition, explaining this magic.
Also, there is this block in the nv_delete() implementation:
#if 0
else
{
sfprintf(sfstderr,"%s not deleted\n",nv_name(np));
sfsync(sfstderr);
}
#endif
Which implies that if I change that to #if 1 I should see an error message in this case. Yet I do not.
There was a problem hiding this comment.
FWIW, The rest of the change looks okay. I just don't like, or understand, the magic "return non-zero" from nv_delete() in this case. I could live with that magic if it was documented. But it seems to me like a better modification, albeit outside the scope of this change, is to have nv_delete() return a status and have the caller decide if a long jump is warranted.
There was a problem hiding this comment.
@siteshwar In case it wasn't clear I am obviously not blaming you for the magic behavior of nv_delete() with regard to doing a longjmp() on error. My concern is that
a) you are taking advantage of it without an explicit comment to that effect, and
b) it doesn't write a message to stderr.
But those are minor objections. My main concernt is that the absence of the } else if (type == ALIAS) { block doesn't actually affect whether the alias is removed. Only whether the action results in a zero (success) or non-zero (failure) exit status. Which suggests that the actual removal of the alias is occurring before your newly introduced block is being run. And the execution of that block is solely for the effect of returning a failure status (presumably due to the undocumented longjmp).
There was a problem hiding this comment.
b) it doesn't write a message to stderr.
I updated this PR to write a message on stderr. It does not have to be done inside nv_delete().
unalias should exit with non-zero status while trying to unalias an undefined alias and show an error message. Resolves: #909
|
LGTM. Much better now that it doesn't rely on the magic longjmp. |
No description provided.