Miscellaneous src/inet cleanup #10450

kpschoedel · 2021-10-13T15:02:03Z

Problem

Working toward #7715 Virtualize System and Inet interfaces

Change overview

Convert some enum to enum class, and use BitFlags for flag sets.
Rename some class members with m preifx for consistency within src/inet.
Revise IPAddress construction.
Fix outdated IPAddress documentation.
Convert file-local functions from static to anonymous namespace.
Revise IPEndPointBasis to reduce duplication and remove dead code.
Remove unused flags argument from SendTo() and SendMsg().
Replace various sockaddr unions with SockAddr.
Remove some exit: return and use ReturnError… more.
Move some comments from definition (.cpp) to declaration (.h).
Use ObjectLifeCycle for InetLayer.
Make some TCPEndPoint members private with TCPTest for tests.

Testing

CI; no changes to functionality intended.

#### Problem Working toward project-chip#7715 Virtualize System and Inet interfaces #### Change overview - Convert some `enum` to `enum class`, and use `BitFlags` for flag sets. - Rename some class members with `m` preifx for consistency within `src/inet`. - Revise `IPAddress` construction. - Fix outdated `IPAddress` documentation. - Convert file-local functions from `static` to anonymous `namespace`. - Revise `IPEndPointBasis` to reduce duplication and remove dead code. - Remove unused flags argument from `SendTo()` and `SendMsg()`. - Replace various `sockaddr` unions with `SockAddr`. - Remove some `exit: return` and use `ReturnError…` more. - Move some comments from definition (.cpp) to declaration (.h). - Use `ObjectLifeCycle` for `InetLayer`. - Make some `TCPEndPoint` members `private` with `TCPTest` for tests. #### Testing CI; no changes to functionality intended.

Damian-Nordic · 2021-10-13T16:38:44Z

src/inet/AsyncDNSResolverSockets.cpp

@@ -90,7 +90,11 @@ CHIP_ERROR AsyncDNSResolverSockets::Shutdown()

    AsyncMutexLock();

-    mInet->State = InetLayer::kState_ShutdownInProgress;
+    if (mInet->mLayerState.GetState() != ObjectLifeCycle::State::ShuttingDown)


Is the (Async)DNSResolver used/needed at all beyond tests? Or we're keeping it just in case?

I honestly don't know. If someone can tell me definitively that AsyncDNSResolver in particular is not used, I'll be very happy to get rid of it, since it's the only thing using threads internal to the SDK, and therefore an obstacle to simplification (cf #7725).

I believe CHIP should only use mDNS and we have either platform or minmdns for those , nothing in inet.

Lets delete it. If I am wrong we can always add it back.

Damian-Nordic · 2021-10-13T16:54:40Z

src/lib/support/ObjectLifeCycle.h

-        }
-        return false;
-    }
+    bool Initializing() { return Transition<State::Uninitialized, State::Initializing>(); }


nit: not sure if it's just me, but imho state.Initializing() looks like a getter (is it in "Initializing" state). state.SetInitializing() would be less ambiguous.

OK. I'll rename them all in some consistent way.

tcarmelveilleux · 2021-10-13T17:14:02Z

src/lib/dnssd/Resolver.h

