PATCH: zselect builtin.

PATCH: zselect builtin.

[Peter Stephenson]

Here's a builtin to act as a front end to a `select' system call.  The
name clash with the `select' builtin is unfortunate but seems inevitable
--- trying to disguise the system operation underneath looks pointless,
although I could call it zpoll if I implemented the poll-based version.

Any comments before I commit this?

Index: Src/Modules/zselect.c
===================================================================
RCS file: Src/Modules/zselect.c
diff -N Src/Modules/zselect.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ Src/Modules/zselect.c	7 May 2002 11:22:22 -0000
@@ -0,0 +1,268 @@
+/*
+ * zselect.c - builtin support for select system call
+ *
+ * This file is part of zsh, the Z shell.
+ *
+ * Copyright (c) 1998-2001 Peter Stephenson
+ * All rights reserved.
+ *
+ * Permission is hereby granted, without written agreement and without
+ * license or royalty fees, to use, copy, modify, and distribute this
+ * software and to distribute modified versions of this software for any
+ * purpose, provided that the above copyright notice and the following
+ * two paragraphs appear in all copies of this software.
+ *
+ * In no event shall Peter Stephenson or the Zsh Development
+ * Group be liable to any party for direct, indirect, special, incidental,
+ * or consequential damages arising out of the use of this software and
+ * its documentation, even if Peter Stephenson, and the Zsh
+ * Development Group have been advised of the possibility of such damage.
+ *
+ * Peter Stephenson and the Zsh Development Group specifically
+ * disclaim any warranties, including, but not limited to, the implied
+ * warranties of merchantability and fitness for a particular purpose.  The
+ * software provided hereunder is on an "as is" basis, and Peter Stephenson
+ * and the Zsh Development Group have no obligation to provide maintenance,
+ * support, updates, enhancements, or modifications.
+ *
+ */
+
+#include "zselect.mdh"
+#include "zselect.pro"
+
+/* Helper functions */
+
+/*
+ * Handle an fd by adding it to the current fd_set.
+ * Return 1 for error (after printing a message), 0 for OK.
+ */
+static int
+handle_digits(char *nam, char *argptr, fd_set *fdset, int *fdcount,
+	      int *fdmax)
+{
+    int fd;
+    char *endptr;
+
+    if (!isdigit(STOUC(*argptr))) {
+	zwarnnam(nam, "expecting file descriptor: %s", argptr, 0);
+	return 1;
+    }
+    fd = (int)zstrtol(argptr, &endptr, 10);
+    if (*endptr) {
+	zwarnnam(nam, "garbage after file descriptor: %s", endptr, 0);
+	return 1;
+    }
+
+    FD_SET(fd, fdset);
+    (*fdcount)++;
+    if (fd+1 > *fdmax)
+	*fdmax = fd+1;
+    return 0;
+}
+
+/* The builtin itself */
+
+/**/
+static int
+bin_zselect(char *nam, char **args, char *ops, int func)
+{
+#ifdef HAVE_SELECT
+    int i, fd, fdsetind = 0, fdcount = 0, fdmax = 0;
+    fd_set fdset[3];
+    const char fdchar[3] = "rwe";
+    struct timeval tv, *tvptr = NULL;
+    char *outarray = "reply", **outdata, **outptr;
+    LinkList fdlist;
+
+    for (i = 0; i < 3; i++)
+	FD_ZERO(fdset+i);
+
+    for (; *args; args++) {
+	char *argptr = *args, *endptr;
+	zlong tempnum;
+	if (*argptr == '-') {
+	    for (argptr++; *argptr; argptr++) {
+		switch (*argptr) {
+		    /*
+		     * Array name for reply, if not $reply.
+		     * This gets set to e.g. `-r 0 -w 1' if 0 is ready
+		     * for reading and 1 is ready for writing.
+		     */
+		case 'a':
+		    if (argptr[1])
+			argptr++;
+		    else if (args[1]) {
+			argptr = *++args;
+		    } else {
+			zwarnnam(nam, "argument expected after -%c", NULL,
+				 *argptr);
+			return 1;
+		    }
+		    if (idigit(*argptr) || !isident(argptr)) {
+			zwarnnam(nam, "invalid array name: %s", argptr, 0);
+			return 1;
+		    }
+		    outarray = argptr;
+		    /* set argptr to next to last char because of increment */
+		    while (argptr[1])
+			argptr++;
+		    break;
+
+		    /* Following numbers indicate fd's for reading */
+		case 'r':
+		    fdsetind = 0;
+		    break;
+
+		    /* Following numbers indicate fd's for writing */
+		case 'w':
+		    fdsetind = 1;
+		    break;
+
+		    /* Following numbers indicate fd's for errors */
+		case 'e':
+		    fdsetind = 2;
+		    break;
+
+		    /*
+		     * Get a timeout value in hundredths of a second
+		     * (same units as KEYTIMEOUT).  0 means just poll.
+		     * If not given, blocks indefinitely.
+		     */
+		case 't':
+		    if (argptr[1])
+			argptr++;
+		    else if (args[1]) {
+			argptr = *++args;
+		    } else {
+			zwarnnam(nam, "argument expected after -%c", NULL, 
+				 *argptr);
+			return 1;
+		    }
+		    if (!idigit(*argptr)) {
+			zwarnnam(nam, "number expected after -t", NULL, 0);
+			return 1;
+		    }
+		    tempnum = zstrtol(argptr, &endptr, 10);
+		    if (*endptr) {
+			zwarnnam(nam, "garbage after -t argument: %s",
+				 endptr, 0);
+			return 1;
+		    }
+		    /* timevalue now active */
+		    tvptr = &tv;
+		    tv.tv_sec = (long)(tempnum / 100);
+		    tv.tv_usec = (long)(tempnum % 100) * 10000L;
+
+		    /* remember argptr is incremented at end of loop */
+		    argptr = endptr - 1;
+		    break;
+
+		    /* Digits following option without arguments are fd's. */
+		default:
+		    if (handle_digits(nam, argptr, fdset+fdsetind,
+				      &fdcount, &fdmax))
+			return 1;
+		}
+	    }
+	} else if (handle_digits(nam, argptr, fdset+fdsetind, &fdcount,
+				 &fdmax))
+	    return 1;
+    }
+
+    if (!fdcount) {
+	/* No fd's selected. Not an error, but not succesful. */
+	return 1;
+    }
+
+    errno = 0;
+    do {
+	i = select(fdmax, (SELECT_ARG_2_T)fdset, (SELECT_ARG_2_T)(fdset+1),
+		   (SELECT_ARG_2_T)(fdset+2), tvptr);
+    } while (i < 0 && errno == EINTR && !errflag);
+
+    if (i <= 0) {
+	if (i < 0)
+	    zwarnnam(nam, "error on select: %e", NULL, errno);
+	/* else no fd's set.  Presumably a timeout. */
+	return 1;
+    }
+
+    /*
+     * Make a linked list of all file descriptors which are ready.
+     * These go into an array preceded by -r, -w or -e for read, write,
+     * error as appropriate.  Typically there will only be one set
+     * so this looks rather like overkill.
+     */
+    fdlist = znewlinklist();
+    for (i = 0; i < 3; i++) {
+	int doneit = 0;
+	for (fd = 0; fd < fdmax; fd++) {
+	    if (FD_ISSET(fd, fdset+i)) {
+		char buf[BDIGBUFSIZE];
+		if (!doneit) {
+		    buf[0] = '-';
+		    buf[1] = fdchar[i];
+		    buf[2] = 0;
+		    zaddlinknode(fdlist, ztrdup(buf));
+		    doneit = 1;
+		}
+		convbase(buf, fd, 10);
+		zaddlinknode(fdlist, ztrdup(buf));
+	    }
+	}
+    }
+
+    /* convert list to array */
+    fdcount = countlinknodes(fdlist);
+    outptr = outdata = (char **)zalloc(fdcount*sizeof(char *));
+    while (nonempty(fdlist))
+	*outptr++ = getlinknode(fdlist);
+    *outptr = '\0';
+    /* and store in array parameter */
+    setaparam(outarray, outdata);
+    freelinklist(fdlist, NULL);
+
+    return 0;
+#else
+    /* TODO: use poll */
+    zerrnam(nam, "your system does not implement the select system call.",
+	    NULL, );
+    return 2;
+#endif
+}
+
+static struct builtin bintab[] = {
+    BUILTIN("zselect", 0, bin_zselect, 0, -1, 0, NULL, NULL),
+};
+
+/* The load/unload routines required by the zsh library interface */
+
+/**/
+int
+setup_(Module m)
+{
+    return 0;
+}
+
+/**/
+int
+boot_(Module m)
+{
+    return !addbuiltins(m->nam, bintab, sizeof(bintab)/sizeof(*bintab));
+}
+
+
+/**/
+int
+cleanup_(Module m)
+{
+    deletebuiltins("zselect", bintab, sizeof(bintab)/sizeof(*bintab));
+    return 0;
+}
+
+/**/
+int
+finish_(Module m)
+{
+    return 0;
+}
Index: Src/Modules/zselect.mdd
===================================================================
RCS file: Src/Modules/zselect.mdd
diff -N Src/Modules/zselect.mdd
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ Src/Modules/zselect.mdd	7 May 2002 11:22:22 -0000
@@ -0,0 +1,6 @@
+name=zsh/zselect
+link=dynamic
+load=no
+
+objects="zselect.o"
+autobins="zselect"
Index: Doc/Zsh/mod_zselect.yo
===================================================================
RCS file: Doc/Zsh/mod_zselect.yo
diff -N Doc/Zsh/mod_zselect.yo
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ Doc/Zsh/mod_zselect.yo	7 May 2002 11:22:22 -0000
@@ -0,0 +1,49 @@
+COMMENT(!MOD!zsh/zselect
+Block and return when file descriptors are ready.
+!MOD!)
+The tt(zsh/zselect) module makes available one builtin command:
+
+startitem()
+findex(zselect)
+cindex(select, system call)
+cindex(file descriptors, waiting for)
+item(tt(zselect) [ tt(-rwe) tt(-t) var(timeout) tt(-a) var(array) ] [ var(fd) ... ])(
+The tt(zselect) builtin is a front-end to the `select' system call, which
+blocks until a file descriptor is ready for reading or writing, or has an
+error condition, with an optional timeout.  If this is not available on
+your system, the command prints an error message and returns status 2
+(normal errors return status 1).  For more information, see your systems
+documentation for manref(select)(3).  Note there is no connection with the
+shell builtin of the same name.
+
+Arguments and options may be intermingled in any order.  Non-option
+arguments are file descriptors, which must be decimal integers.  By
+default, file descriptors are to be tested for reading, i.e. tt(zselect)
+will return when data is availble to be read from the file descriptor, or
+more precisely, when a read operation from the file descriptor will not
+block.  After a tt(-r), tt(-w) and tt(-e), the given file descriptors are
+to be tested for reading, writing, or error conditions.  These options and
+an arbitrary list of file descriptors may be given in any order.
+
+The option `tt(-t) var(timeout)' specifies a timeout in hundredths of a
+second.  This may be zero, in which case the file descriptors will simply
+be polled and tt(zselect) will return immediately.
+
+The option `tt(-a) var(array)' indicates that tt(array) should be set to
+indicate the file descriptor(s) which are ready.  If the option is not
+given, the array tt(reply) will be used for this purpose.  The array will
+contain a string similar to the arguments for tt(zselect).  For example,
+
+example(zselect -t 0 -r 0 -w 1)
+
+might return immediately with status 0 and tt($reply) containing `tt(-r 0 -w
+1)' to show that both file descriptors are ready for the requested
+operations.
+
+The command returns 0 if some file descriptors are ready for reading.  If
+the operation timed out, or a timeout of 0 was given and no file
+descriptors were ready, or there was an error, it returns status 1 and
+the array will not be set (nor modified in any way).  If there was an error
+in the select operation the appropriate error message is printed.
+)
+enditem()

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 392070


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

