[WLANware] SNMP plugin for OSLR - GSoC 2011

[Marek Lindner]

Hi,

> All the headers in the packet are being parsed by functions
> handle_***_packet().For example handle_ip_packet() parses the ip
> header.If the protocol field in the header is IPPROTO_TCP it calls
> handle_tcp_packet() else if it is IPPROTO_UDP it calls
> handle_udp_packet().handle_udp_packet() parses the udp headers but
> also handles the request and processes it based on the opcode.I
> thought it would be more modular if handle_udp_packet() just parses
> the header and calls some other function to do the packet
> processing.In the same way for the handle_tcp_packet() instead of
> processing the packet(for example ,handling telnet) also there itself,
> we could just parse the tcp header and have a function that handles
> telnet.

I fail to see how it makes the code more modular. It seems to be more a 
question of personal taste than a functional change in the code. As long as 
there is no other UDP consumer it won't make much of a difference. We can still 
split the functions as soon as we have one. 


> Further, in handle_udp_packet() when we have to send the image
> over, we are updating some attributes and displaying some messages
> before actually sending the block.This sending is being done by
> tftp_packet_send_data().If that fails, these attributes are not
> getting cleaned up.Shouldn't we update the attributes only after the
> sending was successful?

I went over the code but could not see how this translates into a problem. Can 
you be more precise which variable is updated too soon / should be "cleaned 
up" ?


> raw_sock was declared in socket.c (not local to any function) and when
> I try to link socket.c with my dummy main where I also have a global
> raw_sock variable, this gives up an error.As raw_sock is a very
> commonly used variable-name I thought we would have problems to link
> socket.c with other code.One idea is make the variable name a little
> less common.Another is to not make it global but pass it as parameter
> to any functions that use it.I prefer the second as not having it as
> global would greatly increase the readability.But was easy access of
> the variable in other functions your intention or did you have
> something else in mind?

The idea was to keep it simple. However, I don't understand why you would want 
to have another variable called raw_sock in ap51-flash or link arbitrary files 
with other files. 
But you easily could "solve" this by declaring raw_sock "static".

Regards,
Marek