-
Notifications
You must be signed in to change notification settings - Fork 18.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
libnet: Controller: more c.store clean-ups #47820
libnet: Controller: more c.store clean-ups #47820
Conversation
@@ -105,15 +105,15 @@ func (ec *endpointCnt) EndpointCnt() uint64 { | |||
} | |||
|
|||
func (ec *endpointCnt) updateStore() error { | |||
store := ec.n.getController().getStore() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also removes the getController()
part, which does appear to have synchronisation; is it safe to skip it here?
At least at a quick glance, it looks like there's potential paths that mutate that field, so maybe we still need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's safe to remove this call -- there's no way n.ctrler
can be mutated once the network is loaded from the datastore. But I've undone this change and will do that in a separate commit / PR.
This was done in a separate method, called by the ctrler constructor. This method was returning a nil datastore when c.cfg was nil -- but that can't happen in practice! This was giving the impression that the controller could be run without a datastore properly configured. It's not the case, so make it explicit by instantiating the datastore before `Controller`. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Previous commit made it clear that c.store can't be nil. Hence, `c.store.Close()` can be called without checking if c.store is nil. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
This method does nothing more than `return c.store`. It has no value and adds an unecessary level of indirection. Let's ditch it. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
9583539
to
6d21574
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
libnet: init datastore in ctrler constructor
This was done in a separate method, called by the ctrler constructor.
This method was returning a nil datastore when c. cfg was nil -- but that can't happen in practice!
This was giving the impression the controller could be run without a datastore properly configured. It's not the case, so make it explicit by instantiating the datastore before
Controller
.libnet: Controller: drop closeStores
Previous commit made it clear that c.store can't be nil. Hence,
c.store.Close()
can be called without checking if c.store is nil.libnet: Controller: drop getStore()
This method does nothing more than
return c.store
. It has no value and adds an unecessary level of indirection. Let's ditch it.- How I did it
Static analysis.
- How to verify it
CI.
- A picture of a cute animal (not mandatory but encouraged)