Rockbox

Tasklist

FS#11347 - *dir LUA functions

Attached to Project: Rockbox
Opened by Rafaël Carré (funman) - Thursday, 03 June 2010, 00:37 GMT
Last edited by Rafaël Carré (funman) - Friday, 18 June 2010, 13:11 GMT
Task Type Patches
Category Plugins
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

LUA doesn't implement any directory functions because those aren't in ANSI C

There is an implementation on http://www.keplerproject.org/luafilesystem with :

attributes (filepath [, attributename]) (stat())
chdir (path)
currentdir () (getcwd)
dir (path) (readdir() iterator)
lock (fh, mode) ?
lfs.lock_dir (path) ?
mkdir (path)
rmdir (path)
setmode (filepath, mode) (TEXT/BINARY)
symlinkattributes (filepath [, attributename]) (lstat())
touch (filepath [, atime [, mtime]])
unlock (fh) ?


I only took the code for mkdir/rmdir and the directory iterator

The iterator returns 2 arguments: string (path) and bool (is directory), while the original code only returned a string relied on attributes/stat() to know if it was a directory.

It's a new "luadir" module although I think it could be in rocklib
Tested in sim and target (fuzev1)
This task depends upon

Closed by  Rafaël Carré (funman)
Friday, 18 June 2010, 13:11 GMT
Reason for closing:  Accepted
Additional comments about closing:  r26913

correct suggested return value mapping: 0 -> true, non 0 -> false
Comment by Maurus Cuelenaere (mcuelenaere) - Saturday, 05 June 2010, 13:45 GMT
Looks good.

* You can #include "rocklibc.h" for errno (and remove the #include <errno.h>)
* I'd prefer a #define LUA_(LUA)DIRLIBNAME luaopen_luadir in luadir.h for consistency
* Perhaps rb->rmdir and rb->mkdir should be added to forbidden_functions in rocklib_aux.pl

EDIT:
Shouldn't you return 1 in make_dir() and remove_dir() on failure? You're only pushing one variable on the stack.

EDIT2:
I'd prefer always returning a boolean instead of sometimes nil in make_dir() and remove_dir(), this simplifies error checking in Lua scripts.
Comment by Rafaël Carré (funman) - Saturday, 05 June 2010, 17:03 GMT
- #include "rockblibc.h" for errno
- use a define for module entry
- I don't remember what the rockblib_aux.pl does :/ can you figure if it's needed at all?

I removed the push of an error string in make_dir/remove_dir but forgot to adjust return number, both return 1 now

Also I'm not sure of which meaning should the boolean return value have (false == not success; or false == 0 == success like in C)
This version returns true if the function succeeded (so the opposite of C function retval)
Comment by Maurus Cuelenaere (mcuelenaere) - Saturday, 05 June 2010, 17:13 GMT
Hmm, the attachment gives a 'file does not exist anymore' error for me.. Flyspray bug?

I'd go with a 0->true, nonzero->false mapping (not sure what the original luafs did?).

Rocklib_aux.pl automatically generates wrappers from plugin.h, so if mkdir and rmdir wouldn't be removed, there'd be 2 ways to call them.
Comment by Rafaël Carré (funman) - Saturday, 05 June 2010, 20:41 GMT
Sorry I did upload an incomplete patch and replaced it almost just after hoping you wouldn't be so fast ;)

About mkdir / rmdir I didn't know they were accessible from lua already, I only really needed directory iterator and left mkdir/rmdir in the patch because they were present in luafilesystem and usable in rockbox (unlike functions using *stat() / symlinks etc..)

Perhaps then this patch should only contain the iterator?
Comment by Maurus Cuelenaere (mcuelenaere) - Sunday, 06 June 2010, 16:35 GMT
I prefer mkdir/rmdir being accesible from luadir instead of rb.

* I made a mistake, LUA_DIRLIBNAME should be defined to "luadir", not luaopen_luadir :) (you could also use this in luaopen_luadir())
* The line "lua_pushboolean (L, entry->attribute & ATTR_DIRECTORY);" isn't aligned properly
* You can remove #define errno *rb->__errno, that's what #include "rocklibc.h" is for (eventually the errno used in Lua should be replace with *rb->__errno)
Comment by Rafaël Carré (funman) - Monday, 07 June 2010, 15:11 GMT
This one also removes tabs
Comment by Maurus Cuelenaere (mcuelenaere) - Friday, 18 June 2010, 12:33 GMT
Shouldn't that be !rb->mkdir(path) and !rb->rmdir(path)?

Apart from that it looks OK, are you going to commit it?

Loading...