[Ns-developers] Cooperative ns-3 simulation with third party software via JSON-RPC

[Mathieu Lacage]

On Wed, 2009-01-14 at 17:24 +0100, Sébastien Vincent wrote:

ok,

Coding style: you already went through a previous review for the ipv6
stuff so, I was hoping I would not have to point you to:
http://www.nsnam.org/codingstyle.html

One recurring example:

      if(m_remoteAddressLength == 0)
      {
        memcpy(&m_remoteAddress, &addr, sizeof(struct
sockaddr_storage));
        m_remoteAddressLength = addrlen;
      }

should be:

      if(m_remoteAddressLength == 0)
        {
          memcpy(&m_remoteAddress, &addr, sizeof(struct
sockaddr_storage));
          m_remoteAddressLength = addrlen;
        }


> > New files:
> >
> > README.JsonRpc
> >
> > src/rpc-agent/ns-3-rpc-server.cc,h

a) doxygen should include a brief summary of the purpose of this class:
if I am not mistaken, "receive json requests from a socket and forward
them to the json parser/handler".

b) can't you merge the udp and tcp implementations: from a high-level
review, these look almost exactly similar

c) ns3::rpc::Ns3RpcServer is fully redundant: ns3::rpc::Server or
ns3::RpcServer would be more appropriate.

d) although this is not specified in the coding style, all the ns-3
codebase does this. The following:

namespace ns3
{
  namespace rpc 
  {

    Ns3RpcServer::Ns3RpcServer(const std::string& address, uint16_t
port) : Json::Rpc::Server(address, port)
    {

is re-indented to be:

namespace ns3
{
namespace rpc 
{

Ns3RpcServer::Ns3RpcServer(const std::string& address, uint16_t port) :
Json::Rpc::Server(address, port)
{

same for the header and all other files you submitted.


> > src/rpc-agent/rpc-agent.cc,h

doxygen should include a short description of the purpose of this class.
I have no idea what it is.

> > src/rpc-agent/synchronisation-agent.cc,h

I think I understand what you are doing here.

> > src/rpc-agent/update-agent.cc,h

If the purpose is to "get/set attributes in NS-3 objects", why not name
UpdateAgent an AttributeAgent instead ? I went through the code and I
have to confess that I don't really see the relationship between the
code and the stated purpose of the class (in doxygen). This class seems
to do a lot of very different things, a lot of which have nothing to do
with setting and getting attributes in ns-3 objects.

> > src/rpc-agent/wscript
> >
> > src/rpc-applications/rpc-application.cc,h

This is a subclass of the Application base class. It should be located
in src/applications/rpc-applications.

> > src/rpc-applications/rpc-udp-echo/rpc-udp-echo-client.cc,h
> > src/rpc-applications/rpc-udp-echo/rpc-udp-echo-server.cc,h
> > src/rpc-applications/rpc-udp-echo/wscript
> >
> > Modified files:
> >
> > src/node/socket.cc,h => SourceSocketAddressTag class
> > Note that this can be avoided by adding two SocketAddressTag (one for 
> > destination and one for source address) and know at the application 
> > level which is source and which is destination.

why not.

> >
> > src/internet-stack/udp-l4-protocol.cc => add a SourceSocketAddressTag 
> > tag in the packet (::Receive())
> > The same modification should be done for tcp-l4-protocol.cc to allow 
> > an "rpc-tcp-echo-server/client" like to be implemented.

ok.

> >
> > src/wscript => add modules (rpc-agent, rpc-applications and 
> > rpc-applications/rpc-udp-echo)
> >
> > wscript => Add link to libjson and libjsonrpc

> I forgot some files:
> src/example/rpc-synchronisation.cc
> src/example/rpc-simulation.cc
> src/example/rpc-udp-echo.cc
> src/example/wscript (modified)
> 
> src/mobility/rpc-mobility-model.cc,h
> src/mobility/wscript (modified)
> 
> src/helper/rpc-udp-echo-helper.cc,h
> src/helper/wscript (modified)

ok.

Overall, I have to confess that I positively dislike what you have done
here. Code such as this:

    bool UpdateAgent::GetDirection(const Json::Value& msg, Json::Value&
response)
    {
      if(!msg.isMember("params") ||
          !msg["params"].isMember("nodeid") || !
msg["params"]["nodeid"].isInt())
      {
        /* error */
        Json::Value error;
        response["jsonrpc"] = "2.0";
        error["code"] = Json::Rpc::INVALID_PARAMS;
        error["message"] = "Invalid parameter(s)";
        response["id"] = msg["id"];
        response["error"] = error;
        return false;
      }

      uint32_t nodeId = msg["params"]["nodeid"].asInt();
      Json::Value params;

      Ptr<Node> object = GetNode(nodeId);
      if(!object)
      {
        /* TODO send error message */
        return false;
      }

      Ptr<RpcMobilityModel> model = object->GetObject<RpcMobilityModel>
();

      if(!model)
      {
        /* no mobility model installed on this node */
        response["jsonrpc"] = "2.0";
        response["id"] = msg["id"];
        params["x"] = Json::Value::null;
        params["y"] = Json::Value::null;
        params["z"] = Json::Value::null;
        response["params"] = params;

        return false;

Should not exist: you are implementing your own string-based RPC all by
hand. This makes absolutely zero sense: the very large amount of code
dedicated to the sole purpose of marshalling and demarshalling all the
input and output arguments is scary and is going to be horribly
unmaintainable. 

The choice of API you export is also obviously very focused on solving
your own set of simulation scenarios. I mostly refer to
UpdateAgent::BuildSimulation which hardcodes all the simulation topology
and most parameters. This is probably the biggest issue with the
approach you have chosen: I fail to see how it could be of much use
outside of your specific simulation scenarios to other people. More
specifically, I see how we could extend your current approach to make
the system export more flexbility to different kinds of users but I also
see the huge implementation cost of doing so.

Now, I understand that you might be reluctant to use RPC-XML for various
reasons but:
  - it is totally free because we have a complete python binding so, you
can trivially call RPC-XML services _today_ and remotely construct
arbitrary ns3 topologies
  - it is orders of magnitude more extendable and flexible
  - it will solve your problem (among others): your mobility simulator
is written in python so, you don't even have the excuse that you don't
have an XML-RPC layer in your target language

I believe that you mentioned performance as a potential issue with
XML-RPC (other than sheer dislike of XML): do you have any hard numbers
which show that this would be problematic for your use-case ? I mean,
sure, json is theoretically faster than an xml-based serialization, but,
does it actually matter for your use-case ?

To summarize, I see very little added benefit in merging this code and a
lot of downsides. I would much rather point users who wish to
inter-operate with ns-3 to XML-RPC than this very partial, complex, and
hard to maintain solution.

I am really very sorry to be so negative after the review :/

Mathieu