[En-Nut-Discussion] Duplicate code in httpd.c and ssi.c

[Thiago A. Corrêa]

On Wed, May 14, 2008 at 5:16 AM, Ole Reinhardt
<ole.reinhardt at embedded-it.de> wrote:
> Hi Thiago,
>
>>    Actually it's probably non-related. I still have to figure out
>> what's wrong, if it was the ethernut upgrade or something else. I have
>> an index.html that redirects to index.asp (my code is older than the
>> patch that introduces index.asp as a valid default page).
>>    So, I get to load index.html but not index.asp. Typing the cgi urls
>> that I have directly also doesn't help. The browser just sits there
>> "loading" indefinitly.
>
> Strange, try to trace your network traffic with wireshark to see at what
> point the communication stops. Could be a stack overflow anywhere in
> your code?

I don't think so, as I have reverted my code to previous working
revisions. Plus, other threads keep working while the HTTP connection
seams to be stuck. I have just run a Wireshark session on the HTTP
traffic, and now I'm quite convinced that the recent changes have
smoething to do to it. I see that the HTML output is right for my
index.asp, but there is no connection closing, no FIN.

I guess the next step is revert those changes in my local copy and
recompile ethernut to see if there is a difference. I have inspected
the code, and it sounds solid though. If this change is indeed the
problem, more eye balls there would be quite helpfull.

> Right. We should avoid malloc / free and use NutHeapAlloc / NutHeapFree
> instead. That would be more consistent.

Is there an unspoken rule of the thumb on using NutHeapAlloc and
malloc? Is malloc supposed to be used only on client code?

>> My proposal isn't really to fix a problem, but just rather get rid of
>> a duplicate function. Should reduce code size a little bit. Anyway, I
>> produced a patch that I'm attaching to this mail. Will commit if
>> everything is in order.
>
> I'm wondering why you remove the DestroyRequestInfo in both files?
> Btw: Those two versions of DestroyRequestInfo are different! The one in
> httpd.c frees one more member of the request struct.
>
> So remove the right one :)

It seams that none of the files is the right one to keep it :)
So, I created an httpd_p.c/h. This is borrowed from my experience
hacking Qt internals. They use the _p suffix for files that are
"private", containing functions that are not part of the public API.
It makes a lot of sense to me, so I thought I could borrow the concept
here.
The DestroyRequestInfo in httpd.c seams more complete. It checks for
more fields, and checks for bad parameter.

> Perhaps the best way would be to place some common functions into a
> httpd_commom.c. There are some more functions I found that are used in
> ssi.c and are included from httpd.c. Cross referencing functions between
> c files is not a realy good coding style :)

That's what the linker is good for :)
Since I've introduced a private header that declares the function, it
seams ok. I thought for a second about forward declaring the function
in the c file directly, that would be quite bad :)
Doesn't seam much different than dencode.c/h, except that I added
comments explicitly warning that those are not part of the public API
and are subject to change or removal at any time.

Those other functions are static or part of the public API? I think we
could place all those shared functions that are not part of the API
there, and reduce the code size of the lib and increase
maintainability as a bonus.

Regards,
    Thiago A. Correa