PATCH: separate watch/log functionality out into a module

PATCH: separate watch/log functionality out into a module

[Oliver Kiddle]

The log builtin and watch variable likely don't get as much use as they
did in the days of shared servers at Universities. On one particular
Linux system, I can get it to crash with a variable, descriptively
named bv, being null. On FreeBSD it only prints "not available on this
system". Though it doesn't seem to work from tcsh on FreeBSD either –
it originates as a tcsh feature.

This patch extracts the functionality out into a zsh/watch module. I've
set load=yes in the .mdd file so anyone who uses the feature doesn't
need to add zmodload commands to their .zshrc. $watch/$WATCH and the log
builtin are autoloadable features. The variables are tied which needed a
little extra setup. I was concerned that it'd break things if one or
other of them are disabled with zmodload but it seems that if you
disable either one, both end up missing. That's not especially useful
but I didn't want it crashing. Maybe someone else can see a way in which
this setup could be a problem?

WATCHFMT and LOGCHECK are merely set to the defaults if not already set.
That avoids loading the module for anyone like me who sets them in
.zshrc but only conditionally sets $watch on particular hosts.

This change could result in pre-prompt actions running in a different
order but the advantage is that a user perhaps has more control by
deciding which of sched and watch to load first.

Oliver

diff --git a/Src/watch.c b/Src/Modules/watch.c
similarity index 87%
rename from Src/watch.c
rename to Src/Modules/watch.c
index c41704315..02f0562fc 100644
--- a/Src/watch.c
+++ b/Src/Modules/watch.c
@@ -27,7 +27,7 @@
  *
  */
 
-#include "zsh.mdh"
+#include "watch.mdh"
 
 /* Headers for utmp/utmpx structures */
 #ifdef HAVE_UTMP_H
@@ -139,9 +139,6 @@
 # define DEFAULT_WATCHFMT "%n has %a %l."
 #endif /* !WATCH_UTMP_UT_HOST */
 
-/**/
-char const * const default_watchfmt = DEFAULT_WATCHFMT;
-
 #ifdef WATCH_STRUCT_UTMP
 
 # include "watch.pro"
@@ -152,11 +149,14 @@ char const * const default_watchfmt = DEFAULT_WATCHFMT;
 
 static int wtabsz = 0;
 static WATCH_STRUCT_UTMP *wtab = NULL;
+
+/* the last time we checked the people in the WATCH variable */
+static time_t lastwatch;
+
 static time_t lastutmpcheck = 0;
 
 /* get the time of login/logout for WATCH */
 
