Hi!

On Sun, 2007-07-29 at 22:18 +0200, Jo-Philipp Wich wrote:
[...]
> >> [snip]
> > Nice. Some comments and questions though:
> > - From the "strict C" department: since strstr(3) returns a pointer,
> >    comparing to NULL is more appropriate (though 0 is technically not
> >    wrong).
> > - strstr(3) is a basicylly substring search. I don't know if a mere
> >    substring search is not too broad. Personally I would define exact
> >    URLs (like the given ones) and check it more strict to be on the
> > right
> >    position in the request.
> 
> Okay, I'm a C beginner and blindly adapted the old functions ;)

No problem, that's how most people start ;-)

> > - From the department of "beautiful implementation": I wonder if it 
> >    wouldn't be nicer to put the functions + URL parts in a table, look
> >    for the (relevant part of the) given URL and call the associated
> >    function.
> >    If none is found, call all functions.
> >    If the sequence of the table resembles the current sequence, the
> >    output won't change AFAICS.
> 
> Yes, you're right. Now I use a lookup table where possible request urls
> are mapped to function pointers. The reworked patch is attached.
> 
> I left the combined links + neighbours output ("/neighbours", with "u")
> in place to keep the whole thing somewhat backwards compatible. Instead
> I added the additional url "/neighbors" (named after the actual table)
> to get the neighbor table alone.

Good. Believe me, it is not good to break already working stuff;-)

> Bad HTTP requests, e.g. request urls longer than 16 bytes, will trigger
> a "400 Bad HTTP Request" response and unknown ones will generate a
> friendly "not supported" message.
> 
> The patch was developed against olsrd 0.4.10 but seems to apply fine to
> version 0.5.1 . I tested the changed plugin with both olsrd 0.4.10 for
> mipsel running on OpenWRT Whiterussian and with olsrd 0.5.1 for mipsel
> running on OpenWRT Kamikaze.

Hmm, applying against CVS-HEAD gives lots of rejects due to whitespace
mismatch AFAICS. Are there other patches applied before this one?

	Bernd
-- 
Firmix Software GmbH                   http://www.firmix.at/
mobil: +43 664 4416156                 fax: +43 1 7890849-55
          Embedded Linux Development and Services




