Bug 194660 - lang/mono Add mono/metadata/mono-route.c for non-Linux ParseRouteInfo_local
Summary: lang/mono Add mono/metadata/mono-route.c for non-Linux ParseRouteInfo_local
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-mono (Nobody)
URL: https://github.com/mono/mono/pull/1404
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-28 13:48 UTC by Ben Woods
Modified: 2015-03-08 12:31 UTC (History)
3 users (show)

See Also:


Attachments
Use netstat to get routes if no /proc/net/route (3.53 KB, patch)
2014-10-28 13:48 UTC, Ben Woods
no flags Details | Diff
PR194660: Use netstat to get routes if no /proc/net/route (3.48 KB, patch)
2014-11-04 13:55 UTC, Ben Woods
no flags Details | Diff
patch-mcs_class_System_System.Net.NetworkInformation_GatewayIPAddressInformationCollection.cs (1.17 KB, patch)
2014-11-15 17:34 UTC, Ben Woods
no flags Details | Diff
PR194660: Add mono/metadata/mono-route.c for non-Linux ParseRouteInfo_local (29.18 KB, patch)
2014-11-15 17:37 UTC, Ben Woods
no flags Details | Diff
test.cs: Test C# program to print list of network interfaces and associated gateways to console (827 bytes, text/plain)
2014-11-23 02:33 UTC, Ben Woods
no flags Details
PR194660: Add mono/metadata/mono-route.c for non-Linux ParseRouteInfo_local (28.31 KB, patch)
2014-12-06 11:22 UTC, Ben Woods
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Woods freebsd_committer 2014-10-28 13:48:13 UTC
Created attachment 148734 [details]
Use netstat to get routes if no /proc/net/route

Since FreeBSD does not have /proc/net/route (even within linprocfs), mono should instead use netstat to get routing information.

Refer to the ParseRouteInfo function in mono source file mcs/class/System/System.Net.NetworkInformation/IPInterfaceProperties.cs

I have submitted a pull request upstream for this, but it is yet to be accepted: https://github.com/mono/mono/pull/1310
Comment 1 Bugzilla Automation freebsd_committer 2014-10-28 13:48:13 UTC
Maintainers CC'd
Comment 2 Ben Woods freebsd_committer 2014-10-28 13:51:45 UTC
This issue was brought to light when attempting to port mediabrowser.tv to FreeBSD.
Comment 3 Ben Woods freebsd_committer 2014-11-04 13:55:46 UTC
Created attachment 149029 [details]
PR194660: Use netstat to get routes if no /proc/net/route

Updated patch to ensure gateways is initialised for either ParseRouteInfo option
Comment 4 Romain Tartière freebsd_committer 2014-11-06 09:39:00 UTC
Hello

I quite agree with the comments in the github pull request but #194845 makes me think that maybe the MacOS code (if there is some) may also be suitable for FreeBSD?

I have no MacOS knowledge at all, I just did a quick web search about "/proc/net/route" on MacOS and the results told me that it is not available.

Unfortunately, I can't afford the time to dig into this ATM :-(  However, this might be an interesting thing to investigate in order to fix this problem in a more elegant way.

As a last resort, maybe creating a specialized FreeBSDIPInterfaceProperties class which overrides the ParseRouteInfo() method may make the code more straightforward to read?
Comment 5 Romain Tartière freebsd_committer 2014-11-06 09:43:13 UTC
Oh, reading your comment in bug 194845, comment 8 I understand that this is also broken on MacOS?

So the netstat invocation seems unavoidable…
Comment 6 Ben Woods freebsd_committer 2014-11-06 09:51:51 UTC
To explain further why both are required.
Refer to the code here: https://github.com/mono/mono/blob/master/mcs/class/System/System.Net.NetworkInformation/IPInterfaceProperties.cs

This code defines a base class IPInterfaceProperties.
There are 2 derived classes that inherit the members of IPInterfaceProperties:
 - UnixIPInterfaceProperties (common to Linux, Mac, FreeBSD)
 - Win32IPInterfaceProperties2

There are 2 derived classes that inherit the members of UnixIPInterfaceProperties:
    - LinuxIPInterfaceProperties
    - MacOsIPInterfaceProperties

The ParseRouteInfo method is a part of the UnixIPInterfaceProperties class (common to Linux, Mac, FreeBSD). Hence, changing mono on FreeBSD to use the MacOsNetworkInterface class (as per the patches attached to PR194845) does not fix this issue.
Comment 7 Ben Woods freebsd_committer 2014-11-06 09:53:30 UTC
Furthermore, I don't see how parsing the output of netstat (per these patches) is any less elegant that parsing the file /proc/net/route (per the existing mono code).
Comment 8 Romain Tartière freebsd_committer 2014-11-06 10:09:23 UTC
The "problem" is that this implementation spawns a new process, and this kind of situation should be avoided if possible.  Moreover, each netstat implementation will have slight differences in it's output, so the code might quickly become hard to maintain…


Since executing netstat look nearly unavoidable on FreeBSD (and maybe MacOS), using a switch / case to detect the platform and do the right thing (read /proc/net/route on Linux, run and parse FreeBSD's netstat output on FreeBSD, run and parse MacOS's netstat output on MacOS, …) instead of trying one solution and falling back on another would be more elegant (think about the code you changed in attachment 149099 [details]).

While here, using a full path to the program would be more reliable (in case $PATH does not contain a 'netstat' command or if the user has a custom 'netstat' in his PATH that takes precedence).

There may be additional things to take into account, I guess the mono team would help formalizing this.  I'll point them here so that they may provide further feedback.
Comment 9 Ben Woods freebsd_committer 2014-11-06 10:27:20 UTC
All good suggestions. The netstat on FreeBSD and Mac OSX do have slightly different outputs (with the Gateway being in different columns). That is why I wrote this code to first parse which column the gateway is in. I have tested it on both FreeBSD and Mac OSX and it works on both.
Comment 10 Ben Woods freebsd_committer 2014-11-15 17:34:19 UTC
Created attachment 149451 [details]
patch-mcs_class_System_System.Net.NetworkInformation_GatewayIPAddressInformationCollection.cs
Comment 11 Ben Woods freebsd_committer 2014-11-15 17:37:36 UTC
Created attachment 149452 [details]
PR194660: Add mono/metadata/mono-route.c for non-Linux ParseRouteInfo_local

This new patch adopts a different approach which does not fork a new netstat process.
This new patch still exists to provide the ability to read the gateway addresses on Unix systems which don't have the Linux /proc/net/route file.
This is now achieved using sysctl(3) calls in the new file mono/metadata/mono-route.c.
This code has been tested to work on both FreeBSD and Mac OS X.
This has been submitted upstream for consideration here: https://github.com/mono/mono/pull/1404
Comment 12 Ben Woods freebsd_committer 2014-11-23 02:33:59 UTC
Created attachment 149739 [details]
test.cs: Test C# program to print list of network interfaces and associated gateways to console

This test program shows the missing functionality when run on FreeBSD or Mac OSX. For example, when run on my Macbook Pro:

$ mcs test.cs
$ mono test.exe
Gateways
lo0
gif0
stf0
en0
en1
fw0
p2p0

$ mono --trace=N:nothing test.exe
Gateways
[0xa0d031d4:] EXCEPTION handling: System.IO.DirectoryNotFoundException: Could not find a part of the path "/proc/net/route".
lo0
[0xa0d031d4:] EXCEPTION handling: System.IO.DirectoryNotFoundException: Could not find a part of the path "/proc/net/route".
gif0
[0xa0d031d4:] EXCEPTION handling: System.IO.DirectoryNotFoundException: Could not find a part of the path "/proc/net/route".
stf0
[0xa0d031d4:] EXCEPTION handling: System.IO.DirectoryNotFoundException: Could not find a part of the path "/proc/net/route".
en0
[0xa0d031d4:] EXCEPTION handling: System.IO.DirectoryNotFoundException: Could not find a part of the path "/proc/net/route".
en1
[0xa0d031d4:] EXCEPTION handling: System.IO.DirectoryNotFoundException: Could not find a part of the path "/proc/net/route".
fw0
[0xa0d031d4:] EXCEPTION handling: System.IO.DirectoryNotFoundException: Could not find a part of the path "/proc/net/route".
p2p0


