shithub: lu9-p9

Download patch

ref: d081f55ca03d306fb13df409375e927b9940d95b
parent: 77da164f11d49df4956c34348407d1631f653935
author: kvik <kvik@a-b.xyz>
date: Sat Apr 24 19:15:14 EDT 2021 

file: rework api and implementation, fix nasty bug as well

The File object is reimplemented as a userdata type which cleans up the
implementation.

The confusing file:dup() method is gone.  Its functionality is now split
between a simple file:dup() which takes no arguments and returns a clone
of file, and a new method file:set() which rewires the file descriptor.

The p9.file() constructor is gone.  It was a lazy approach for accessing
already-open file descriptors like the standard input and output but it
turned out to be awkward to use and a very bad idea: the way you'd
access the, for example, standard output was to write:

	p9.file(1):write("hello")

This would create a File object wrapping the fd, call the write method,
after which the newly created file handle would become unreachable and
get picked up by the garbage collector, closing the standard output fd
at some arbitrary time.  This is obviously a very bad interface and I
got punished hard for making and misusing it.

An alternative solution introduced in this patch is to have a p9.fd
table through which arbitrary already-open file descriptors might be
used without needing to explicitly wrap and worry about them being
collected under you. Thus the above example becomes:

	p9.fd[1]:write("hello")

Currently, only the fd 0 through 2 are exposed here, but in some
following patch the table will act as a proxy for any file descriptor.

Some other unimportant changes to code structure sneaked in as well.


--- a/base/base.c
+++ b/base/base.c
@@ -17,10 +17,10 @@
 static luaL_Reg p9_module[] = {
 	{"open", p9_open},
 	{"create", p9_create},
-	{"file", p9_file},
 	{"pipe", p9_pipe},
 	{"remove", p9_remove},
 	{"access", p9_access},
+	{"fd", nil}, /* table placeholder */
 	
 	{"stat", p9_stat},
 	{"wstat", p9_wstat},
@@ -32,6 +32,7 @@
 	
 	{"getenv", p9_getenv},
 	{"setenv", p9_setenv},
+	{"env", nil}, /* table placeholder */
 	
 	{"abort", p9_abort},
 	{"exits", p9_exits},
@@ -58,25 +59,26 @@
 	int lib;
 	Buf *buf;
 	
+	luaL_newlib(L, p9_module);
+	lib = lua_gettop(L);
+	
 	buf = resizebuffer(L, nil, Iosize);
 	lua_pushlightuserdata(L, buf);
 	lua_setfield(L, LUA_REGISTRYINDEX, "p9-buffer");
 	
-	static luaL_Reg filemt[] = {
-		{"close", p9_file_close},
-		{"read", p9_file_read},
-		{"slurp", p9_file_slurp},
-		{"write", p9_file_write},
-		{"seek", p9_file_seek},
-		{"iounit", p9_file_iounit},
-		{"path", p9_file_path},
-		{"dup", p9_file_dup},
-		{nil, nil},
-	};
 	luaL_newmetatable(L, "p9-File");
-	luaL_setfuncs(L, filemt, 0);
-	lua_pop(L, 1);
+	luaL_setfuncs(L, p9_file_proto, 0);
+	lua_pushvalue(L, -1);
+	lua_setfield(L, -2, "__index");
 	
+	/* Populate p9.fd[] with standard descriptors */
+	lua_createtable(L, 3, 0);
+	for(int i = 0; i < 3; i++){
+		filenew(L, i);
+		lua_rawseti(L, -2, i);
+	}
+	lua_setfield(L, lib, "fd");
+	
 	static luaL_Reg walkmt[] = {
 		{"__close", p9_walkclose},
 		{nil, nil},
@@ -83,11 +85,7 @@
 	};
 	luaL_newmetatable(L, "p9-Walk");
 	luaL_setfuncs(L, walkmt, 0);
-	lua_pop(L, 1);
 	
-	luaL_newlib(L, p9_module);
-	lib = lua_gettop(L);
-	
 	static luaL_Reg envmt[] = {
 		{"__index", p9_getenv_index},
 		{"__newindex", p9_setenv_newindex},
@@ -99,5 +97,6 @@
 	lua_setmetatable(L, -2);
 	lua_setfield(L, lib, "env");
 	
+	lua_pushvalue(L, lib);
 	return 1;
 }
--- a/base/common.c
+++ b/base/common.c
@@ -86,3 +86,15 @@
 	lua_pop(L, 1);
 	return resizebuffer(L, buf, sz)->b;
 }
