[Mono-devel-list] [PATCH] System.Net.FtpWebRequest

[Martin Hinks]

Hi Miguel,

I have read your comments and will make some changes...

1.) ABOR - yes, you are write - the Out-of-band thing only applies if
there is a connected data socket correct?

2.) I'll add the code to send TYPE commands for mode

3.) MS.NET states that you cannot change the properties while an
operation is in progress and that a InvalidOperationException should
be thrown, so this is the correct behaviour.

4.) I am not sure what I should do about broken comms, MS.Net does not
specify what action it will take in the MSDN docs if it loses the
connection, but, seeing as the rest of the spec is using
ProtocolViolationExceptions when there is a problem it will probably
throw one of these, maybe I will do that for now.

5.) Hehe, sorry for the XML comments then, damn! that took ages :p

6.) As regards my .RegisterPrefix comment, when you write a pluggable
protocol using WebRequest you have to register the prefix, so at the
moment you have to do WebRequest.RegisterPrefix("ftp:") and ("ftps")
in the application using System.Net.FtpWebRequest. This is incorrect
behaviour as can be seen using the HttpWebRequest where it is NOT
necessary to register "http" as it is pre-registered somewhere... I'll
do a post asking about it.


Thanks for the feedback, I'll make those changes and check out the
source you sent, hopefully I can get my contribution used :)

Martin

On 6/26/05, Miguel de Icaza <miguel at ximian.com> wrote:
> Hello,
> 
>   Excellent work Martin.
> 
>   I have attached a few comments:
> 
> > 4.) I did XML doc for it all - don't know how much that helps....
> 
> In Mono we tend not to use the inline-XML documentation, and instead we
> put the documentation on a separate file.   Could you resend the code
> without XML comments?
> 
> The comments we can reuse in Monodoc (install your new libraries, and
> run the monodocer command).
> 
> > 5.) I haven't modified it so that you don't need to do .RegisterPrefix
> > (and am not sure how either)
> 
> Am confused about that.
> 
> > The main classes (FtpAsyncResult, FtpWebRequest, FtpWebResponse, FtpEnums)
> > FtpTest.cs - a usage example - takes ftp:// or ftps:// as cmd line arg
> > and connects, lists the dir etc...
> > FtpWebRequestTest.cs - a VERY basic test class - it will test you
> > didn't completely break it but not much else yet - I'll do more later.
> 
> I have not looked very closely to the code, but a few things are worth
> mentioning:
> 
>        * The abort code does not prepare the socket to send an
>          Out-of-Band message before it sends the abort.
> 
>        * The abort code I think is "ABOR", not "ABORT"
> 
>        * Although there are properties for setting binary transfer,
>          there does not seem to be any code to actually change the
>          transfer mode, since the default in most servers is ASCII
>          this probably will cause trouble.
> 
>        * When changing properties of a connection, maybe the code
>          should queue changes (unless the MS.NET implementation does
>          it differently), for example, it should be possible to
>          change the binary mode while a transfer is in progress.
> 
>          On the next operation, it could send all of the pending
>          changes down the line.
> 
>        * The code does not seem to handle broken communications,
>          for instance, consider the "SendCommand" routine, it
>          should probably loop around:
> 
>           net.Write(sendBuffer, 0, sendBuffer.GetLength(0));
> 
>          And catch errors;  And if an error happens while sending
>          the command or receiving the reply, it should throw the
>          proper exception (or try to recover, not sure what MS
>          does here).
> 
> I have attached the source code to the ftp client that is used in the
> Midnight Commander which might be useful as a reference for
> miss-behaving ftp sites.  The code has extra stuff that you can safely
> ignore, but the underlying ftp client should be a good reference.
> 
> Miguel.
> 
> 
> 


-- 
Martin Hinks