Whilst the expected output would be:
$ mcs test.cs
$ mono test.exe
Gateways
lo0
  Gateway Address ......................... : 127.0.0.1
gif0
stf0
en0
en1
  Gateway Address ......................... : 192.168.1.1
fw0
p2p0
Comment 13 Ben Woods freebsd_committer 2014-12-06 11:22:40 UTC
Created attachment 150260 [details]
PR194660: Add mono/metadata/mono-route.c for non-Linux ParseRouteInfo_local

Updated patch to not break Windows or Linux builds.
Also updated in upstream pull request.
Comment 14 Ben Woods freebsd_committer 2014-12-06 11:25:49 UTC
Romain - can you explain why the FreeBSD port has Makefile.in and Makefile.am, but the upstream github repository only has Makefile.am?

In the patch I have submitted here on Bugzilla I have had to additionally patch Makefile.in (which I obviously could not do in my Github pull request as the file does not exist there).
Comment 15 Romain Tartière freebsd_committer 2014-12-06 11:40:29 UTC
A Makefile.am file is only in the source repository because it is used by the autotools to generate the corresponding Makefile.in file, which in turn is used at the ./configure stage to generate the final Makefile.

Usualy, a repository will therefore only contain a Makefile.am, but release tarballs will include both the Makefile.am and the generated Makefile.in files (to avoid users to have all the autotools chain installed.  The Makefile.am file is not useful for final users unless they want to regenerate a new Makefile.in).


So, as far as the ports tree is concerned, only the changes to the Makefile.in are relevant for a patch (but changes for the corresponding Makefile.am do not hurt), and for upstream, only the changes to Makefile.am are relevant.

This new patch is better shaped than the previous one, I am giving it a try at the moment.  Thank you for your work on this!
Comment 16 Ben Woods freebsd_committer 2014-12-06 12:01:12 UTC
Thanks for that.
So if my pull request is accepted upstream, and we incorporate the change into the FreeBSD port using a PATCHFILES statement in the Makefile, I assume the Makefile.in will not be changed? Would we have to maintain the changes to that as a separate patch file?
Comment 17 Romain Tartière freebsd_committer 2014-12-06 18:16:41 UTC
(In reply to Ben Woods from comment #16)
> Thanks for that.
> So if my pull request is accepted upstream, and we incorporate the change
> into the FreeBSD port using a PATCHFILES statement in the Makefile, I assume
> the Makefile.in will not be changed? Would we have to maintain the changes
> to that as a separate patch file?

Exactly, if possible, I prefer having reference to bug-fixes as PATCHFILES but since this change is quite large and has an impact on the generated Makefile.in, a separate patch is preferred in this case (as you attached).

As soon as upstream merges your changes, I'll commit that patch, but in case they have some more suggestions, I guess we would have benefits to wait for a "final" version.
Comment 18 Ben Woods freebsd_committer 2015-01-19 13:07:14 UTC
These patches have now been accepted upstream (with slight modifications):
https://github.com/mono/mono/commit/396d9dbeb1a93665ede08c0e24e3e051e2c3e1de

Is it possible to include this commit in the FreeBSD mono port?

Note that the upstream GitHub code does not have Makefile.in (it only has Makefile.am). Will the necessary changes to the Makefile.in be automatically pulled in, or do we need to independently patch that file?
Comment 19 Ben Woods freebsd_committer 2015-03-08 01:22:10 UTC
Note that this patch has not been included in the upstream Mono 3.12 branch, so will not come with the upstream source code until 3.14.
Comment 20 commit-hook freebsd_committer 2015-03-08 12:30:29 UTC
A commit references this bug:

Author: romain
Date: Sun Mar  8 12:29:59 UTC 2015
New revision: 380771
URL: https://svnweb.freebsd.org/changeset/ports/380771

Log:
  Import upstream patch for non-Linux ParseRouteInfo.

  PR:		ports/194660
  Submitted by:	Ben Woods <woodsb02@gmail.com>

Changes:
  head/lang/mono/Makefile
  head/lang/mono/distinfo
  head/lang/mono/files/patch-mono_metadata_Makefile.in
Comment 21 Romain Tartière freebsd_committer 2015-03-08 12:31:29 UTC
After some delay, I have just pushed this.

Thank you for your work on this!