9front - general discussion about 9front
 help / color / mirror / Atom feed
* [9front] [PATCH] rc: keep env vars and functions in sync
@ 2024-07-11  5:32 Romano
  2024-07-12 19:36 ` Romano
  0 siblings, 1 reply; 8+ messages in thread
From: Romano @ 2024-07-11  5:32 UTC (permalink / raw)
  To: 9front


	From env(3):
	          The env device serves a one-level directory containing files
	          with arbitrary names and contents.  The intention is that
	          the file name is the name of an environment variable (see
	          rc(1)), and the content is the variable's current value.

	And from rc(1):
        Environment
          The environment is a list of strings made available to exe-
          cuting binaries by the env device (see env(3)).

	But currently, the environment variables and functions are not kept
	in sync with /env . For instance, '>/env/a echo -n b' will not
	update $a to be 'b'. Also, even when the namespaces are shared
	between processes, once the child process completes the namespaces
	are out of sync.

	This patch attempts to address that by updating the environment
	variables and functions after children processes exit when the
	children share the same namespace. Tests are added for the
	functionality as well.
---
diff 8a64e5171aad8bb0201a61e7df3046892ee6617f c17447a0c0bc8211015c87261abe57ed46ffe0ca
--- a/sys/src/cmd/rc/fns.h
+++ b/sys/src/cmd/rc/fns.h
@@ -27,6 +27,7 @@
 void	Setstatus(char*);
 void	Trapinit(void);
 void	Updenv(void);
+void	Updenvall(void);
 void	Vinit(void);
 int	Waitfor(int);
 long	Write(int, void*, long);
--- a/sys/src/cmd/rc/havefork.c
+++ b/sys/src/cmd/rc/havefork.c
@@ -64,6 +64,7 @@
 		setvar("apid", newword(npid, (word *)0));
 		break;
 	}
+	Updenvall();
 }
 
 void
@@ -99,6 +100,7 @@
 		p->pid = pid;
 		break;
 	}
+	Updenvall();
 }
 
 /*
@@ -128,7 +130,7 @@
 		Close(pfd[PRD]);
 		start(runq->code, runq->pc+1, runq->local, runq->redir);
 		pushredir(ROPEN, pfd[PWR], 1);
-		return;
+		break;
 	default:
 		addwaitpid(pid);
 		Close(pfd[PWR]);
@@ -149,8 +151,9 @@
 		Waitfor(pid);
 
 		runq->pc = runq->code[runq->pc].i;
-		return;
+		break;
 	}
+	Updenvall();
 }
 
 void
@@ -196,6 +199,7 @@
 		p->pc = p->code[pc+1].i;
 		break;
 	}
+	Updenvall();
 }
 
 void
@@ -219,6 +223,7 @@
 		runq->pc = runq->code[runq->pc].i;
 		break;
 	}
+	Updenvall();
 }
 
 int
--- a/sys/src/cmd/rc/plan9.c
+++ b/sys/src/cmd/rc/plan9.c
@@ -259,6 +259,17 @@
 }
 
 void
+Updenvall(void)
+{
+	Vinit();
+	char *cmds = estrdup("for(fn in /env/fn'#'*) { . -bq $fn }\n");
+	int line = runq->line;
+	execcmds(openiocore(cmds, strlen(cmds)), estrdup(srcfile(runq)), runq->local, runq->redir);
+	runq->lex->line = line;
+	runq->lex->qflag = 1;
+}
+
+void
 Exec(char **argv)
 {
 	exec(argv[0], argv+1);
--- a/sys/src/cmd/rc/simple.c
+++ b/sys/src/cmd/rc/simple.c
@@ -102,6 +102,7 @@
 			/* interrupts don't get us out */
 			while(Waitfor(pid) < 0)
 				;
+			Updenvall();
 		}
 	}
 }
--- /dev/null
+++ b/sys/src/cmd/rc/test/mkfile
@@ -1,0 +1,6 @@
+</$objtype/mkfile
+
+TEST=\
+	updenvall\
+
+</sys/src/cmd/mktest
--- /dev/null
+++ b/sys/src/cmd/rc/test/updenvall.file
@@ -1,0 +1,80 @@
+>/env/var1 echo -n a
+ok `\x04{whatis var1} 'var1=a
+' '$var1 is /env/var1'
+ok $var1 a '$var1 is a'
+ok `\x04{cat /env/var1} a '/env/var1 is a'
+
+echo testing var with rc
+../$O.out -c 'var1=ab'
+ok `\x04{whatis var1} 'var1=ab
+' 'var1 is ab'
+ok $var1 ab '$var1 is ab'
+ok `\x04{cat /env/var1} ab '/env/var1 is ab'
+
+echo testing var with rc -c
+../$O.out -c 'var1=b'
+ok `\x04{whatis var1} 'var1=b
+' 'var1 is b'
+ok $var1 b '$var1 is b'
+ok `\x04{cat /env/var1} b '/env/var1 is b'
+
+echo testing var with subshell
+@{ var1=c }
+ok `\x04{whatis var1} 'var1=c
+' 'var1 is c'
+ok $var1 c '$var1 is c'
+ok `\x04{cat /env/var1} c '/env/var1 is c'
+
+echo testing var with backtick
+echo `\x04{ var1=d } >/dev/null
+ok `\x04{whatis var1} 'var1=d
+' 'var1 is d'
+ok $var1 d '$var1 is d'
+ok `\x04{cat /env/var1} d '/env/var1 is d'
+
+a=()
+> '/env/fn#a' echo -n 'fn a {
+	echo a
+}
+'
+ok `\x04{whatis a} 'fn a {
+	echo a
+}
+' 'fn a is echo a'
+ok `\x04{cat '/env/fn#a'} 'fn a {
+	echo a
+}
+' '/env/fn#a is echo a'
+
+echo testing fn with rc -c
+../$O.out -c 'fn a { echo b }'
+ok `\x04{whatis a} 'fn a {
+	echo b
+}
+' 'fn a is echo b'
+ok `\x04{cat '/env/fn#a'} 'fn a {
+	echo b
+}
+' '/env/fn#a is echo b'
+
+echo testing fn with subshell
+@{ fn a { echo c } }
+ok `\x04{whatis a} 'fn a {
+	echo c
+}
+' 'fn b is echo c'
+ok `\x04{cat '/env/fn#a'} 'fn a {
+	echo c
+}
+' '/env/fn#a is echo c'
+
+echo testing fn with backtick
+echo `\x04{fn a { echo d } } >/dev/null
+ok `\x04{whatis a} 'fn a {
+	echo d
+}
+' 'fn a is echo d'
+ok `\x04{cat '/env/fn#a'} 'fn a {
+	echo d
+}
+' '/env/fn#a is echo d'
--- /dev/null
+++ b/sys/src/cmd/rc/test/updenvall.rc
@@ -1,0 +1,16 @@
+#!/bin/rc
+# test that all environment variables and functions
+# are in sync.
+
+fn ok {
+	actual=$1
+	shift
+	expected=$1
+	shift
+	desc=$1
+	if(~ $"actual $"expected)	echo ok $desc
+	if not echo 'not ok '^$"desc^', got '$"actual
+}
+
+# now let's run the test with our build
+../$O.out <updenvall.file

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [9front] [PATCH] rc: keep env vars and functions in sync
  2024-07-11  5:32 [9front] [PATCH] rc: keep env vars and functions in sync Romano
@ 2024-07-12 19:36 ` Romano
  2024-07-12 19:58   ` ori
  0 siblings, 1 reply; 8+ messages in thread
From: Romano @ 2024-07-12 19:36 UTC (permalink / raw)
  To: 9front

After dogfooding this on my own system, I am liking how it works, save
for how it messed with $*, $pid, and $apid, which are all documented as
special variables and (at least for $*) affects the use of fn calls. So,
in other words, the previous patch had desiderata.

I also noticed the bug re: $status being overwritten when using ~ to
check its value. That is now fixed with the patch below.

In case anyone wants to try it out, after applying both the previous
patch and the patch below, you can run 'mk test' and then run 6.out
(which has a handy prompt, 'broken!', so you know what you're dealing).
For more widespread testing I have been then doing:
	cp 6.out $home/bin/$objtype/rc.share
to see how it compares with rc with some scripts.

From: Romano <me+unobe@fallglow.com>
Date: Fri, 12 Jul 2024 19:24:48 +0000
Subject: [PATCH] rc: do not share certain special vars; fix $status bug for most cases


Certain special vars (namely $*, $pid, $apid, and $status)
are used internally and should not be set after a child
returns.

There has been a long-standing bug where checking the value
of $status with ~ overwrites the value of $status. $status
is now restored in most cases, other than running it
as a standalone built-in command.

Man page for rc is updated
---
diff 7637f8f5fd6490d07ba64bcf404321028e698ed9 675bcd62992e59002c834ef445d515b346f4374d
--- a/sys/src/cmd/rc/code.c
+++ b/sys/src/cmd/rc/code.c
@@ -275,6 +275,8 @@
 		codeswitch(t, eflag);
 		break;
 	case TWIDDLE:
+		if(c0 && c0->type=='$' && c0->child[0] && (c0->child[0]->type==WORD && strcmp(c0->child[0]->str, "status")==0))
+			emitf(Xstashstatus);
 		emitf(Xmark);
 		noglobs(c1, 1);
 		outcode(c1, eflag);
--- a/sys/src/cmd/rc/exec.c
+++ b/sys/src/cmd/rc/exec.c
@@ -282,6 +282,16 @@
 			dotrap();
 	}
 }
+
+void
+unstashstatus(void)
+{
+	var *oldstatus = vlook("status-");
+	if(oldstatus->val && oldstatus->val->word){
+		setstatus(oldstatus->val->word);
+	}
+}
+
 /*
  * Opcode routines
  * Arguments on stack (...)
@@ -415,6 +425,7 @@
 void
 Xifnot(void)
 {
+	unstashstatus();
 	if(ifnot)
 		runq->pc++;
 	else
@@ -597,7 +608,9 @@
 void
 Xtrue(void)
 {
-	if(truestatus()) runq->pc++;
+	int stat = truestatus();
+	unstashstatus();
+	if(stat) runq->pc++;
 	else runq->pc = runq->code[runq->pc].i;
 }
 
@@ -605,7 +618,9 @@
 Xif(void)
 {
 	ifnot = 1;
-	if(truestatus()) runq->pc++;
+	int stat = truestatus();
+	unstashstatus();
+	if(stat) runq->pc++;
 	else runq->pc = runq->code[runq->pc].i;
 }
 
@@ -661,6 +676,12 @@
 		}
 	poplist();
 	poplist();
+}
+
+void
+Xstashstatus(void)
+{
+	setvar("status-", Newword(estrdup(getstatus()), (word *)0));
 }
 
 void
--- a/sys/src/cmd/rc/exec.h
+++ b/sys/src/cmd/rc/exec.h
@@ -4,7 +4,7 @@
 extern void Xappend(void), Xasync(void), Xbackq(void), Xbang(void), Xclose(void);
 extern void Xconc(void), Xcount(void), Xdelfn(void), Xdol(void), Xqw(void), Xdup(void);
 extern void Xexit(void), Xfalse(void), Xfn(void), Xfor(void), Xglob(void);
-extern void Xjump(void), Xmark(void), Xmatch(void), Xpipe(void), Xread(void), Xhere(void), Xhereq(void);
+extern void Xjump(void), Xmark(void), Xmatch(void), Xstashstatus(void), Xpipe(void), Xread(void), Xhere(void), Xhereq(void);
 extern void Xrdwr(void), Xsrcline(void);
 extern void Xunredir(void), Xstar(void), Xreturn(void), Xsubshell(void);
 extern void Xtrue(void), Xword(void), Xwrite(void), Xpipefd(void), Xcase(void);
--- a/sys/src/cmd/rc/pfnc.c
+++ b/sys/src/cmd/rc/pfnc.c
@@ -30,6 +30,7 @@
 	Xword, "Xword",
 	Xwrite, "Xwrite",
 	Xmatch, "Xmatch",
+	Xstashstatus, "Xstashstatus",
 	Xcase, "Xcase",
 	Xconc, "Xconc",
 	Xassign, "Xassign",
--- a/sys/src/cmd/rc/plan9.c
+++ b/sys/src/cmd/rc/plan9.c
@@ -144,7 +144,12 @@
 		if(n <= 0)
 			break;
 		for(i = 0; i<n; i++){
-			if(ent[i].length<=0 || strncmp(ent[i].name, "fn#", 3)==0)
+			if(ent[i].length<=0
+				|| strncmp(ent[i].name, "fn#", 3)==0
+				|| strcmp(ent[i].name,"*")==0
+				|| strcmp(ent[i].name,"apid")==0
+				|| strcmp(ent[i].name,"pid")==0
+				|| strcmp(ent[i].name,"status")==0)
 				continue;
 			if((fd = Open(Env(ent[i].name, 0), 0))>=0){
 				io *f = openiofd(fd);
--- a/sys/src/cmd/rc/test/updenvall.file
+++ b/sys/src/cmd/rc/test/updenvall.file
@@ -78,3 +78,40 @@
 	echo d
 }
 ' '/env/fn#a is echo d'
+
+echo testing special vars
+*=b
+echo -n a > /env/'*'
+ok $"* 'b' '* not set'
+echo -n $"pid >/tmp/pid.rc.test
+rc -c ''
+ok $"pid `{cat /tmp/pid.rc.test} 'pid not set'
+rm /tmp/pid.rc.test
+echo -n `{apid=foobar; echo $apid} >/dev/null
+ok $"apid '' 'apid not set'
+echo -n foo > /env/status
+ok $"status '' 'status not set'
+status=foo
+~ $status foo
+ok $"status '' 'naked ~ sets status to '''''
+status=`{echo foo}
+ok $"status foo 'backtick sets status'
+if(~ $status foo)
+	ok $"status foo '$status'
+if(! ~ $status a)
+	ok $"status foo '$status for ! ~'
+if(~ $status a)
+	;
+if not
+	ok $status foo '$status for if not'
+if(~ $"status foo)
+	ok $"status foo '$"status'
+if(echo a | grep -s a || ls)
+	ok $"status foo '||'
+while(! ~ $status foo)
+	status=bar
+ok $status foo 'after while not match'
+while(~ $status foo)
+	status=bar;
+ok $status bar 'after while match'
+status=''
--- a/sys/src/cmd/rc/test/updenvall.rc
+++ b/sys/src/cmd/rc/test/updenvall.rc
@@ -9,7 +9,7 @@
 	shift
 	desc=$1
 	if(~ $"actual $"expected)	echo ok $desc
-	if not echo 'not ok '^$"desc^', got '$"actual
+	if not echo 'NOT OK '^$"desc^', got '$"actual
 }
 
 # now let's run the test with our build

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [9front] [PATCH] rc: keep env vars and functions in sync
  2024-07-12 19:36 ` Romano
@ 2024-07-12 19:58   ` ori
  2024-07-12 20:15     ` Dave Woodman
  0 siblings, 1 reply; 8+ messages in thread
From: ori @ 2024-07-12 19:58 UTC (permalink / raw)
  To: 9front

this seems like it would make rc ridiculously slow.

Quoth Romano <me+unobe@fallglow.com>:
> After dogfooding this on my own system, I am liking how it works, save
> for how it messed with $*, $pid, and $apid, which are all documented as
> special variables and (at least for $*) affects the use of fn calls. So,
> in other words, the previous patch had desiderata.
> 
> I also noticed the bug re: $status being overwritten when using ~ to
> check its value. That is now fixed with the patch below.
> 
> In case anyone wants to try it out, after applying both the previous
> patch and the patch below, you can run 'mk test' and then run 6.out
> (which has a handy prompt, 'broken!', so you know what you're dealing).
> For more widespread testing I have been then doing:
> 	cp 6.out $home/bin/$objtype/rc.share
> to see how it compares with rc with some scripts.
> 
> From: Romano <me+unobe@fallglow.com>
> Date: Fri, 12 Jul 2024 19:24:48 +0000
> Subject: [PATCH] rc: do not share certain special vars; fix $status bug for most cases
> 
> 
> Certain special vars (namely $*, $pid, $apid, and $status)
> are used internally and should not be set after a child
> returns.
> 
> There has been a long-standing bug where checking the value
> of $status with ~ overwrites the value of $status. $status
> is now restored in most cases, other than running it
> as a standalone built-in command.

this is not a bug; ~ is a command.

> 
> Man page for rc is updated
> ---
> diff 7637f8f5fd6490d07ba64bcf404321028e698ed9 675bcd62992e59002c834ef445d515b346f4374d
> --- a/sys/src/cmd/rc/code.c
> +++ b/sys/src/cmd/rc/code.c
> @@ -275,6 +275,8 @@
>  		codeswitch(t, eflag);
>  		break;
>  	case TWIDDLE:
> +		if(c0 && c0->type=='$' && c0->child[0] && (c0->child[0]->type==WORD && strcmp(c0->child[0]->str, "status")==0))
> +			emitf(Xstashstatus);
>  		emitf(Xmark);
>  		noglobs(c1, 1);
>  		outcode(c1, eflag);
> --- a/sys/src/cmd/rc/exec.c
> +++ b/sys/src/cmd/rc/exec.c
> @@ -282,6 +282,16 @@
>  			dotrap();
>  	}
>  }
> +
> +void
> +unstashstatus(void)
> +{
> +	var *oldstatus = vlook("status-");
> +	if(oldstatus->val && oldstatus->val->word){
> +		setstatus(oldstatus->val->word);
> +	}
> +}
> +
>  /*
>   * Opcode routines
>   * Arguments on stack (...)
> @@ -415,6 +425,7 @@
>  void
>  Xifnot(void)
>  {
> +	unstashstatus();
>  	if(ifnot)
>  		runq->pc++;
>  	else
> @@ -597,7 +608,9 @@
>  void
>  Xtrue(void)
>  {
> -	if(truestatus()) runq->pc++;
> +	int stat = truestatus();
> +	unstashstatus();
> +	if(stat) runq->pc++;
>  	else runq->pc = runq->code[runq->pc].i;
>  }
>  
> @@ -605,7 +618,9 @@
>  Xif(void)
>  {
>  	ifnot = 1;
> -	if(truestatus()) runq->pc++;
> +	int stat = truestatus();
> +	unstashstatus();
> +	if(stat) runq->pc++;
>  	else runq->pc = runq->code[runq->pc].i;
>  }
>  
> @@ -661,6 +676,12 @@
>  		}
>  	poplist();
>  	poplist();
> +}
> +
> +void
> +Xstashstatus(void)
> +{
> +	setvar("status-", Newword(estrdup(getstatus()), (word *)0));
>  }
>  
>  void
> --- a/sys/src/cmd/rc/exec.h
> +++ b/sys/src/cmd/rc/exec.h
> @@ -4,7 +4,7 @@
>  extern void Xappend(void), Xasync(void), Xbackq(void), Xbang(void), Xclose(void);
>  extern void Xconc(void), Xcount(void), Xdelfn(void), Xdol(void), Xqw(void), Xdup(void);
>  extern void Xexit(void), Xfalse(void), Xfn(void), Xfor(void), Xglob(void);
> -extern void Xjump(void), Xmark(void), Xmatch(void), Xpipe(void), Xread(void), Xhere(void), Xhereq(void);
> +extern void Xjump(void), Xmark(void), Xmatch(void), Xstashstatus(void), Xpipe(void), Xread(void), Xhere(void), Xhereq(void);
>  extern void Xrdwr(void), Xsrcline(void);
>  extern void Xunredir(void), Xstar(void), Xreturn(void), Xsubshell(void);
>  extern void Xtrue(void), Xword(void), Xwrite(void), Xpipefd(void), Xcase(void);
> --- a/sys/src/cmd/rc/pfnc.c
> +++ b/sys/src/cmd/rc/pfnc.c
> @@ -30,6 +30,7 @@
>  	Xword, "Xword",
>  	Xwrite, "Xwrite",
>  	Xmatch, "Xmatch",
> +	Xstashstatus, "Xstashstatus",
>  	Xcase, "Xcase",
>  	Xconc, "Xconc",
>  	Xassign, "Xassign",
> --- a/sys/src/cmd/rc/plan9.c
> +++ b/sys/src/cmd/rc/plan9.c
> @@ -144,7 +144,12 @@
>  		if(n <= 0)
>  			break;
>  		for(i = 0; i<n; i++){
> -			if(ent[i].length<=0 || strncmp(ent[i].name, "fn#", 3)==0)
> +			if(ent[i].length<=0
> +				|| strncmp(ent[i].name, "fn#", 3)==0
> +				|| strcmp(ent[i].name,"*")==0
> +				|| strcmp(ent[i].name,"apid")==0
> +				|| strcmp(ent[i].name,"pid")==0
> +				|| strcmp(ent[i].name,"status")==0)
>  				continue;
>  			if((fd = Open(Env(ent[i].name, 0), 0))>=0){
>  				io *f = openiofd(fd);
> --- a/sys/src/cmd/rc/test/updenvall.file
> +++ b/sys/src/cmd/rc/test/updenvall.file
> @@ -78,3 +78,40 @@
>  	echo d
>  }
>  ' '/env/fn#a is echo d'
> +
> +echo testing special vars
> +*=b
> +echo -n a > /env/'*'
> +ok $"* 'b' '* not set'
> +echo -n $"pid >/tmp/pid.rc.test
> +rc -c ''
> +ok $"pid `{cat /tmp/pid.rc.test} 'pid not set'
> +rm /tmp/pid.rc.test
> +echo -n `{apid=foobar; echo $apid} >/dev/null
> +ok $"apid '' 'apid not set'
> +echo -n foo > /env/status
> +ok $"status '' 'status not set'
> +status=foo
> +~ $status foo
> +ok $"status '' 'naked ~ sets status to '''''
> +status=`{echo foo}
> +ok $"status foo 'backtick sets status'
> +if(~ $status foo)
> +	ok $"status foo '$status'
> +if(! ~ $status a)
> +	ok $"status foo '$status for ! ~'
> +if(~ $status a)
> +	;
> +if not
> +	ok $status foo '$status for if not'
> +if(~ $"status foo)
> +	ok $"status foo '$"status'
> +if(echo a | grep -s a || ls)
> +	ok $"status foo '||'
> +while(! ~ $status foo)
> +	status=bar
> +ok $status foo 'after while not match'
> +while(~ $status foo)
> +	status=bar;
> +ok $status bar 'after while match'
> +status=''
> --- a/sys/src/cmd/rc/test/updenvall.rc
> +++ b/sys/src/cmd/rc/test/updenvall.rc
> @@ -9,7 +9,7 @@
>  	shift
>  	desc=$1
>  	if(~ $"actual $"expected)	echo ok $desc
> -	if not echo 'not ok '^$"desc^', got '$"actual
> +	if not echo 'NOT OK '^$"desc^', got '$"actual
>  }
>  
>  # now let's run the test with our build


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [9front] [PATCH] rc: keep env vars and functions in sync
  2024-07-12 19:58   ` ori
@ 2024-07-12 20:15     ` Dave Woodman
  2024-07-12 20:52       ` Romano
  0 siblings, 1 reply; 8+ messages in thread
From: Dave Woodman @ 2024-07-12 20:15 UTC (permalink / raw)
  To: 9front

To Ori's point regarding ~, the rc documentation would seem to *require* 
that ~ update $status:

~ subject pattern ...
     The subject is matched against each pattern in
     sequence. If it matches any pattern, $status is set to
     zero. Otherwise, $status is set to one.

So, as Ori points out, it is not a bug.

Just my tuppence ha'penny,

Dave.



On 12/07/2024 20:58, ori@eigenstate.org wrote:
> this seems like it would make rc ridiculously slow.
>
> Quoth Romano <me+unobe@fallglow.com>:
>> After dogfooding this on my own system, I am liking how it works, save
>> for how it messed with $*, $pid, and $apid, which are all documented as
>> special variables and (at least for $*) affects the use of fn calls. So,
>> in other words, the previous patch had desiderata.
>>
>> I also noticed the bug re: $status being overwritten when using ~ to
>> check its value. That is now fixed with the patch below.
>>
>> In case anyone wants to try it out, after applying both the previous
>> patch and the patch below, you can run 'mk test' and then run 6.out
>> (which has a handy prompt, 'broken!', so you know what you're dealing).
>> For more widespread testing I have been then doing:
>> 	cp 6.out $home/bin/$objtype/rc.share
>> to see how it compares with rc with some scripts.
>>
>> From: Romano <me+unobe@fallglow.com>
>> Date: Fri, 12 Jul 2024 19:24:48 +0000
>> Subject: [PATCH] rc: do not share certain special vars; fix $status bug for most cases
>>
>>
>> Certain special vars (namely $*, $pid, $apid, and $status)
>> are used internally and should not be set after a child
>> returns.
>>
>> There has been a long-standing bug where checking the value
>> of $status with ~ overwrites the value of $status. $status
>> is now restored in most cases, other than running it
>> as a standalone built-in command.
> this is not a bug; ~ is a command.
>
>> Man page for rc is updated
>> ---
>> diff 7637f8f5fd6490d07ba64bcf404321028e698ed9 675bcd62992e59002c834ef445d515b346f4374d
>> --- a/sys/src/cmd/rc/code.c
>> +++ b/sys/src/cmd/rc/code.c
>> @@ -275,6 +275,8 @@
>>   		codeswitch(t, eflag);
>>   		break;
>>   	case TWIDDLE:
>> +		if(c0 && c0->type=='$' && c0->child[0] && (c0->child[0]->type==WORD && strcmp(c0->child[0]->str, "status")==0))
>> +			emitf(Xstashstatus);
>>   		emitf(Xmark);
>>   		noglobs(c1, 1);
>>   		outcode(c1, eflag);
>> --- a/sys/src/cmd/rc/exec.c
>> +++ b/sys/src/cmd/rc/exec.c
>> @@ -282,6 +282,16 @@
>>   			dotrap();
>>   	}
>>   }
>> +
>> +void
>> +unstashstatus(void)
>> +{
>> +	var *oldstatus = vlook("status-");
>> +	if(oldstatus->val && oldstatus->val->word){
>> +		setstatus(oldstatus->val->word);
>> +	}
>> +}
>> +
>>   /*
>>    * Opcode routines
>>    * Arguments on stack (...)
>> @@ -415,6 +425,7 @@
>>   void
>>   Xifnot(void)
>>   {
>> +	unstashstatus();
>>   	if(ifnot)
>>   		runq->pc++;
>>   	else
>> @@ -597,7 +608,9 @@
>>   void
>>   Xtrue(void)
>>   {
>> -	if(truestatus()) runq->pc++;
>> +	int stat = truestatus();
>> +	unstashstatus();
>> +	if(stat) runq->pc++;
>>   	else runq->pc = runq->code[runq->pc].i;
>>   }
>>   
>> @@ -605,7 +618,9 @@
>>   Xif(void)
>>   {
>>   	ifnot = 1;
>> -	if(truestatus()) runq->pc++;
>> +	int stat = truestatus();
>> +	unstashstatus();
>> +	if(stat) runq->pc++;
>>   	else runq->pc = runq->code[runq->pc].i;
>>   }
>>   
>> @@ -661,6 +676,12 @@
>>   		}
>>   	poplist();
>>   	poplist();
>> +}
>> +
>> +void
>> +Xstashstatus(void)
>> +{
>> +	setvar("status-", Newword(estrdup(getstatus()), (word *)0));
>>   }
>>   
>>   void
>> --- a/sys/src/cmd/rc/exec.h
>> +++ b/sys/src/cmd/rc/exec.h
>> @@ -4,7 +4,7 @@
>>   extern void Xappend(void), Xasync(void), Xbackq(void), Xbang(void), Xclose(void);
>>   extern void Xconc(void), Xcount(void), Xdelfn(void), Xdol(void), Xqw(void), Xdup(void);
>>   extern void Xexit(void), Xfalse(void), Xfn(void), Xfor(void), Xglob(void);
>> -extern void Xjump(void), Xmark(void), Xmatch(void), Xpipe(void), Xread(void), Xhere(void), Xhereq(void);
>> +extern void Xjump(void), Xmark(void), Xmatch(void), Xstashstatus(void), Xpipe(void), Xread(void), Xhere(void), Xhereq(void);
>>   extern void Xrdwr(void), Xsrcline(void);
>>   extern void Xunredir(void), Xstar(void), Xreturn(void), Xsubshell(void);
>>   extern void Xtrue(void), Xword(void), Xwrite(void), Xpipefd(void), Xcase(void);
>> --- a/sys/src/cmd/rc/pfnc.c
>> +++ b/sys/src/cmd/rc/pfnc.c
>> @@ -30,6 +30,7 @@
>>   	Xword, "Xword",
>>   	Xwrite, "Xwrite",
>>   	Xmatch, "Xmatch",
>> +	Xstashstatus, "Xstashstatus",
>>   	Xcase, "Xcase",
>>   	Xconc, "Xconc",
>>   	Xassign, "Xassign",
>> --- a/sys/src/cmd/rc/plan9.c
>> +++ b/sys/src/cmd/rc/plan9.c
>> @@ -144,7 +144,12 @@
>>   		if(n <= 0)
>>   			break;
>>   		for(i = 0; i<n; i++){
>> -			if(ent[i].length<=0 || strncmp(ent[i].name, "fn#", 3)==0)
>> +			if(ent[i].length<=0
>> +				|| strncmp(ent[i].name, "fn#", 3)==0
>> +				|| strcmp(ent[i].name,"*")==0
>> +				|| strcmp(ent[i].name,"apid")==0
>> +				|| strcmp(ent[i].name,"pid")==0
>> +				|| strcmp(ent[i].name,"status")==0)
>>   				continue;
>>   			if((fd = Open(Env(ent[i].name, 0), 0))>=0){
>>   				io *f = openiofd(fd);
>> --- a/sys/src/cmd/rc/test/updenvall.file
>> +++ b/sys/src/cmd/rc/test/updenvall.file
>> @@ -78,3 +78,40 @@
>>   	echo d
>>   }
>>   ' '/env/fn#a is echo d'
>> +
>> +echo testing special vars
>> +*=b
>> +echo -n a > /env/'*'
>> +ok $"* 'b' '* not set'
>> +echo -n $"pid >/tmp/pid.rc.test
>> +rc -c ''
>> +ok $"pid `{cat /tmp/pid.rc.test} 'pid not set'
>> +rm /tmp/pid.rc.test
>> +echo -n `{apid=foobar; echo $apid} >/dev/null
>> +ok $"apid '' 'apid not set'
>> +echo -n foo > /env/status
>> +ok $"status '' 'status not set'
>> +status=foo
>> +~ $status foo
>> +ok $"status '' 'naked ~ sets status to '''''
>> +status=`{echo foo}
>> +ok $"status foo 'backtick sets status'
>> +if(~ $status foo)
>> +	ok $"status foo '$status'
>> +if(! ~ $status a)
>> +	ok $"status foo '$status for ! ~'
>> +if(~ $status a)
>> +	;
>> +if not
>> +	ok $status foo '$status for if not'
>> +if(~ $"status foo)
>> +	ok $"status foo '$"status'
>> +if(echo a | grep -s a || ls)
>> +	ok $"status foo '||'
>> +while(! ~ $status foo)
>> +	status=bar
>> +ok $status foo 'after while not match'
>> +while(~ $status foo)
>> +	status=bar;
>> +ok $status bar 'after while match'
>> +status=''
>> --- a/sys/src/cmd/rc/test/updenvall.rc
>> +++ b/sys/src/cmd/rc/test/updenvall.rc
>> @@ -9,7 +9,7 @@
>>   	shift
>>   	desc=$1
>>   	if(~ $"actual $"expected)	echo ok $desc
>> -	if not echo 'not ok '^$"desc^', got '$"actual
>> +	if not echo 'NOT OK '^$"desc^', got '$"actual
>>   }
>>   
>>   # now let's run the test with our build

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [9front] [PATCH] rc: keep env vars and functions in sync
  2024-07-12 20:15     ` Dave Woodman
@ 2024-07-12 20:52       ` Romano
  2024-07-12 21:25         ` Dave Woodman
  2024-07-12 21:43         ` Romano
  0 siblings, 2 replies; 8+ messages in thread