@@ -185,7 +185,7 @@ struct DiscoveredNodeData
        for (int j = 0; j < numIPs; j++)
        {
 #if CHIP_DETAIL_LOGGING
-            char buf[Inet::kMaxIPAddressStringLength];
+            char buf[Inet::IPAddress::kMaxStringLength];


Why are the lines above:

char addrBuffer[Inet::IPAddress::kMaxStringLength + 1]; mAddress.ToString(addrBuffer);

and here it's not +1. Why?

Presumably one author didn't check whether kMaxStringLength includes the terminating NUL (it does), since it isn't documented at the declaration (it will be).

src/lib/support/ObjectLifeCycle.h

bzbarsky-apple

I stopped at the beginning of IPEndPointBasis.cpp. I just don't have a block of time of sufficient size to review something like this carefully... Might have been better as multiple smaller PRs.

bzbarsky-apple · 2021-10-13T20:39:52Z

src/inet/AsyncDNSResolverSockets.cpp

+    resolver.mNumAddrs                      = 0;
+    resolver.mDNSOptions                    = options;
+    resolver.mAddrArray                     = addrArray;
+    resolver.AppState                       = appState;


Why not mAppState here?

It belongs to System::Object, which I didn't want to add to this PR.

bzbarsky-apple · 2021-10-13T20:41:12Z

src/inet/DNSResolver.cpp

    // when the request completes.
    //
-    // To ensure the right thing happens, we NULL the OnComplete pointer here, which signals the
+    // To ensure the right thing happens, we nullptr the mOnComplete pointer here, which signals the


Suggested change

// To ensure the right thing happens, we nullptr the mOnComplete pointer here, which signals the

// To ensure the right thing happens, we null out the mOnComplete pointer here, which signals the

"nullptr" is not a verb....

Fixed and checked for other instances.

bzbarsky-apple · 2021-10-13T20:42:02Z

src/inet/DNSResolver.cpp

@@ -186,7 +188,7 @@ void DNSResolver::HandleResolveComplete()
 *  This method is called by LwIP network stack on success, failure, or timeout
 *  of a DNS request.
 *
- *  @param[in]  name            A pointer to a NULL-terminated C string
+ *  @param[in]  name            A pointer to a nullptr-terminated C string


No, it's not nullptr-terminated. It's terminated by '\0', presumably....

bzbarsky-apple · 2021-10-13T20:43:05Z

src/inet/DNSResolver.h

+        kUnused   = 0, ///< Used to indicate that the DNSResolver object is not used.
+        kActive   = 2, ///< Used to indicate that a DNS resolution is being performed on the DNSResolver object.
+        kComplete = 3, ///< Used to indicate that the DNS resolution on the DNSResolver object is complete.
+        kCanceled = 4, ///< Used to indicate that the DNS resolution on the DNSResolver has been canceled.


Suggested change

kUnused = 0, ///< Used to indicate that the DNSResolver object is not used.

kActive = 2, ///< Used to indicate that a DNS resolution is being performed on the DNSResolver object.

kComplete = 3, ///< Used to indicate that the DNS resolution on the DNSResolver object is complete.

kCanceled = 4, ///< Used to indicate that the DNS resolution on the DNSResolver has been canceled.

Unused = 0, ///< Used to indicate that the DNSResolver object is not used.

Active = 2, ///< Used to indicate that a DNS resolution is being performed on the DNSResolver object.

Complete = 3, ///< Used to indicate that the DNS resolution on the DNSResolver object is complete.

Canceled = 4, ///< Used to indicate that the DNS resolution on the DNSResolver has been canceled.

That "k" is just noise for enum class values....

bzbarsky-apple · 2021-10-13T20:44:42Z

src/inet/EndPointBasis.h

+        kUnknown = 0,
+        kRaw     = 1,
+        kUDP     = 2,
+        kUCP     = 3,
+        kTCP     = 4


Why this addition of "k"? We don't generally do that for enum classes, as far as I can tell, nor should we.

That got requested in a previous review. Although I wouldn't choose it personally, existing code uses the k prefix about 2/3 of the time in enum class. https://gist.github.com/kpschoedel/dc99300e71e9292d95c0c26471101397

We should come up with some clear guidelines for this. I've now seen people changing it to "no-k" and back to "with-k" in a tug-of-war, which is just wasting everyone's time. @woody-apple @andy31415

Again, my personal preference is to drop the k; I've generally seen it appear as a result of mass-conversions to enum class that kept it because it was there for the non-enum-class version....

I am on the use k side of the fence. The logic being that I would like to trivially see when constants are used in the code. If I have:

class Foo { public: constexpr int kBar = 1; } enum class Baz { kVal = 1; }

then Foo::kBar and Baz::kVal seem trivially visible as constants regardless where they come from. Otherwise having Baz::Val and Foo::kBar seems inconsitent and switching everything to drop k would make Foo access constants as just Bar which seems wrong.

bzbarsky-apple · 2021-10-13T20:49:29Z

src/inet/IPAddress.h

+    kUnknown = 0, ///< Not used.
+    kIPv4    = 1, ///< Internet protocol version 4.
+    kIPv6    = 2, ///< Internet protocol version 6.
+    kAny     = 3  ///< The unspecified internet address (independent of protocol version).


Again, would be better without the "k" prefix, which is a holdover from non-scoped enums.

bzbarsky-apple · 2021-10-13T20:49:57Z

src/inet/IPAddress.h

+    kTransient = 0x01,

    /** The multicast address is (1) based on a network prefix. */
-    kIPv6MulticastFlag_Prefix = 0x02
-} IPv6MulticastFlag;
+    kPrefix = 0x02


And here, better without "k".

kpschoedel · 2021-10-14T14:49:20Z

I stopped at the beginning of IPEndPointBasis.cpp. I just don't have a block of time of sufficient size to review something like this carefully... Might have been better as multiple smaller PRs.

