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

fix_scope_shadowing_err_in_netboot_ConfigureInterface #538

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

youngifif
Copy link

@youngifif youngifif commented Jun 3, 2024

Here is my long story:

I find that netlink doesn't allow to add the same address to one dev multiple time.
TestConfigureInterface should fail to add 10.20.30.40 to eth0 in second time/second test case ,but it dosen't.

So,I think there is a bug in the func TestConfigureInterface located in netboot/netconf_integ_test.go
And the reason TestConfigureInterface dosen't fail is that anonther bug located in func ConfigureInterface

To prove that, i use iproute2 tools in debian12 to illustrate the problem as follow:
ip address flush dev enp0s8
To clean all address attached to dev enp0s8
ip address add 10.20.30.40 dev enp0s8
First time,to add 10.20.30.40 succesfully
ip address add 10.20.30.40 dev enp0s8
Second time, an error return and terminal display RTNETLINK answers: File exists from iproute2.

Same reuslt in my test code which use rt.AddrAdd(iface,&addr) twice.

package iproute2

import (
	"net"
	"testing"

	"github.com/jsimonetti/rtnetlink/rtnl"
	"github.com/stretchr/testify/require"
)

var (

	ifname string = "enp0s8"//change ifname to  dev name in your machine
)



//before test,run  `sudo ip addr flush dev enp0s8`  to clean all addr on dev
func TestNetLinkAddrAdd(t *testing.T) {
	

	iface, err := net.InterfaceByName(ifname)
	require.NoError(t, err)


	rt, err := rtnl.Dial(nil)
	require.NoError(t, err)

	addr1 :=  net.IPNet{IP: net.ParseIP("10.20.30.40")}
	err1 := rt.AddrAdd(iface,&addr1)
	require.NoError(t, err1)//First Time,we succeed to add the addr to dev.




	addr2 :=  net.IPNet{IP: net.ParseIP("10.20.30.40")}
	err2 := rt.AddrAdd(iface,&addr2)
	require.Error(t, err2)//Second Time,we failed to add the addr to dev.
	// Error:      	Received unexpected error:
	// netlink receive: file exists
	
}

After a further reaserch, i believe that the func netboot.ConfigureInterface has a bug, too.
to be briefly,ConfigureInterface always return nil.

func ConfigureInterface(ifname string, netconf *NetConf) (err error) { 
	iface, err := net.InterfaceByName(ifname)
	if err != nil {
		return err
	}
	rt, err := rtnl.Dial(nil)
	if err != nil {
		return err
	}
	defer func() {
		if cerr := rt.Close(); err != nil {  //line1 
			err = cerr
		}
	}()
	for _, addr := range netconf.Addresses {
		if err := rt.AddrAdd(iface, &addr.IPNet); err != nil {  
			return fmt.Errorf("cannot configure %s on %s: %v", ifname, addr.IPNet, err) //line2

		}
	}
  //...
}

When line2 return err which is not nil ,the defer closure check the err != nil in line1, then reassign the err to cerr.
But cerr := rt.Close() is nil in most case ,
which means that ConfigureInterface nearly always return nil
even if line2 return err != nil inside the function .
I think we should only reassign reutrn err to cerr,when return err is nil.

Signed-off-by: youngifif <youngifif@github.com>
@pmazzini
Copy link
Collaborator

pmazzini commented Jun 4, 2024

Integration tests are failing.

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