From: Romano @ 2024-07-12 20:52 UTC (permalink / raw)
  To: 9front

Thanks, dave and ori. See my responses below.

On Fri Jul 12 13:19:53 -0700 2024, dave@naffnet.org.uk wrote:
> To Ori's point regarding ~, the rc documentation would seem to *require* 
> that ~ update $status:
> 
> ~ subject pattern ...
>      The subject is matched against each pattern in
>      sequence. If it matches any pattern, $status is set to
>      zero. Otherwise, $status is set to one.
> 
> So, as Ori points out, it is not a bug.

The BUGS section of the man page listed it as one, which is what
I was going off of. And I realize I didn't commit the man page
patch. That's now included below so you can see where in the
man page I'm referring to, and how I updated it which I hope
clarifies the scope.

> On 12/07/2024 20:58, ori@eigenstate.org wrote:
> > this seems like it would make rc ridiculously slow.

I haven't benchmarked it, but I haven't noticed a significant
slowdown in the scripts I've run thus far. Granted, they are
not enormously complex. The processing of very large /env/
files would add some time, surely; the max size of env
variables (under /env) is currently set to 1M.

Do you have an idea of how you would want a benchmark for
this to look like?

To be clear, I don't expect this to be signed off on any time
soon but did want to show it to the list for feedback.

