file: list now takes optional pattern for filtering #2452

nwf · 2018-08-09T09:18:43Z

This PR is for the dev branch rather than for master.
This PR is compliant with the other contributing guidelines as well (if not, please describe why).
I have thoroughly tested my contribution.
The code changes are reflected in the documentation at docs/en/*.

This adds an optional pattern to file.list() to filter results before handing them to Lua. This optimizes the relatively common pattern of

for f,s in pairs(file.list()) do
  if s:match(pattern) then
    -- process matching file name
  end
end

into the more concise, faster, and less-memory-consuming

for f,s in pairs (file.list(pattern)) do
  -- process each matching file name
end

(neglecting the possibility that pattern might raise an error. If that happens, file.list(pattern) will now return nil, errorstr, as typical).

This has been tested by running comprehensive test cases:

for k,v in pairs(file.list()) do print(k,v) end -- all files
for k,v in pairs(file.list("asdf")) do print(k,v) end -- pattern with no matches
for k,v in pairs(file.list("ini")) do print(k,v) end -- pattern some matches
for k,v in pairs(file.list({})) do print(k,v) end -- wrong type passed as argument
_, y = file.list("%fx"); print (y) -- pattern error passed up to caller

nwf · 2018-08-09T09:21:18Z

P.S. please be gentle, but please do review this for sanity and proper stack handling; it's only my second or third time programming against the Lua FFI.

marcelstoer · 2018-08-09T10:09:50Z

docs/en/modules/file.md

-a Lua table which contains the {file name: file size} pairs
+a Lua table which contains all {file name: file size} pairs, if no pattern
+given.  If a pattern is given, only those file names matching the pattern
+(interpreted as a traditional Lua pattern, not, say, a UNIX shell glob) will be


Linking to https://www.lua.org/pil/20.2.html or http://lua-users.org/wiki/PatternsTutorial here?

TerryE · 2018-08-09T18:10:08Z

app/modules/file.c

+      lua_pushstring( L, stat.name );
+      lua_pushvalue( L, -4 );
+      /* Call transforms stack from T P F F N P to T P F Result */
+      pcres = lua_pcall( L, 2, 1, 0 );


Don't do a lua_pcall(), in this case just do a plain lua_call() is more appropriate. If the app programmer can't use a valid pattern then the correct thing to do is to let lstrlib.c throw a Lua error.

No, that's not sufficient. Lua will longjmp over our activation frame and not give us the opportunity to call vfs_closedir, which might imply a leak of a directory iterator. If not in SPIFFS or FATFS, some day, all the same.

TerryE · 2018-08-09T18:15:25Z

app/modules/file.c

