22299 - FTP support for VMS

Status: VERIFIED FIXED

(Core Graveyard :: Networking: FTP, defect, P3)

Description

[Judson Valeski]

I'm pushing this off as an RFE. Jim, does IBM need this?

Comment 1

[Jim Dunn]

I asked and never got a response... so from my end, don't worry
about it

Comment 2

[Judson Valeski]

*** Bug 27979 has been marked as a duplicate of this bug. ***

Comment 3

[Gagan]

robert-- welcome to necko!

Comment 4

[Gagan]

->nobody.

Comment 5

[benc]

mass move, v2.
qa to me.

Comment 6

[benc]

If this is 4xp parity, someone please let me know.

Comment 7

[Judson Valeski]

as scary as it is... it is.

Comment 8

[Doug Turner (:dougt)]

this may be causing a crash... move to my plate to investigation.

Comment 9

[Doug Turner (:dougt)]

This would be something easy to pump out given the time.  

Comment 10

[Mike Kaply [:mkaply]]

Can I ask a silly question?

Why is this so hard to support given that all the code to 
do it is in mkftp.c in 4.x?

Comment 11

[Doug Turner (:dougt)]

Mike, no one ever said it was hard.  The problem is time.  Feel free to submit a
patch.

Comment 12

[benc]

and a VMS host (or a qa contact:)

Comment 13

[Doug Turner (:dougt)]

*** Bug 84472 has been marked as a duplicate of this bug. ***

Comment 14

[Mike Kaply [:mkaply]]

Info from our TCP/IP team about why Mozilla can't connect to an OS/2 FTP 
server. Might be relevant to the OpenVMS problem:

How Netscape V4.6 works:
After the initial logon sequence Netscape V4.6 client issues a SYST command and 
waits for the server to respond back with its Operating System details.  Then 
the client issues a command "PASV" for the passive connection. The server 
responds back with its new port number for data connection.At this point of time 
 a handshake takes place between the client and the server over the data ports. 
Later when the client issues a "NLST" command to list the directory , the data 
flows from the new data connection already established between the client and 
the Server.

How Netscape V6 works:

After the initial logon sequence  Netscape V6.0 client issues a SYST command and 
waits for the server to respond back with its Operating System details.  Then 
the client issues a command "PASV" for the passive connection. The server 
responds back with its new port number for data connection. Here there is a 
change in the way the client responds . The client does not establish a data 
port connection here but just stores the Server port numbers for later use.  
Later when the client issues a "NLST" command to list the directory , it 
suddenly issues a SYN to the Server port number that it had initially stored and 
tries establishing a data port connection. 
 
The difference is that Netscape V4.6 establishes a data port connection just 
after receiving the Server port number after the client issues "PASV" command. 
But Netscape V6.0 does not establish a connection at that point of time but 
waits till it sends a command to list the directory.

The Command that Netscape client sends to OS/2 to list the directory is "NLST" . 
The client should send a SYN just after issuing "NSLT" command which it does not 
do. The client should initiate the connection form his side.
Netscape V6.0 works fine with Linux, AIX and Windows because the command that it 
uses to list the directory is " LIST" instead of "NLST" for OS/2.  And in case 
of other Operating Systems it issues a SYN immediately after sending a "LIST"  
which it does not do after "NLST".

We changed our code to see it our above understanding was correct or not. What 
we did was when the client issues a SYST request , instead of sending the 
Operating system string as OS/2 we forced out code to send the OS as 
Windows/UNIX . The directory listing came up properly when we said the OS was 
Windows. In this case after the "LIST" command Netscape 6.0 did send a SYN to 
establish a connection.

Comment 15

[Mike Kaply [:mkaply]]

More info after working with doug on this.

We added OS/2 to the check in R_syst and set it to FTP_NT_TYPE.

From here, we could FTP to subdirectories on the server if we knew them, but we 
got "You are not authorized to see the "/" directory" if we tried to login with 
just the hostname (no subdir)

Once I got listings up from a subdir, they were parsed incorrectly for display. 
I was able to fix this by using NLST instead of LIST in S_list.

I tried making the VMS ftp server behave as NT, but it just crashed the browser. 
Looks like we have list parsing issues.