I appreciate the feedback thus far.

From: Romano <me+unobe@fallglow.com>
Date: Fri, 12 Jul 2024 20:40:40 +0000
Subject: [PATCH] rc: updated man page

---
diff 675bcd62992e59002c834ef445d515b346f4374d c32dabd4853888f62f09a6d3f8e0deed4077b6a7
--- a/sys/man/1/rc
+++ b/sys/man/1/rc
@@ -825,7 +825,13 @@
 When
 .I rc
 starts executing it reads variable and function definitions from its
-environment.
+environment, except for the special variables
+.BR $* ,
+.BR $pid ,
+and
+.BR $apid ,
+and
+.BR $status .
 .SS Special Variables
 The following variables are set or used by
 .IR rc .
@@ -1019,7 +1025,16 @@
 .B ~
 to check the value of
 .B $status
-changes
-.BR $status .
+as a changes
+.BR $status 
+when used alone as a built-in command. E.g.,
+.EX
+cpu% status=b; ~ $status a; echo $status
+no match
+cpu% status=b; if(~ $status a; echo $status) echo $status
+no match
+b
+cpu%
+.EE
 .PP
 Free carets don't get inserted next to keywords.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [9front] [PATCH] rc: keep env vars and functions in sync
  2024-07-12 20:52       ` Romano
@ 2024-07-12 21:25         ` Dave Woodman
  2024-07-12 21:36           ` Romano
  2024-07-12 21:43         ` Romano
  1 sibling, 1 reply; 8+ messages in thread
