PATCH: move $ERRNO to the system module

PATCH: move $ERRNO to the system module

[Oliver Kiddle]

This would not be a strictly backward-compatible change.

My justification is that, given how close it is to the system, any
reliance on it is already liable to break just because we might reorder
or add system calls. ERRNO is not especially portable, either.

ERRNO predates the system module, and loadable modules in general. I had
even forgotten it existed until it was mentioned recently. I doubt it's
especially widely used. system.mdd does specify load=no so there would
be no automatic loading. The trick with defining it as PM_UNSET doesn't
appear to work from a module. But that's less necessary if the variable
is in a module.

I'll allow plenty of time for dissenting opinions on this but it'd also
be good to hear if anyone agrees.

Oliver

diff --git a/Doc/Zsh/mod_system.yo b/Doc/Zsh/mod_system.yo
index e25201faa..88bc5ee9b 100644
--- a/Doc/Zsh/mod_system.yo
+++ b/Doc/Zsh/mod_system.yo
@@ -252,6 +252,13 @@ enditem()
 subsect(Parameters)
 
 startitem()
+vindex(ERRNO)
+item(tt(ERRNO))(
+The value of tt(errno) (see manref(errno)(3))
+as set by the most recently failed system call.
+This value is system dependent and is intended for debugging
+purposes.
+)
 vindex(errnos)
 item(tt(errnos))(
 A readonly array of the names of errors defined on the system.  These
diff --git a/Doc/Zsh/params.yo b/Doc/Zsh/params.yo
index 2a30085a8..a7863a136 100644
--- a/Doc/Zsh/params.yo
+++ b/Doc/Zsh/params.yo
@@ -743,17 +743,6 @@ effective user ID by `tt(LPAR()EUID=)var(uid)tt(; command+RPAR())'
 If this is made local, it is not implicitly set to 0, but may be
 explicitly set locally.
 )
-vindex(ERRNO)
-item(tt(ERRNO) <S>)(
-The value of tt(errno) (see manref(errno)(3))
-as set by the most recently failed system call.
-This value is system dependent and is intended for debugging
-purposes.  It is also useful with the tt(zsh/system) module which
-allows the number to be turned into a name or message.
-
-To use this parameter, it must first be assigned a value (typically
-0 (zero)).  It is initially unset for scripting compatibility.
-)
 vindex(FUNCNEST)
 item(tt(FUNCNEST) <S>)(
 Integer.  If greater than or equal to zero, the maximum nesting depth of
diff --git a/Src/Modules/system.c b/Src/Modules/system.c
index 929a8b002..8ee1e6258 100644
--- a/Src/Modules/system.c
+++ b/Src/Modules/system.c
@@ -835,9 +835,30 @@ errnosgetfn(UNUSED(Param pm))
     return arrdup((char **)sys_errnames);
 }
 
+/* Function to set value for special parameter `ERRNO' */
+
+/**/
+void
+errnosetfn(UNUSED(Param pm), zlong x)
+{
+    errno = (int)x;
+    if ((zlong)errno != x)
+	zwarn("errno truncated on assignment");
+}
+
+/* Function to get value for special parameter `ERRNO' */
+
+/**/
+zlong
+errnogetfn(UNUSED(Param pm))
+{
+    return errno;
+}
+
 static const struct gsu_array errnos_gsu =
 { errnosgetfn, arrsetfn, stdunsetfn };
-
+static const struct gsu_integer errno_gsu =
+{ errnogetfn, errnosetfn, stdunsetfn };
 
 /* Functions for the sysparams special parameter. */
 
@@ -899,6 +920,7 @@ static struct mathfunc mftab[] = {
 };
 
 static struct paramdef partab[] = {
+    PARAMDEF("ERRNO", PM_INTEGER, NULL, &errno_gsu),
     SPECIALPMDEF("errnos", PM_ARRAY|PM_READONLY,
 		 &errnos_gsu, NULL, NULL),
     SPECIALPMDEF("sysparams", PM_READONLY,
diff --git a/Src/Modules/system.mdd b/Src/Modules/system.mdd
index 00a3e7896..1ee6ba140 100644
--- a/Src/Modules/system.mdd
+++ b/Src/Modules/system.mdd
@@ -2,7 +2,7 @@ name=zsh/system
 link=dynamic
 load=no
 
-autofeatures="b:sysread b:syswrite b:sysopen b:sysseek b:syserror p:errnos f:systell"
+autofeatures="b:sysread b:syswrite b:sysopen b:sysseek b:syserror p:ERRNO p:errnos f:systell"
 
 objects="system.o errnames.o"
 
diff --git a/Src/params.c b/Src/params.c
index 2e4a6eae2..3bc5a0d80 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -201,8 +201,6 @@ mod_export const struct gsu_scalar colonarr_gsu =
 /* Non standard methods (not exported) */
 static const struct gsu_integer pound_gsu =
 { poundgetfn, nullintsetfn, stdunsetfn };
-static const struct gsu_integer errno_gsu =
-{ errnogetfn, errnosetfn, stdunsetfn };
 static const struct gsu_integer gid_gsu =
 { gidgetfn, gidsetfn, stdunsetfn };
 static const struct gsu_integer egid_gsu =
@@ -297,7 +295,6 @@ static initparam special_params[] ={
 #define NULL_GSU BR((GsuScalar)(void *)NULL)
 #define IPDEF1(A,B,C) {{NULL,A,PM_INTEGER|PM_SPECIAL|C},BR(NULL),GSU(B),10,0,NULL,NULL,NULL,0}
 IPDEF1("#", pound_gsu, PM_READONLY_SPECIAL),
-IPDEF1("ERRNO", errno_gsu, PM_UNSET),
 IPDEF1("GID", gid_gsu, PM_DONTIMPORT | PM_RESTRICTED),
 IPDEF1("EGID", egid_gsu, PM_DONTIMPORT | PM_RESTRICTED),
 IPDEF1("HISTSIZE", histsize_gsu, PM_RESTRICTED),
@@ -4771,26 +4768,6 @@ savehistsizesetfn(UNUSED(Param pm), zlong v)
 	savehistsiz = 0;
 }
 
-/* Function to set value for special parameter `ERRNO' */
-
-/**/
-void
-errnosetfn(UNUSED(Param pm), zlong x)
-{
-    errno = (int)x;
-    if ((zlong)errno != x)
-	zwarn("errno truncated on assignment");
-}
-
-/* Function to get value for special parameter `ERRNO' */
-
-/**/
-zlong
-errnogetfn(UNUSED(Param pm))
-{
-    return errno;
-}
-
 /* Function to get value for special parameter `KEYBOARD_HACK' */
 
 /**/
diff --git a/Test/V01zmodload.ztst b/Test/V01zmodload.ztst
index daf49cd72..52cba69e9 100644
--- a/Test/V01zmodload.ztst
+++ b/Test/V01zmodload.ztst
@@ -293,6 +293,7 @@
 >+b:sysseek
 >+b:zsystem
 >+f:systell
+>+p:ERRNO
 >+p:errnos
 >+p:sysparams
 >0
@@ -303,6 +304,7 @@
 >+b:sysseek
 >+b:zsystem
 >+f:systell
+>+p:ERRNO
 >-p:errnos
 >+p:sysparams
 >1
@@ -313,6 +315,7 @@
 >+b:sysseek
 >+b:zsystem
 >+f:systell
+>+p:ERRNO
 >+p:errnos
 >+p:sysparams
 
@@ -334,6 +337,7 @@
 >+b:sysseek
 >+b:zsystem
 >-f:systell
+>+p:ERRNO
 >+p:errnos
 >+p:sysparams
 >+b:syserror
@@ -343,6 +347,7 @@
 >+b:sysseek
 >+b:zsystem
 >+f:systell
+>+p:ERRNO
 >+p:errnos
 >+p:sysparams
 ?(eval):6: unknown function: systell

Re: PATCH: move $ERRNO to the system module

[Stephane Chazelas]

2023-01-12 00:19:48 +0100, Oliver Kiddle:
> This would not be a strictly backward-compatible change.
[...]

Use cases where people go out of their way to check $ERRNO,
would be ones where it matters that errors be checked.

If you move it to a module, then scripts that do:
ERRNO=0; something && ((!ERRNO))
will start to silently ignore errors.

See
https://unix.stackexchange.com/search?q=user%3A22565+zsh+errno
for some use cases of $ERRNO by me.

Note that we need zsh/system to translate ERRNOs to EXXX codes
or error messages, but we don't always need to.

It's true ERRNO is not very reliable though and one should
probably not rely too heavily on it, though in general the
problem is that it reports more errors than you'd like. The
EILSEQs that get in the way when you try to check whether a glob
expansion can be trusted to have found all matches in particular
come to mind.

-- 
Stephane

Re: PATCH: move $ERRNO to the system module

[Daniel Shahaf]

Oliver Kiddle wrote on Wed, 11 Jan 2023 23:19 +00:00:
> This would not be a strictly backward-compatible change.
>

Does moving $ERRNO from the core to a module "reorder or add system
calls" to the codepath that expands/evaluates uses of ERRNO?  I.e.,
even assuming scripts already do load zsh/system, is it safe to
assume the value of errno by the time Src/Modules/system.c code runs
will be the same value errno had at the point execution with this patch
diverges from execution without it?

> My justification is that, given how close it is to the system, any
> reliance on it is already liable to break just because we might reorder
> or add system calls.

How does the conclusion "It's fine to move ERRNO to a module" follow
from the premise "Scripts that rely on ERRNO are liable to break if zsh.c
changes"?  It's not immediately clear that a script that guards
against some C code modifying errno [possibly as a side effect] will
also be safe against ERRNO having been moved to a module, cf.
Stephane's example.

>                      ERRNO is not especially portable, either.

Portability is neither here nor there.

> ERRNO predates the system module, and loadable modules in general. I had
> even forgotten it existed until it was mentioned recently. I doubt it's
> especially widely used. system.mdd does specify load=no so there would
> be no automatic loading. The trick with defining it as PM_UNSET doesn't
> appear to work from a module. But that's less necessary if the variable
> is in a module.
>
> I'll allow plenty of time for dissenting opinions on this but it'd also
> be good to hear if anyone agrees.

Well, it's backwards incompatible and I don't understand the
justification given, so I can't say I'm +1 on this.

Cheers,

Daniel

Re: PATCH: move $ERRNO to the system module

[Bart Schaefer]

On Thu, Jan 12, 2023 at 9:15 AM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> Does moving $ERRNO from the core to a module "reorder or add system
> calls" to the codepath that expands/evaluates uses of ERRNO?

As far as I can tell system call changes would only occur in
circumstances where the module could not be loaded at all.  On the
other hand, that circumstance also renders ERRNO useless.

> Oliver Kiddle wrote on Wed, 11 Jan 2023 23:19 +00:00:
> > ERRNO predates the system module, and loadable modules in general.

It doesn't just predate the system module, it predates the zsh source
import to git.

> > The trick with defining it as PM_UNSET doesn't
> > appear to work from a module.

Hm, that might be worthy of some further digging.

Aside, is Src/modentry.c present for any reason other than as some
kind of example?  It's been around since at least 2007 (workers/23479,
approximately) but it's not referenced anywhere, not even in
zsh-development-guide.

> Well, it's backwards incompatible and I don't understand the
> justification given, so I can't say I'm +1 on this.

On the one hand, workers/32337 already made a backwards-incompatible
change to ERRNO in the name of POSIX script compatibility, so any
really old scripts that used it will already have needed updating.
Moving it to a module is just another step in that direction, and
actually fixes the "must set before use" issue (obviously by replacing
it with "must load module before use").

On the opposing hand, the existence/use of ERRNO has been recently
discussed on the lists, the change to document the previous
incompatible change is also recent, and there have been other changes
to make the value of ERRNO more correctly reflect actual error states.
So his might not be an ideal time to introduce yet another such
change.

I'm net-zero on this.