Re: cvs commit: squid3/src ftp.cc

[Henrik Nordstrom]

On ons, 2007-08-15 at 09:28 +1200, Amos Jeffries wrote:
> >> > > If you have time, it may be better to replace a virtual
> >> > > FtpStateData::haveControlChannel(char*) into a static
> >> > > FtpStateData::HaveControlChannel(FtpStateData*, char*) and let it
> >> check
> >> > > the ftpState pointer itself. This will avoid more code repetition
> >> and
> >> > > may even work faster.
> >> >
> >> > Whats with the uppercasing? the method is not a type. Is this a new
> >> > convention not mentioned in the wiki/devel-faq?
> >>
> >> Hmm.. it should be in the style guide somewhere.
> >
> > http://www.squid-cache.org/Devel/squid-3-style.txt
> >
> 
> What I based the following on. Alex posted an upper-cased
> internal-but-public method (NOT data member).

It's a static class function, not an object method, which brings it into
the "Global" naming recommendation.

   static FtpStateData::HaveControlChannel(FtpStateData*, char*)

Regards
Henrik


signature.asc
Description: This is a digitally signed message part

Re: cvs commit: squid3/src ftp.cc

[Amos Jeffries]

> On tis, 2007-08-14 at 23:01 +1200, Amos Jeffries wrote:
>> Alex Rousskov wrote:
>> > On Mon, 2007-08-13 at 12:01 +0200, Guido Serassio wrote:
>> >
>> >> Now fixed.
>> >
>> > If you have time, it may be better to replace a virtual
>> > FtpStateData::haveControlChannel(char*) into a static
>> > FtpStateData::HaveControlChannel(FtpStateData*, char*) and let it
>> check
>> > the ftpState pointer itself. This will avoid more code repetition and
>> > may even work faster.
>>
>> Whats with the uppercasing? the method is not a type. Is this a new
>> convention not mentioned in the wiki/devel-faq?
>
> Hmm.. it should be in the style guide somewhere.
>
> object local methods and variables start with a lowercase, global
> variables and static class members with an upper case.
>
>> The FTP code is all a it messy still with its partial conversion and
>> overlaps with other protocols code.
>
> Can you be specific on what you think need further conversion or
> overlaps?

Ah, sorry, I said that without taking a good overview look at it (yay
doxygen). At the line-level it seems to have a lot of global stuff. I must
have gotten FTP confused with the HttpRequest usages.

The main gist of my thinking already exists in the form of ServerStateData
for FTP at least.

Alex made some comments earlier about use of temporary buffers in several
of the protocol automata. And here is a short list of currently global
functions which I think should be provided either by the main body of
squid (non-FTP specific) or part of the classes private methods.

 is_month()
 escapeAC()
 ftpOpenListenSocket()
 ftpFail()
 ftpUrlWith2f() - should be part of URL class when its ready
 ftpTrySlashHack()