I'll see whether I can split off parts without introducing too much sequential dependency.

#### Problem `Inet::IPAddress` has some redundancies and outdated comments. #### Change overview - Convert `IPAddress::From…()` functions that are semantically constructors into constructors. - Replace several identical `union`s with `IPAddress::SockAddr`. - Fix outdated documentation regarding IPv4-in-IPv6. - Fix a use of `IPAddress::kMaxStringLength` (from project-chip#10450 review). #### Testing CI; no changes to functionality intended.

* Revisions to Inet::IPAddress #### Problem `Inet::IPAddress` has some redundancies and outdated comments. #### Change overview - Convert `IPAddress::From…()` functions that are semantically constructors into constructors. - Replace several identical `union`s with `IPAddress::SockAddr`. - Fix outdated documentation regarding IPv4-in-IPv6. - Fix a use of `IPAddress::kMaxStringLength` (from #10450 review). #### Testing CI; no changes to functionality intended. * review

stale · 2021-10-25T15:08:01Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

kpschoedel · 2021-10-25T15:09:53Z

Closed because most of this has now moved to other PRs.

boring-cyborg bot added app controller examples inet labels Oct 13, 2021

pullapprove bot requested review from vivien-apple, wbschiller, woody-apple and yunhanw-google October 13, 2021 15:03

pullapprove bot added the review - pending label Oct 13, 2021

build/merge fixes

1b7db62

Damian-Nordic approved these changes Oct 13, 2021

View reviewed changes

tcarmelveilleux reviewed Oct 13, 2021

View reviewed changes

src/lib/support/ObjectLifeCycle.h Show resolved Hide resolved

tcarmelveilleux approved these changes Oct 13, 2021

View reviewed changes

review changes

52c7015

bzbarsky-apple reviewed Oct 13, 2021

View reviewed changes

review changes

bf8a495

kpschoedel mentioned this pull request Oct 14, 2021

Convert to enum class in src/inet #10504

Merged

kpschoedel marked this pull request as draft October 14, 2021 17:46

This was referenced Oct 14, 2021

Use ObjectLifeCycle for InetLayer #10522

Merged

Remove unused Send flags arguments #10532

Merged

Fix locking in UDPEndPoint::SendMsgImpl() #10533

Merged

kpschoedel mentioned this pull request Oct 21, 2021

Revisions to Inet::IPAddress #10791

Merged

kpschoedel mentioned this pull request Oct 22, 2021

Rearrange code in IPEndPointBasis #10849

Merged

stale bot added the stale Stale issue or PR label Oct 25, 2021

kpschoedel closed this Oct 25, 2021

kpschoedel deleted the x7715-inet-3 branch October 25, 2021 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miscellaneous src/inet cleanup #10450

Miscellaneous src/inet cleanup #10450

kpschoedel commented Oct 13, 2021

Damian-Nordic Oct 13, 2021

kpschoedel Oct 13, 2021

andy31415 Oct 18, 2021

Damian-Nordic Oct 13, 2021

kpschoedel Oct 13, 2021

tcarmelveilleux Oct 13, 2021

kpschoedel Oct 13, 2021

bzbarsky-apple left a comment

bzbarsky-apple Oct 13, 2021

kpschoedel Oct 13, 2021

bzbarsky-apple Oct 13, 2021

kpschoedel Oct 13, 2021

bzbarsky-apple Oct 13, 2021

kpschoedel Oct 13, 2021

bzbarsky-apple Oct 13, 2021

bzbarsky-apple Oct 13, 2021

kpschoedel Oct 13, 2021

bzbarsky-apple Oct 13, 2021

andy31415 Oct 18, 2021 •

edited

Loading

bzbarsky-apple Oct 13, 2021

bzbarsky-apple Oct 13, 2021

kpschoedel commented Oct 14, 2021

stale bot commented Oct 25, 2021

kpschoedel commented Oct 25, 2021

	// To ensure the right thing happens, we nullptr the mOnComplete pointer here, which signals the
	// To ensure the right thing happens, we null out the mOnComplete pointer here, which signals the

Miscellaneous src/inet cleanup #10450

Miscellaneous src/inet cleanup #10450

Conversation

kpschoedel commented Oct 13, 2021

Problem

Change overview

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzbarsky-apple left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy31415 Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpschoedel commented Oct 14, 2021

stale bot commented Oct 25, 2021

kpschoedel commented Oct 25, 2021

andy31415 Oct 18, 2021 •

edited

Loading