+
+static void
+dumpstack(lua_State *L)
+{
+	int i, top;
+	
+	top = lua_gettop(L);
+	for(i = 1; i <= top; i++){
+		fprint(2, "%d: %s\n", i, luaL_tolstring(L, i, nil));
+		lua_pop(L, 1);
+	}
+}
--- a/base/fs.c
+++ b/base/fs.c
@@ -1,24 +1,64 @@
 /*
  * The File object
 
- * p9.file(fd) takes an open file descriptor and returns a
- * File object f which provides a convenient method interface
- * to the usual file operations.
- * p9.open and p9.create take a file name to open or create.
-
- * The file descriptor stored in f.fd is garbage collected,
- * that is, it will be automatically closed once the File
- * object becomes unreachable. Note how this means that f.fd
- * should be used sparringly and with much care. In particular
- * you shouldn't store it outside of f, since the actual file
- * descriptor number might become invalid (closed) or refer
- * to a completely different file after f is collected.
+ * p9.open and p9.create create a file object representing an
+ * open file descriptor.  This object's methods are then used
+ * for performing I/O and other operations.
  *
- * Store the File object in some global place to prevent it
- * from being collected.
+ * The file object holds a reference to a system resource, the
+ * open file descriptor, which you likely want to release as
+ * soon as it's not needed anymore.
+ * To help with that the file implements both __gc and __close
+ * metamethods, which will close it for you as soon as the file
+ * falls out of reach.
+ * file:close() will close the file descriptor immediately.
+ * file:keep() will prevent automatic closing of the file
+ * descriptor, which may be needed in some situations.
+ *
+ * During module load the standard file descriptors 0 through 2
+ * are wrapped in File objects and get stashed in p9.fd table:
+ * p9.fd[0] thus refers to the standard input, and so on.
+ * Being reachable through the module these aren't subject to
+ * garbage collection -- at least until Lua state is closed.
+ * You may use this table for stashing files for similar reasons.
+ * TODO: use this table to implement access to arbitrary
+ * already-open file descriptors.
  */
  
+typedef struct File File;
+
+struct File {
+	int fd;
+	int keep;
+};
+
 static int
+filenew(lua_State *L, int fd)
+{
+	File *f;
+	
+	f = lua_newuserdatauv(L, sizeof(File), 0);
+	f->fd = fd;
+	f->keep = 0;
+	luaL_setmetatable(L, "p9-File");
+	return 1;
+}
+
+static int
+fileclose(lua_State *L)
+{
+	File *f;
+	
+	f = luaL_checkudata(L, 1, "p9-File");
+	if(f->keep || f->fd == -1)
+		return 0;
+	if(close(f->fd) == -1)
+		return error(L, "close: %r");
+	f->fd = -1;
+	return 0;
+}
+
+static int
 openmode(lua_State *L, char *s)
 {
 	int i, n, mode;
@@ -86,66 +126,7 @@
 	return perm;
 }
 
-static int filenew(lua_State*, int);
-static int fileclose(lua_State*);
-static int filefd(lua_State*, int);
-
 static int
-filenew(lua_State *L, int fd)
-{
-	int f;
-
-	lua_createtable(L, 0, 4);
-	f = lua_gettop(L);
-	lua_pushinteger(L, fd);
-		lua_setfield(L, f, "fd");
-	luaL_getmetatable(L, "p9-File");
-		lua_setfield(L, f, "__index");
-	lua_pushcfunction(L, fileclose);
-		lua_setfield(L, f, "__close");
-	lua_pushcfunction(L, fileclose);
-		lua_setfield(L, f, "__gc");
-	lua_pushvalue(L, f);
-		lua_setmetatable(L, f);
-	return 1;
-}
-
-static int
-fileclose(lua_State *L)
-{
-	int fd;
-	
-	fd = filefd(L, 1);
-	if(fd == -1)
-		return 0;
-	lua_pushinteger(L, -1);
-		lua_setfield(L, 1, "fd");
-	close(fd);
-	return 0;
-}
-
-static int
-filefd(lua_State *L, int idx)
-{
-	int fd;
-	
-	if(lua_getfield(L, idx, "fd") != LUA_TNUMBER)
-		return luaL_error(L, "fd must be integer");
-	fd = lua_tonumber(L, -1);
-	lua_pop(L, 1);
-	return fd;
-}
-
-static int
-p9_file(lua_State *L)
-{
-	int fd;
-	
-	fd = luaL_checkinteger(L, 1);
-	return filenew(L, fd);
-}
-
-static int
 p9_open(lua_State *L)
 {
 	const char *file;
@@ -175,14 +156,40 @@
 }
 
 static int
+p9_pipe(lua_State *L)
+{
+	int fd[2];
+	
+	if(pipe(fd) == -1)
+		return error(L, "pipe: %r");
+	filenew(L, fd[0]);
+	filenew(L, fd[1]);
+	return 2;
+}
+
+static int
 p9_file_close(lua_State *L)
 {
-	if(close(filefd(L, 1)) == -1)
+	File *f;
+	
+	f = luaL_checkudata(L, 1, "p9-File");
+	if(close(f->fd) == -1)
 		return error(L, "close: %r");
+	f->fd = -1;
 	return 0;
 }
 
 static int