Comment 16

[Bradley Baetz (:bbaetz)]

mkaply: see bug 84242. Could that be related?

Comment 17

[Doug Turner (:dougt)]

reassigning to [e-mail].

Comment 18

[Bradley Baetz (:bbaetz)]

*** Bug 100812 has been marked as a duplicate of this bug. ***

Comment 19

[benc]

server URL from dupe:
ftp://ftp.montagar.com

Comment 20

[benc]

RELNOTE for 6.2:
FTP does not work with most VMS servers in this release.

Comment 21

[benc]

testcase:
site from another dupe: ftp://kuhub.cc.ukans.edu/

There was a better error dialog in Bug 84472. What do I need to do to get it 
in? If we put that in, I can get rid of yet another release note entry.

Comment 22

[Bradley Baetz (:bbaetz)]

->future, +helpwanted

Comment 23

[Bradley Baetz (:bbaetz)]

*** Bug 134085 has been marked as a duplicate of this bug. ***

Comment 24

[Matthias Versen [:Matti]]

*** Bug 138278 has been marked as a duplicate of this bug. ***

Comment 25

[sujay]

cc: myself, we just got in a bug report saying that ftp publish does not
work for OpenVMS.

Comment 26

[Colin Blake]

I can fix this one. I'd have fixed it ages ago if I'd seen it. Why wasnt' the OS
set to OpenvMS!!!!

Comment 27

[Matthias Versen [:Matti]]

because you get the error messages on all Platforms if you connect to an openVMS 
server ?

Comment 28

[Bradley Baetz (:bbaetz)]

Probably because its not the client which is openvms, but the server - it
affects all client OSs

Publish should be easy - I have a bug on me to clean up when we pop up the
'don't support Foo' dialog, since what it really means is 'don't support
directory listings', and so we could publish, and even see the results
afterwards. Once that is fixed, publish shoulw just work.

There are a lot of vms hacks in the old mozclassic code to deal with this - even
ncftp won't handle the <n>;name style of names. OTOH, this is probably easier
since I changed the backend to support a displayed name different to the
viewable name.

Comment 29

[Colin Blake]

If you can let me know what needs fixing for publish to work, I'll start with 
that. Then for directory listing to work we need some code to parse a VMS server 
directory listing, is that the deal?

Comment 30

[Bradley Baetz (:bbaetz)]

In the ftp state machine in nsFtpConnectionThread.cpp, move the call to send
SYST to just before we do a CWD in preparation for LIST.

ie replace FTP_S_CWD with FTP_S_SYST, and FTP_S_SYST with FTP_S_PWD. Then change
teh error message to reflect tha this only affects directory listings.

I know I've mentioned doing this in a bug, somewhere, but I can't find it.

Either file a new bug, or use bug 95590, I guess. Probably a new one.

Its reaonably easy to add a new type to be parsed - check out
netwerk/streamconv/converters/nsFTPDirListingConv.{cpp,h}

Comment 31

[Colin Blake]

Navigator V3 and V4 both had support for OpenVMS FTP servers. Of course, not 
being at Netscape/AOL at the moment I don't have access to this code. But this 
is what I need in Mozilla!!

What's the best way to go about getting this existing code into Mozilla? 
Shipping 1.0 without it is a regression.

Comment 32

[Mike Kaply [:mkaply]]

Here is the code:

http://lxr.mozilla.org/classic/source/network/protocol/ftp/mkftp.c

It was contributed to Mozilla.

Comment 33

[Colin Blake]

Attachment: Basic support for VMS FTP server

This problem was worse than I thought. Not only couldn't I publish, but I
couldn't even download a file via FTP.

The patch provides basic support for a VMS FTP server. With this I can publish
and download files from a VMS FTP server. What I can't do yet is "browse" via
FTP (ie. get directory listings). I believe I need to write a stream converter
for this, but don't have time right now. So instead of the
"scs->AsyncConvertData failed" message, for now I have prevented it from even
trying the RETR on the dirspec, and so a 550 error is displayed. This seems a
reasonable temporary measure.

