Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(70)

Issue 1872049: ns-3-openflow

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 2 months ago by Blake Hurd
Modified:
10 years, 7 months ago
CC:
ns-3-reviews_googlegroups.com
Visibility:
Public.

Description

This is the code review for the GSOC 2010 ns-3-OpenFlow project. As documented in the project wiki <http://www.nsnam.org/wiki/index.php/GSOC2010OpenFlow>, there is one main dependency in this project, the OpenFlow Software Implementation Distribution. It is left out of this patchset in order to keep the patchset trim (the OFSID is ~2000 files) and because the OFSID may be changed at points in the future. The OFSID can be obtained here: <http://www.openflowswitch.org/wk/index.php/OpenFlowMPLS> or the direct link at <http://arl.wustl.edu/~mah5/openflow-mpls.tgz> Other dependencies: The OFSID requires OpenSSL (for an SSL virtual connection), libxml2 (for MPLS FIB xml file parsing), libdl (for address fault checking), and boost (for assert) libraries to be installed. The Switch module does not use the code in the OFSID that uses OpenSSL, libxml2 and libdl. The code that uses OpenSSL is guaranteed to not be used in the Switch module, so you can delete the vconn-ssl.h{.c} from the OFSID to remove the dependency. When the project is finalized, libxml2 and libdl may no longer be required as well.

Patch Set 1 : initial files #

Total comments: 12

Patch Set 2 : new OFSID library creation/linking system #

Total comments: 15

Patch Set 3 : Patch 3, various changes according to commentary #

Total comments: 7

Patch Set 4 : Fixed two merge errors, and namespace ofi is now in ns3 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4717 lines, -2 lines) Patch
A examples/switch/csma-switch.cc View 1 2 3 1 chunk +213 lines, -0 lines 0 comments Download
A examples/switch/wscript View 3 1 chunk +5 lines, -0 lines 0 comments Download
A src/devices/switch/openflow-interface.h View 1 2 3 1 chunk +566 lines, -0 lines 0 comments Download
A src/devices/switch/openflow-interface.cc View 1 2 3 1 chunk +1135 lines, -0 lines 0 comments Download
A src/devices/switch/openflow/waf View 1 Binary file 0 comments Download
A src/devices/switch/openflow/wscript View 1 1 chunk +28 lines, -0 lines 0 comments Download
A src/devices/switch/switch.h View 1 2 1 chunk +140 lines, -0 lines 0 comments Download
A src/devices/switch/switch-helper.h View 1 2 3 1 chunk +105 lines, -0 lines 0 comments Download
A src/devices/switch/switch-helper.cc View 1 2 3 1 chunk +97 lines, -0 lines 0 comments Download
A src/devices/switch/switch-net-device.h View 1 2 1 chunk +550 lines, -0 lines 0 comments Download
A src/devices/switch/switch-net-device.cc View 1 2 1 chunk +1584 lines, -0 lines 0 comments Download
A src/devices/switch/switch-test-suite.cc View 1 2 1 chunk +176 lines, -0 lines 0 comments Download
A src/devices/switch/waf View 1 chunk +1 line, -0 lines 0 comments Download
A src/devices/switch/wscript View 1 2 1 chunk +114 lines, -0 lines 0 comments Download
M src/wscript View 1 2 2 chunks +2 lines, -0 lines 0 comments Download
M wscript View 1 2 3 chunks +1 line, -2 lines 0 comments Download

Messages

Total messages: 7
Lalith Suresh
Hello Blake, I can't give you any good technical feedback, as I don't understand OFSID ...
11 years, 2 months ago (2010-08-13 05:45:07 UTC) #1
Tom Henderson
Blake, your code looks really nice. I left a few comments; my main suggestion for ...
11 years, 2 months ago (2010-08-13 05:50:55 UTC) #2
Josh Pelkey
Some brief minor comments after initial pass. http://codereview.appspot.com/1872049/diff/32001/examples/switch/wscript File examples/switch/wscript (right): http://codereview.appspot.com/1872049/diff/32001/examples/switch/wscript#newcode3 examples/switch/wscript:3: def build(bld): ...
10 years, 8 months ago (2011-02-09 18:13:36 UTC) #3
Josh Pelkey
http://codereview.appspot.com/1872049/diff/32001/src/wscript#newcode1 > src/wscript:1: ## -*- Mode: python; py-indent-offset: 4; indent-tabs-mode: nil; > coding: utf-8; -*- ...
10 years, 8 months ago (2011-02-09 18:14:50 UTC) #4
Blake Hurd
http://codereview.appspot.com/1872049/diff/2001/src/devices/switch/switch-net-device.h File src/devices/switch/switch-net-device.h (right): http://codereview.appspot.com/1872049/diff/2001/src/devices/switch/switch-net-device.h#newcode133 src/devices/switch/switch-net-device.h:133: * never on its port netdevices. On 2010/08/13 05:50:55, ...
10 years, 8 months ago (2011-02-10 04:20:24 UTC) #5
Tom Henderson
I've reviewed a few times so I will try to summarize some issues and suggested ...
10 years, 8 months ago (2011-02-25 21:33:45 UTC) #6
Josh Pelkey
10 years, 7 months ago (2011-03-08 18:11:32 UTC) #7
Tom, I have updated this repository according to your comments. I've posted it
at a separate code review, here --http://codereview.appspot.com/4266051/

Summary of changes:
1) Move everything to src/openflow for modularization.
2) Rename module from switch to openflow.
3) Rename files to be more specific to openflow:
   switch-net-device to openflow-switch-net-device
   switch-helper to openflow-switch-helper
   csma-switch.cc to openflow-switch.cc
   switch.h to openflow-switch.h
4) Rename corresponding classes (since changing file name above)
5) Fix up wscripts for this and add openflow example to test.py
6) Update documentation to reflect the changes

To do -- create openflow-mpls respository that links more easily with this code
and update the building documentation.

On 2011/02/25 21:33:45, Tom Henderson wrote:
> I've reviewed a few times so I will try to summarize some issues and suggested
> resolutions.
> 
> - please consider to hook the downloading and building of the openflow-mpls
code
> to download.py and build.py
>  - (download.py can get it; build.py could copy over the waf/wscript and
> orchestrate the build)
>  - build.py could do a conf check on requisite packages and warn the user if
> building is skipped due to lack of packages
> 
> - where to download it from?  If there need to be local changes maintained,
> perhaps put it on a repository on http://code.nsnam.org.
> -- there appear to be 32/64 bit issues; I was getting size_t compilation
errors
> on 64-bit machine
> -- I am getting error: ‘ssl_vconn_class’ undeclared errors on ns-test machine
> (not sure what I am missing).
> 
> - should you upgrade the openflow-mpls to the more recent release (12 Aug
2010)
> from here?  
> http://www.openflow.org/wk/index.php/File:Openflow_100_mpls.ppt
> 
> - the test code needs updating to ns-3-dev; specifically, there were bug 699
> DoRun() return value changes.
> 
> - I would like to avoid using the name "switch" for this module.  Can you pick
> something other specific to openflow (such as of-switch, openflow-switch, ...)
> 
> - I would like to create a manual page for this module.  I will give it a
start
> (openflow-switch.rst) and send to you for review/completion.
> 
> As I said before, I think the ns-3 code looks nice, so I would support merging
> this now and working the openflow-mpls integration issues during the release
> preparation cycle.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b