Create ftpserver.lua #2345

ghost · 2018-04-05T11:57:36Z

Fixes #2343 .

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/*.

Some times need upload files withuot rs232 connection. FTP is best solution.

TerryE · 2018-04-05T12:08:40Z

lua_modules/ftp/server.lua

@@ -0,0 +1,157 @@
+-- a simple ftp server
+USER = "test"
+PASS = "12345"


Keep these in locals, and they will be accesses upvals in nested closures. See my FAQ for moy explanation:

local USER, PASS = "test", "12345"

TerryE · 2018-04-05T12:12:56Z

lua_modules/ftp/server.lua

+USER = "test"
+PASS = "12345"
+local file,net,pairs,print,string,table = file,net,pairs,print,string,table
+data_fnc = nil


local data_fnc = function() end

and then the listener becomes ftp_data:listen(20, function (s) data_fnc(s) end) You still need the extra closure because you overwrite data_fnc/

and owerwrite with blank function after STOR,RETR is comlete?

TerryE · 2018-04-05T12:13:30Z

lua_modules/ftp/server.lua

+  local buf = ""
+  local t = 0
+  socket:on("receive", function(c, d)
+    a = {}


Should be a local.

TerryE · 2018-04-05T12:15:19Z

lua_modules/ftp/server.lua

+    for i in string.gmatch(d, "([^ \r\n]+)") do
+      table.insert(a,i)
+    end
+    if(a[1] == nil or a[1] == "")then return end


local a1,a2=unpack(a)

and use a1 and a2 below instead of a[1] and a[2]; faster and generates less code.

TerryE · 2018-04-05T12:18:53Z

lua_modules/ftp/server.lua

+      table.insert(a,i)
+    end
+    if(a[1] == nil or a[1] == "")then return end
+    if(s == 0 and a[1] == "USER")then


You know that the Lua syntax is if cond then .... This isn't C or Javascript 😃. IMO if a2 ~= USER thenis clearer, but even if you want to keep the () or even ((())) then add the spaces to make it readable.

Yes - wery more JS and Cpp make me use If() automaticly

TerryE · 2018-04-05T12:41:12Z

lua_modules/ftp/server.lua

+        for k,v in pairs(l) do
+          if(a[1] == "NLST")then
+            sd:send(k.."\r\n")
+          else


As I said previously multiple sends in a single CB is a no-no. So you should do something like

local resp={} for k,v in pairs(file.list()) do resp[#resp+1] = (a[1] == "NLST") and (k.."\r\n") or ("+r,s"..v..",\t"..k.."\r\n") end multi_send(resp)

Where multi_send(resp) is a separate routine declared above to do the multi-send. I'll discuss this separately. Note the sd used here is an upval which wll create a resource leak.

just simplify code:

sd:send((a[1] == "NLST") and (k.."\r\n") or ("+r,s"..v..",\t"..k.."\r\n"))

TerryE · 2018-04-05T12:57:51Z

@NeiroNx, Read my FAQ and particularly how to use debug.getregistry() to get a handle on registered upvals which can cause resource leaks. Using upvals is in general good practice, but there are bearpits because these can create circular references through the Lua Registry that the LGC can't clear down unless you break the circular links.

Re the mutil_resp() routine, there are examples around. I can give you an example or you might prefer to do this yourself. Its a pity that Lua doesn't have a table.spilt() , but in general the sum of the lines will be <1Kb so you can just do a table.concat() to form a response, but if it is longer than 1024 then you want to split the string into the first bit (<1Kb) and the rest and send the first bit with an on("sent") CB to send the next 1Kb chunk, etc.

This non-blocking I/O can be a real PITA sometimes. 😄

devsaurus · 2018-04-05T13:22:12Z

lua_modules/ftp/server.lua

+        sd:on("receive", function(cd, dd)
+          f:write(dd)
+        end)
+        socket:on("disconnection", function(c)


Should be sd:on instead of socket:on?

turn it in module like "http"

ghost

Make changes and make module like "http"

ghost · 2018-04-05T13:54:13Z

lua_modules/ftp/server.lua

+        for k,v in pairs(l) do
+          if(a[1] == "NLST")then
+            sd:send(k.."\r\n")
+          else


just simplify code:

sd:send((a[1] == "NLST") and (k.."\r\n") or ("+r,s"..v..",\t"..k.."\r\n"))

ghost · 2018-04-05T14:06:08Z

lua_modules/ftp/server.lua

+USER = "test"
+PASS = "12345"
+local file,net,pairs,print,string,table = file,net,pairs,print,string,table
+data_fnc = nil


and owerwrite with blank function after STOR,RETR is comlete?

ghost · 2018-04-05T14:08:54Z

lua_modules/ftp/server.lua

+      table.insert(a,i)
+    end
+    if(a[1] == nil or a[1] == "")then return end
+    if(s == 0 and a[1] == "USER")then


Yes - wery more JS and Cpp make me use If() automaticly

HHHartmann · 2018-04-05T15:45:42Z

lua_modules/ftp/server.lua

+          if b then
+            sd:send(b)
+            b=f:read(1024)
+          else


Great module. This was on my wish list for some time.

Reading the next chunk of the file right before sending it and setting the buffer to nil right afterwards would save some memory.

b=f:read(1024) sd:send(b) b=nil

The logic would have to be adapted also.

Update file reading algoritm - reading chunk before sending and nil it after send called.

eku · 2018-04-05T17:34:57Z

May I ask why ftp and not tftp?

marcelstoer · 2018-04-05T19:18:38Z

Your ftp folder is missing a README that documents the module. Not all Lua modules have that but all should have. A good example is https://github.com/nodemcu/nodemcu-firmware/tree/master/lua_modules/ds18b20.

ghost · 2018-04-05T20:39:00Z

May I ask why ftp and not tftp?

TFTP:

is difficult to use from phone or tablet.
have no directory listing.
do not have authorization.
...

drawkula · 2018-04-05T20:49:27Z

On some OSes you can mount FTP servers and many file managers can access FTP semi-transparently without mounting, so this will be a nice way to handle code on your favourite ESP-pets.

devsaurus

Some hints on locals and memory leaking upvals.

devsaurus · 2018-04-05T21:11:19Z

lua_modules/ftp/ftpserver.lua

+      return c:send("200 S OK\r\n")
+    end
+    if a1 == "PASV" then
+      local _,ip = socket:getaddr()


socket -> c

devsaurus · 2018-04-05T21:12:11Z

lua_modules/ftp/ftpserver.lua

+      return
+    end
+    if a1 == "RETR" then
+      f = file.open(a2:gsub("%/",""),"r")


devsaurus · 2018-04-05T21:14:32Z

lua_modules/ftp/ftpserver.lua

+      end
+      c:send("150 Accepted data connection\r\n")
+      data_fnc = function(sd)
+        sd:on("sent", function(cd)


Not sure if b should be declared local before the sent callback. Just to avoid that b in the lines below refers to a global variable.

devsaurus · 2018-04-05T21:14:49Z

lua_modules/ftp/ftpserver.lua

+        sd:on("sent", function(cd)
+          b=f:read(1024)
+          if b then
+            sd:send(b)


sd -> cd

devsaurus · 2018-04-05T21:15:03Z

lua_modules/ftp/ftpserver.lua

+            sd:send(b)
+            b=nil
+          else
+            sd:close()


sd -> cd

devsaurus · 2018-04-05T21:15:31Z

lua_modules/ftp/ftpserver.lua

+      return
+    end
+    if a1 == "STOR" then
+      f = file.open(a2:gsub("%/",""),"w")


marcelstoer · 2018-04-05T21:31:30Z

lua_modules/ftp/README.md

+FTP server uses only one specified user and password to authenticate clients.
+All clients need authentication - anonymous access is not supported yet.
+All files have RW access.
+Directory creation do not supported because SPIFFS do not have that.


While this is technically correct you might want to mention that you can mimic directories by prefixing file names.

devsaurus

It took me some time to understand why this module doesn't work with the standard ftp client on my linux machine. E.g. the ls command would hang after the server responded with

150 Accepted data connection

Reason is the module's assumption that a client will not connect to the data port before the actual LIST command (or RETR, STOR). It seems that my ftp client already connects right after the server's response to PASV but data_func is still nil at that time. As a consequence, the listen event is lost and the data transfer never starts.

I suggest to extend the logic in this module to handle the "early connect" scenario as laid out in the inline comments below. These changes work fine for me but I can't test whether they break the server for a client which connects late.

devsaurus · 2018-04-05T21:41:55Z

lua_modules/ftp/ftpserver.lua

+local createServer = function (user, pass)
+local data_fnc = nil
+ftp_data = net.createServer(net.TCP, 180)
+ftp_data:listen(20, function (s) if data_fnc then data_fnc(s) end end)


Proposal to handle "early connect" scenario:

if data_fnc then data_fnc(s) else data_sock = s end

With a local data_sock = nil above.

devsaurus · 2018-04-05T21:44:12Z

lua_modules/ftp/ftpserver.lua

+        data_fnc = nil
+        c:send("226 Transfer complete.\r\n")
+      end
+      return


Proposal for "early connect" scenario:

if data_sock then node.task.post(function() data_fnc(data_sock); data_sock = nil end) end return

This starts data_fnc manually after the return.

devsaurus · 2018-04-05T21:44:59Z

lua_modules/ftp/ftpserver.lua

+        sd:send(b)
+        b=nil
+      end
+      return


Also post data_fnc here for "early connect".

devsaurus · 2018-04-05T21:45:13Z

lua_modules/ftp/ftpserver.lua

+        end)
+        c:send("226 Transfer complete.\r\n")
+      end
+      return


Also post data_fnc here for "early connect".

Test with Filezilla - it do not connect before transfer. It send PASV every time

Fix "early connect" logic

devsaurus · 2018-04-06T16:05:37Z

Works very well in my environment after the latest changes - thanks a lot @NeiroNx for your contribution and dedication!

FrankX0 · 2018-04-06T18:52:42Z

The current implementation does not work in my environment (Windows and FileZilla).
This is because the ports used by this implementation are fixed to 20 and 21.
However, when the ftp client sends a PASV command, the server should respond with the actual port used by the client, and start listening on that port. So I changed your code from using a fixed port for the data connection (21) to a dynamic version, as dictated by the client:

    if a1 == "PASV" then
      local _,ip = c:getaddr()
      local _,_,i1,i2,i3,i4 = string.find(ip,"(%d+).(%d+).(%d+).(%d+)")
      return c:send("227 Entering Passive Mode ("..i1..","..i2..","..i3..","..i4..",0,20).\r\n")
    end

Changed into:

        if(a[1] == "PASV")then
            local port, _ = c:getpeer()
            local _,_,i1,i2,i3,i4 = string.find(IP,"(%d+).(%d+).(%d+).(%d+)")
            c:send("227 Entering Passive Mode ("..i1..","..i2..","..i3..","..i4..","..(port / 256)..","..(port % 256)..").\r\n")
            if ftp_data ~= nil then
                ftp_data:close()
            end
            ftp_data = net.createServer(net.TCP, 180)
            ftp_data:listen(port, function (s)
                if data_fnc then
                    data_fnc(s)
                end
            end)
            return
        end

Then my connection with FileZilla in (passive mode) works correctly (on an integer build).
Looking at some found information, this seems in general necessary.

devsaurus · 2018-04-06T19:44:27Z

Then my connection with FileZilla in (passive mode) works correctly

@FrankX0 The only missing server constraint I can deduce from the linked information is that the server's data port should be >1023 in passive mode. This is achieved for sure by your dynamic port switching code.
Would FileZilla work for you as well if ftp_data listens statically e.g on port 2048 (and the PASV response announces this accordingly)?

FrankX0 · 2018-04-06T20:11:52Z

@devsaurus Yep! Then also FileZilla works!
So the (current) hardcoded ftp data port 20 should be either:

configurable,
dynamic, or
hardcoded above 1023.

FrankX0

With these suggested changes, it will be possible to use ftp functionality from a browser (Chrome on Windows in my case).

FrankX0 · 2018-04-07T14:22:30Z

lua_modules/ftp/ftpserver.lua

+          end
+        end)
+        local b=f:read(1024)
+        sd:send(b)


Check for b not being nil, to prevent crashing reading an empty file.

FrankX0 · 2018-04-07T14:24:55Z

lua_modules/ftp/ftpserver.lua

+    if a1 == "QUIT" then
+      return c:send("221 Goodbye\r\n", function (s) s:close() end)
+    end
+    c:send("500 Unknown error\r\n")


Add support for SIZE command: this is generally used by web-browsers.

if a1 == "SIZE" then local st,size st = file.stat(string.sub(a2, 2)) if st then return c:send("213 "..st.size.."\r\n") else return c:send("550 Could not get file size.\r\n") end end

FrankX0 · 2018-04-07T14:30:31Z

lua_modules/ftp/ftpserver.lua

+        return c:send("257 \""..cwd.."\" is your current directory\r\n")
+      end
+      cwd = a2
+      return c:send("250 OK. Current directory is "..cwd.."\r\n")


Suggest to change to:

if a2 == "/" then cwd = a2 return c:send("250 OK. Current directory is "..cwd.."\r\n") end return c:send("550 Failed to change directory.\r\n")

Needed to support web-browsers to retrieve the index of the root and prevent a CWD into a file.

"cwd" in file is not main problem.
Some clients send a "LIST -a /test.txt" commands and i am do not know how-to process them.

Add SIZE and SYST commands, Fix "cwd" into file. Next stage will be check with file.stat() Test with "Chrome" and "ES Explorer"

ghost · 2018-04-08T15:31:45Z

need more tests, may be needs MLST,MLSD - but it needs more and more code and more memory for it

remove debug print

TerryE · 2018-04-09T13:50:38Z

@NeiroNx, you are still breaking the SDK rules in the LIST processing. You can't do

        for k,v in pairs(l) do
          sd:send( a1 == "NLST" and k.."\r\n" or "-rwxrwxrwx 1 esp esp "..v.." Jan  1  2018 "..k.."\r\n")
        end

Also I've done a rework your code in the latest of my gists. I don't have the time or desire to create an alternative to your implementation, as you have done all of the hard work and have the energy to see this to completion. But copy this example and consider some of the Lua coding techniques in your own style 😄.

For example:

RFC 95 is the main specification for the FTP protocol and sec 5.3.1 summarises the list of commands that you are implementing. These are all of the format
- <CMD><CRLF>
- <CMD><space><argument><CRLF>
Note that the command ignores case, rather than being UPPERCASE, and that the argument can include spaces. (e.g. filenames can include embedded spaces).
You should have a well defined and consistent approach to handling paths. Yes, SPIFFS stores filenames in a single flat directory but "/" is still a vaild filename character and I still use it to create logical subdirectories or namespaces. Also NodeMCU supports the FatFS which very much uses a directory separator.
Lay your code out logically and for readability as per my example. Comments are good. Structure is good.
Also note my technique for splitting a ~20-long if cascade into 3 more manageable sections: bare commands, simple commands, complex commands. The Lua pattern matching algo doesn't support PCRE-style alternatives, but using the string.find() function to search for " PWD " in " CDUP NOOP PASV PWD QUIT " is a fast way of doing this.
Always look at the luac.cross -l output, and grepping this for GLOBAL is a good way to see if you've missed local declarations or are not local-caching some global that you should. Grepping for SETUPVAL is a good way to validate where you are using upvals to store state, as is SETTABLE.
Also note my use of send() as a piece of peephole optimisation. This 9 instruction function saves typically 4 instructions inline per send and also give a single point where you can add debug diagnostics if you need to.

Anyway have a look at this coding example and pick / choose anything that you feel might enhance your coding and implementation.

Best regards Terry

HHHartmann · 2018-04-21T08:30:19Z

Hi @NeiroNx first of all thx for the great work.
Just wanted to ask if you continue work on this. I have a fixe for the LIST commands and for STOR which only works for short files.

drawkula · 2018-04-21T08:37:14Z

@HHHartmann ... please publish your changes.

TerryE · 2018-04-21T08:40:47Z

I have been a little concerned about @NeiroNx silence. I've got to get the LFS patch out and then I will be on a Greek island in May / early June. If he hasn't progressed it then I will finish this off then.

HHHartmann · 2018-04-21T08:49:04Z

lua_modules/ftp/ftpserver.lua

+      if data_sock then
+        node.task.post(function() data_fnc(data_sock);data_sock=nil end)
+      end
+      return


To make the list commands work use this data_fnc. tested with 70+ files

data_fnc = function(sd) sd:on("sent", function(cd) if #files > 0 then local file = table.remove(files,1) local size = l and l[file] cd:send( a1 == "NLST" and file.."\r\n" or "-rwxrwxrwx 1 esp esp "..size.." Jan 1 2018 "..file.."\r\n") else l = nil files = nil cd:close() data_fnc = nil c:send("226 Transfer complete.\r\n") collectgarbage() end end) l = file.list(); files = {} for file in pairs(l) do table.insert(files, file) end if a1 == "NLST" then l = nil end local file = table.remove(files,1) local size = l and l[file] sd:send( a1 == "NLST" and file.."\r\n" or "-rwxrwxrwx 1 esp esp "..size.." Jan 1 2018 "..file.."\r\n") end

HHHartmann · 2018-04-21T08:56:55Z

lua_modules/ftp/ftpserver.lua

+          f:close()
+          data_fnc = nil
+        end)
+        c:send("226 Transfer complete.\r\n")


The c:send("226 Transfer complete.\r\n") has to be executed after disconnect only. Else it stops sending the file after a couple of blocks which results in cut off files.

The on disconnection should look loke this: (also note the rename of the function param from c to cd)

sd:on("disconnection", function(cd) f:close() data_fnc = nil c:send("226 Transfer complete.\r\n") end)

HHHartmann · 2018-04-21T09:07:09Z

@TerryE If you would not finish it, is there a way/best practice to hand over a PR to someone else (without commit rights)?
Btw: tested with Win10 file explorer.
Next I will lock into directory emulation to handle the files containing "/" in their names.
With LFS in place the size concern that @NeiroNx had does not matter that much either hopefully.

TerryE · 2018-04-21T09:23:48Z

@HHHartmann Gregor, The committers don't commit their own contributions anyway. Anyone is free to fork nodemcu-firmware or a fork or it and raise a PR.

Let's give NeiroNx another week or so to do the work before anyone takes over. But if you do want to, then have a look at my gist ftpserver.lua for some suggested coding improvements.

TerryE · 2018-06-29T11:43:07Z

I think that @NeiroNx has timed out. I will close this PR and open another based on my re-implementation. No withstanding this, the impetus and original work has been done by NeiroNx so he or she deserves our thanks and full acknowledgment. 👍

Create server.lua

8791710

TerryE reviewed Apr 5, 2018

View reviewed changes

devsaurus reviewed Apr 5, 2018

View reviewed changes

NeiroNx added 3 commits April 5, 2018 18:54

Update and rename server.lua to ftpserver.lua

edaf946

turn it in module like "http"

Create ftpserver-example.lua

2a6f413

Update ftpserver-example.lua

64fbf98

ghost commented Apr 5, 2018

View reviewed changes

HHHartmann reviewed Apr 5, 2018

View reviewed changes

ghost changed the title ~~Create server.lua~~ Create ftpserver.lua Apr 5, 2018

Update ftpserver.lua

c011da5

Update file reading algoritm - reading chunk before sending and nil it after send called.

marcelstoer added the new module label Apr 5, 2018

marcelstoer added this to the 2.2-follow-up milestone Apr 5, 2018

Create README.md

212ee81

devsaurus reviewed Apr 5, 2018

View reviewed changes

Fix small spelling & grammar issues

b707ab1

marcelstoer reviewed Apr 5, 2018

View reviewed changes

Add link to SPIFFS documentation

3281e28

devsaurus reviewed Apr 5, 2018

View reviewed changes

NeiroNx added 2 commits April 6, 2018 09:43

Update ftpserver.lua

12fbd5f

Update ftpserver.lua

7628537

Fix "early connect" logic

FrankX0 suggested changes Apr 7, 2018

View reviewed changes

Update ftpserver.lua

22b1736

Add SIZE and SYST commands, Fix "cwd" into file. Next stage will be check with file.stat() Test with "Chrome" and "ES Explorer"

Update ftpserver.lua

3ef850d

remove debug print

HHHartmann requested changes Apr 21, 2018

View reviewed changes

devsaurus mentioned this pull request Apr 23, 2018

LFS evaluation version -- second release #2301

Merged

4 tasks

marcelstoer removed this from the 2.2-follow-up milestone Jun 2, 2018

TerryE closed this Jun 29, 2018

TerryE mentioned this pull request Jul 2, 2018

New FTP module #2417

Merged

4 tasks

Create ftpserver.lua #2345

Create ftpserver.lua #2345

Conversation

ghost commented Apr 5, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TerryE commented Apr 5, 2018

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eku commented Apr 5, 2018

marcelstoer commented Apr 5, 2018

ghost commented Apr 5, 2018 • edited by marcelstoer Loading

drawkula commented Apr 5, 2018

devsaurus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsaurus left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsaurus commented Apr 6, 2018

FrankX0 commented Apr 6, 2018

devsaurus commented Apr 6, 2018

FrankX0 commented Apr 6, 2018

FrankX0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Apr 8, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

ghost commented Apr 8, 2018

TerryE commented Apr 9, 2018 • edited by marcelstoer Loading

HHHartmann commented Apr 21, 2018

drawkula commented Apr 21, 2018

TerryE commented Apr 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HHHartmann commented Apr 21, 2018

TerryE commented Apr 21, 2018

TerryE commented Jun 29, 2018

ghost commented Apr 5, 2018 •

edited by ghost

Loading

ghost commented Apr 5, 2018 •

edited by marcelstoer

Loading

devsaurus left a comment •

edited

Loading

ghost Apr 8, 2018 •

edited by ghost

Loading

TerryE commented Apr 9, 2018 •

edited by marcelstoer

Loading