I'm leaving for vacation tonight and won't be back until the middle of next
week. I'd REALLY like to get this into RC2 so I'd appreciate some reviews. And
if some kind soul would like to check it in perhaps......

Comment 34

[Bradley Baetz (:bbaetz)]

Do we have to have the VMS-fu everywhere? I suppose so...

See my comment 30 for how to provide a better error for directory listings. Oh.
I just realised that that won't work, since you have to care about the type when
converting the file. Bleh. There goes that iea, I think. Is that correct?

Comment 35

[Colin Blake]

Who owns the FTP code? I'd like to get a review.

Comment 36

[Doug Turner (:dougt)]

Bradley does.  If he is too busy, ask him to delegate.

Comment 37

[Bradley Baetz (:bbaetz)]

That would sort of be me - see my comment #34 + questions, though. Is there a
cleaner way to do this?

Comment 38

[Colin Blake]

I thought you answered your own questions in 34 :-)

Comment 39

[Bradley Baetz (:bbaetz)]

Well, I was mainly musing, true.
Is there no way to get a vms server to, when asked for the contents of a
directory, give file names which actually are the names of files we can use for
RETR/LIST?

Comment 40

[Colin Blake]

This patch isn't dealing with directory formats; that work will require a stream 
converter (and no, you can't get an OpenVMS FTP server to generate "UNIX output" 
(I believe the spec says the output is platform specific so we're not broken)). 
This patch is just dealing with uploading files.

Comment 41

[Bradley Baetz (:bbaetz)]

Comment on attachment 80790 [details] [diff] [review]
Basic support for VMS FTP server

r=bbaetz. Yeah, the ftp spec's lack of requirements about most things is a real
pain...

Comment 42

[Scott Collins]

Comment on attachment 80790 [details] [diff] [review]
Basic support for VMS FTP server

``looks clear'' sr=scc

Comment 43

[Bradley Baetz (:bbaetz)]

colin: Do you have cvs access, or do you want me to check this in?

Comment 44

[Colin Blake]

Bradley: I have CVS access, but I'm still waiting for driver approval for 1.0 
branch checkin (I sent mail the other day). I guess it could go into tip now, 
only I don't have a tip tree I'm building at the moment. Could you check it in 
to tip, or I'll do it in a day or two.

Comment 45

[Randell Jesup [:jesup] (needinfo me)]

Let's get this into trunk NOW.  No reason to wait for branch approval, which may
not come anyways, plus anything baked in trunk is considered more stable for
branch inclusion.

Comment 46

[Colin Blake]

I don't have a trunk build at the moment, so I can't make this change right now.
In a few days when I have an up to date trunk build I'll merge in my changes and
submit a new patch (the FTP code has changed).

Comment 47

[Bradley Baetz (:bbaetz)]

-> patch author

colin: I don't have time this week to merge this stuff, sorry.

Comment 48

[Colin Blake]

Well, the branch has changed too since I made my patch, so I've merged and will
be testing a new patch once my build is done (tomorrow). Branch and trunk now
differ by only an nsUnescape/NS_Unescape call - off to track down the difference.

Comment 49

[Colin Blake]

Attachment: Updated patch. Basic support for 1.0 branch.

The 1.0 branch code in nsFtpConnectionThread.cpp has been modified since I
posted patch 80790. This is an updated version of the previous patch.

Comment 50

[Colin Blake]

Attachment: Patch for trunk

Here's a patch for the trunk. I only have a branch build environment at the
moment so I'm unable to test this patch, but if some kind soul could test it
and check it in, that'd be great.

Comment 51

[Bradley Baetz (:bbaetz)]

*** Bug 151501 has been marked as a duplicate of this bug. ***

Comment 52

[Colin Blake]

Attachment: Same patch but for current trunk

Sorry for the delay on this. I finally have a trunk build that I've been able
to test this fix with. I'm attaching a revised patch but it should be the same
as the provious one.

Since its been a few weeks do I need to get the r=bbaetz and sr=scc re-issued
or can I just go ahead and check in?

Comment 53

[Colin Blake]

Checked in patch 87923 for initial publish support in OpenVMS.

Comment 54

[benc]

Mozilla 1.2.1, Mac OS X

I see errors w/ a "550" now.

Is this a regression of the "unsupported dialog" or some change you might have made?

Comment 55

[Colin Blake]

This is connecting to an FTP server on an OpenVMS system?

Comment 56

[benc]

Yes, I'm using the link in the URL field.

Comment 57

[Jeremy M. Dolan]

The site does still work, I can log in manually:

USER anonymous
331 anonymous user ok. Send real ident as password.
PASS foo@
230-Guest User FOO@ logged into DUA8:[ANON_FTP] at Fri 6-Dec-2002 5:01PM-CST,
job 17e828.
230 Access restrictions apply
PWD
257 "DUA8:[ANON_FTP]" is current directory.

Comment 58

[Colin Blake]

What do you get back from a SYST command?

Comment 59

[Jeremy M. Dolan]

215 VMS MultiNet V4.4(91)

Comment 60

[Doug Turner (:dougt)]

There are at least two problems: first there is a bug in our R_retr handler such
that any VMS listings will fail if RETR fails.  Colin, are you sure we need that
check?  The second problem is that ConvertDirspecToVMS is incorrectly inserting
a . at offset 1 in the dirSpec (colin: i could be wrong about that -- i would
never claim to be a VMS filepath expert).

Comment 61

[Doug Turner (:dougt)]

*** Bug 202387 has been marked as a duplicate of this bug. ***

Comment 62

[benc]

We are about to ship Mozilla 1.4f with a regression vs. Mozilla 1.0.2, where we
give a reasonable error message when connecting to VMS sites.

Is this okay? If implementing VMS directory parsing is not going to happen, we
should back out whatever regressed that friendly dialog box.

Comment 63

[benc]

+regression from dupe.

Comment 64

[Colin Blake]

Attachment: Patch for full support on OpenVMS

I believe this patch (for the 1.5 tip) provides full FTP support for OpenVMS
servers. At least it does in my testing of surfing around FTP urls on an
OpenVMS server, and in publishing via ftp to an OpenVMS server.

Looking for some reviews.

Comment 65

[Doug Turner (:dougt)]

Comment on attachment 126836 [details] [diff] [review]
Patch for full support on OpenVMS

I was hoping to avoid using the SYST (and mServerType).  Is this possible?

Comment 66

[Colin Blake]

I guess if we're still always doing a PWD at the start of a session, we could
look to see if the returned working directory string is VMS format (search for
":["). That would probably work, but its kinda hacky.

Comment 67

[Doug Turner (:dougt)]

I don't want to use PWD either. See 209972.  

Comment 68

[Colin Blake]

Attachment: OpenVMS patch for Doug's new code

Comment 69

[Doug Turner (:dougt)]

Comment on attachment 126930 [details] [diff] [review]
OpenVMS patch for Doug's new code

stay away from the string "fileSpec".  This bring back memories of the
nsFileSpec class which is obsolete.  s/fileSpec/fileString/g  if possible.


+    if (mServerType == FTP_VMS_TYPE) {
+	 if (!mPath.IsEmpty()) {
+	     ConvertDirspecToVMS(listString);
+	 }
+	 listString.Append(";0");
+    }

Can you have ConvertDirspecToVMS handle empty strings?

     return SendFTPCommand(listString);
+
 }

kill that blank line.

you have some printf's still in this patch.


how much of these functions are new - or - are they just being readded after my
removal?

+    void ConvertFilespecToVMS(nsCString& fileSpec);
+    void ConvertDirspecToVMS(nsCString& fileSpec);
+    void ConvertDirspecFromVMS(nsCString& fileSpec)

why the changes in ParseFTPList.cpp?  We should run these by
[e-mail].

Comment 70

[Cyrus Patel]

Colin's changes to ParseFTPList.cpp are to
a) obtain filesize for listings served by Multinet hosts 
   (num_blocks->filesize conversion by multiplying num_blocks by 512).
