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.

Sign up for GitHub

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

Jump to bottom

net: interface_linux.go has redundant code #62683

Open
wlynxg opened this issue Sep 17, 2023 · 6 comments
Open

net: interface_linux.go has redundant code #62683

wlynxg opened this issue Sep 17, 2023 · 6 comments
Labels
help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@wlynxg
Copy link

wlynxg commented Sep 17, 2023

What version of Go are you using (go version)?

$ go version
go version go1.20 linux/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env

What did you do?

func addrTable(ift []Interface, ifi *Interface, msgs []syscall.NetlinkMessage) ([]Addr, error) {
	var ifat []Addr
loop:
	for _, m := range msgs {
		switch m.Header.Type {
		case syscall.NLMSG_DONE:
			break loop
		case syscall.RTM_NEWADDR:
			ifam := (*syscall.IfAddrmsg)(unsafe.Pointer(&m.Data[0]))
			if len(ift) != 0 || ifi.Index == int(ifam.Index) {
				if len(ift) != 0 {
					var err error
					ifi, err = interfaceByIndex(ift, int(ifam.Index))
					if err != nil {
						return nil, err
					}
				}
				attrs, err := syscall.ParseNetlinkRouteAttr(&m)
				if err != nil {
					return nil, os.NewSyscallError("parsenetlinkrouteattr", err)
				}
				ifa := newAddr(ifam, attrs)
				if ifa != nil {
					ifat = append(ifat, ifa)
				}
			}
		}
	}
	return ifat, nil
}

In the above code, the following code logic seems to be an invalid code logic.

if len(ift) != 0 {
    var err error
    ifi, err = interfaceByIndex(ift, int(ifam.Index))
    if err != nil {
        return nil, err
    }
}

len(ift) != 0 || ifi.Index == int(ifam.Index) never executes ifi.Index == int(ifam.Index) when len(ift) != 0,
therefore reassigning ifi has no effect.

What did you expect to see?

What did you see instead?
@panjf2000
Copy link
Member

I assume what you meant is that it's needless to call interfaceByIndex reassigning the ifi when len(ift) != 0 and ifi.Index == int(ifam.Index) are both true, right?

@wlynxg
Copy link
Author

wlynxg commented Sep 17, 2023

I assume what you meant is that it's needless to call interfaceByIndex reassigning the ifi when len(ift) != 0 and ifi.Index == int(ifam.Index) are both true, right?

When len(ift) != 0 is true, the judgment logic of ifi.Index == int(ifam.Index) will not take effect, so there is no point in reassigning ifi.

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 18, 2023
@ianlancetaylor
Copy link
Contributor

I agree that this code doesn't make sense, but what I don't know is what it is supposed to do. When this code was introduced in https://go.dev/cl/7400055, it made sense because ifi was passed to newAddr. That was changed in https://go.dev/cl/37892, because newAddr didn't use that parameter. But newAddr used to use the parameter; that part changed in (I think) https://go.dev/cl/11352. So I guess that we could just stop setting ifi in the loop today. But I'd rather that somebody who knows what this code is doing double-check that it is doing what it is supposed to do.

@ianlancetaylor ianlancetaylor added this to the Backlog milestone Sep 18, 2023
wlynxg added a commit to wlynxg/anet that referenced this issue Sep 19, 2023
@wlynxg
fix: remove redundant code, see for details: golang/go#62683
aa9594d
@aimuz
Copy link
Contributor

aimuz commented Sep 26, 2023

Can I try to finish the job?

@wlynxg
Copy link
Author

wlynxg commented Sep 26, 2023 •
edited

Can I try to finish the job?

Now we need to wait for the person who left this code to confirm that deleting this code will not have some other effects.

@aimuz
Copy link
Contributor

aimuz commented Sep 26, 2023 •
edited

Okey

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
5 participants
@ianlancetaylor @panjf2000 @seankhliao @aimuz @wlynxg