Skip to content
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: don't check if ctrler store is nil #47810

Merged
merged 1 commit into from
May 9, 2024

Conversation

akerouanton
Copy link
Member

- What I did

Since commit befff0e, (*Controller).getStore() never returns nil except if c.store isn't initialized yet. This can't happen unless New() returned an error and it wasn't proper caught.

- How to verify it

CI

Since commit befff0e, `(*Controller).getStore()` never returns nil
except if `c.store` isn't initialized yet. This can't happen unless
`New()` returned an error and it wasn't proper caught.

Signed-off-by: Albin Kerouanton <albinker@gmail.com>
@thaJeztah
Copy link
Member

Looks like we may be able to clean up more (in a follow-up); let me jot down some things I noticed;

Perhaps we should remove Controller.initStores(). It looks like it's ONLY called from New(), so it would be clearer to be part of that constructor. It also currently has a if c.cfg == nil check, which should belong there if it should never have an empty config; currently reading that code may lead to the assumption that the store COULD be nil (no config), and that not producing an error;

moby/libnetwork/store.go

Lines 13 to 24 in 4554d87

func (c *Controller) initStores() error {
if c.cfg == nil {
return nil
}
var err error
c.store, err = datastore.New(c.cfg.Scope)
if err != nil {
return err
}
return nil
}

But looking at the constructor, it can never be nil, so that nil-check is redundant (and deceiving);

cfg: config.New(cfgOptions...),

Based on this PR, perhaps Controller.closeStores() is also overly cautious and could have the nil-check removed;

moby/libnetwork/store.go

Lines 26 to 30 in 4554d87

func (c *Controller) closeStores() {
if store := c.store; store != nil {
store.Close()
}
}

And, finally, perhaps we should consider reducing use of Controller.getStore() when used internally; it looks like it does not do any synchronisation (which, if it's set in the constructor, and never mutated, wouldn't be needed), which means that checks like the one below are also redundant (and could just be using c.store directly;

moby/libnetwork/store.go

Lines 48 to 53 in 4554d87

store := c.getStore()
if store == nil {
return nil, nil
}
kvol, err := store.List(&Network{ctrlr: c})

@thaJeztah thaJeztah added this to the 27.0.0 milestone May 9, 2024
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah thaJeztah merged commit 64da0e0 into moby:master May 9, 2024
141 checks passed
@akerouanton akerouanton deleted the libnet-store-is-never-nil branch May 9, 2024 13:43
@akerouanton akerouanton self-assigned this May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants