Bug 760

I am fine with adding this helper capability.  For class Ipv4 and Ipv6 changes, it does break slightly the design pattern of keeping the public API minimal since it can be implemented with public API (and hence could be implemented entirely in base class Ipv4 or outside of the class).

It should be possible to add this feature directly in src/node/ipv[46].h/cc.

Iterates over interfaces (via GetNInterfaces()) then iterates over addresses (GetNAddresses) compare with address you want to remove and finally call RemoveAddress(ifIndex, addrIndex) when found.

WDYT ?

Hi

I don't think the patch supplied would apply to the latest code. I tried applying the changes from the patch only to the ipv4 files. On running ./waf, I get the below errors

[1425/2095] cxx: build/src/internet/bindings/ns3module.cc -> build/src/internet/bindings/ns3module.cc.7.o
src/internet/bindings/ns3module.cc: In function ‘int _wrap_PyNs3Ipv4__tp_init__0(PyNs3Ipv4*, PyObject*, PyObject*, PyObject**)’:
src/internet/bindings/ns3module.cc:63909:75: error: cannot allocate an object of abstract type ‘PyNs3Ipv4__PythonHelper’
src/internet/bindings/ns3module.h:5475:7: note:   because the following virtual functions are pure within ‘PyNs3Ipv4__PythonHelper’:
./ns3/ipv4.h:258:16: note: 	virtual bool ns3::Ipv4::RemoveAddress(uint32_t, ns3::Ipv4Address)
src/internet/bindings/ns3module.cc: In function ‘int _wrap_PyNs3Ipv4__tp_init__1(PyNs3Ipv4*, PyObject*, PyObject*, PyObject**)’:
src/internet/bindings/ns3module.cc:63938:49: error: cannot allocate an object of abstract type ‘PyNs3Ipv4__PythonHelper’
src/internet/bindings/ns3module.h:5475:7: note:   since type ‘PyNs3Ipv4__PythonHelper’ has pure virtual functions
Waf: Leaving directory `/home/otv/Programming/ns-3-allinone/ns-3-dev/build'
Build failed
 -> task in 'ns3module_internet' failed (exit status 1): 
	{task 161794636: cxx ns3module.cc -> ns3module.cc.7.o}
['/usr/bin/g++', '-O0', '-ggdb', '-g3', '-Wall', '-Werror', '-Wno-error=deprecated-declarations', '-fstrict-aliasing', '-Wstrict-aliasing', '-fPIC', '-pthread', '-fno-strict-aliasing', '-fwrapv', '-fstack-protector', '-fno-strict-aliasing', '-fvisibility=hidden', '-Wno-array-bounds', '-pthread', '-pthread', '-fno-strict-aliasing', '-fwrapv', '-fstack-protector', '-fno-strict-aliasing', '-I.', '-I..', '-Isrc/internet/bindings', '-I../src/internet/bindings', '-I/usr/include/python2.7', '-I/usr/include/gtk-2.0', '-I/usr/lib/i386-linux-gnu/gtk-2.0/include', '-I/usr/include/atk-1.0', '-I/usr/include/cairo', '-I/usr/include/gdk-pixbuf-2.0', '-I/usr/include/pango-1.0', '-I/usr/include/gio-unix-2.0', '-I/usr/include/glib-2.0', '-I/usr/lib/i386-linux-gnu/glib-2.0/include', '-I/usr/include/pixman-1', '-I/usr/include/freetype2', '-I/usr/include/libpng12', '-I/usr/include/libxml2', '-DNS3_ASSERT_ENABLE', '-DNS3_LOG_ENABLE', '-DHAVE_SYS_IOCTL_H=1', '-DHAVE_IF_NETS_H=1', '-DHAVE_PACKET_H=1', '-DHAVE_DL=1', '-DHAVE_SQLITE3=1', '-DHAVE_IF_TUN_H=1', '-DHAVE_GSL=1', '-DNS_DEPRECATED=', '-DNS3_DEPRECATED_H', '-DNDEBUG', '-DNDEBUG', 'src/internet/bindings/ns3module.cc', '-c', '-o', 'src/internet/bindings/ns3module.cc.7.o']

I'd like to work on this if someone can just provide me with some direction.

> 
> I'd like to work on this if someone can just provide me with some direction.

Try again after disabling python bindings  ('./waf configure --enable-examples --enable-tests --disable-python' and then './waf build'); since the patch is changing the API and not the bindings, the bindings will be out of sync.

Checking the patch file, you'll see some issues that should be easy to fix.
1) the patch still refers to the old ns-3 folders layout, e.g.:
src/internet-stack/ipv4-interface.cc
src/node/ipv4.h
src/node/ipv6.h

The best thing to do is to open the patch with a text editor and copy/paste carefully the added functions in the right files (they've been moved).

Another thing to do is to double check the doxygen comments. E.g.;

--- a/src/internet-stack/ipv4-interface.h	Mon Nov 30 16:38:28 2009 +0100
+++ b/src/internet-stack/ipv4-interface.h	Tue Dec 01 11:24:08 2009 +0100
@@ -152,6 +152,12 @@
    */
   Ipv4InterfaceAddress RemoveAddress (uint32_t index);
 
+  /**
+   * \param addr The address to remove
+   * \returns The Ipv4InterfaceAddress whose address is addr
+   */
+  Ipv4InterfaceAddress RemoveAddress (Ipv4Address addr);
+

The doxygen description doesn't state the function purpose and it doesn't warn that, upon failure, the Ipv4InterfaceAddress is the default one (i.e., null address).

Beside these points, I'm fine with this feature. The only thing I'd rather add to the code is a check. Removing 127.0.0.1 or ::1 (i.e., the loopback interfaces) should be forbidden. In the proposed code it isn't.
Note that it *is* possible to remove the loopback addresses by using the present APIs. However I'd make it more painful to do it, so to discourage the dumb user.
I mean, you want to remove the loopback interface ? Fine, but you'll have to think twice to do it.

Created attachment 1615 [details]
Patch with  proposed changes to ipv4 only.

Hi

Attached is a patch. You could call this a draft patch. I've made changes only to the Ipv4. I've added the check to see that Loopback address "127.0.0.1" cannot be removed. Please let me know what you think of it.

Also, I've built it locally and ran the tests. It builds OK (you have to disable python) and all the tests pass.

(In reply to comment #7)
> Also, I've built it locally and ran the tests. It builds OK (you have to
> disable python) and all the tests pass.

Good work, but I don't agree... on the "IPv4 only" part.

The patch contains also the IPv6 code changes. It's only missing these things:
1) check against the loopback removal (it's the very same code as IPv4), and
2) IPv6 tests.

Oh, btw, GetLoopback() is a static functions, so it could be Ipv4Address::GetLoopback()
Not a biggie, calling a static function from an instance does work as well.

T.

Created attachment 1618 [details]
Patch with  proposed changes to ipv4 and ipv6.

Again, I've built it locally and ran the tests. It builds OK (you have to
disable python) and all the tests pass.

I did a drycheck on the code (i.e., I didn't run it myself) but the tests are ok, so if they do pass, I'm confident that everything is working as intended.

The code itself is fine, i.e., I can't spot any potential bug.

+1

I had to compile this by disabling python bindings as the python bindings were not changed and only the API was. Would a separate ticket be needed for the python bindings ?

(In reply to comment #12)
> I had to compile this by disabling python bindings as the python bindings were
> not changed and only the API was. Would a separate ticket be needed for the
> python bindings ?

No, I can do that as part of the commit process.  Note that python binding generation is cumbersome at the moment due to bug 1622 and 1530.

I reviewed your patch and think it is ready to commit, modulo a small fix in the error string on the tests (where it says "number of interfaces" it should say "number of addresses").  I can fix this before committing.  I'll commit tomorrow if there are no other comments.

+1 for me

T.

pushed in changeset: 9915:d4c2228d3c30

Created attachment 1643 [details]
Changes for click

Attached is patch with changes to the file src/click/model/ipv4-l3-click-protocol.h and .cc. I'll attach this to the bug report too. I haven't added any tests as there weren't any existing for the an already existent RemoveAddress method and therefore I feel this would not be needed.

(In reply to comment #17)
> Attached is patch with changes to the file
> src/click/model/ipv4-l3-click-protocol.h and .cc. I'll attach this to the bug
> report too. I haven't added any tests as there weren't any existing for the an
> already existent RemoveAddress method and therefore I feel this would not be
> needed.

Thanks! Pushed.

changeset:   9956:73bee538fdcb
tag:         tip
user:        Alexander D'souza <moijes12@gmail.com>
date:        Thu Jul 18 14:07:56 2013 +0200
summary:     Follow up patch for Click: Bug 760 - IP address removal can be painful