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

Update methods NewGroup, DeleteGroup and GetGroup of OFSwitch #46

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hongliangl
Copy link

Add parameter NewGroup to decide whether to use cache in OFSwitch when create a group.

Signed-off-by: Hongliang Liu lhongliang@vmware.com

Copy link

@wenyingd wenyingd left a comment

Choose a reason for hiding this comment

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

Some errors happen with go-lint check, could you check?

ofctrl/fgraphSwitch.go Outdated Show resolved Hide resolved
ofctrl/fgraphSwitch.go Outdated Show resolved Hide resolved
ofctrl/fgraphSwitch.go Outdated Show resolved Hide resolved
@hongliangl hongliangl force-pushed the 20230302-update-NewGroup branch 2 times, most recently from 3f1b729 to b791ab7 Compare March 2, 2023 09:13
@hongliangl
Copy link
Author

Some errors happen with go-lint check, could you check?

Done

@wenyingd
Copy link

Maybe you can split this one into 2 patches: one is for yout group operation changes, and the other is for golangci change; as I see other patches also have been impacted by it.

@hongliangl
Copy link
Author

hongliangl commented Mar 10, 2023

Maybe you can split this one into 2 patches: one is for yout group operation changes, and the other is for golangci change; as I see other patches also have been impacted by it.

Add #48 for this.

ofctrl/fgraphSwitch.go Outdated Show resolved Hide resolved
if self.groupDb[groupId] != nil {
return nil, errors.New("group already exists")
// NewGroup creates a new group; when using cache, returns an error if the group ID already exists, otherwise returns the
// group object; when not using cache, returns the new group object, and the caller should ensure the groupID is not

Choose a reason for hiding this comment

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

Suggested change
// group object; when not using cache, returns the new group object and the caller should ensure the groupID is not
// group object; when useCache is false, a new group object is returned and the caller should ensure the groupID is not

Copy link
Author

Choose a reason for hiding this comment

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

Thanks.

Not related to this PR: you could select multiple lines when reviewing code on time.

// duplicated.
func (self *OFSwitch) NewGroup(groupID uint32, groupType GroupType, useCache bool) (*Group, error) {
// Check if the group already exists.
if useCache {

Choose a reason for hiding this comment

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

Use this format?

if !useCache {
    return newGroup(groupID, groupType, self), nil
}
if self.groupDb[groupID] != nil {
    return nil, errors.New("group already exists")
}
return newGroup(groupID, groupType, self), nil

Copy link
Author

Choose a reason for hiding this comment

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

Updated

ofctrl/fgraphSwitch.go Outdated Show resolved Hide resolved
Signed-off-by: Hongliang Liu <lhongliang@vmware.com>
Copy link

@wenyingd wenyingd left a comment

Choose a reason for hiding this comment

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

LGTM

@wenyingd wenyingd requested a review from tnqn March 31, 2023 10:26
@wenyingd
Copy link

wenyingd commented Apr 7, 2023

@hongliangl Could you resolve the conflicts on version changes?
@tnqn Could you help review this patch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants