Re: wrapper functions in modules

Re: wrapper functions in modules

[Sven Wischnowsky]

Bart Schaefer wrote:

> ...
> 
> Is the new first parameter of doshfunc() needed any longer?  runshfunc()
> doesn't do anything with it except pass it to wrap->handler(), and at
> least in the example module the handler doesn't do anything with it but
> pass it back to runshfunc() again.  Why are we slinging this around?

[For those who don't want to look at the code: the argument is the
name of the function to be executed.]

I added the argument since modules may be interested in it and there
may be no other way to get at this information (depending on FUNCTIONARGZERO).

The example uses it in the call to strncmp().

Bye
 Sven


--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: wrapper functions in modules

[Bart Schaefer]

On Dec 15,  1:03pm, Sven Wischnowsky wrote:
} Subject: Re: wrapper functions in modules
}
} Bart Schaefer wrote:
} > Is the new first parameter of doshfunc() needed any longer?
} 
} [For those who don't want to look at the code: the argument is the
} name of the function to be executed.]
} 
} I added the argument since modules may be interested in it

Hm.  I'm not sure that modules *ought* to be interested in it, but ...

One thing a wrapper function might legitimately be interested in is the
context in which it was called.  By that I mean, the wrapper might want
to do something different if the function is being run by the completion
widget code (the call to doshfunc() in zle_main.c), the compctl -K code
or -Y code (zle_tricky.c), or the signal traps (signals.c).  The signal
handlers can sort of be determined by examination of the name, but that's
not what I'd call the best way to do it (e.g. it's possible to invoke the
trap functions manually without a signal having been received), and that
doesn't work for the other cases.

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

Re: wrapper functions in modules

[Bart Schaefer]

On Dec 16,  9:05am, Sven Wischnowsky wrote:
} Subject: Re: wrapper functions in modules
}
} Bart Schaefer wrote:
} 
} > On Dec 15,  1:03pm, Sven Wischnowsky wrote:
} > }
} > } Bart Schaefer wrote:
} > } > Is the new first parameter of doshfunc() needed any longer?
} > } 
} > } [For those who don't want to look at the code: the argument is the
} > } name of the function to be executed.]
} > } 
} > } I added the argument since modules may be interested in it
} > 
} > Hm.  I'm not sure that modules *ought* to be interested in it, but...
} 
} I wouldn't resist to remove this or to use a global variable for it.

A parameter is almost always better than a global, for things that get
stacked and restored like this.

} > One thing a wrapper function might legitimately be interested in is the
} > context in which it was called.
} 
} Yes, that would be good to have. Using a global integer variable or an 
} argument and a couple of constants?

Here a global makes more sense, since it only changes once as you enter
(say) the completion code and remains constant all the way down the
shell function call chain.

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

Re: wrapper functions in modules

[Sven Wischnowsky]

Bart Schaefer wrote:

> 
> On Dec 15,  1:03pm, Sven Wischnowsky wrote:
> } Subject: Re: wrapper functions in modules
> }
> } Bart Schaefer wrote:
> } > Is the new first parameter of doshfunc() needed any longer?
> } 
> } [For those who don't want to look at the code: the argument is the
> } name of the function to be executed.]
> } 
> } I added the argument since modules may be interested in it
> 
> Hm.  I'm not sure that modules *ought* to be interested in it, but...

It's just that wrappers can't get at this information. But I wouldn't
resist to remove this or to use a global variable for it.
Apropos variables: we could use two static variables for the other
arguments a wrapper function gets and which are only passed to
runshfunc()... hm, I might produce a patch for this some time.

> 
> One thing a wrapper function might legitimately be interested in is the
> context in which it was called.  By that I mean, the wrapper might want
> to do something different if the function is being run by the completion
> widget code (the call to doshfunc() in zle_main.c), the compctl -K code
> or -Y code (zle_tricky.c), or the signal traps (signals.c).  The signal
> handlers can sort of be determined by examination of the name, but that's
> not what I'd call the best way to do it (e.g. it's possible to invoke the
> trap functions manually without a signal having been received), and that
> doesn't work for the other cases.

Yes, that would be good to have. Using a global integer variable or an 
argument and a couple of constants?

Bye
 Sven


--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: wrapper functions in modules

[Bart Schaefer]

On Dec 14, 11:11am, Sven Wischnowsky wrote:
} Subject: Re: wrapper functions in modules
}
} > } [...] my first idea was to let modules register only one function
} > } which would have to call back the execution code [...]
} > 
} > This would indeed be an improvement.
} 
} The patch below implements this suggestion with a slight modification
} [...]

Is the new first parameter of doshfunc() needed any longer?  runshfunc()
doesn't do anything with it except pass it to wrap->handler(), and at
least in the example module the handler doesn't do anything with it but
pass it back to runshfunc() again.  Why are we slinging this around?

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

Re: wrapper functions in modules

[Sven Wischnowsky]

Bart Schaefer wrote:

> 
> On Dec 11,  3:16pm, Sven Wischnowsky wrote:
> } Subject: Re: wrapper functions in modules
> }
> } [...] my first idea was to let modules register only one function
> } which would have to call back the execution code [...]
> } This would solve the call stack problems you mentioned. Also, writing
> } wrappers would be easier in modules since you can use local variables, 
> } static local variables for number-of-calls and so on...
> 
> This would indeed be an improvement.  An interesting side-effect is that
> it would permit a module to decide not to run the shell function at all;
> it could instead branch off into any code it liked.
> 

The patch below implements this suggestion with a slight modification: 
the wrapper function now returns an integer value. Returning zero
means that the shell function was executed (by calling
runshfunc()). Returning non-zero means that the execution code has to
execute the function itself. This way a module can easily define
wrapper function that are to be used only under certain conditions or
that don't need cleanup code.

> That in turn probably makes modules even more dangerous than before, from
> a run-time perspective.  An innoccuous module placed in root's module path
> could suddenly have someone else driving the shell.  It might even be a
> good idea to have zsh refuse to dynamically load modules when EUID==0, or
> at least refuse to auto-load them.

The fact that module can choose not to execute shell functions was one 
of the reasons why I first implemented to separate functions (the
other was that unloading-modules-thing, I didn't realise all the
implications that has).

> } About the problems with unloading: I would vote for completely
> } disallowing to unload a module if a wrapper is active for it.
> } This is relatively easy to keep track of and seems to be the savest
> 
> (Safest.)  This still isn't quite good enough -- you can't unload
> modules upon which other modules depend, so if X has a wrapper and
> depends on Y, you can't unload Y even though Y has no wrapper.
> 
> This probably means you end up refusing to unload any modules at all
> whenever there are any wrappers at all, which may be problematic.

But this interferes only with wrappers that are currently being
executed.
The patch doesn't change the unloading stuff, though we will have to
find a solution for it.

> 
> } the question is: how often does one want to unload modules in shell
> } functions?
> 
> I could envision someone wanting to write a pair of functions, one that
> installs several related modules and another that uninstalls them again.

Hm, right. Which brings us back to the separation of user-interface
and internals you mentioned.
We could keep the current function interface and offer a more basic
one for separately defining and deleting the user-visible and internal 
things so that only modules with wrapper functions that call
runshfunc() would be affected.

> 
> } Finally about the order in which installed wrappers are to be called:
> } looking at load_module() it should be enough to build the wrappers
> } list by appending new definitions to the end.
> 
> Yes, as I said, if the only constraint is module dependencies this will
> work fine.

The patch does this.

Another things this patch doesn't do is: add documentation. I agree
that we should have documentation for all this, but the file
Util/zsh-development-guide you mentioned doesn't contain anything
about modules...

Maybe I find some time to add it in the next days (although others are 
certainly more qualified to describe the basic module loading
mechanisms).


Bye
 Sven

*** os/zsh.h	Mon Dec 14 10:53:29 1998
--- Src/zsh.h	Mon Dec 14 10:24:28 1998
***************
*** 774,795 ****
  
  /* node in list of function call wrappers */
  
! typedef void (*WrapBefore) _((FuncWrap, char *));
! typedef void (*WrapAfter) _((FuncWrap, char *, int));
  
  struct funcwrap {
      FuncWrap next;
      int flags;
!     WrapBefore before;
!     WrapAfter after;
      Module module;
      int count;
  };
  
  #define WRAPF_ADDED 1
  
! #define WRAPDEF(before, after) \
!     { NULL, 0, before, after, NULL, 0 }
  
  /* node in builtin command hash table (builtintab) */
  
--- 774,793 ----
  
  /* node in list of function call wrappers */
  
! typedef int (*WrapFunc) _((List, FuncWrap, char *));
  
  struct funcwrap {
      FuncWrap next;
      int flags;
!     WrapFunc handler;
      Module module;
      int count;
  };
  
  #define WRAPF_ADDED 1
  
! #define WRAPDEF(func) \
!     { NULL, 0, func, NULL, 0 }
  
  /* node in builtin command hash table (builtintab) */
  
*** os/exec.c	Mon Dec 14 10:53:28 1998
--- Src/exec.c	Mon Dec 14 10:36:29 1998
***************
*** 2657,2667 ****
  {
      char **tab, **x, *oargv0 = NULL;
      int xexittr, newexittr, oldzoptind, oldlastval;
-     char *ou;
      void *xexitfn, *newexitfn;
      char saveopts[OPT_SIZE];
      int obreaks = breaks;
-     FuncWrap wrap, nwrap;
  
      HEAPALLOC {
  	pushheap();
--- 2657,2665 ----
***************
*** 2706,2740 ****
  		argzero = ztrdup(argzero);
  	    }
  	}
! 	for (wrap = wrappers; wrap; wrap = wrap->next) {
! 	    if (wrap->before)
! 		wrap->before(wrap, name);
! 	    if (wrap->after) {
! 		wrap->module->flags |= MOD_WRAPPER;
! 		wrap->count++;
! 	    }
! 	}
! 	startparamscope();
! 	ou = underscore;
! 	underscore = ztrdup(underscore);
! 	execlist(dupstruct(list), 1, 0);
! 	zsfree(underscore);
! 	underscore = ou;
! 	endparamscope();
! 	for (wrap = wrappers; wrap; wrap = nwrap) {
! 	    nwrap = wrap->next;
! 	    if (wrap->after) {
! 		wrap->after(wrap, name, lastval);
! 		wrap->count--;
! 		if (!wrap->count) {
! 		    wrap->module->flags &= ~MOD_WRAPPER;
! 		    if (wrap->module->flags & MOD_UNLOAD) {
! 			wrap->module->flags &= ~MOD_UNLOAD;
! 			unload_module(wrap->module, NULL);
! 		    }
! 		}
! 	    }
! 	}
  	if (retflag) {
  	    retflag = 0;
  	    breaks = obreaks;
--- 2704,2710 ----
  		argzero = ztrdup(argzero);
  	    }
  	}
! 	runshfunc(list, wrappers, name);
  	if (retflag) {
  	    retflag = 0;
  	    breaks = obreaks;
***************
*** 2787,2792 ****
--- 2757,2798 ----
  	    lastval = oldlastval;
  	popheap();
      } LASTALLOC;
+ }
+ 
+ /* This finally executes a shell function and any function wrappers     *
+  * defined by modules. This works by calling the wrapper function which *
+  * in turn has to call back this function with the arguments it gets.   */
+ 
+ /**/
+ void
+ runshfunc(List list, FuncWrap wrap, char *name)
+ {
+     int cont;
+     char *ou;
+ 
+     while (wrap) {
+ 	wrap->module->flags |= MOD_WRAPPER;
+ 	wrap->count++;
+ 	cont = wrap->handler(list, wrap->next, name);
+ 	wrap->count--;
+ 	if (!wrap->count) {
+ 	    wrap->module->flags &= ~MOD_WRAPPER;
+ 	    if (wrap->module->flags & MOD_UNLOAD) {
+ 		wrap->module->flags &= ~MOD_UNLOAD;
+ 		unload_module(wrap->module, NULL);
+ 	    }
+ 	}
+ 	if (!cont)
+ 	    return;
+ 	wrap = wrap->next;
+     }
+     startparamscope();
+     ou = underscore;
+     underscore = ztrdup(underscore);
+     execlist(dupstruct(list), 1, 0);
+     zsfree(underscore);
+     underscore = ou;
+     endparamscope();
  }
  
  /* Search fpath for an undefined function.  Finds the file, and returns the *
*** os/module.c	Mon Dec 14 10:53:29 1998
--- Src/module.c	Mon Dec 14 10:26:21 1998
***************
*** 328,337 ****
  int
  addwrapper(Module m, FuncWrap w)
  {
      if (w->flags & WRAPF_ADDED)
  	return 1;
!     w->next = wrappers;
!     wrappers = w;
      w->flags |= WRAPF_ADDED;
      w->module = m;
      w->count = 0;
--- 328,343 ----
  int
  addwrapper(Module m, FuncWrap w)
  {
+     FuncWrap p, q;
+ 
      if (w->flags & WRAPF_ADDED)
  	return 1;
!     for (p = wrappers, q = NULL; p; q = p, p = p->next);
!     if (q)
! 	q->next = w;
!     else
! 	wrappers = w;
!     w->next = NULL;
      w->flags |= WRAPF_ADDED;
      w->module = m;
      w->count = 0;
*** os/Modules/example.c	Wed Dec  9 15:45:33 1998
--- Src/Modules/example.c	Mon Dec 14 10:45:05 1998
***************
*** 79,112 ****
      return !strcmp("example", dyncat(s1, s2));
  }
  
- struct ogd {
-     struct ogd *next;
-     int val;
- };
- 
- static struct ogd *ogds;
- 
  /**/
! static void
! wrap_before(FuncWrap w, char *name)
  {
!     if (!strncmp(name, "example", 7)) {
! 	struct ogd *n = (struct ogd *) halloc(sizeof(*n));
  
- 	n->next = ogds;
- 	ogds = n;
- 	n->val = opts[GLOBDOTS];
  	opts[GLOBDOTS] = 1;
!     }
! }
  
! /**/
! static void
! wrap_after(FuncWrap w, char *name, int ret)
! {
!     if (!strncmp(name, "example", 7)) {
! 	opts[GLOBDOTS] = ogds->val;
! 	ogds = ogds->next;
      }
  }
  
--- 79,98 ----
      return !strcmp("example", dyncat(s1, s2));
  }
  
  /**/
! static int
! ex_wrapper(List list, FuncWrap w, char *name)
  {
!     if (strncmp(name, "example", 7))
! 	return 1;
!     else {
! 	int ogd = opts[GLOBDOTS];
  
  	opts[GLOBDOTS] = 1;
! 	runshfunc(list, w, name);
! 	opts[GLOBDOTS] = ogd;
  
! 	return 0;
      }
  }
  
***************
*** 124,140 ****
  };
  
  static struct funcwrap wrapper[] = {
!     WRAPDEF(wrap_before, wrap_after),
  };
  
  /**/
  int
  boot_example(Module m)
  {
-     ogds = NULL;
      return !(addbuiltins(m->nam, bintab, sizeof(bintab)/sizeof(*bintab)) |
  	     addconddefs(m->nam, cotab, sizeof(cotab)/sizeof(*cotab)) |
! 	     !addwrapper(wrapper));
  }
  
  #ifdef MODULE
--- 110,125 ----
  };
  
  static struct funcwrap wrapper[] = {
!     WRAPDEF(ex_wrapper),
  };
  
  /**/
  int
  boot_example(Module m)
  {
      return !(addbuiltins(m->nam, bintab, sizeof(bintab)/sizeof(*bintab)) |
  	     addconddefs(m->nam, cotab, sizeof(cotab)/sizeof(*cotab)) |
! 	     !addwrapper(m, wrapper));
  }
  
  #ifdef MODULE
***************
*** 145,151 ****
  {
      deletebuiltins(m->nam, bintab, sizeof(bintab)/sizeof(*bintab));
      deleteconddefs(m->nam, cotab, sizeof(cotab)/sizeof(*cotab));
!     deletewrapper(wrapper);
      return 0;
  }
  #endif
--- 130,136 ----
  {
      deletebuiltins(m->nam, bintab, sizeof(bintab)/sizeof(*bintab));
      deleteconddefs(m->nam, cotab, sizeof(cotab)/sizeof(*cotab));
!     deletewrapper(m, wrapper);
      return 0;
  }
  #endif

--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: wrapper functions in modules

[Bart Schaefer]

On Dec 11,  3:16pm, Sven Wischnowsky wrote:
} Subject: Re: wrapper functions in modules
}
} [...] my first idea was to let modules register only one function
} which would have to call back the execution code [...]
} This would solve the call stack problems you mentioned. Also, writing
} wrappers would be easier in modules since you can use local variables, 
} static local variables for number-of-calls and so on...

This would indeed be an improvement.  An interesting side-effect is that
it would permit a module to decide not to run the shell function at all;
it could instead branch off into any code it liked.

That in turn probably makes modules even more dangerous than before, from
a run-time perspective.  An innoccuous module placed in root's module path
could suddenly have someone else driving the shell.  It might even be a
good idea to have zsh refuse to dynamically load modules when EUID==0, or
at least refuse to auto-load them.

} About the problems with unloading: I would vote for completely
} disallowing to unload a module if a wrapper is active for it.
} This is relatively easy to keep track of and seems to be the savest

(Safest.)  This still isn't quite good enough -- you can't unload
modules upon which other modules depend, so if X has a wrapper and
depends on Y, you can't unload Y even though Y has no wrapper.

This probably means you end up refusing to unload any modules at all
whenever there are any wrappers at all, which may be problematic.

} the question is: how often does one want to unload modules in shell
} functions?

I could envision someone wanting to write a pair of functions, one that
installs several related modules and another that uninstalls them again.

} Finally about the order in which installed wrappers are to be called:
} looking at load_module() it should be enough to build the wrappers
} list by appending new definitions to the end.

Yes, as I said, if the only constraint is module dependencies this will
work fine.

Side note to Phil Pennock:  If we get this worked out, then an emulator
module for ksh would be the perfect way to fudge WORDCHARS and various
keybindings for later restoration ...

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

Re: wrapper functions in modules

[Sven Wischnowsky]

Bart Schaefer wrote:

> ... [ several serious problems with the wrapper functions ]

I stand corrected.

And damn, my first idea was to let modules register only one function
which would have to call back the execution code, i.e.:

doshfunc(...)
{
  ...
  runshfunc(list, wrappers);
  ...
}

runshfunc(List list, FuncWrap wrap)
{
  if (wrap)
    wrap->func(list, wrap->next);
  else {
    ...
    execlist(...);
    ...
  }
}

example_wrapper(List list, FuncWrap wrap)  // from module
{
  ... do something

  runshfunc(list, wrap);

  ... restore something
}

This would solve the call stack problems you mentioned. Also, writing
wrappers would be easier in modules since you can use local variables, 
static local variables for number-of-calls and so on...

About the problems with unloading: I would vote for completely
disallowing to unload a module if a wrapper is active for it.
This is relatively easy to keep track of and seems to be the savest,
the question is: how often does one want to unload modules in shell
functions?

Finally about the order in which installed wrappers are to be called:
looking at load_module() it should be enough to build the wrappers
list by appending new definitions to the end.

Ok. No patch for now, just the question: does this sound ok?

And, of course, if someone has ideas for a completely different
solution, I would like to hear about it.

Bye
 Sven


--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: wrapper functions in modules

[Bart Schaefer]

On Dec 9,  3:44pm, Sven Wischnowsky wrote:
} Subject: PATCH: wrapper functions in modules
}
} The patch below allows modules to register functions that are to be
} executed before and after a function is called.

I understand why you did this, but it's a potential pit of vipers,
especially given the linear list traversal used to call the wrappers.

My first concern is with ordering dependencies among modules.  The
module dependency mechanism can be used to assure that module X's
"before" wrapper is in the linked list ahead of module Y's, but what
about the "after" wrappers?  Surely they should be called in opposite
order from the "before" wrappers, so that Y can unwind its state before
X yanks some vital data out from under it?

Also, the additional patches to delay module unloading introduce other
problems.  Suppose I have a module that redefines the "echo" builtin.
Consider:

	function call-the-wrong-echo {
	    zmodload my_echo
	    echo "This should be my own echo."
	    zmodload -u my_echo
	    echo "This ought to be builtin echo."
	}

You have to split unloading into two parts, one that takes away the
"user interface" and another that finally disengages the C functions.

That probably further introduces the need to split the module's own
cleanup function into those same two parts, because "zmodload -u" has
to fail at the moment that it's executed if it's going to fail at all,
not some long while later when the shell-function call-stack unwinds.

And then we're back to the dependency problems again -- when you're done
calling all the wrapper functions, you have to unload the modules IN THE
ORDER THAT THE zmodload -u COMMANDS WERE EXECUTED, not in whatever order
the modules happen to have been added (or not) to the wrappers list.  It's
not good enough to delay the unloading only of modules that have wrappers,
you also have to delay unloading all the way along the module dependency
chains.

Even with all that fixed, there's the problem of shell functions calling
other shell functions.  Here's the most glaring example:

	function call-too-many-after-wrappers {
	    if (($1 == 10))
	    then
	    	zmodload example
	    else
		call-too-many-after-wrappers $[$1+1]
	    fi
	}

Even in nonrecursive cases, every module's wrapper has to maintain its
own idea of the state of the call stack to "do the right thing" with
respect to nested shell function scopes.

Worse, this interacts with the staged-unload problem, because once you
have called "zmodload -u", any further calls to the "before" wrappers
ought to become no-ops; but you have to be sure the "after" wrappers
still get called at exactly the scopes where the "before" wrappers were
active.

You can fix or reduce some of these problems by:
1.  Make the list of "after" wrappers to be called a part of the
    doshfunc() stack frame, rather than always using the global list.
2.  Build that list by pushing wrappers on the front as you loop over
    the "before" wrappers, to get the reversed call sequence.
3.  Pass a "call stack depth" parameter or similar identifier to the
    before/after wrappers, to simplify the tests they must do.  (Even
    better, pass a pointer to some kind of scope structure that has
    the stack depth as a member.)

Nevertheless, this is so tangled that I'm sure I haven't yet listed all
the possible bad interactions.  We should think hard about whether there
is some other way to deal with the problems the wrappers are intended to
solve.

} This is completely hidden from the user so there is no documentation

There ought to be *developer* documentation for it, e.g., in the file
Util/zsh-development-guide or where other module developer documentation
lives (which is rather scatterbrained at the moment).  The example
module is totally insufficient to give an understanding of this kind of
complexity.

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