PATCH: kio_http [IMPORTANT]

[Dawit A.]

On Saturday 23 November 2002 12:05, Waldo Bastian wrote:
> > Hi,
> >
> > The following is a patch for kio_http and it addresses the following
> > issues. All of which I myself have encountered...
> >
> > 1st section:
> >
> > Removes a meaningless do loop which was appropriate (I added it :)) when
> > we were directly accessing the system ::read call.  However, since we now
> > call TCPSlaveBase::read either needs to be moved there or completely
> > discarded, no ?
>
> It is still needed. Moving it to TCPSlaveBase::read might be ok though.
> There is no urgent need for that though, so it should happen after 3.1

Ok.

> > 2nd section:
> >
> > Fixes a rather nasty bug where we force a resume download even when the
> > server does not support it.  To compound the problem we correctly signal
> > the calling job that the server does not support resumption.
>
> That is this part of the fix?

Yes.

[snipped code]

> The change in the "if ( bCanResume && m_request.offset )" is only
> cosmetical, right? The else part looks ok.

Hmm... I would not call it cosmetic change, but it indeed is not a critical 
fix.  However, the issue of using int/long as one would a boolean is very 
misleading and abused practice that can result in subtle and hard to find 
bugs IMHO. The value of the long/integer can easily be negative in which case 
the if statement could return true and result in a unexpected/unintended 
action.  It falls into the category where some people use  if (1 == 
somevariable) instead of the common if (somevariable ==1) format to make the 
compiler catch the most common programming error, a typo. Anyways, it really 
does not matter since these are debatable coding conventions I guess...

> > 3rd section:
> >
> > Fixes two problems:
> >
> > First is the issue of using the negation (not) operator with signed
> > long/int variables in conditional statements. It is my understanding (or
> > rather was mentioned to me) that doing can also cause undefined
> > conditions and in general a bad idea besides making it to understand the
> > intention of the coder.
>
> I don't think this is very important.
>
> > The following part:
> >
> > -  if (bytesReceived <= 0)
> > -     return -1; // Error: connection lost
> > +  if (bytesReceived < 0)
> > +     return -1; // ERROR: Connection lost...
> >
> > fixes a long running problem I have had with downloading the config file
> > off of my router's web interface.  I always get a "Connection broken"
> > error message even though the entire file was correctly downloaded.  Why
> > on earth do we consider 0 byte returns as being error conditions ????
>
> When the webserver says it is going to send 100 bytes and then disconnects
> after 90 it is an error. Note that this is readLimited() and that is used
> when the size of data is known. Can you send me a copy of the http log for
> such a download?

Ordinarily I agree with you on this point.  The server states that the size is 
8212 bytes when the file is actually 8208 bytes.  However, returning a 0 
during a ::read call is a valid way to indicate EOF so this should be an 
acceptable behavior IMHO.  For comparison I tried downloading the same file 
with Mozilla and IE and the resulting file is 8208 bytes as well.  Neither 
one of these browsers showed an error message however and so I presume this 
is one of those acceptable workaround ???  I cannot find anything that 
disallows it in the RFC 2616 either.

Regards,
Dawit A.