b) force non-Multinet hosts (CMU/VMS-IP FTP and older UCX FTP) to 
   use the same filesize conversion as that used for Multinet.

The original code didn't make this conversion (it returned size=""),
because, first, I wasn't sure whether blocksize was always 512; 
second, I was of the opinion that presenting no filesize was 
better than presenting an inaccurate filesize (too large);
third, ftpmirror and squid didn't list a filesize either. 

I have no idea how prevalent such non-Multinet FTP servers are, but I took
that possibility into account at time of writing. Current versions of UCX FTP
serve listings in ls -l style (eg ftp.service.digital.com), so this issue is
moot for them.

(According to Lynx) non-Multinet FTP listings can also be in <bytes>/<blocks> 
or in <used>/<allocated> format. This may be wrong. But if it isn't, Colin's
patch will break listings with the former format.

Even if using _only_ the blocksize format (incidentally, Colin's "#if 0" is 
two lines too low), the conversion needs review. 
a) the conversion needs to consider that the resultant filesize can 
   be larger than a 32bit int, so Colin's code isn't right. 
   And of course, sprintf() isn't NSPR, even if we were to use
   "%llu" instead of "%d". :)
b) retrieving will result in a file that will be "too short" (as 
   compared to the displayed filesize). I don't think anyone will 
   actually compare filesizes, so this is ok IMO, but perhaps it 
   ought to be discussed anyway - someone may have an idea how 
   to obtain a more accurate filesize. 