(maybe others, I haven't looked closely at each one yet)

maybe as ftpListParts open class:
  ftpListPartsFree()
  ftpListPartsParse()


>
>> Ah, quite right. doneWithServer() is only one of the cases that method
>> should handle.
>> I'll be adding the other soon.
>
> And quite likely the whole function is not needed any more after the
> state fixes, but it's a reasonable safeguard so I don't object it's
> addition.

truely.

Amos

Re: cvs commit: squid3/src ftp.cc

[Alex Rousskov]

On Wed, 2007-08-15 at 09:28 +1200, Amos Jeffries wrote:
> > On Tue, 2007-08-14 at 17:58 +0200, Henrik Nordstrom wrote:
> >> On tis, 2007-08-14 at 23:01 +1200, Amos Jeffries wrote:
> >> > Alex Rousskov wrote:
> >> > > On Mon, 2007-08-13 at 12:01 +0200, Guido Serassio wrote:
> >> > >
> >> > >> Now fixed.
> >> > >
> >> > > If you have time, it may be better to replace a virtual
> >> > > FtpStateData::haveControlChannel(char*) into a static
> >> > > FtpStateData::HaveControlChannel(FtpStateData*, char*) and let it
> >> check
> >> > > the ftpState pointer itself. This will avoid more code repetition
> >> and
> >> > > may even work faster.
> >> >
> >> > Whats with the uppercasing? the method is not a type. Is this a new
> >> > convention not mentioned in the wiki/devel-faq?
> >>
> >> Hmm.. it should be in the style guide somewhere.
> >
> > http://www.squid-cache.org/Devel/squid-3-style.txt
> >
> 
> What I based the following on. Alex posted an upper-cased
> internal-but-public method (NOT data member).

The style example and its description are not in sync. The verbal
description should say something like:

  - Global names, including static class members are capitalized.

which would include static class methods like the Instance method in the
style example or the FtpStateData::HaveControlChannel in my suggestions.

HTH,

Alex.

Re: cvs commit: squid3/src ftp.cc

[Amos Jeffries]

> On Tue, 2007-08-14 at 17:58 +0200, Henrik Nordstrom wrote:
>> On tis, 2007-08-14 at 23:01 +1200, Amos Jeffries wrote:
>> > Alex Rousskov wrote:
>> > > On Mon, 2007-08-13 at 12:01 +0200, Guido Serassio wrote:
>> > >
>> > >> Now fixed.
>> > >
>> > > If you have time, it may be better to replace a virtual
>> > > FtpStateData::haveControlChannel(char*) into a static
>> > > FtpStateData::HaveControlChannel(FtpStateData*, char*) and let it
>> check
>> > > the ftpState pointer itself. This will avoid more code repetition
>> and
>> > > may even work faster.
>> >
>> > Whats with the uppercasing? the method is not a type. Is this a new
>> > convention not mentioned in the wiki/devel-faq?
>>
>> Hmm.. it should be in the style guide somewhere.
>
> http://www.squid-cache.org/Devel/squid-3-style.txt
>

What I based the following on. Alex posted an upper-cased
internal-but-public method (NOT data member).

>> object local methods and variables start with a lowercase, global
>> variables and static class members with an upper case.
>

Re: cvs commit: squid3/src ftp.cc

[Robert Collins]

On Tue, 2007-08-14 at 17:58 +0200, Henrik Nordstrom wrote:
> On tis, 2007-08-14 at 23:01 +1200, Amos Jeffries wrote:
> > Alex Rousskov wrote:
> > > On Mon, 2007-08-13 at 12:01 +0200, Guido Serassio wrote:
> > > 
> > >> Now fixed.
> > > 
> > > If you have time, it may be better to replace a virtual
> > > FtpStateData::haveControlChannel(char*) into a static
> > > FtpStateData::HaveControlChannel(FtpStateData*, char*) and let it check
> > > the ftpState pointer itself. This will avoid more code repetition and
> > > may even work faster.
> > 
> > Whats with the uppercasing? the method is not a type. Is this a new 
> > convention not mentioned in the wiki/devel-faq?
> 
> Hmm.. it should be in the style guide somewhere.

http://www.squid-cache.org/Devel/squid-3-style.txt

> object local methods and variables start with a lowercase, global
> variables and static class members with an upper case.

-Rob
-- 
GPG key available at: .


signature.asc
Description: This is a digitally signed message part

Re: cvs commit: squid3/src ftp.cc

[Henrik Nordstrom]

On tis, 2007-08-14 at 23:01 +1200, Amos Jeffries wrote:
> Alex Rousskov wrote:
> > On Mon, 2007-08-13 at 12:01 +0200, Guido Serassio wrote:
> > 
> >> Now fixed.
> > 
> > If you have time, it may be better to replace a virtual
> > FtpStateData::haveControlChannel(char*) into a static
> > FtpStateData::HaveControlChannel(FtpStateData*, char*) and let it check
> > the ftpState pointer itself. This will avoid more code repetition and
> > may even work faster.
> 
> Whats with the uppercasing? the method is not a type. Is this a new 
> convention not mentioned in the wiki/devel-faq?

Hmm.. it should be in the style guide somewhere.

object local methods and variables start with a lowercase, global
variables and static class members with an upper case.

> The FTP code is all a it messy still with its partial conversion and 
> overlaps with other protocols code.

Can you be specific on what you think need further conversion or
overlaps?

> Ah, quite right. doneWithServer() is only one of the cases that method 
> should handle.
> I'll be adding the other soon.

And quite likely the whole function is not needed any more after the
state fixes, but it's a reasonable safeguard so I don't object it's
addition.

Regards
Henrik


signature.asc
Description: This is a digitally signed message part

Re: cvs commit: squid3/src ftp.cc

[Amos Jeffries]

Alex Rousskov wrote:

On Mon, 2007-08-13 at 12:01 +0200, Guido Serassio wrote:


Now fixed.


If you have time, it may be better to replace a virtual
FtpStateData::haveControlChannel(char*) into a static
FtpStateData::HaveControlChannel(FtpStateData*, char*) and let it check
the ftpState pointer itself. This will avoid more code repetition and
may even work faster.


Whats with the uppercasing? the method is not a type. Is this a new 
convention not mentioned in the wiki/devel-faq?


The FTP code is all a it messy still with its partial conversion and 
overlaps with other protocols code.
When the whole area is cleaned up several of those functions should be 
inline. I was wondering about an *this check inside (does away with the 
extraneous parameter), but I'm not certain enough to code it for 3.0.




I would also suggest to use a different method name since the code
assumes it is only called to detect attempts to call a method when a
control channel is open. Also, doneWithServer() actually checks for the
presence of both data and control channels.


Ah, quite right. doneWithServer() is only one of the cases that method 
should handle.

I'll be adding the other soon.

Amos

Re: cvs commit: squid3/src ftp.cc

[Alex Rousskov]

On Mon, 2007-08-13 at 12:01 +0200, Guido Serassio wrote:

> Now fixed.

If you have time, it may be better to replace a virtual
FtpStateData::haveControlChannel(char*) into a static
FtpStateData::HaveControlChannel(FtpStateData*, char*) and let it check
the ftpState pointer itself. This will avoid more code repetition and
may even work faster.

I would also suggest to use a different method name since the code
assumes it is only called to detect attempts to call a method when a
control channel is open. Also, doneWithServer() actually checks for the
presence of both data and control channels.

Thank you,

Alex.

Re: cvs commit: squid3/src ftp.cc

[Guido Serassio]

Hi,

At 10.45 13/08/2007, Guido Serassio wrote:


Something is missing in your commit:


Now fixed.

Regards

Guido



-

Guido Serassio
Acme Consulting S.r.l. - Microsoft Certified Partner
Via Lucia Savarino, 1   10098 - Rivoli (TO) - ITALY
Tel. : +39.011.9530135  Fax. : +39.011.9781115
Email: [EMAIL PROTECTED]
WWW: http://www.acmeconsulting.it/

Re: cvs commit: squid3/src ftp.cc

[Guido Serassio]

Hi Amos,

At 01.57 13/08/2007, Amos Jeffries wrote:

amosjeffries2007/08/12 17:57:28 MDT

  Modified files:
src  ftp.cc
  Log:
  Fix bug 1560 : Bad filedescriptor in ftpSend actions.



Something is missing in your commit:

if /usr/bin/g++-3.4 -DHAVE_CONFIG_H 
-DDEFAULT_CONFIG_FILE=\"/usr/local/squid3/etc/squid.conf\" -I. -I. 
-I../include -I. -I. -I../include -I../include 
-I../lib/libTrie/include   -I/usr/include/libxml2  -Werror -Wall 
-Wpointer-arith -Wwrite-strings -Wcomments  -D_REENTRANT -m32 
-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -g -O2 -MT ftp.o -MD -MP 
-MF "$depbase.Tpo" -c -o ftp.o ftp.cc; \

then mv -f "$depbase.Tpo" "$depbase.Po"; else rm -f "$depbase.Tpo"; exit 1; fi
ftp.cc: In function `void ftpSendUser(FtpStateData*)':
ftp.cc:1859: error: `haveControlChannel' undeclared (first use this function)
ftp.cc:1859: error: (Each undeclared identifier is reported only once 
for each function it appears in.)

ftp.cc: In function `void ftpSendPass(FtpStateData*)':
ftp.cc:1893: error: `haveControlChannel' undeclared (first use this function)
ftp.cc: In function `void ftpSendType(FtpStateData*)':
ftp.cc:1922: error: `haveControlChannel' undeclared (first use this function)
ftp.cc: In function `void ftpSendCwd(FtpStateData*)':
ftp.cc:2047: error: `haveControlChannel' undeclared (first use this function)
ftp.cc: In function `void ftpSendMkdir(FtpStateData*)':
ftp.cc:2101: error: `haveControlChannel' undeclared (first use this function)
ftp.cc: In function `void ftpSendMdtm(FtpStateData*)':
ftp.cc:2155: error: `haveControlChannel' undeclared (first use this function)
ftp.cc: In function `void ftpSendSize(FtpStateData*)':
ftp.cc:2185: error: `haveControlChannel' undeclared (first use this function)
ftp.cc: In function `void ftpSendPasv(FtpStateData*)':
ftp.cc:2234: error: `haveControlChannel' undeclared (first use this function)
ftp.cc: In function `void ftpSendPort(FtpStateData*)':
ftp.cc:2510: error: `haveControlChannel' undeclared (first use this function)
ftp.cc: In function `void ftpSendStor(FtpStateData*)':
ftp.cc:2639: error: `haveControlChannel' undeclared (first use this function)
ftp.cc: In function `void ftpSendRest(FtpStateData*)':
ftp.cc:2704: error: `haveControlChannel' undeclared (first use this function)
ftp.cc: In function `void ftpSendList(FtpStateData*)':
ftp.cc:2762: error: `haveControlChannel' undeclared (first use this function)
ftp.cc: In function `void ftpSendNlst(FtpStateData*)':
ftp.cc:2779: error: `haveControlChannel' undeclared (first use this function)
ftp.cc: In function `void ftpSendRetr(FtpStateData*)':
ftp.cc:2833: error: `haveControlChannel' undeclared (first use this function)
ftp.cc: In function `void ftpSendQuit(FtpStateData*)':
ftp.cc:2940: error: `haveControlChannel' undeclared (first use this function)
ftp.cc: In function `bool haveControlChannel(const char*)':
ftp.cc:3405: error: `bool haveControlChannel(const char*)' used prior 
to declaration

ftp.cc:3406: error: `doneWithServer' undeclared (first use this function)
make[3]: *** [ftp.o] Error 1
make[3]: Leaving directory `/home/serassio/squid-cache.org/squid3/src'
make[2]: *** [all-recursive] Error 1
make[2]: Leaving directory `/home/serassio/squid-cache.org/squid3/src'
make[1]: *** [all] Error 2
make[1]: Leaving directory `/home/serassio/squid-cache.org/squid3/src'
make: *** [all-recursive] Error 1

Regards

Guido


  This was caused by some FTP operations failing and calling the ftpFail
  properly, but then going on to call an ftpSend.
  It may also occur when a ftpSend event is scheduled prior to the server
  control channel dying or being closed.

  This patch adds a function haveControlChannel(const char *caller_name)
  which displays a debug notice at level 3 and returns false if the server
  control channels are not available. This is now called by each Sending
  operation before it begins.

  Revision  ChangesPath
  1.433 +76 -3 squid3/src/ftp.cc



-

Guido Serassio
Acme Consulting S.r.l. - Microsoft Certified Partner
Via Lucia Savarino, 1   10098 - Rivoli (TO) - ITALY
Tel. : +39.011.9530135  Fax. : +39.011.9781115
Email: [EMAIL PROTECTED]
WWW: http://www.acmeconsulting.it/