209972 - Reduce FTP states

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[Doug Turner (:dougt)]

I think we can remove two states (two round trips) from the FTP state machine.  

CWD can be removed if we use the optional parameter path parameter of LIST
command.  There is no need to have the login actually change directory.  So:

CWD path
LIST

Becomes 

LIST path  


We can also remove PWD since everything should be determined by the path part
(+1) of the url.  For example:

ftp://server.com/file   - means get file in the CWD of the login
ftp://server.com//file  - means get the file at /file

All paths should be relative to the CWD.  There is no need to prepend the PWD to
any request.  For example:

LIST <pwd>/file 

Becomes 

LIST file

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

first draft.  I think I can also remove one more state - details later.

Comment 2

[Darin Fisher]

Comment on attachment 126038 [details] [diff] [review]
patch v.1

this patch makes sense to me.  can we eliminate the code for the CWD state
then?

Comment 3

[Doug Turner (:dougt)]

yeah, we can.  I guess, i kept it cause i have (very) long time plans to make
ftp more controlable via some low level API.  i guess we always have CVS for
history.

Comment 4

[Doug Turner (:dougt)]

Attachment: patch v.2

Remove SYST too.

Comment 5

[Darin Fisher]

Comment on attachment 126070 [details] [diff] [review]
patch v.2

can you explain why we want to remove SYST as well?

Comment 6

[Doug Turner (:dougt)]

of course...

SYST use to be used to determine how exactly to parse the LIST response.  Some
time in 1.2 (iirc), we landed a new parser that can determine the LIST response
format solely based on the response -- there was no longer a need to know about
the server type.  Today, it continues to be in the code because Colin's support
VMS (which is currently broken) depends on it.  I spoke with Colin, and we think
we don't need the server type anymore to handle VMS.  Since it is broken today,
we agreed to just nix it for now.

Comment 7

[Bradley Baetz (:bbaetz)]

We can't do this, because we need to CWD each component separately, since we
don't nkow the path separator. Thats in theoryl in practice may be different.
Andreas?

Comment 8

[Doug Turner (:dougt)]

we don't do that today.  currently we just CWD <path>, then do a <LIST>.  

Comment 9

[Andreas Otte]

I think we should try the patch against all those special fixed ftp bugs first.
There was a reason for the pwd-stuff if I remember correctly. 

Comment 10

[Doug Turner (:dougt)]

no doubt.  this is a major change, much testing is required.

Comment 11

[Bradley Baetz (:bbaetz)]

Could have been os2, perhaps?

Comment 12

[Doug Turner (:dougt)]

Bradley, nope. for all server types, we issue complete paths to CWD.  For
example ftp://example.com/blah/a/

will do this series of commands:

PWD
CWD <pwd result>/blah/a
LIST

This patch makes us do:

LIST blah/a

Comment 13

[benc]

What is the goal of this? Is there some advantage to doing it this way?

Comment 14

[Doug Turner (:dougt)]

faster ftp operations, of course.

Comment 15

[Darin Fisher]

Comment on attachment 126070 [details] [diff] [review]
patch v.2

>Index: nsFtpConnectionThread.cpp

>     nsCAutoString           mControlReadCarryOverBuf;

shouldn't this be a nsCString?

sr=darin

Comment 16

[Doug Turner (:dougt)]

probably.  figure i would leave it since I wasn't completely sure which was
better.  

Comment 17

[Kathleen :Brade]

I asked dougt this question right before he was leaving for lunch so I'll pose
it here:
+    nsCAutoString listString;
+    if (mPath.Length() == 0)
+        listString = "LIST" CRLF;
+    else {
+        listString = mPath;
+        listString.Insert("LIST ",0);
+        listString.Append(CRLF);
+    }

For the above block, I'd use "IsEmpty" instead of "Length() == 0".
Also, I wondered why the else block is ordered that way.  Wouldn't it be better
to assign "LIST " and then append path and crlf?

Comment 18

[Doug Turner (:dougt)]

I could use this:

    nsCAutoString listString = "LIST";
    if (!mPath.IsEmpty())
        listString.Append(" ");
        listString.Append(mPath);
    }    
    listString.Append(CRLF);

Comment 19

[Doug Turner (:dougt)]

and something that compiles:

    nsCAutoString listString("LIST");
    if (!mPath.IsEmpty()) {
        listString.Append(" ");
        listString.Append(mPath);
    }    
    listString.Append(CRLF);

Comment 20

[Doug Turner (:dougt)]

Attachment: patch v.3

This fixes STOR to use the same path as RETR.

Comment 21

[Kathleen :Brade]

Comment on attachment 126567 [details] [diff] [review]
patch v.3

you might want to reorder these so mIPv6Checked and mSuspendCount are next to
each other:

+    PRUint8		       mSuspendCount;// number of times we've been
suspended.
+    nsCOMPtr<nsIInputStream>  mWriteStream; // This stream is written to the
server.
+    PRUint32		       mWriteCount;
+    PRPackedBool	       mIPv6Checked;

Comment 22

[Doug Turner (:dougt)]

yup.  thanks.

Comment 23

[Bradley Baetz (:bbaetz)]

Comment on attachment 126567 [details] [diff] [review]
patch v.3

This looks fine. Make sure to test file upload, as well as an os2 server.

Comment 24

[Doug Turner (:dougt)]

Attachment: final patch

this adds back SYST state inorder to support colin's VMS patch.

Comment 25

[Doug Turner (:dougt)]

Checked in:

Checking in nsFTPChannel.cpp;
/cvsroot/mozilla/netwerk/protocol/ftp/src/nsFTPChannel.cpp,v  <--  nsFTPChannel.cpp
new revision: 1.129; previous revision: 1.128
done
Checking in nsFtpConnectionThread.cpp;
/cvsroot/mozilla/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp,v  <-- 
nsFtpConnectionThread.cpp
new revision: 1.259; previous revision: 1.258
done
Checking in nsFtpConnectionThread.h;
/cvsroot/mozilla/netwerk/protocol/ftp/src/nsFtpConnectionThread.h,v  <-- 
nsFtpConnectionThread.h
new revision: 1.104; previous revision: 1.103
done

Comment 26

[Michael Lefevre]

I suspect this may be the cause of bug 211729

CWD path-to-symlink
LIST

is apparently different from 

LIST path-to-symlink

- one lists the linked directory, the other shows the directory entry for the link.

Comment 27

[Jesse Ruderman]

From bug 212439, it sounds like this was backed out completely.  Should it be
reopened?

Comment 28

[Doug Turner (:dougt)]

reopened to mark as wontfix, if you really want to.