With respect to determining if the server is a VMS server or not:
You can avoid the use of SYST and PWD by checking the list_state->lstyle 
member on successful return from ParseFTPList(). It will be 'V' for VMS listings.

Comment 71

[Colin Blake]

> stay away from the string "fileSpec".  This bring back memories of the
> nsFileSpec class which is obsolete.  s/fileSpec/fileString/g  if possible.

Done.

> Can you have ConvertDirspecToVMS handle empty strings?

Yes. Done.

> kill that blank line.

Done.

> you have some printf's still in this patch.

Not any more :-)

> how much of these functions are new - or - are they just being readded
> after my removal?
>
> +    void ConvertFilespecToVMS(nsCString& fileSpec);
> +    void ConvertDirspecToVMS(nsCString& fileSpec);
> +    void ConvertDirspecFromVMS(nsCString& fileSpec)

ConvertFilespecToVMS was there before, but I've pretty much rewritten it to make
it work in all cases.

ConvertDirspecToVMS was there before but didn't work too well. I've made it now
use ConvertFilespecToVMS.

ConvertDirspecFromVMS I have removed since it was only needed to convert what
came back from the PWD command.

> why the changes in ParseFTPList.cpp? 

The OWNER test I removed because it was looking for something in the format
"[UIC]" but now we can also have an identifier as the owner (which doesn't have
the [] around it) or a hexadecimal string. Testing for all these different owner
formats would be a pain, so I removed it.

The SIZE change is explained by the comments. The SIZE always comes back in
blocks and a block is always 512 bytes.

The DATE change was needed because we were passing in years such as 02 and 03,
and NSPR wants four digits such as 2002 and 2003.

Comment 72

[Colin Blake]

Cyrus:

> The original code didn't make this conversion (it returned size=""),
> because, first, I wasn't sure whether blocksize was always 512; 
> second, I was of the opinion that presenting no filesize was 
> better than presenting an inaccurate filesize (too large);
> third, ftpmirror and squid didn't list a filesize either. 

The filesize is always going to be a multiple of 512 and will therefore be
slightly high most of the time. But I figured its better to tell someone that a
file is 1K or 100MB, even if it is off by a few bytes.

> I have no idea how prevalent such non-Multinet FTP servers are, but I took
> that possibility into account at time of writing. Current versions of UCX FTP
> serve listings in ls -l style (eg ftp.service.digital.com), so this issue is
> moot for them.

UCX is probably more common than Multinet and the rest, these days.

UCX doesn't list in "ls -l" format (ftp.service.digital.com is a UNIX FTP server!!).

> (According to Lynx) non-Multinet FTP listings can also be in <bytes>/<blocks> 
> or in <used>/<allocated> format. This may be wrong. But if it isn't, Colin's
> patch will break listings with the former format.

I have never seen anything other than <used>/<allocated> 

Comment 73

[Colin Blake]

> With respect to determining if the server is a VMS server or not:
> You can avoid the use of SYST and PWD by checking the list_state->lstyle 
> member on successful return from ParseFTPList(). It will be 'V' for VMS
> listings.

