[Ns-developers] Status ns-3 quagga porting of last week

[Mathieu Lacage]

hi liu,

I browsed through your tree and see the great progress you have made! A
couple of comments:

1) please, do not use global enums as in netlink-types.h but use instead
the local enums I showed in my previous review. i.e., I prefer:
class Foo
{
public:
  enum {
   MY_BAR_1,
   MY_BAR_2
  };
};
to 
enum {
  FOO_MY_BAR_1,
  FOO_MY_BAR_2
};

For example, NetlinkMsgFlag_e could be defined as a member enum in
NetlinkMsgHeader.

2) I know that it might sound like nitpicking but it would be really
nice if you could try to use:
void SetMsgLen (uint32_t v);
instead of:
void SetMsgLen(uint32_t v);
(notice the extra space before the left paren). The former is what a
large fraction of our existing codebase does and it would be nice to
attempt to be consistent.

3) can you split netlink-msg.h in multiple header files ?
netlink-message-header.h, netlink-header.h ?

4) I find it confusing that NetlinkMessage represents both a single
message in a sequence of multiple messages and the sequence of messages
itself. Can't you define MultipartNetlinkMessage to represent the
sequence (even if there is _only_ one instance in there for a lot of
cases) and NetlinkMessage to represent one message within a
MultipartNetlinkMessage ? If you do this, I suspect that the only class
which needs to derive from the Header base class will be
MultipartNetlinkMessage and that you can remove the base class from
NetlinkMsgHeader and NetlinkMessage. This will also allow you to rename
the postfix-underscore variants such as Serialize_ to Serialize.

5) Can you rename NetlinkMsgHeader to NetlinkMessageHeader ?

Overall, again, this looks great: the above comments are really small
details...

regards,
Mathieu

On Mon, 2008-06-09 at 15:43 +0800, Liu Jian wrote:
> hi all,
>  
> last week, i implemented a class NetlinkMessage(netlink-msg.h/.cc),
> which 
>  
> contains messages Netlinksocket to send/recv. it has a common
> structue:
> class NetlinkMessage
> {
>  NetlinkMessageHeader;
>  ServiceMessage;
>  ServiceAttributes;
>  ...//other modules
> }
> the all members were subclass of Header for its serialize/desiralize
> for NS3 
>  
> kernel.
>  
> For testing(example/simple-netlink-socket.cc), i build some different
> types of 
>  
> messages as the realworld application: GET,NEW,Multipart,etc.now the 
>  
> netlinkmessage can be add/remove at Packet with right values for
> socket's 
>  
> operation.
>  
> But the orignal serialize()/deserialize() of Header class were not
> useful for 
>  
> NetlinkMessage. instead, i add new functions for my work.changes from:
> void Serialize (Buffer::Iterator start) const;
> uint32_t Deserialize (Buffer::Iterator start);
> to:
> void Serialize_ (Buffer::Iterator&start) const;
> uint32_t Deserialize_ (Buffer::Iterator&start);
>  
> so the new functions can finish it well.
> void Serialize_ (Buffer::Iterator&start) const
> {
>  NetlinkMessageHeader.Serialize_(&start);
>  ServiceMessage.Serialize_(&start);
>  ServiceAttributes.Serialize_(&start);
> }
>  
> here, as my experience, the (Buffer::Iterator&start) would useful for
> the future 
>  
> work especially if one member of the header type is still a header
> type.
>  
> 
> Next work: 
> 1, implement the netlink err-message in NetlinkMessage.
> 2, read kernel code, let the NetlinkSocket do the write parsing job.
>  
> 
> ______________________________________________________________________
> Liu Jian
> 2008-06-09