Re: PATCH: zselect builtin.

[Bart Schaefer]

On May 7, 12:25pm, Peter Stephenson wrote:
} Subject: PATCH: zselect builtin.
}
} Here's a builtin to act as a front end to a `select' system call.  The
} name clash with the `select' builtin is unfortunate but seems inevitable

Call it "selectfd" perhaps ... or "sys_select" the way perl does it.

} Any comments before I commit this?

How about a -A (or -H, following `stat') option to store into an assoc?
It might be nice if the keys returned were the FDs and the values were
strings like "rwx" for the state of each descriptor, but if the keys were
"-r", "-w" etc. with strings of space-separated FD numbers as values you
could use `select ${(kv)=replyhash}', so I'm undecided.

(I presume `select $reply' is the reason for the format you chose for the
reply array.)

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

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: PATCH: zselect builtin.

[Peter Stephenson]

"Bart Schaefer" wrote:
> On May 7, 12:25pm, Peter Stephenson wrote:
> } Subject: PATCH: zselect builtin.
> }
> } Here's a builtin to act as a front end to a `select' system call.  The
> } name clash with the `select' builtin is unfortunate but seems inevitable
> 
> Call it "selectfd" perhaps ... or "sys_select" the way perl does it.

Well, the convention with other bits and pieces is just to stick a `z'
in front to keep the namespace reasonably unpolluted.  Maybe zselectfd
would be OK?