No can do. You don't get a successful return from ParseFTPList() until you've
passed a valid LIST command to the server, and for OpenVMS this needs to be in
OpenVMS format. Chicken and egg....

Comment 74

[Colin Blake]

The three main (almost only) FTP servers on VMS these days are:
  - TCP/IP Services for OpenVMS (UCX)
  - Multinet
  - TCPware

These are the three I have been testing with this morning.

I found that TCPware puts 1/100th seconds on the timestamp, so I had to change
the parser to allow for that.

Multinet does a UNIX emulation these days, so the VMS code isn't even involved.

I'll post a new patch next.

By the way, I found a BSD system that doesn't work with the new scheme. Doug,
can you try ftp://ftp.digital.com/ and see if it works for you. For me it just
hangs.

Comment 75

[Colin Blake]

Attachment: Incorporate feedback and add support for TCPware.

Comment 76

[Colin Blake]

I would like to get this checked in to the trunk. The sooner its in the more
testing it will get and it would appear that we've already missed the 1.5a boat.
Its got to be better than what's currently in Mozilla (no support at all) and we
can fix bugs as they are found.

Comment 77

[Doug Turner (:dougt)]

Comment on attachment 127353 [details] [diff] [review]
Incorporate feedback and add support for TCPware.

+	 ConvertDirspecToVMS(listString);
+	 listString.Append(";0");

Can't ConvertDirspecToVMS append the ";0"?

Maybe you should just fixup the mPath in one place, such as nsFtpState::Init? 
I am not sure if you can do this, but it sure would be cleaner.

The ParseFTPList code -- maybe we should just intergrate the latest version?

Comment 78

[Colin Blake]

> Can't ConvertDirspecToVMS append the ";0"?

Sure, I can do that.

> Maybe you should just fixup the mPath in one place, such as nsFtpState::Init? 
> I am not sure if you can do this, but it sure would be cleaner.

Problem is that we don't know how to convert mPath (file or directory) until we
know what we are doing with it.

> The ParseFTPList code -- maybe we should just intergrate the latest version?

You mean Cyrus' latest round of changes? I'd prefer to get in the basic "make it
work" changes I've made, which I've tested with, and then move on to Cyrus'
changes. 

Comment 79

[Cyrus Patel]

I'd agree if we hadn't already missed the boat. Otherwise, I'd prefer we used 
the latest version since it requires changes to nsFTPDirListingConv as well 
for the VMS long filename handling, and are as such part of the VMS fix set.

Comment 80

[Colin Blake]

I'll post a new patch which includes Cyrus's changes, but people will have to
realize that this patch now includes more than just OpenVMS support.

Comment 81

[Colin Blake]

We've missed the 1.5a boat, so once 1.5a is out and I'm building 1.5 trunk again
I'll resubmit my patches based on the current FTP code since things seemed to
change since my previous set of patches.

Doug, sorry to hear that you got hit by last weeks hatchet attack. Are you still
going to be involved in this FTP code?

Comment 82

[Doug Turner (:dougt)]

thanks - i was lucky.  I still will be involved as much as I can be.  

Comment 83

[Colin Blake]

Attachment: Patch for trunk

I finally got some time to get back to this. Gee, the trunk has changed since
my previous patch! Anyway, this patch is for the current 1.5 trunk. Built and
tested on OpenVMS.

Cyrus, this patch doesn't contain your latest changes because I view your work
as separate from mine. Once my changes are in I'll be more than happy to help
you get yours checked in too. I think two smaller checkins is easier to manage
than one big one.

Now, who can review this for me? I don't want to miss the 1.5b boat too :-)

Colin (who is out until next Wednesday).

Comment 84

[Doug Turner (:dougt)]

Comment on attachment 129030 [details] [diff] [review]
Patch for trunk

suppose i have a quote '"' in a file, on non VMS servers, can i still FTP retr
it?


+    p = strdup(fileString.get());

what if this fails?


+#if 0
+     

comment why this is ifdef off.

What kind of testing has been done?

Comment 85

[Colin Blake]

> suppose i have a quote '"' in a file, on non VMS servers, can i still
> FTP retr it?