From: Dave Woodman @ 2024-07-12 21:25 UTC (permalink / raw)
  To: 9front

I always understood (from so many years of UNIX and UNIX-like systems) 
that BUGS did not necessarily mean defect.

 From a "modern" source:

*"BUGS A list of limitations, known defects or inconveniences, and other 
questionable activities"*

*I, personally, would characterise the ~ behaviour as a limitation**.
*

*Happy to bow to the majority, however.*

*Dave.
*

On 12/07/2024 21:52, Romano wrote:
> Thanks, dave and ori. See my responses below.
>
> On Fri Jul 12 13:19:53 -0700 2024, dave@naffnet.org.uk wrote:
>> To Ori's point regarding ~, the rc documentation would seem to *require*
>> that ~ update $status:
>>
>> ~ subject pattern ...
>>       The subject is matched against each pattern in
>>       sequence. If it matches any pattern, $status is set to
>>       zero. Otherwise, $status is set to one.
>>
>> So, as Ori points out, it is not a bug.
> The BUGS section of the man page listed it as one, which is what
> I was going off of. And I realize I didn't commit the man page
> patch. That's now included below so you can see where in the
> man page I'm referring to, and how I updated it which I hope
> clarifies the scope.
>
>> On 12/07/2024 20:58, ori@eigenstate.org wrote:
>>> this seems like it would make rc ridiculously slow.
> I haven't benchmarked it, but I haven't noticed a significant
> slowdown in the scripts I've run thus far. Granted, they are
> not enormously complex. The processing of very large /env/
> files would add some time, surely; the max size of env
> variables (under /env) is currently set to 1M.
>
> Do you have an idea of how you would want a benchmark for
> this to look like?
>
> To be clear, I don't expect this to be signed off on any time
> soon but did want to show it to the list for feedback.
>
> I appreciate the feedback thus far.
>
> From: Romano <me+unobe@fallglow.com>
> Date: Fri, 12 Jul 2024 20:40:40 +0000
> Subject: [PATCH] rc: updated man page
>
> ---
> diff 675bcd62992e59002c834ef445d515b346f4374d c32dabd4853888f62f09a6d3f8e0deed4077b6a7
> --- a/sys/man/1/rc
> +++ b/sys/man/1/rc
> @@ -825,7 +825,13 @@
>   When
>   .I rc
>   starts executing it reads variable and function definitions from its
> -environment.
> +environment, except for the special variables
> +.BR $* ,
> +.BR $pid ,
> +and
> +.BR $apid ,
> +and
> +.BR $status .
>   .SS Special Variables
>   The following variables are set or used by
>   .IR rc .
> @@ -1019,7 +1025,16 @@
>   .B ~
>   to check the value of
>   .B $status
> -changes
> -.BR $status .
> +as a changes
> +.BR $status
> +when used alone as a built-in command. E.g.,
> +.EX
> +cpu% status=b; ~ $status a; echo $status
> +no match
> +cpu% status=b; if(~ $status a; echo $status) echo $status
> +no match
> +b
> +cpu%
> +.EE
>   .PP
>   Free carets don't get inserted next to keywords.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [9front] [PATCH] rc: keep env vars and functions in sync
  2024-07-12 21:25         ` Dave Woodman
@ 2024-07-12 21:36           ` Romano
  0 siblings, 0 replies; 8+ messages in thread
