DOCGroup
Fix #2131 - Make UIPMC's -ORBListenerInterfaces work properly as documented and tested
See the detailed explanation for this change in #2131.
A quick note about this change: it includes a new test, $TAO_ROOT/orbsvcs/tests/Miop/McastListenerInterfaces. In order for this test to compile and run, $TAO_ROOT/TAO_ACE.mwc has to be edited to comment out or remove the line orbsvcs/tests in the exclude block. It seems that all orbsvcs tests are currently disabled in master. I did not include this change in my pull request, because it seems those tests were disabled for a reason, which I won't second-guess. I know, for example, that most of the tests in $TAO_ROOT/orbsvcs/tests/Miop/ fail on a clean master.
Summary by CodeRabbit
Based on the comprehensive changes, here are the release notes:
-
New Features
- Enhanced network interface handling in ACE library
- Added support for storing and retrieving interface names in network addresses
- Improved multicast listener interface testing capabilities
-
Test Improvements
- New test suite for multicast listener interfaces
- Added comprehensive test scenarios for IPv4 and IPv6 network configurations
-
Bug Fixes
- Refined network interface detection and address management
- Improved error handling in network-related operations
-
Chores
- Updated
.gitignoreto exclude macOS-specific files - Added new test configuration and helper scripts
- Updated
The changes primarily focus on network interface management and testing infrastructure, with significant improvements to the ACE library's address handling capabilities.
I wouldn't put all gitignore files as part of this PR, when you want to add that, make that a separate PR. Also by using the C++ uniform initialization, std::unique_ptr, std instead of ACE_OS this could be more modern C++.
I haven't looked at the issue in detail, this probably needs some detailed review, also the fact that you mention that your approach is maybe not the best will trigger review
I wouldn't put all gitignore files as part of this PR, when you want to add that, make that a separate PR.
Done in #2133. The changes will disappear from here when that's merged and this is rebased.
Also by using the C++ uniform initialization, std::unique_ptr, std instead of ACE_OS this could be more modern C++.
Do you have specific callouts? I actually took pains to use the same techniques, tools, and styles used elsewhere in the code that I was changing, because it was not clear to me which things (like unique_ptr) were and were not allowed, or even which C++ standard (C++11, C++17, etc.) this master is targeting.
I haven't looked at the issue in detail, this probably needs some detailed review, also the fact that you mention that your approach is maybe not the best will trigger review
I certainly welcome the review. I came up with the best solution that I knew how to, but I do not know this code nearly as well as do you, and it's possible that you just want to do it a different way, or maybe you'll even do a complete refactor of the affected areas to achieve other goals I'm not aware of.
I resolved the Fuzz issues, and I worked through all the Codacy issues that I thought were valid. Two of them are clearly false positives caused by the use of the ACE_ERROR_RETURN macro. The other is, I believe, also a false positive, but it may go away based on resolution of my question above ("Do you have specific callouts?"), i.e. I may made this a std::string instead of a char *. I'm torn on the CodeFactor issues, because they are indeed complex methods, but they were already complex methods, and there are even-more complex methods in that file. Refactoring them to be less-complex (splitting into multiple methods) might not be worth the effort.
I'm waiting on platform-specific build results now.
Should "ORBListener" by replaced by "ORBListen"? See TAO/docs/ORBEndpoint.html for established usage.
New test should be added to bin/lst file
~I can't find a file named lst, so this must be short for something I don't understand. Could you elaborate?~
I just found TAO/bin/tao_other_tests.lst. I'll add it there.
I've rebased to remove all the unrelated .gitignore additions, and responded to the above comments with several refactors. The two remaining Codacy issues are clear false positives. The CodeFactor issues are a subjective matter of opinion that I welcome feedback on. I can provide additional refactorings to simplify these functions/methods, but I'm worried about the scope creeping too much on this change. I think it's good to merge as-is, but will proceed however requested.
One thing I did notice through all of this work is that there are about half a dozen different places that use getifaddrs and/or GetAdaptersAddresses to go through interfaces and addresses, and many of them have the same if windows do this else do that macro logic. A major refactor that could be a benefit would be to extract this into a set of platform-generic C++11-modern helper classes, which would simplify many functions/methods, including the two mentioned by CodeFactor. However, that would touch a lot of major areas in excess of the minimum areas needed to fix this issue, so I'm not comfortable proceeding with that without some kind of prior approval.
Please open a new issue for the getifaddrs/GetAdaptersAddresses proposal, that way it doesn't get lost.