Unrelated to this patch. All my code is purely for VMS servers.

> What kind of testing has been done?

Tested against TCP/IP Services for OpenVMS (formerly UCX), TCPware, and
Multinet. These are the three major IP stacks available for OpenVMS.

I'll fix the other things and repost the patch.

Comment 86

[Doug Turner (:dougt)]

Comment on attachment 129030 [details] [diff] [review]
Patch for trunk

in R_pwd(), you are now checking for " in the response.  unless, i am missing
something :-), i think my question is spot on.

Comment 87

[Doug Turner (:dougt)]

Comment on attachment 129030 [details] [diff] [review]
Patch for trunk

in R_pwd(), you are now checking for " in the response.  unless, i am missing
something :-), i think my question is spot on.

Comment 88

[Colin Blake]

> in R_pwd(), you are now checking for " in the response.  

That test for " was always there. I haven't added it.

Comment 89

[Doug Turner (:dougt)]

Comment on attachment 129030 [details] [diff] [review]
Patch for trunk

r=dougt

Comment 90

[Colin Blake]

Attachment: Incorporate Doug's comments

Doug, this version of the patch doesn't use strdup and has better comments
explaining why a section of code is ifdef'd out. Can you take a look and then
carry forward your r= from the previous patch. Thanks.

Comment 91

[Colin Blake]

Adding 1.5b flag. Originally I tried to get this into 1.5a but failed because
the FTP code was a moving target. If I don't get it in for 1.5b then I probably
won't be allowed to check it in for 1.5 final, and that'll likely mean we'll
never seen FTP support for OpenVMS in Mozilla "the suite".

Comment 92

[Darin Fisher]

Comment on attachment 129437 [details] [diff] [review]
Incorporate Doug's comments

>Index: netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp

>     retrStr.Insert("RETR ",0);

nit: add whitespace between function arguments.  this nit
applies to entire patch.. i see a lot of places where this
should be cleaned up to match existing style.


>+    t = strtok((char *)fileStringCopy.get(),"/");
>+    if (t) while (strtok(NULL,"/")) ntok++; // count number of terms (tokens)
...
>+            fileString.Append(strtok(NULL,"/"));

strtok should never be used because it is not re-entrant safe,
and mozilla may be used in a multithreaded environment.  e.g.,
someone could even spawn more than one gecko instance on its
own primordial thread in the same address space.  please use
nsCRT::strtok instead.