-/**/
 static time_t
 getlogtime(WATCH_STRUCT_UTMP *u, int inout)
 {
@@ -202,7 +202,6 @@ getlogtime(WATCH_STRUCT_UTMP *u, int inout)
 # define BEGIN3 '('
 # define END3 ')'
 
-/**/
 static char *
 watch3ary(int inout, WATCH_STRUCT_UTMP *u, char *fmt, int prnt)
 {
@@ -407,7 +406,6 @@ watchlog_match(char *teststr, char *actual, int len)
 
 /* check the List for login/logouts */
 
-/**/
 static void
 watchlog(int inout, WATCH_STRUCT_UTMP *u, char **w, char *fmt)
 {
@@ -470,7 +468,6 @@ watchlog(int inout, WATCH_STRUCT_UTMP *u, char **w, char *fmt)
 
 /* compare 2 utmp entries */
 
-/**/
 static int
 ucmp(WATCH_STRUCT_UTMP *u, WATCH_STRUCT_UTMP *v)
 {
@@ -481,7 +478,6 @@ ucmp(WATCH_STRUCT_UTMP *u, WATCH_STRUCT_UTMP *v)
 
 /* initialize the user List */
 
-/**/
 static int
 readwtab(WATCH_STRUCT_UTMP **head, int initial_sz)
 {
@@ -592,10 +588,19 @@ dowatch(void)
     wtab = utab;
     wtabsz = utabsz;
     fflush(stdout);
+    lastwatch = time(NULL);
+}
+
+static void
+checksched(void)
+{
+    /* Do nothing if WATCH is not set, or LOGCHECK has not elapsed */
+    if (watch && (int) difftime(time(NULL), lastwatch) > getiparam("LOGCHECK"))
+	dowatch();
 }
 
 /**/
-int
+static int
 bin_log(UNUSED(char *nam), UNUSED(char **argv), UNUSED(Options ops), UNUSED(int func))
 {
     if (!watch)
@@ -611,16 +616,101 @@ bin_log(UNUSED(char *nam), UNUSED(char **argv), UNUSED(Options ops), UNUSED(int
 
 #else /* !WATCH_STRUCT_UTMP */
 
-/**/
-void dowatch(void)
+static void
+checksched(void)
 {
 }
 
 /**/
-int
+static int
 bin_log(char *nam, char **argv, Options ops, int func)
 {
     return bin_notavail(nam, argv, ops, func);
 }
 
 #endif /* !WATCH_STRUCT_UTMP */
+
+/**/
+static char **watch; /* $watch */
+
+/* module setup */
+
+static struct builtin bintab[] = {
+    BUILTIN("log", 0, bin_log, 0, 0, 0, NULL, NULL),
+};
+
+static struct paramdef partab[] = {
+    PARAMDEF("WATCH", PM_TIED|PM_SCALAR|PM_SPECIAL, &watch, &colonarr_gsu),
+    PARAMDEF("watch", PM_TIED|PM_ARRAY|PM_SPECIAL, &watch, &vararray_gsu),
+};
+
+static struct features module_features = {
+    bintab, sizeof(bintab)/sizeof(*bintab),
+    NULL, 0,
+    NULL, 0,
+    partab, sizeof(partab)/sizeof(*partab),
+    0
+};
+
+/**/
+int
+setup_(UNUSED(Module m))
+{
+    return 0;
+}
+
+/**/
+int
+features_(Module m, char ***features)
+{
+    *features = featuresarray(m, &module_features);
+    return 0;
+}
+
+/**/
+int
+enables_(Module m, int **enables)
+{
+    return handlefeatures(m, &module_features, enables);
+}
+
+/**/
+int
+boot_(UNUSED(Module m))
+{
+    static char const * const default_watchfmt = DEFAULT_WATCHFMT;
+    Param pm;
+
+    if ((pm = (Param) paramtab->getnode(paramtab, "watch")))
+	pm->ename = "WATCH";
+    if ((pm = (Param) paramtab->getnode(paramtab, "WATCH")))
+	pm->ename = "watch";
+    watch = mkarray(NULL);
+
+    /* These two parameters are only set to defaults if not set.
+     * So setting them in .zshrc will not be enough to load the
+     * module. It's useless until the watch array is set anyway. */
+    if (!paramtab->getnode(paramtab, "WATCHFMT"))
+	setsparam("WATCHFMT", ztrdup_metafy(default_watchfmt));
+    if (!paramtab->getnode(paramtab, "LOGCHECK"))
+	setiparam("LOGCHECK", 60);
+
+    addprepromptfn(&checksched);
+
+    return 0;
+}
+
+/**/
+int
+cleanup_(Module m)
+{
+    delprepromptfn(&checksched);
+    return setfeatureenables(m, &module_features, NULL);
+}
+
+/**/
+int
+finish_(UNUSED(Module m))
+{
+    return 0;
+}
diff --git a/Src/Modules/watch.mdd b/Src/Modules/watch.mdd
new file mode 100644
index 000000000..7e8454ede
--- /dev/null
+++ b/Src/Modules/watch.mdd
@@ -0,0 +1,7 @@
+name=zsh/watch
+link=dynamic
+load=yes
+
+autofeatures="b:log p:WATCH p:watch"
+
+objects="watch.o"
diff --git a/Src/builtin.c b/Src/builtin.c
index 89bcd98db..8ef678b22 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -89,7 +89,6 @@ static struct builtin builtins[] =
     BUILTIN("kill", BINF_HANDLES_OPTS, bin_kill, 0, -1, 0, NULL, NULL),
     BUILTIN("let", 0, bin_let, 1, -1, 0, NULL, NULL),
     BUILTIN("local", BINF_PLUSOPTS | BINF_MAGICEQUALS | BINF_PSPECIAL | BINF_ASSIGN, (HandlerFunc)bin_typeset, 0, -1, 0, "AE:%F:%HL:%R:%TUZ:%ahi:%lp:%rtux", NULL),
-    BUILTIN("log", 0, bin_log, 0, 0, 0, NULL, NULL),
     BUILTIN("logout", 0, bin_break, 0, 1, BIN_LOGOUT, NULL, NULL),
 
 #if defined(ZSH_MEM) & defined(ZSH_MEM_DEBUG)
diff --git a/Src/init.c b/Src/init.c
index 878a53a37..871d46b12 100644
--- a/Src/init.c
+++ b/Src/init.c
@@ -1042,7 +1042,6 @@ setupvals(char *cmd, char *runscript, char *zsh_name)
 #endif /* FPATH_NEEDS_INIT */
 
     mailpath = mkarray(NULL);
-    watch    = mkarray(NULL);
     psvar    = mkarray(NULL);
     module_path = mkarray(ztrdup(MODULE_DIR));
     modulestab = newmoduletable(17, "modules");
diff --git a/Src/params.c b/Src/params.c
index b703a97ce..dadf83129 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -63,7 +63,6 @@ char **pparams,		/* $argv        */
      **mailpath,	/* $mailpath    */
      **manpath,		/* $manpath     */
      **psvar,		/* $psvar       */
-     **watch,		/* $watch       */
      **zsh_eval_context; /* $zsh_eval_context */
 /**/
 mod_export
@@ -194,6 +193,10 @@ mod_export const struct gsu_hash stdhash_gsu =
 mod_export const struct gsu_hash nullsethash_gsu =
 { hashgetfn, nullsethashfn, nullunsetfn };
 
+/**/
+mod_export const struct gsu_scalar colonarr_gsu =
+{ colonarrgetfn, colonarrsetfn, stdunsetfn };
+
 
 /* Non standard methods (not exported) */
 static const struct gsu_integer pound_gsu =
@@ -259,9 +262,6 @@ static const struct gsu_integer varint_readonly_gsu =
 static const struct gsu_integer zlevar_gsu =
 { intvargetfn, zlevarsetfn, stdunsetfn };
 
-static const struct gsu_scalar colonarr_gsu =
-{ colonarrgetfn, colonarrsetfn, stdunsetfn };
-
 static const struct gsu_integer argc_gsu =
 { poundgetfn, nullintsetfn, stdunsetfn };
 static const struct gsu_array pipestatus_gsu =
@@ -398,7 +398,6 @@ IPDEF8("CDPATH", &cdpath, "cdpath", PM_TIED),
 IPDEF8("FIGNORE", &fignore, "fignore", PM_TIED),
 IPDEF8("FPATH", &fpath, "fpath", PM_TIED),
 IPDEF8("MAILPATH", &mailpath, "mailpath", PM_TIED),
-IPDEF8("WATCH", &watch, "watch", PM_TIED),
 IPDEF8("PATH", &path, "path", PM_RESTRICTED|PM_TIED),
 IPDEF8("PSVAR", &psvar, "psvar", PM_TIED),
 IPDEF8("ZSH_EVAL_CONTEXT", &zsh_eval_context, "zsh_eval_context", PM_READONLY_SPECIAL|PM_TIED),
@@ -430,7 +429,6 @@ IPDEF9("fpath", &fpath, "FPATH", PM_TIED),
 IPDEF9("mailpath", &mailpath, "MAILPATH", PM_TIED),
 IPDEF9("manpath", &manpath, "MANPATH", PM_TIED),
 IPDEF9("psvar", &psvar, "PSVAR", PM_TIED),
-IPDEF9("watch", &watch, "WATCH", PM_TIED),
 
 IPDEF9("zsh_eval_context", &zsh_eval_context, "ZSH_EVAL_CONTEXT", PM_TIED|PM_READONLY_SPECIAL),
 
@@ -453,7 +451,6 @@ IPDEF8("CDPATH", &cdpath, NULL, 0),
 IPDEF8("FIGNORE", &fignore, NULL, 0),
 IPDEF8("FPATH", &fpath, NULL, 0),
 IPDEF8("MAILPATH", &mailpath, NULL, 0),
-IPDEF8("WATCH", &watch, NULL, 0),
 IPDEF8("PATH", &path, NULL, PM_RESTRICTED),
 IPDEF8("PSVAR", &psvar, NULL, 0),
 IPDEF8("ZSH_EVAL_CONTEXT", &zsh_eval_context, NULL, PM_READONLY_SPECIAL),
@@ -836,7 +833,6 @@ createparamtable(void)
      */
     setsparam("TMPPREFIX", ztrdup_metafy(DEFAULT_TMPPREFIX));
     setsparam("TIMEFMT", ztrdup_metafy(DEFAULT_TIMEFMT));
-    setsparam("WATCHFMT", ztrdup_metafy(default_watchfmt));
 
     hostnam = (char *)zalloc(256);
     gethostname(hostnam, 256);
@@ -4093,7 +4089,7 @@ arrvarsetfn(Param pm, char **x)
 }
 
 /**/
-char *
+mod_export char *
 colonarrgetfn(Param pm)
 {
     char ***dptr = (char ***)pm->u.data;
@@ -4101,7 +4097,7 @@ colonarrgetfn(Param pm)
 }
 
 /**/
-void
+mod_export void
 colonarrsetfn(Param pm, char *x)
 {
     char ***dptr = (char ***)pm->u.data;
diff --git a/Src/utils.c b/Src/utils.c
index ed3690172..8adab2bd7 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -1494,11 +1494,6 @@ deltimedfn(voidvoidfnptr_t func)
 /**/
 time_t lastmailcheck;
 
-/* the last time we checked the people in the WATCH variable */
-
-/**/
-time_t lastwatch;
-
 /*
  * Call a function given by "name" with optional arguments
  * "lnklst".  If these are present the first argument is the function name.
@@ -1637,17 +1632,6 @@ preprompt(void)
     if (errflag)
 	return;
 
-    /* If WATCH is set, then check for the *
-     * specified login/logout events.      */
-    if (watch) {
-	if ((int) difftime(time(NULL), lastwatch) > getiparam("LOGCHECK")) {
-	    dowatch();
-	    lastwatch = time(NULL);
-	}
-    }
-    if (errflag)
-	return;
-
     /* Check mail */
     currentmailcheck = time(NULL);
     if (mailcheck &&
diff --git a/Src/zsh.mdd b/Src/zsh.mdd
index 9bcaccae5..da8d58322 100644
--- a/Src/zsh.mdd
+++ b/Src/zsh.mdd
@@ -13,7 +13,7 @@ objects="builtin.o compat.o cond.o context.o \
 exec.o glob.o hashtable.o hashnameddir.o \
 hist.o init.o input.o jobs.o lex.o linklist.o loop.o math.o \
 mem.o module.o options.o params.o parse.o pattern.o prompt.o signals.o \
-signames.o sort.o string.o subst.o text.o utils.o watch.o \
+signames.o sort.o string.o subst.o text.o utils.o \
 openssh_bsd_setres_id.o"
 
 headers="../config.h zsh_system.h zsh.h sigcount.h signals.h \

Re: PATCH: separate watch/log functionality out into a module

[Oliver Kiddle]

I wrote:
> This patch extracts the functionality out into a zsh/watch module.

The following is a corresponding documentation patch. This only
moves existing documentation to come under zshmodules(1) instead of
zshparam(1). The moved blocks are not otherwise changed.

The autoloading of modules from parameters is not ideal where the
parameters come from the environment. If WATCH is in the imported
environment it will now print an error that the parameter already
exists. I don't think this matters especially for WATCH but this would
need dealing with before we could do something similar for $MAIL.

For an example with an unpatched zsh, this is not great:
  options=foo zsh -df
  zsh: options: attempt to set slice of associative array
And unlike for WATCH, that's a fatal error so zsh exits.

Oliver

diff --git a/Doc/Makefile.in b/Doc/Makefile.in
index 5a6a705ff..23e5fc7e2 100644
--- a/Doc/Makefile.in
+++ b/Doc/Makefile.in
@@ -69,6 +69,7 @@ Zsh/mod_parameter.yo Zsh/mod_pcre.yo Zsh/mod_private.yo \
 Zsh/mod_regex.yo Zsh/mod_sched.yo Zsh/mod_socket.yo \
 Zsh/mod_stat.yo  Zsh/mod_system.yo Zsh/mod_tcp.yo \
 Zsh/mod_termcap.yo Zsh/mod_terminfo.yo \
+Zsh/mod_watch.yo \
 Zsh/mod_zftp.yo Zsh/mod_zle.yo Zsh/mod_zleparameter.yo \
 Zsh/mod_zprof.yo Zsh/mod_zpty.yo Zsh/mod_zselect.yo \
 Zsh/mod_zutil.yo
diff --git a/Doc/Zsh/builtins.yo b/Doc/Zsh/builtins.yo
index ddbcd4363..733d8f185 100644
--- a/Doc/Zsh/builtins.yo
+++ b/Doc/Zsh/builtins.yo
@@ -1235,14 +1235,6 @@ Same as tt(typeset), except that the options tt(-g), and
 tt(-f) are not permitted.  In this case the tt(-x) option does not force
 the use of tt(-g), i.e. exported variables will be local to functions.
 )
-findex(log)
-vindex(watch, use of)
-cindex(watching users)
-cindex(users, watching)
-item(tt(log))(
-List all users currently logged in who are affected by
-the current setting of the tt(watch) parameter.
-)
 findex(logout)
 item(tt(logout) [ var(n) ])(
 Same as tt(exit), except that it only works in a login shell.
diff --git a/Doc/Zsh/compat.yo b/Doc/Zsh/compat.yo
index 6e4dbcfa4..4d3567d45 100644
--- a/Doc/Zsh/compat.yo
+++ b/Doc/Zsh/compat.yo
@@ -30,8 +30,7 @@ tt(PROMPT2),
 tt(PROMPT3),
 tt(PROMPT4),
 tt(psvar),
-tt(status),
-tt(watch).
+tt(status).
 
 vindex(ENV, use of)
 The usual zsh startup/shutdown scripts are not executed.  Login shells
diff --git a/Doc/Zsh/mod_watch.yo b/Doc/Zsh/mod_watch.yo
new file mode 100644
index 000000000..4eea89e23
--- /dev/null
+++ b/Doc/Zsh/mod_watch.yo
@@ -0,0 +1,140 @@
+COMMENT(!MOD!zsh/watch
+Reporting of login and logout events.
+!MOD!)
+The tt(zsh/watch) module can be used to report when specific users log in or
+out. This is controlled via the following parameters.
+
+startitem()
+vindex(LOGCHECK)
+item(tt(LOGCHECK))(
+The interval in seconds between checks for login/logout activity
+using the tt(watch) parameter.
+)
+vindex(watch)
+vindex(WATCH)
+item(tt(watch) <S> <Z> (tt(WATCH) <S>))(
+An array (colon-separated list) of login/logout events to report.
+
+If it contains the single word `tt(all)', then all login/logout events
+are reported.  If it contains the single word `tt(notme)', then all
+events are reported as with `tt(all)' except tt($USERNAME).
+
+An entry in this list may consist of a username,
+an `tt(@)' followed by a remote hostname,
+and a `tt(%)' followed by a line (tty).  Any of these may
+be a pattern (be sure to quote this during the assignment to
+tt(watch) so that it does not immediately perform file generation);
+the setting of the tt(EXTENDED_GLOB) option is respected.
+Any or all of these components may be present in an entry;
+if a login/logout event matches all of them,
+it is reported.
+
+For example, with the tt(EXTENDED_GLOB) option set, the following:
+
+example(watch=('^(pws|barts)'))
+
+causes reports for activity associated with any user other than tt(pws)
+or tt(barts).
+)
+vindex(WATCHFMT)
+item(tt(WATCHFMT))(
+The format of login/logout reports if the tt(watch) parameter is set.
+Default is `tt(%n has %a %l from %m)'.
+Recognizes the following escape sequences:
+
+startitem()
+item(tt(%n))(
+The name of the user that logged in/out.
+)
+item(tt(%a))(
+The observed action, i.e. "logged on" or "logged off".
+)
+item(tt(%l))(
+The line (tty) the user is logged in on.
+)
+item(tt(%M))(
+The full hostname of the remote host.
+)
+item(tt(%m))(
+The hostname up to the first `tt(.)'.  If only the
+IP address is available or the utmp field contains
+the name of an X-windows display, the whole name is printed.
+
+em(NOTE:)
+The `tt(%m)' and `tt(%M)' escapes will work only if there is a host name
+field in the utmp on your machine.  Otherwise they are
+treated as ordinary strings.
+)
+item(tt(%S) LPAR()tt(%s)RPAR())(
+Start (stop) standout mode.
+)
+item(tt(%U) LPAR()tt(%u)RPAR())(
+Start (stop) underline mode.
+)
+item(tt(%B) LPAR()tt(%b)RPAR())(
+Start (stop) boldface mode.
+)
+xitem(tt(%t))
+item(tt(%@))(
+The time, in 12-hour, am/pm format.
+)
+item(tt(%T))(
+The time, in 24-hour format.
+)
+item(tt(%w))(
+The date in `var(day)tt(-)var(dd)' format.
+)
+item(tt(%W))(
+The date in `var(mm)tt(/)var(dd)tt(/)var(yy)' format.
+)
+item(tt(%D))(
+The date in `var(yy)tt(-)var(mm)tt(-)var(dd)' format.
+)
+item(tt(%D{)var(string)tt(}))(
+The date formatted as var(string) using the tt(strftime) function, with
+zsh extensions as described by
+ifzman(EXPANSION OF PROMPT SEQUENCES in zmanref(zshmisc))\
+ifnzman(noderef(Prompt Expansion)).
+)
+item(tt(%LPAR())var(x)tt(:)var(true-text)tt(:)var(false-text)tt(RPAR()))(
+Specifies a ternary expression.
+The character following the var(x) is
+arbitrary; the same character is used to separate the text
+for the "true" result from that for the "false" result.
+Both the separator and the right parenthesis may be escaped
+with a backslash.
+Ternary expressions may be nested.
+
+The test character var(x) may be any one of `tt(l)', `tt(n)', `tt(m)'
+or `tt(M)', which indicate a `true' result if the corresponding
+escape sequence would return a non-empty value; or it may be `tt(a)',
+which indicates a `true' result if the watched user has logged in,
+or `false' if he has logged out.
+Other characters evaluate to neither true nor false; the entire
+expression is omitted in this case.
+
+If the result is `true', then the var(true-text)
+is formatted according to the rules above and printed,
+and the var(false-text) is skipped.
+If `false', the var(true-text) is skipped and the var(false-text)
+is formatted and printed.
+Either or both of the branches may be empty, but
+both separators must be present in any case.
+)
+enditem()
+)
+enditem()
+
+Furthermore, the tt(zsh/watch) module makes available one builtin
+command:
+
+startitem()
+findex(log)
+vindex(watch, use of)
+cindex(watching users)
+cindex(users, watching)
+item(tt(log))(
+List all users currently logged in who are affected by
+the current setting of the tt(watch) parameter.
+)
+enditem()
diff --git a/Doc/Zsh/params.yo b/Doc/Zsh/params.yo
index a88e44d4f..1f2f01f55 100644
--- a/Doc/Zsh/params.yo
+++ b/Doc/Zsh/params.yo
@@ -1332,11 +1332,6 @@ most as many lines as given by the absolute value.
 If set to zero, the shell asks only if the top of the listing would scroll
 off the screen.
 )
-vindex(LOGCHECK)
-item(tt(LOGCHECK))(
-The interval in seconds between checks for login/logout activity
-using the tt(watch) parameter.
-)
 vindex(MAIL)
 item(tt(MAIL))(
 If this parameter is set and tt(mailpath) is not set,
@@ -1670,119 +1665,6 @@ to be interpreted as a file extension.  The default is not to append
 any suffix, thus this parameter should be assigned only when needed
 and then unset again.
 )
-vindex(watch)
-vindex(WATCH)
-item(tt(watch) <S> <Z> (tt(WATCH) <S>))(
-An array (colon-separated list) of login/logout events to report.
-
-If it contains the single word `tt(all)', then all login/logout events
-are reported.  If it contains the single word `tt(notme)', then all
-events are reported as with `tt(all)' except tt($USERNAME).
-
-An entry in this list may consist of a username,
-an `tt(@)' followed by a remote hostname,
-and a `tt(%)' followed by a line (tty).  Any of these may
-be a pattern (be sure to quote this during the assignment to
-tt(watch) so that it does not immediately perform file generation);
-the setting of the tt(EXTENDED_GLOB) option is respected.
-Any or all of these components may be present in an entry;
-if a login/logout event matches all of them,
-it is reported.
-
-For example, with the tt(EXTENDED_GLOB) option set, the following:
-
-example(watch=('^(pws|barts)'))
-
-causes reports for activity associated with any user other than tt(pws)
-or tt(barts).
-)
-vindex(WATCHFMT)
-item(tt(WATCHFMT))(
-The format of login/logout reports if the tt(watch) parameter is set.
-Default is `tt(%n has %a %l from %m)'.
-Recognizes the following escape sequences:
-
-startitem()
-item(tt(%n))(
-The name of the user that logged in/out.
-)
-item(tt(%a))(
-The observed action, i.e. "logged on" or "logged off".
-)
-item(tt(%l))(
-The line (tty) the user is logged in on.
-)
-item(tt(%M))(
-The full hostname of the remote host.
-)
-item(tt(%m))(
-The hostname up to the first `tt(.)'.  If only the
-IP address is available or the utmp field contains
-the name of an X-windows display, the whole name is printed.
-
-em(NOTE:)
-The `tt(%m)' and `tt(%M)' escapes will work only if there is a host name
-field in the utmp on your machine.  Otherwise they are
-treated as ordinary strings.
-)
-item(tt(%S) LPAR()tt(%s)RPAR())(
-Start (stop) standout mode.
-)
-item(tt(%U) LPAR()tt(%u)RPAR())(
-Start (stop) underline mode.
-)
-item(tt(%B) LPAR()tt(%b)RPAR())(
-Start (stop) boldface mode.
-)
-xitem(tt(%t))
-item(tt(%@))(
-The time, in 12-hour, am/pm format.
-)
-item(tt(%T))(
-The time, in 24-hour format.
-)
-item(tt(%w))(
-The date in `var(day)tt(-)var(dd)' format.
-)
-item(tt(%W))(
-The date in `var(mm)tt(/)var(dd)tt(/)var(yy)' format.
-)
-item(tt(%D))(
-The date in `var(yy)tt(-)var(mm)tt(-)var(dd)' format.
-)
-item(tt(%D{)var(string)tt(}))(
-The date formatted as var(string) using the tt(strftime) function, with
-zsh extensions as described by
-ifzman(EXPANSION OF PROMPT SEQUENCES in zmanref(zshmisc))\
-ifnzman(noderef(Prompt Expansion)).
-)
-item(tt(%LPAR())var(x)tt(:)var(true-text)tt(:)var(false-text)tt(RPAR()))(
-Specifies a ternary expression.
-The character following the var(x) is
-arbitrary; the same character is used to separate the text
-for the "true" result from that for the "false" result.
-Both the separator and the right parenthesis may be escaped
-with a backslash.
-Ternary expressions may be nested.
-
-The test character var(x) may be any one of `tt(l)', `tt(n)', `tt(m)'
-or `tt(M)', which indicate a `true' result if the corresponding
-escape sequence would return a non-empty value; or it may be `tt(a)',
-which indicates a `true' result if the watched user has logged in,
-or `false' if he has logged out.
-Other characters evaluate to neither true nor false; the entire
-expression is omitted in this case.
-
-If the result is `true', then the var(true-text)
-is formatted according to the rules above and printed,
-and the var(false-text) is skipped.
-If `false', the var(true-text) is skipped and the var(false-text)
-is formatted and printed.
-Either or both of the branches may be empty, but
-both separators must be present in any case.
-)
-enditem()
-)
 vindex(WORDCHARS)
 item(tt(WORDCHARS) <S>)(
 A list of non-alphanumeric characters considered part of a word

Re: PATCH: separate watch/log functionality out into a module

[Bart Schaefer]

On Sun, Oct 31, 2021 at 5:07 PM Oliver Kiddle <opk@zsh.org> wrote:
>
> The autoloading of modules from parameters is not ideal where the
> parameters come from the environment.

Hm.  What would the right behavior be?  Do we need a flag on the
autoloaded parameter for whether the value is [not?] allowed to come
from the environment?  PM_DONTIMPORT seems potentially relevant.

> For an example with an unpatched zsh, this is not great:
>   options=foo zsh -df
>   zsh: options: attempt to set slice of associative array
> And unlike for WATCH, that's a fatal error so zsh exits.

I think you're misinterpreting what's happening.  If I use gdb:

(gdb) set environ options=foo
(gdb) run -df
Starting program: Src/zsh -df
zsh: Can't add module parameter `options': parameter already exists
ubuntu%

I get your error if I try to assign to options in the environment from
the current zsh:

ubuntu% options=foo Src/zsh -df
zsh: options: attempt to set slice of associative array
ubuntu%

But the shell where I did that does not exit, the error aborts the
command so no new shell is ever started.

If I unset options first, the new shell complains:

ubuntu% (echo $$; unset options; options=foo Src/zsh -df )
695688
zsh: Can't add module parameter `options': parameter already exists
ubuntu% echo $$
695694

(this is all without your patches)

Re: PATCH: separate watch/log functionality out into a module

[Oliver Kiddle]

On 31 Oct, Bart Schaefer wrote:

(reordering Bart's reply here)

> I think you're misinterpreting what's happening.  If I use gdb:

Yes, you're right so I retract most of what I said. I'm used to
just prefixing commands to export values but the assignment is, of
course, taking place in the initial shell.

> Hm.  What would the right behavior be?  Do we need a flag on the
> autoloaded parameter for whether the value is [not?] allowed to come
> from the environment?  PM_DONTIMPORT seems potentially relevant.

I'm not sure whether PM_DONTIMPORT really helps at all.
It is probably necessary this way but even with env, we do get:
  env options=foo zsh -df
  zsh: Can't add module parameter `options': parameter already exists

But for a parameter like WATCH, it'd ideally load the module and use the
imported value. Is it an important feature that $WATCH can come from the
environment?

With sh emulation we don't get that error message which was really what
I was concerned about.

> (this is all without your patches)

I don't think my patches will have changed anything in this regard other
than for what happens with $watch and $WATCH.

On further testing zsh/watch does need to better guard against one of
them coming from the environment. They can only be tied if both are
there. The patch below covers this case.

Can someone with a Mac confirm something: have they disabled the log
builtin because it clashed with an external command. I did have a link
for their code modifications but the link appears to be broken. A log(1)
man page does seem to exist based on a web search. If watch is a module,
hopefully they include it in future and only disable the autoloading.

Oliver

diff --git a/Src/Modules/watch.c b/Src/Modules/watch.c
index 02f0562fc..5ce604c63 100644
--- a/Src/Modules/watch.c
+++ b/Src/Modules/watch.c
@@ -640,8 +640,8 @@ static struct builtin bintab[] = {
 };
 
 static struct paramdef partab[] = {
-    PARAMDEF("WATCH", PM_TIED|PM_SCALAR|PM_SPECIAL, &watch, &colonarr_gsu),
-    PARAMDEF("watch", PM_TIED|PM_ARRAY|PM_SPECIAL, &watch, &vararray_gsu),
+    PARAMDEF("WATCH", PM_SCALAR|PM_SPECIAL, &watch, &colonarr_gsu),
+    PARAMDEF("watch", PM_ARRAY|PM_SPECIAL, &watch, &vararray_gsu),
 };
 
 static struct features module_features = {
@@ -679,12 +679,16 @@ int
 boot_(UNUSED(Module m))
 {
     static char const * const default_watchfmt = DEFAULT_WATCHFMT;
-    Param pm;
 
-    if ((pm = (Param) paramtab->getnode(paramtab, "watch")))
-	pm->ename = "WATCH";
-    if ((pm = (Param) paramtab->getnode(paramtab, "WATCH")))
-	pm->ename = "watch";
+    Param pma = (Param) paramtab->getnode(paramtab, "watch");
+    Param pms = (Param) paramtab->getnode(paramtab, "WATCH");
+    if (pma && pms && pma->u.arr == watch && pms->u.arr == watch) {
+	/* only tie the two parameters if both were added */
+	pma->ename = "WATCH";
+	pms->ename = "watch";
+	pma->node.flags |= PM_TIED;
+	pms->node.flags |= PM_TIED;
+    }
     watch = mkarray(NULL);
 
     /* These two parameters are only set to defaults if not set.

Re: PATCH: separate watch/log functionality out into a module

[Bart Schaefer]

On Fri, Nov 5, 2021 at 5:17 PM Oliver Kiddle <opk@zsh.org> wrote:
>
> > Hm.  What would the right behavior be?  Do we need a flag on the
> > autoloaded parameter for whether the value is [not?] allowed to come
> > from the environment?  PM_DONTIMPORT seems potentially relevant.
>
> I'm not sure whether PM_DONTIMPORT really helps at all.

My thought was that the module bootstrap could examine its paramdef
array for PM_DONTIMPORT and either warn about, or skip marking for
autoload, any variable with that flag that is already in the
environment.

> But for a parameter like WATCH, it'd ideally load the module and use the
> imported value. Is it an important feature that $WATCH can come from the
> environment?

I don't have a feel for this.  As you pointed out earlier in the
thread, the prevalence of multi-user environments has diminished.

> On further testing zsh/watch does need to better guard against one of
> them coming from the environment. They can only be tied if both are
> there. The patch below covers this case.

How was this dealt with in the base shell, before you modularized it?
Array values ($watch) can't be imported, so something must have
happened when WATCH alone is an environment value at shell startup.

I would think the ideal behavior would be "as if":

  local W=$WATCH
  unset WATCH
  zmodload zsh/watch
  [[ -n $W ]] && WATCH=$W  # triggers autoload and tie

> Can someone with a Mac confirm something: have they disabled the log
> builtin because it clashed with an external command.

/bin/zsh on Catalina has not disabled the builtin; I don't have a
Monterey install to check.

> A log(1)
> man page does seem to exist

NAME
     log -- Access system wide log messages created by os_log, os_trace and
     other logging systems.

Seems to be borrowed from BSD.

Re: PATCH: separate watch/log functionality out into a module

[Jun. T]

> 2021/11/05 17:17, Oliver Kiddle <opk@zsh.org> wrote:
> 
> Can someone with a Mac confirm something: have they disabled the log
> builtin because it clashed with an external command.

Apple puts the following lines to its /etc/zshrc:

# Disable the log builtin, so we don't conflict with /usr/bin/log
disable log

Re: PATCH: separate watch/log functionality out into a module

[Daniel Shahaf]

Jun. T wrote on Tue, 09 Nov 2021 10:27 +00:00:
> Apple puts the following lines to its /etc/zshrc:
>
> # Disable the log builtin, so we don't conflict with /usr/bin/log
> disable log

Might be better for them to change that to «log() { command log "$@" }».
This would preserve their change of default behaviour but make it
possible for users to explicitly call the builtin (through the syntax
«builtin log …»).

Re: PATCH: separate watch/log functionality out into a module

[Jun T]

> 2021/10/30 7:15, Oliver Kiddle <opk@zsh.org> wrote:
> 
> This patch extracts the functionality out into a zsh/watch module.

With this patch (either with or without the one in worker/49544⁩),
build fails on Cygwin as follows:

gcc -c ...(snip)...  -o watch..o watch.c
In file included from ../../Src/zsh.mdh:16,
                 from watch.mdh:15,
                 from watch.c:30:
../../Src/zsh.h:2097:34: error: initializer element is not constant
 2097 |     { name, flags, (void *) var, (void *) gsu, \
      |                                  ^
watch.c:643:5: note: in expansion of macro ‘PARAMDEF’
  643 |     PARAMDEF("WATCH", PM_SCALAR|PM_SPECIAL, &watch, &colonarr_gsu),
      |     ^~~~~~~~

(and the same error for &vararray_gsu)

On Cygwin, params.o (in which colonarr_gsu is defined) is not included in
the main zsh.exe but in libzsh.dll (I don't know why).
This means &colonarr_gsu is not a build-time constant and can't be used
for the initialization of the static variable partab[].
A simple workaround is to set the .gsu in the setup_() function.
This takes virtually no time to execute and I think we don't need to use
#ifdef __CYGWIN__.




diff --git a/Src/Modules/watch.c b/Src/Modules/watch.c
index 5ce604c63..1c2766dda 100644
--- a/Src/Modules/watch.c
+++ b/Src/Modules/watch.c
@@ -640,8 +640,8 @@ static struct builtin bintab[] = {
 };
 
 static struct paramdef partab[] = {
-    PARAMDEF("WATCH", PM_SCALAR|PM_SPECIAL, &watch, &colonarr_gsu),
-    PARAMDEF("watch", PM_ARRAY|PM_SPECIAL, &watch, &vararray_gsu),
+    PARAMDEF("WATCH", PM_SCALAR|PM_SPECIAL, &watch, NULL),
+    PARAMDEF("watch", PM_ARRAY|PM_SPECIAL, &watch, NULL),
 };
 
 static struct features module_features = {
@@ -656,6 +656,10 @@ static struct features module_features = {
 int
 setup_(UNUSED(Module m))
 {
+    /* On Cygwin, colonarr_gsu exists in libzsh.dll and we can't
+     * use &colonarr_gsu in the initialization of bintab[] above */
+    partab[0].gsu = (void *)&colonarr_gsu;
+    partab[1].gsu = (void *)&vararray_gsu;
     return 0;
 }
 

Re: PATCH: separate watch/log functionality out into a module

[Oliver Kiddle]

Jun T wrote:
>
> With this patch (either with or without the one in worker/49544⁩),
> build fails on Cygwin as follows:

Thanks for catching this.

> A simple workaround is to set the .gsu in the setup_() function.
> This takes virtually no time to execute and I think we don't need to use
> #ifdef __CYGWIN__.

That sounds reasonable to me.

That is assuming I didn't get something wrong when tagging visibility
on the structs and their members. I wouldn't have complete faith in
the build system to adapt to those changes so, if you didn't already,
start with an absolutely clean build.

colonarr_gsu did change from static to mod_export along with the get and
set function it includes. vararray_gsu didn't change other than in
being referenced from a module. Looking for other examples, I'm not
really finding any. Modules otherwise only include variables with
access functions that are unique to them.

Oliver

Re: PATCH: separate watch/log functionality out into a module

[Jun T]

> 2021/11/12 2:08, Oliver Kiddle <opk@zsh.org> wrote:
> 
>> A simple workaround is to set the .gsu in the setup_() function.
>> This takes virtually no time to execute and I think we don't need to use
>> #ifdef __CYGWIN__.
> 
> That sounds reasonable to me.
> 
> That is assuming I didn't get something wrong when tagging visibility
> on the structs and their members. I wouldn't have complete faith in
> the build system to adapt to those changes so, if you didn't already,
> start with an absolutely clean build.

I had tested with clean build because I wanted to test with load=yes and
load=no in watch.mdd.

Re: PATCH: separate watch/log functionality out into a module

[Oliver Kiddle]

On 6 Nov, Bart Schaefer wrote:
> My thought was that the module bootstrap could examine its paramdef
> array for PM_DONTIMPORT and either warn about, or skip marking for
> autoload, any variable with that flag that is already in the
> environment.

The paramdef structure and PM_ flags are stored in the module so we
can't access them until the module is loaded. What we have to work with
is what goes into Src/bltinmods.list. That only has zmodload
feature flags like "p:WATCH", originally from the .mdd file. The final
parameter to autofeatures() is a FEAT_ flag, currently hardcoded to 1
(FEAT_IGNORE) so we could do something with that at a module rather than
parameter level of granularity. We're importing variables from the
environment before setting up features that autoload parameters.

> How was this dealt with in the base shell, before you modularized it?
> Array values ($watch) can't be imported, so something must have
> happened when WATCH alone is an environment value at shell startup.

Special parameters are setup before importing from the environment so
the imported WATCH value was used and also reflected in $watch.

The $WATCH form probably only exists to allow it to be carried around in
the environment. With a more modern security perspective, this doesn't
seem especially clever. Treating the environment as untrusted, it
adds the watch code to the attack surface. Passing it's value via the
environment isn't especially useful. So if compatibility can be restored
that'd be nice but if it is too tricky, I won't be overly concerned.

> I would think the ideal behavior would be "as if":
>
>   local W=$WATCH
>   unset WATCH
>   zmodload zsh/watch
>   [[ -n $W ]] && WATCH=$W  # triggers autoload and tie

We could use a PM flag to indicate that an existing value should be
saved and restored (assuming that value was compatible). Or even do this
for any non-readonly. Rearranging the initialisation code to import the
environment after setting up autoloadable parameters may be an option if
it doesn't have other unwanted side-effects.

The following small patch avoids the error on startup which is better
in my view. I think this was actually the intention when the code was
written. Certainly my understanding of the preceding comment is that the
message was supposed to be used if the existing variable was local. I
increased context for this patch to include the whole comment. opt_i is
set for zmodload -i or from the autoloading mechanism. pm points to an
existing parameter with the same name so pm->level would indicate that
it is local.

Oliver

diff --git a/Src/module.c b/Src/module.c
index f41b82f25..bab4d8d73 100644
--- a/Src/module.c
+++ b/Src/module.c
@@ -1035,15 +1035,15 @@ checkaddparam(const char *nam, int opt_i)
 	 * -i suppresses "it's already that way" warnings,
 	 * but not "this can't possibly work" warnings, so we print
 	 * the message anyway if there's a local parameter blocking
 	 * the parameter we want to add, not if there's a
 	 * non-autoloadable parameter already there.  This
 	 * is consistent with the way add_auto* functions work.
 	 */
-	if (!opt_i || !pm->level) {
+	if (!opt_i || pm->level) {
 	    zwarn("Can't add module parameter `%s': %s",
 		  nam, pm->level ?
 		  "local parameter exists" :
 		  "parameter already exists");
 	    return 1;
 	}
 	return 2;

Re: PATCH: separate watch/log functionality out into a module

[Bart Schaefer]

On Sun, Nov 28, 2021 at 12:56 PM Oliver Kiddle <opk@zsh.org> wrote:
>
> The paramdef structure and PM_ flags are stored in the module so we
> can't access them until the module is loaded. What we have to work with
> is what goes into Src/bltinmods.list.

Ah, of course.

> Rearranging the initialisation code to import the
> environment after setting up autoloadable parameters may be an option if
> it doesn't have other unwanted side-effects.

I don't think that's do-able.  It would only be possible for builtin
modules, and there's a chicken-and-egg problem with any environment
variable that a module declares as an autoloaded parameter; tied
arrays are just extra hairy.

> The following small patch avoids the error on startup which is better

Agree.