+  /* If we're doing a match, push the Pattern and Function */
+  if (pattern != emptystr) {
+    lua_pushstring( L, pattern );
+    /* 


Why push this here? It was the call arg, so is already on the stack.

Fair enough. I suppose luaL_checkoptstring just returns what's already on the stack.

TerryE · 2018-08-09T18:21:17Z

app/modules/file.c

+  int res = 1;
+
+  pattern = luaL_optlstring(L, 1, emptystr, NULL);
+


You can just use NULL as the default and this works fine. It also makes the following code easier. Also it is often just easier to fix the stack length so a lua_settop(L,1); will add the add the nil is missing and trim excess args so that you can now use base relative rather than top relative addressing. A lot easier to follow, IMO.

TerryE · 2018-08-09T21:49:03Z

app/modules/file.c

+      lua_pop( L, 1 );
+    } else {
+      /* Stack just holds result table at the top; add result */
+      lua_pushinteger( L, stat.size );


these to push/setfield calls can be folded easily

I debated doing that. I can, if you like.

TerryE · 2018-08-09T21:54:39Z

Nathaniel, just out of interest, here was my attempt. Does this make sense to you? I know the manual talks as if it is a stack, but sometimes its just a lot simpler to base relative addressing.

static int file_list( lua_State* L ) {
  vfs_dir  *dir;
  const char *pattern;
  struct vfs_stat stat;

  /* make sure the stack contains exactly one string or nil */ 
  lua_settop(L, 1);
  pattern = luaL_optstring(L, 1, NULL);                  /* pattern at 1 */
  
  if ( (dir = vfs_pendir("")) == NULL) {
    return 0;
  }

  /* Allocate result table T */
  lua_newtable( L );                                     /* table at 2 */

  /* If we're doing a match, push the string.match function */
  if (pattern) {
    luaL_getmetafield( L, 1, "match" );                  /* function at 3 */
  }

  while (vfs_readdir(dir, &stat) == VFS_RES_OK) {
    if (pattern) {
      lua_settop( L,  3 );
      /* string.match(stat_name, pattern */
      lua_pushvalue( L, 3 );                             /* function at 4 */
      lua_pushstring( L, stat.name );                    /* filename at 5 */
      lua_pushvalue( L, 1 );                             /* pattern at 6 */
      lua_call( L, 2, 1 );                               /* result at 4 */
      if (lua_isnil( L, -1 )) {
        continue;
      }
    }
    lua_pushinteger( L, stat.size );
    lua_setfield( L, 2, stat.name );
  }

  lua_remove( L, 1 );           /* remove pattern at 1, so table now at 1 */              
  vfs_closedir(dir);
  return 1;
}

nwf · 2018-08-09T23:42:41Z

@TerryE Thanks for that. I reworked mine in light of yours and re-ran the tests I enumerated above.

TerryE · 2018-08-09T23:58:27Z

@nwf, it takes a bit to get your head around using the API, but once you do, it is really quite potent. The more of us the merrier.

Are you using the gdb stub? It's really quite good. This is one time that I use esplorer, because I am cycling reboot/tweak and start script in explorer and then toogle into the xtensa gdb. Also note the .gdbinit and .gdbinitlua in the dev branch which contain useful macros.

TerryE · 2018-08-10T00:09:55Z

app/modules/file.c

+      lua_pushvalue( L, 3 );
+      lua_pushstring( L, stat.name );
+      lua_pushvalue( L, 1 );
+      pcres = lua_pcall( L, 2, 1, 0 );


This is really the wrong thing to do. You should just use a lua_call() and let the lua error system do its stuff. So for example if you do something like:

for name, len in pairs(files.list('[a-f]%')) do`

you want it to throw a malformed pattern (ends with '%') rather than a bad argument #1 to 'pairs' (table expected, got number). 9 times out of 10 the pattern is programmed, and on the 10th time, the programmer can always wrap the file.list(filter) in a pcall()

As I said above, I do not believe that longjump-ing out of file.list() is correct, as that will not call vfs_closedir() on its way out. I've reworked the patch to use lua_error to re-throw the exception.

@TerryE

Thanks to @TerryE for many useful suggestions

TerryE · 2018-08-10T00:48:17Z

Sorry, missed that. In which case you don't need the goto. Just do the closedir and throw the error inline in the pcall status test code.

nwf · 2018-08-10T05:26:38Z

@TerryE The goto's gone; mind taking another look?

marcelstoer added the enhancement label Aug 9, 2018

marcelstoer reviewed Aug 9, 2018

View reviewed changes

nwf force-pushed the for-upstream-file-list branch from c417644 to c20d02a Compare August 9, 2018 11:56

TerryE mentioned this pull request Aug 9, 2018

file_list enhancement #2440

Closed

TerryE reviewed Aug 9, 2018

View reviewed changes

nwf force-pushed the for-upstream-file-list branch from c20d02a to b83ecf7 Compare August 9, 2018 23:42

TerryE reviewed Aug 10, 2018

View reviewed changes

file: list now takes optional pattern for filtering

0343283

Thanks to @TerryE for many useful suggestions

nwf force-pushed the for-upstream-file-list branch from b83ecf7 to 0343283 Compare August 10, 2018 00:31

marcelstoer added this to the next-release milestone Aug 10, 2018

TerryE merged commit fd12be9 into nodemcu:dev Aug 10, 2018

nwf deleted the for-upstream-file-list branch August 10, 2018 15:48

		int res = 1;

		pattern = luaL_optlstring(L, 1, emptystr, NULL);

file: list now takes optional pattern for filtering #2452

file: list now takes optional pattern for filtering #2452

Uh oh!

Conversation

nwf commented Aug 9, 2018

Uh oh!

nwf commented Aug 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TerryE commented Aug 9, 2018

Uh oh!

nwf commented Aug 9, 2018

Uh oh!

TerryE commented Aug 9, 2018

Uh oh!

TerryE Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nwf Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TerryE commented Aug 10, 2018

Uh oh!

nwf commented Aug 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TerryE Aug 10, 2018 •

edited

Loading

nwf Aug 10, 2018 •

edited

Loading