> nsFtpState::ConvertDirspecToVMS(nsCString& dirSpec)
> {
>+    PR_LOG(gFTPLog, PR_LOG_DEBUG, ("(%x) ConvertDirspecToVMS from: \"%s\"\n", this, dirSpec.get()));
>+    if (!dirSpec.IsEmpty()) {
>+        if (dirSpec.Last() != '/') dirSpec.Append("/");

nit: insert line break here...

	  if (dirSpec.Last() != '/')
	      dirSpec.Append('/');

>+        // we can use the filespec routine if we make it look like a file name
>+        dirSpec.Append("x");

nit:	  dirSpec.Append('x');


>+nsFtpState::ConvertDirspecFromVMS(nsCString& dirSpec)
>+{
>+    PR_LOG(gFTPLog, PR_LOG_DEBUG, ("(%x) ConvertDirspecFromVMS from: \"%s\"\n", this, dirSpec.get()));
>+    if (dirSpec.IsEmpty()) {
>+        dirSpec.Insert(".",0);
>+    }
>+    else {
>+        dirSpec.Insert("/",0);
>+        dirSpec.ReplaceSubstring(":[","/");
>+        dirSpec.ReplaceChar('.','/');
>+        dirSpec.ReplaceChar(']','/');
>+    }
>+    PR_LOG(gFTPLog, PR_LOG_DEBUG, ("(%x) ConvertDirspecFromVMS   to: \"%s\"\n", this, dirSpec.get()));
> }

nit: insert a space between function arguments, e.g.:

  dirSpec.Insert(".", 0);

oh, and use the character versions of the string methods when possible, e.g.:

  dirSpec.Insert('.', 0);


>Index: netwerk/streamconv/converters/ParseFTPList.cpp

>         else if (  (toklen[2] == 10 || toklen[2] == 11) &&      
>                         (tokens[2][toklen[2]-5]) == '-' &&
>                         (tokens[2][toklen[2]-9]) == '-' &&
>+        (((toklen[3]==4 || toklen[3]==5 || toklen[3]==7 || toklen[3]==8) &&
>+                        (tokens[3][toklen[3]-3]) == ':' ) ||
>+         ((toklen[3]==10 || toklen[3]==11 ) &&
>+                        (tokens[3][toklen[3]-3]) == '.' )
>+        ) &&  /* time */

some kind of comment explaining this conditional would be nice :)


>+// I've never seen size come back in bytes, its always in blocks, and 
>+// the following test fails. So, always perform the "size in blocks".
>+// I'm leaving the "size in bytes" code if'd out in case we ever need
>+// to re-instate it.

use /* */ style comment block instead (stick with prevailing style).


>                * ulltoa(((unsigned long long)fsz)<<9, result->fe_size, 10);
>               */
>+              // A block is always 512 bytes on OpenVMS, compute size.
>+              // So its rounded up to the next block, so what, its better
>+              // than not showing the size at all.
>+              // A block is always 512 bytes on OpenVMS, compute size.
>+              // So its rounded up to the next block, so what, its better
>+              // than not showing the size at all.

nit: prevailing comment style is /* */, so please stick with it.  in
fact, why don't you just continue the previous comment block?


>+              PRUint64 fsz = (PRUint64) strtoul(tokens[1], (char **)0, 10);
>+              LL_MUL(fsz, fsz, 512);
>+              PR_snprintf(result->fe_size, sizeof(result->fe_size), "%lld", fsz);

  PRUint64 fsz, factor;
  LL_UI2L(fsz, strtoul(tokens[1], (char **)0, 10));
  LL_UI2L(factor, 512);
  LL_MUL(fsz, fsz, factor);
  ...

Comment 93

[Colin Blake]

Attachment: Incorporate Darin's comments

I think I've addressed all your comments, Darin, except for putting the space
between the args on some of the lines of code like this:

dirSpec.Insert(".",0);

The lines I added I did insert a space. But many of the lines in my patch I
didn't add, I only moved existing lines. Those ones I didn't change since I
figured leaving them as is was as close to prevailing style as I could get :-)

Comment 94

[Darin Fisher]

Comment on attachment 129797 [details] [diff] [review]
Incorporate Darin's comments

>Index: netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp

>+            if (respStr.Last() != '/')
>+                respStr.Append("/");

nit:		  respStr.Append('/');


yeah, the coding style was already inconsistent... oh well :-/

sr=darin

Comment 95

[Asa Dotzler [:asa]]

Colin, can you take this fix into your build and then land the patch into cvs in
1.6alpha?

Comment 96

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Asa, this bug is for Mozilla support for VMS FTP *servers*, not for VMS-Mozilla.
Please reconsider.

Comment 97

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 129797 [details] [diff] [review]
Incorporate Darin's comments

I think Darin has hit all the issues.  Couldn't find any holes in the
string-handling that might cause problems.  r=rjesup

Comment 98

[chris hofmann]

Comment on attachment 129797 [details] [diff] [review]
Incorporate Darin's comments

a=chofmann for 1.5b..  need to get this in quickly if we are going to take for
beta/final..  dougt can you also do a review on this if you get a chance just
to make sure we don't potencially set up a regression late in the game for
1.5b?

Comment 99

[Colin Blake]

Its checked in! Thanks guys - that was unexpected.

Comment 100

[Colin Blake]

Marking fixed.

Comment 101

[benc]

VERIFIED/MacOS:
1.5RC1 works, 1.5a did not.

Comment 102

[benc]

V/fixed: Mozilla 1.7RC2, Linux

Comment 103

[benc]

V/fixed: mozilla 1.7.2, win xp

Comment 104

[Pulsebot]

Pushed by ffxbld-merge:
https://hg.mozilla.org/releases/mozilla-beta/rev/4eab77f5b13b
[fenix] Close https://github.com/mozilla-mobile/fenix/issues/22299: Add history search term group telemetry