> } Any comments before I commit this?
> 
> How about a -A (or -H, following `stat') option to store into an assoc?
> It might be nice if the keys returned were the FDs and the values were
> strings like "rwx" for the state of each descriptor, but if the keys were
> "-r", "-w" etc. with strings of space-separated FD numbers as values you
> could use `select ${(kv)=replyhash}', so I'm undecided.

The former might well be useful --- $reply[(i)$fd] doesn't easily tell you
about r, w, or x if you need to know that --- but it's pretty easy to
add later when I decide I can be bothered (or when anyone else can), so
at first I'll just commit it without.

> (I presume `select $reply' is the reason for the format you chose for the
> reply array.)

It wasn't so much that you'd ever want to use that as the simplicity of
having only one form for input and output.

By the way, I've fixed two things which I will put in the committed
version:  the array wasn't allocated to be long enough, so the null
char * was off the end, and there's no reason not to perform a `select'
when there are no fd's listed --- this gives you a higher resolution
sleep for free.  (I suppose I ought to reject it when there's no
timeout, either, but I don't know if that's too special a case to worry
about.  Maybe someone needs a blocking command.)

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 392070


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

Re: PATCH: zselect builtin.

[Bart Schaefer]

On Tue, 7 May 2002, Peter Stephenson wrote:

> Well, the convention with other bits and pieces is just to stick a `z'
> in front to keep the namespace reasonably unpolluted.  Maybe zselectfd
> would be OK?

Nah, if you're going to stick the `z' on there anyway, there's no reason
to append `fd'.

RE: PATCH: zselect builtin.

[Borsenkow Andrej]

> 
> Any comments before I commit this?
> 

If we start adding low-level system interface I really wish that we

- put it in separate module (like Perl POSIX)
- or at least under own hierarchy (like zsh/POSIX/select)

I do not know if POSIX is right here; may be just "system" or just "sys"
would be O.K. meaning zsh/sys/select

In which case it would be nice (as Bart already suggested) to have
standard name pattern; posix_select or sys_select (depending upon level
of system independency we are aiming at).

Aso you probably can't fully utilize select without non-blocking I/O.
The simplest way to add it is using fcntl. Which implies yet another
module (or at least builtin). Non-blocking I/O could be used just fine
currently; just use read-kN and test how much has been read; read should
return 0 if anything has been read, non-zero otherwise.

Hmm ... there does not currently seem to be any way to find out how much
has been written ... sys_write/sys_read?

Otherwise as Bart alrady suggested I prefer has as return with keys FD
numbers.

-andrej

Re: PATCH: zselect builtin.

[Peter Stephenson]

Borsenkow Andrej wrote:
> If we start adding low-level system interface I really wish that we
> 
> - put it in separate module (like Perl POSIX)
> - or at least under own hierarchy (like zsh/POSIX/select)
> 
> In which case it would be nice (as Bart already suggested) to have
> standard name pattern; posix_select or sys_select (depending upon level
> of system independency we are aiming at).

It's too late: we have stat, the files module, and various other bits
which talk to the system in other ways like TCP.  Unless we want to move
everything around now, I don't see a good reason for putting this
anywhere special.  (`stat' already clashes with various external
commands which do a similar job, unfortunately.)  Until then, separate
modules for each separate interface seems to me the cleanest way
forward.

Why is `posix_select' or `sys_select' an improvement over our current
mini-convention with the `z', which also covers things which aren't
strictly tied to system calls?  To me, anyway, it implies a greater
tie-in to the underlying system call than you get with the shell builtin
--- no fd sets, no maximum fd, no errno handling.  This is a distinct
difference from perl, which tries to emulate the POSIX interface as
closely as possible in such cases.

(By the way, there was an interesting note on the Austin group list today
that the `error' fd's in the select call are really for exceptions, and
the only one currently defined is out-of-band data on a socket.  I may
point this out, since most select manual pages don't.)

> Aso you probably can't fully utilize select without non-blocking I/O.

I don't see this; you can already poll a blocking fd using a zero
timeout.  But some fcntl interface is certainly a possibility.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 392070


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

Re: PATCH: zselect builtin.

[Peter Stephenson]

Peter Stephenson wrote:
> Here's a builtin to act as a front end to a `select' system call.

I've committed this, with subtle changes over the form I posted mostly as
indicated in previous postings.  I've added `-A assoc' for associative
array handling.

It's still called zselect, and still lives in zsh/zselect.  I'm happy to
consider more logical overall naming, but until then I don't see any
point in introducing new conventions piecemeal.  (Another argument
against tying the name more closely to the system call is that it is
possible and probably useful to provide an alternative implementation
based on poll().)

Warning:  I used this overnight as part of a test and there seems to be
a memory leak somewhere, not necessarily zselect itself (where I can't
see where it would be).  I was using ztcp, but it was the same TCP
connection all the time, so I don't think that's it either.  It's just
possible I was accumulating some humongous array, but I couldn't see
where that would have been, either.  Unfortunately I had to kill the
shell because I made it uninteruptible.

Another note: the zmodload manual page is written to imply that having a
module name the same as a builtin, or other autoloadable entity, is
useful.  As far as I can see, this is never the case now, due to the
hierarchical naming.  It might be better to remove this feature or at
least add implied aliasing into the zsh/* name segment when there's only
one argument.  (You can add module aliases by hand, but it takes away
the point of the shortcut.)

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 392070


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

PATCH: zselect bug, already.

[Peter Stephenson]

Index: Src/Modules/zselect.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Modules/zselect.c,v
retrieving revision 1.1
diff -u -r1.1 zselect.c
--- Src/Modules/zselect.c	8 May 2002 13:13:52 -0000	1.1
+++ Src/Modules/zselect.c	8 May 2002 13:36:26 -0000
@@ -220,7 +220,7 @@
 				strcpy(buf, data);
 				for (ptr = buf; *ptr; ptr++)
 				    ;
-				*ptr++ = fdchar[1];
+				*ptr++ = fdchar[i];
 				*ptr = '\0';
 				zsfree(data);
 				*dataptr = ztrdup(buf);

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 392070


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

Module loading by name (Re: PATCH: zselect builtin.)

[Bart Schaefer]

On May 8,  2:26pm, Peter Stephenson wrote:
}
} Another note: the zmodload manual page is written to imply that having a
} module name the same as a builtin, or other autoloadable entity, is
} useful.  As far as I can see, this is never the case now, due to the
} hierarchical naming.

I can create my own local modules that ignore the hierarchical naming, and
use it for that.  The "zsh/" hierarchy was only supposed to be for modules
that were distributed as part of the zsh sources, after all, and the whole
point of hierarchical naming was so that people could create other modules
that weren't part of the distribution.

} It might be better to remove this feature or at
} least add implied aliasing into the zsh/* name segment when there's only
} one argument.

I actually use flat module names on a daily basis; implicit aliasing into
the zsh hierarchy would break it.  I use flat names when running the zsh
binary out of a dynamically linked build tree, where the modules are not
yet relocated into a single zsh/ subdirectory and instead I have to point
the module_path at the individual module directories in order to find the
shared objects.

} (You can add module aliases by hand, but it takes away
} the point of the shortcut.)

I have no objection to removing the autoloading shortcut, but NOT by having
the names automatically aliased into the zsh/ hierarchy.

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

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: PATCH: zselect builtin.

[Bart Schaefer]

On May 8, 10:59am, Peter Stephenson wrote:
} Subject: Re: PATCH: zselect builtin.
}
} > Aso you probably can't fully utilize select without non-blocking I/O.
} 
} I don't see this; you can already poll a blocking fd using a zero
} timeout.

The problem is not blocking reads, but blocking *writes*.

However, even non-blocking write is not sufficient if you can't find out
what subset of the bytes got written and try again with the remainder.

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

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: PATCH: zselect builtin.

[Peter Stephenson]

Peter Stephenson wrote:
> Warning:  I used this overnight as part of a test and there seems to be
> a memory leak somewhere, not necessarily zselect itself (where I can't
> see where it would be).  I was using ztcp, but it was the same TCP
> connection all the time, so I don't think that's it either.  It's just
> possible I was accumulating some humongous array, but I couldn't see
> where that would have been, either.  Unfortunately I had to kill the
> shell because I made it uninteruptible.
                                /\
                                r

I'm 90% sure I know what's causing this, and it's nothing to do with the
modules.  The memory is released when the code returns to the top level
(at least by the application layer: it doesn't get returned to the
system, but when I restart the function it takes a long while before it
needs more memory from the system, indicating the shell has finished
with the previous allocation).  This indicates something is hogging it
until the return to top-level processing.  There is one obvious candidate.

When a function is executed, it gets copied to a new structure tree.
When this is finished with, it gets passed to freeeprog().  But that
doesn't free it, in case something is still executing it.  It only gets
freed by freeeprogs() when control returns to the top level.  As my
function executes in a while loop, this never happens.  I tested this by
removing all function calls from the while loop and putting the builtins
inline, and the memory stopped increasing.

This is pretty disastrous.  We need a better way of deciding whether to
free Eprog's.  I can't see why we shouldn't use some reference count
mechanism.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 392070


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

Re: PATCH: zselect builtin.

[Peter Stephenson]

Peter Stephenson wrote:
> When a function is executed, it gets copied to a new structure tree.
> When this is finished with, it gets passed to freeeprog().  But that
> doesn't free it, in case something is still executing it.  It only gets
> freed by freeeprogs() when control returns to the top level.  As my
> function executes in a while loop, this never happens.  I tested this by
> removing all function calls from the while loop and putting the builtins
> inline, and the memory stopped increasing.

This isn't quite right; normal function execution doesn't work quite
that way.  What is happening is I have a function that sets a trap,
which gets restored because of the localtraps option.  Freeing a trap
does use freeeprog, with the described behaviour.  So it's less
disastrous, and I can probably avoid using local traps, but would still
be nice to fix.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 392070


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

Re: PATCH: zselect builtin.

[Borsenkow Andrej]

В Срд, 08.05.2002, в 19:55, Bart Schaefer написал:
> On May 8, 10:59am, Peter Stephenson wrote:
> } Subject: Re: PATCH: zselect builtin.
> }
> } > Aso you probably can't fully utilize select without non-blocking I/O.
> } 
> } I don't see this; you can already poll a blocking fd using a zero
> } timeout.
> 
> The problem is not blocking reads, but blocking *writes*.
> 
> However, even non-blocking write is not sufficient if you can't find out
> what subset of the bytes got written and try again with the remainder.
> 

Right. We could make print return number of characters written. For
non-printf case it is pretty trivial - it just amounts to

	iocount = 0;
	for (; *args; args++, len++) {
	    iocount += fwrite(*args, *len, 1, fout);
	    if (args[1] && (EOF !=
		fputc(ops['l'] ? '\n' : ops['N'] ? '\0' : ' ', fout))
		iocount++;
	}
        setiparam("IOCONUNT", iocount);

for printf-like case it is not quite as simple due to large number of
idividual putc's; also it is not clear if it is needed at all (I do not
see how it can be used).

The same is true of read of course. Both can set IOCOUNT indicating
number of characters transmitted in last call.

Alternative is to add raw write function. 

It is user responsibility then to ensure print does not mangle output
string (i.e. call it with -r and possible -n). Usage is obvious

while true; do
  ....
  zselect -w $d
  print -rnu$d $buffer
  (( IOCOUNT )) || break # EOF
  (( IOCOUNT == $#buffer )) || break # IO error
  ....
done

-andrej

Re: PATCH: zselect builtin.

[Sven Wischnowsky]

Peter Stephenson wrote:

> ...
> 
> This isn't quite right; normal function execution doesn't work quite
> that way.  What is happening is I have a function that sets a trap,
> which gets restored because of the localtraps option.  Freeing a trap
> does use freeeprog, with the described behaviour.  So it's less
> disastrous, and I can probably avoid using local traps, but would still
> be nice to fix.

Yes, it would be. It was mostly a pretty simple-minded attempt to
ensure that we don't free stuff too early. I'm not sure where a better
place is to put the calls to free*, though.


Bye
  Sven

-- 
Sven Wischnowsky                          wischnow@berkom.de

Re: PATCH: zselect builtin.

[Oliver Kiddle]

On Sun, May 12, 2002 at 02:42:07PM +0400, Borsenkow Andrej wrote:
> 
> Right. We could make print return number of characters written. For
> non-printf case it is pretty trivial - it just amounts to

It should be simple for the printf case too because you can do
  printf '%s%n' $buffer IOCOUNT
so code for counting the characters is already there.

Oliver

This e-mail and any attachment is for authorised use by the intended recipient(s) only.  It may contain proprietary material, confidential information and/or be subject to legal privilege.  It should not be copied, disclosed to, retained or used by, any other party.  If you are not an intended recipient then please promptly delete this e-mail and any attachment and all copies and inform the sender.  Thank you.