From: Romano @ 2024-07-12 21:36 UTC (permalink / raw)
  To: 9front

On Fri Jul 12 14:29:26 -0700 2024, dave@naffnet.org.uk wrote:
> I always understood (from so many years of UNIX and UNIX-like systems) 
> that BUGS did not necessarily mean defect.
> …

Ah, with that understanding, I see where you're coming from then.
I don't know how the the original author (Duffy) considered this
behavior. But like you, I'll bow to the majority.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [9front] [PATCH] rc: keep env vars and functions in sync
  2024-07-12 20:52       ` Romano
  2024-07-12 21:25         ` Dave Woodman
@ 2024-07-12 21:43         ` Romano
  1 sibling, 0 replies; 8+ messages in thread
From: Romano @ 2024-07-12 21:43 UTC (permalink / raw)
  To: 9front

Also, on the topic of if this syncing the env vars from /env
perhaps a refactor broke it? I haven't used vanilla plan9 in
more than a decade, but Tom Duffy's paper mentions /env
should work like this (quoted from /sys/doc/rc.ms:1036,1038 ):

	A consequence of this organization is that
	commands can change environment entries and
	see the changes reflected in rc.

Without the $status changes, this set of patches would make
what is stated there true.


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-07-12 21:44 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-07-11  5:32 [9front] [PATCH] rc: keep env vars and functions in sync Romano
2024-07-12 19:36 ` Romano
2024-07-12 19:58   ` ori
2024-07-12 20:15     ` Dave Woodman
2024-07-12 20:52       ` Romano
2024-07-12 21:25         ` Dave Woodman
2024-07-12 21:36           ` Romano
2024-07-12 21:43         ` Romano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).