+p9_file_keep(lua_State *L)
+{
+	File *f;
+	
+	f = luaL_checkudata(L, 1, "p9-File");
+	f->keep = 1;
+	return 1;
+}
+
+static int
 seekmode(lua_State *L, char *s)
 {
 	if(strcmp(s, "set") == 0)
@@ -197,13 +204,14 @@
 static int
 p9_file_seek(lua_State *L)
 {
-	int fd, type;
+	File *f;
+	int type;
 	vlong n, off;
 	
-	fd = filefd(L, 1);
+	f = luaL_checkudata(L, 1, "p9-File");
 	n = luaL_checkinteger(L, 2);
 	type = seekmode(L, luaL_optstring(L, 3, "set"));
-	if((off = seek(fd, n, type)) == -1)
+	if((off = seek(f->fd, n, type)) == -1)
 		return error(L, "seek: %r");
 	lua_pushinteger(L, off);
 	return 1;
@@ -212,19 +220,19 @@
 static int
 p9_file_read(lua_State *L)
 {
-	int fd;
+	File *f;
 	long n, nbytes;
 	vlong offset;
 	char *buf;
 	
-	fd = filefd(L, 1);
+	f = luaL_checkudata(L, 1, "p9-File");
 	nbytes = luaL_optinteger(L, 2, Iosize);
 	offset = luaL_optinteger(L, 3, -1);
 	buf = getbuffer(L, nbytes);
 	if(offset == -1)
-		n = read(fd, buf, nbytes);
+		n = read(f->fd, buf, nbytes);
 	else
-		n = pread(fd, buf, nbytes, offset);
+		n = pread(f->fd, buf, nbytes, offset);
 	if(n == -1)
 		return error(L, "read: %r");
 	lua_pushlstring(L, buf, n);
@@ -257,12 +265,12 @@
 static int
 p9_file_slurp(lua_State *L)
 {
-	int fd;
+	File *f;
 	long nbytes;
 	
-	fd = filefd(L, 1);
+	f = luaL_checkudata(L, 1, "p9-File");
 	nbytes = luaL_optinteger(L, 2, -1);
-	slurp(L, fd, nbytes);
+	slurp(L, f->fd, nbytes);
 	return 1;
 }
 
@@ -269,19 +277,20 @@
 static int
 p9_file_write(lua_State *L)
 {
-	lua_Integer fd, offset;
+	File *f;
+	lua_Integer offset;
 	size_t nbytes;
 	const char *buf;
 	long n;
 
-	fd = filefd(L, 1);
+	f = luaL_checkudata(L, 1, "p9-File");
 	buf = luaL_checklstring(L, 2, &nbytes);
 	nbytes = luaL_optinteger(L, 3, nbytes);
 	offset = luaL_optinteger(L, 4, -1);
 	if(offset == -1)
-		n = write(fd, buf, nbytes);
+		n = write(f->fd, buf, nbytes);
 	else
-		n = pwrite(fd, buf, nbytes, offset);
+		n = pwrite(f->fd, buf, nbytes, offset);
 	if(n != nbytes)
 		return error(L, "write: %r");
 	lua_pushinteger(L, n);
@@ -291,12 +300,12 @@
 static int
 p9_file_path(lua_State *L)
 {
-	int fd;
+	File *f;
 	char *buf;
 	
-	fd = filefd(L, 1);
+	f = luaL_checkudata(L, 1, "p9-File");
 	buf = getbuffer(L, Iosize);
-	if(fd2path(fd, buf, Iosize) != 0)
+	if(fd2path(f->fd, buf, Iosize) != 0)
 		return error(L, "fd2path: %r");
 	lua_pushstring(L, buf);
 	return 1;
@@ -305,10 +314,10 @@
 static int
 p9_file_iounit(lua_State *L)
 {
-	int fd;
+	File *f;
 	
-	fd = filefd(L, 1);
-	lua_pushinteger(L, iounit(fd));
+	f = luaL_checkudata(L, 1, "p9-File");
+	lua_pushinteger(L, iounit(f->fd));
 	return 1;
 }
 
@@ -315,25 +324,55 @@
 static int
 p9_file_dup(lua_State *L)
 {
-	int fd, new, na;
+	int fd;
+	File *f;
 	
-	na = lua_gettop(L);
-	fd = filefd(L, 1);
-	if(na == 2)
-		new = filefd(L, 2);
-	else
-		new = -1;
-	if((new = dup(fd, new)) == -1)
+	f = luaL_checkudata(L, 1, "p9-File");
+	if((fd = dup(f->fd, -1)) == -1)
 		return error(L, "dup: %r");
-	if(na == 2){
-		lua_pushinteger(L, new);
-		lua_setfield(L, 2, "fd");
-		return 1;
-	}
-	return filenew(L, new);
+	return filenew(L, fd);
 }
 
 static int
+p9_file_set(lua_State *L)
+{
+	int fd;
+	File *this, *that;
+	
+	this = luaL_checkudata(L, 1, "p9-File");
+	that = luaL_checkudata(L, 2, "p9-File");
+	if((fd = dup(that->fd, this->fd)) == -1)
+		return error(L, "dup: %r");
+	this->fd = fd;
+	lua_pushvalue(L, 1);
+	return 1;
+}
+
+static luaL_Reg p9_file_proto[] = {
+	{"close", p9_file_close},
+	{"keep", p9_file_keep},
+	{"read", p9_file_read},
+	{"slurp", p9_file_slurp},
+	{"write", p9_file_write},
+	{"seek", p9_file_seek},
+	{"iounit", p9_file_iounit},
+	{"path", p9_file_path},
+	{"dup", p9_file_dup},
+	{"set", p9_file_set},
+	
+	{"__gc", fileclose},
+	{"__close", fileclose},
+	{"__index", nil}, /* placeholder for ourselves */
+	
+	{nil, nil},
+};
+
+
+/*
+ * Assorted functions that don't work with or create Files
+ */
+
+static int
 p9_remove(lua_State *L)
 {
 	const char *file;
@@ -343,18 +382,6 @@
 		return error(L, "remove: %r");
 	lua_pushboolean(L, 1);
 	return 1;
-}
-
-static int
-p9_pipe(lua_State *L)
-{
-	int fd[2];
-	
-	if(pipe(fd) == -1)
-		return error(L, "pipe: %r");
-	filenew(L, fd[0]);
-	filenew(L, fd[1]);
-	return 2;
 }
 
 static int
--- a/test/test.lua
+++ b/test/test.lua
@@ -15,6 +15,8 @@
 	os.execute("prompt='p9; ' rc -i")
 end
 
+local leak = false
+
 p9.rfork("env name")
 os.execute("ramfs")
 
@@ -40,33 +42,18 @@
 	assert(fd:slurp(16*1024 + 999) == s)
 	fd:close()
 	
-	fd = p9.open(f, "r")
+	fd = assert(p9.open(f, "r"))
 	assert(fd:seek(0, "end") == 16*1024)
 	assert(fd:seek(8192, "set") == 8192
 		and fd:slurp() == string.rep("ABCD", 2*1024))
-	fd:seek(0)
-	assert(fd:seek(16*1024 - 4, "cur") == 16*1024 - 4
-		and fd:slurp() == "ABCD")
+	assert(fd:seek(0, "set"))
+	assert(fd:seek(16*1024 - 4, "set") == 16*1024 - 4)
+	assert(fd:slurp() == "ABCD")
 	fd:close()
 end
 
 -- File objects
--- Closing
--- Make sure it's closed
-local fd
-do
-	local f <close> = p9.create(tmp())
-	fd = f.fd
-end
-assert(p9.file(fd):close() == nil)
--- Make sure it's not closed
-local fd
-do
-	local f = p9.create(tmp())
-	fd = f.fd
-end
-assert(p9.file(fd):seek(0))
-p9.file(fd):close()
+-- TODO: closing and collecting.
 
 -- file:path()
 do
@@ -82,9 +69,9 @@
 	f:close()
 end
 
--- file:dup()
+-- file:dup() and file:set()
 do
-	local a, b = assert(p9.pipe())
+	local a, b = p9.pipe()
 	local c = assert(a:dup())
 	a:write("hello")
 	assert(b:read() == "hello")
@@ -91,11 +78,11 @@
 	c:write("world")
 	assert(b:read() == "world")
 	a:close() b:close() c:close()
-	
+
 	a = assert(p9.open("/lib/glass"))
-	local buf = a:read()
 	b = assert(p9.open("/lib/bullshit"))
-	b = assert(a:dup(b))
+	local buf = a:read()
+	assert(b:set(a))
 	b:seek(0)
 	assert(b:read() == buf)
 end
@@ -246,9 +233,9 @@
 do
 	local us, them = p9.pipe()
 	if p9.rfork("proc nowait fd") == 0 then
-		them:dup(p9.file(0))
-		them:dup(p9.file(1))
-		them:close()
+		us:close()
+		p9.fd[0]:set(them)
+		p9.fd[1]:set(them)
 		p9.exec("cat")
 	else
 		them:close()
@@ -325,7 +312,7 @@
 
 
 -- Check for leaks (might not come from us)
-do
+if leak then
 	if p9.rfork("proc") == 0 then
 		p9.exec("leak", p9.ppid())
 	end