Internal refactoring to avoid unneeded netlink parsing

The parsing of some netlink structures is performed only for those vdpa objects
that need to be returned to the user.

Signed-off-by: Leonardo Milleri <[email protected]>

cmd/kvdpa-cli/kvdpa-cli.go

@@ -3,10 +3,10 @@ package main
 import (
 	"fmt"
 	"os"
-	"strings"
 	"text/template"
 
 	vdpa "github.com/k8snetworkplumbingwg/govdpa/pkg/kvdpa"
+	util "github.com/k8snetworkplumbingwg/govdpa/pkg/util"
 	cli "github.com/urfave/cli/v2"
 )
 
@@ -26,26 +26,19 @@ const deviceTemplate = ` - Name: {{ .Name }}
 func listAction(c *cli.Context) error {
 	var devs []vdpa.VdpaDevice
 	var err error
-	if c.String("mgmtdev") != "" {
-		var bus, name string
-		nameParts := strings.Split(c.String("mgmtdev"), "/")
-		if len(nameParts) == 1 {
-			name = nameParts[0]
-		} else if len(nameParts) == 2 {
-			bus = nameParts[0]
-			name = nameParts[1]
-		} else {
-			return fmt.Errorf("Invalid management device name %s", c.String("mgmtdev"))
-		}
-		devs, err = vdpa.GetVdpaDevicesByMgmtDev(bus, name)
+	var mgmtDev = c.String("mgmtdev")
+	if mgmtDev != "" {
+		var busName, devName string
+		busName, devName, err = util.ExtractBusAndMgmtDevice(mgmtDev)
 		if err != nil {
 			return err
 		}
+		devs, err = vdpa.GetVdpaDevicesByMgmtDev(busName, devName)
 	} else {
 		devs, err = vdpa.ListVdpaDevices()
-		if err != nil {
-			fmt.Println(err)
-		}
+	}
+	if err != nil {
+		return err
 	}
 	tmpl := template.Must(template.New("device").Parse(deviceTemplate))
 

go.mod

@@ -6,7 +6,7 @@ require (
 	github.com/stretchr/testify v1.7.0
 	github.com/urfave/cli/v2 v2.2.0
 	github.com/vishvananda/netlink v1.1.0
-	golang.org/x/sys v0.1.0
+	golang.org/x/sys v0.12.0
 )
 
 require (

go.sum

@@ -21,8 +21,8 @@ github.com/vishvananda/netlink v1.1.0/go.mod h1:cTgwzPIzzgDAYoQrMm0EdrjRUBkTqKYp
 github.com/vishvananda/netns v0.0.0-20191106174202-0a2b9b5464df h1:OviZH7qLw/7ZovXvuNyL3XQl8UFofeikI1NW1Gypu7k=
 github.com/vishvananda/netns v0.0.0-20191106174202-0a2b9b5464df/go.mod h1:JP3t17pCcGlemwknint6hfoeCVQrEMVwxRLRjXpq+BU=
 golang.org/x/sys v0.0.0-20190606203320-7fc4e5ec1444/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
-golang.org/x/sys v0.1.0 h1:kunALQeHf1/185U1i0GOB/fy1IPRDDpuoOOqRReG57U=
-golang.org/x/sys v0.1.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
+golang.org/x/sys v0.12.0 h1:CM0HF96J0hcLAwsHPJZjfdNzs0gftsLfgKt57wWHJ0o=
+golang.org/x/sys v0.12.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=

pkg/kvdpa/device.go

@@ -1,12 +1,11 @@
 package kvdpa
 
 import (
-	"errors"
 	"os"
 	"path/filepath"
-	"strings"
 	"syscall"
 
+	util "github.com/k8snetworkplumbingwg/govdpa/pkg/util"
 	"github.com/vishvananda/netlink/nl"
 	"golang.org/x/sys/unix"
 )
@@ -185,7 +184,8 @@ func GetVdpaDevice(name string) (VdpaDevice, error) {
 		return nil, err
 	}
 
-	vdpaDevs, err := parseDevLinkVdpaDevList(msgs)
+	// No filters, expecting to parse attributes for the device with the given name
+	vdpaDevs, err := parseDevLinkVdpaDevList("", "", msgs)
 	if err != nil {
 		return nil, err
 	}
@@ -197,50 +197,27 @@ GetVdpaDevicesByMgmtDev returns the VdpaDevice objects whose MgmtDev
 has the given bus and device names.
 */
 func GetVdpaDevicesByMgmtDev(busName, devName string) ([]VdpaDevice, error) {
-	result := []VdpaDevice{}
-	devices, err := ListVdpaDevices()
-	if err != nil {
-		return nil, err
-	}
-	for _, device := range devices {
-		if device.MgmtDev() != nil &&
-			device.MgmtDev().BusName() == busName &&
-			device.MgmtDev().DevName() == devName {
-			result = append(result, device)
-		}
-	}
-	if len(result) == 0 {
-		return nil, syscall.ENODEV
-	}
-	return result, nil
+	return listVdpaDevicesWithBusDevName(busName, devName)
 }
 
 /*ListVdpaDevices returns a list of all available vdpa devices */
 func ListVdpaDevices() ([]VdpaDevice, error) {
+	return listVdpaDevicesWithBusDevName("", "")
+}
+
+func listVdpaDevicesWithBusDevName(busName, devName string) ([]VdpaDevice, error) {
 	msgs, err := GetNetlinkOps().RunVdpaNetlinkCmd(VdpaCmdDevGet, syscall.NLM_F_DUMP, nil)
 	if err != nil {
 		return nil, err
 	}
 
-	vdpaDevs, err := parseDevLinkVdpaDevList(msgs)
+	vdpaDevs, err := parseDevLinkVdpaDevList(busName, devName, msgs)
 	if err != nil {
 		return nil, err
 	}
 	return vdpaDevs, nil
 }
 
-func extractBusNameAndMgmtDeviceName(fullMgmtDeviceName string) (busName string, mgmtDeviceName string, err error) {
-	numSlashes := strings.Count(fullMgmtDeviceName, "/")
-	if numSlashes > 1 {
-		return "", "", errors.New("expected mgmtDeviceName to be either in the format <mgmtBusName>/<mgmtDeviceName> or <mgmtDeviceName>")
-	} else if numSlashes == 0 {
-		return "", fullMgmtDeviceName, nil
-	} else {
-		values := strings.Split(fullMgmtDeviceName, "/")
-		return values[0], values[1], nil
-	}
-}
-
 /*
 GetVdpaDevicesByPciAddress returns the VdpaDevice objects for the given pciAddress
 
@@ -249,7 +226,7 @@ GetVdpaDevicesByPciAddress returns the VdpaDevice objects for the given pciAddre
 	- MgmtDevName
 */
 func GetVdpaDevicesByPciAddress(pciAddress string) ([]VdpaDevice, error) {
-	busName, mgmtDeviceName, err := extractBusNameAndMgmtDeviceName(pciAddress)
+	busName, mgmtDeviceName, err := util.ExtractBusAndMgmtDevice(pciAddress)
 	if err != nil {
 		return nil, unix.EINVAL
 	}
@@ -263,7 +240,7 @@ func AddVdpaDevice(mgmtDeviceName string, vdpaDeviceName string) error {
 		return unix.EINVAL
 	}
 
-	busName, mgmtDeviceName, err := extractBusNameAndMgmtDeviceName(mgmtDeviceName)
+	busName, mgmtDeviceName, err := util.ExtractBusAndMgmtDevice(mgmtDeviceName)
 	if err != nil {
 		return unix.EINVAL
 	}
@@ -317,7 +294,7 @@ func DeleteVdpaDevice(vdpaDeviceName string) error {
 	return nil
 }
 
-func parseDevLinkVdpaDevList(msgs [][]byte) ([]VdpaDevice, error) {
+func parseDevLinkVdpaDevList(busName string, mgmtDeviceName string, msgs [][]byte) ([]VdpaDevice, error) {
 	devices := make([]VdpaDevice, 0, len(msgs))
 
 	for _, m := range msgs {
@@ -329,6 +306,15 @@ func parseDevLinkVdpaDevList(msgs [][]byte) ([]VdpaDevice, error) {
 		if err = dev.parseAttributes(attrs); err != nil {
 			return nil, err
 		}
+
+		if busName != "" && busName != dev.mgmtDev.busName {
+			continue
+		}
+
+		if mgmtDeviceName != "" && mgmtDeviceName != dev.mgmtDev.devName {
+			continue
+		}
+
 		if err = dev.getBusInfo(); err != nil {
 			return nil, err
 		}

pkg/kvdpa/device_test.go

@@ -128,6 +128,98 @@ func TestVdpaDevList(t *testing.T) {
 	}
 }
 
+func TestVdpaDevListWithFilter(t *testing.T) {
+	tests := []struct {
+		name     string
+		err      bool
+		response []VdpaDevice
+	}{
+		{
+			name: "Multiple SR-IOV and SF devices",
+			err:  false,
+			response: []VdpaDevice{
+				&vdpaDev{
+					name: "vdpa0",
+					mgmtDev: &mgmtDev{
+						devName: "0000:01:01",
+					},
+				},
+				&vdpaDev{
+					name: "vdpa1",
+					mgmtDev: &mgmtDev{
+						busName: "pci",
+						devName: "0000:01:02",
+					},
+				},
+				&vdpaDev{
+					name: "vdpa2",
+					mgmtDev: &mgmtDev{
+						busName: "pci",
+						devName: "0000:01:02",
+					},
+				},
+				&vdpaDev{
+					name: "vdpa3",
+					mgmtDev: &mgmtDev{
+						busName: "pci",
+						devName: "0000:01:03",
+					},
+				},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(fmt.Sprintf("%s_%s", "TestVdpaDevListWithFilter", tt.name), func(t *testing.T) {
+			netLinkMock := &mocks.NetlinkOps{}
+			SetNetlinkOps(netLinkMock)
+			netLinkMock.On("RunVdpaNetlinkCmd",
+				VdpaCmdDevGet,
+				mock.MatchedBy(func(flags int) bool {
+					return (flags|syscall.NLM_F_DUMP != 0)
+				}),
+				mock.AnythingOfType("[]*nl.RtAttr")).
+				Return(vdpaDevToNlMessage(t, tt.response...), nil)
+			// no filters, all devices are returned
+			devs, err := ListVdpaDevices()
+			if tt.err {
+				assert.NotNil(t, err)
+			} else {
+				assert.Nil(t, err)
+				assert.Equal(t, tt.response, devs)
+			}
+			// mgmtdev: 0000:01:01
+			devs, err = GetVdpaDevicesByPciAddress("0000:01:01")
+			if tt.err {
+				assert.NotNil(t, err)
+			} else {
+				assert.Nil(t, err)
+				assert.Equal(t, len(devs), 1)
+				assert.Equal(t, tt.response[0], devs[0])
+			}
+			// mgmtdev: pci/0000:01:02
+			devs, err = GetVdpaDevicesByPciAddress("pci/0000:01:02")
+			if tt.err {
+				assert.NotNil(t, err)
+			} else {
+				assert.Nil(t, err)
+				assert.Equal(t, len(devs), 2)
+				assert.Equal(t, tt.response[1], devs[0])
+				assert.Equal(t, tt.response[2], devs[1])
+			}
+			// mgmtdev: pci/0000:01:03
+			devs, err = GetVdpaDevicesByPciAddress("pci/0000:01:03")
+			if tt.err {
+				assert.NotNil(t, err)
+			} else {
+				assert.Nil(t, err)
+				assert.Equal(t, len(devs), 1)
+				assert.Equal(t, tt.response[3], devs[0])
+			}
+		})
+	}
+}
+
 func TestVdpaDevGet(t *testing.T) {
 	tests := []struct {
 		name     string
@@ -304,7 +396,7 @@ func TestVdpaDevGetByMgmt(t *testing.T) {
 		},
 		{
 			name:        "Wrong",
-			err:         syscall.ENODEV,
+			response:    []VdpaDevice{},
 			mgmtDevName: "noDev",
 			mgmtBusName: "wrongBus",
 		},

pkg/util/util.go

@@ -0,0 +1,22 @@
+package util
+
+import (
+	"errors"
+	"strings"
+)
+
+// ExtractBusAndMgmtDevice extracts the busName and deviceName from a full device address (e.g. pci)
+// example 1: pci/65:0000.1   -> "pci", "65:0000.1",  nil
+// example 2: vdpa_sim        -> "",    "vdpa_sim",   nil
+// example 3: pci/65:0000.1/1 -> "",    "",           err
+func ExtractBusAndMgmtDevice(fullMgmtDeviceName string) (busName string, mgmtDeviceName string, err error) {
+	numSlashes := strings.Count(fullMgmtDeviceName, "/")
+	if numSlashes > 1 {
+		return "", "", errors.New("expected mgmtDeviceName to be either in the format <mgmtBusName>/<mgmtDeviceName> or <mgmtDeviceName>")
+	} else if numSlashes == 0 {
+		return "", fullMgmtDeviceName, nil
+	} else {
+		values := strings.Split(fullMgmtDeviceName, "/")
+		return values[0], values[1], nil
+	}
+}

pkg/util/util_test.go

@@ -0,0 +1,53 @@
+package util
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestExtractBusAndMgmtDevice(t *testing.T) {
+	tests := []struct {
+		testName      string
+		deviceAddress string
+		busName       string
+		devName       string
+		err           bool
+	}{
+		{
+			testName:      "regular PCI address",
+			deviceAddress: "pci/0000:65:00.1",
+			busName:       "pci",
+			devName:       "0000:65:00.1",
+			err:           false,
+		},
+		{
+			testName:      "no bus",
+			deviceAddress: "vdpa_sim",
+			busName:       "",
+			devName:       "vdpa_sim",
+			err:           false,
+		},
+		{
+			testName:      "wrong address",
+			deviceAddress: "pci/0000:65:00.1/0",
+			busName:       "",
+			devName:       "",
+			err:           true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(fmt.Sprintf("%s_%s", "TestExtractBusAndMgmtDevice", tt.testName), func(t *testing.T) {
+			busName, devName, err := ExtractBusAndMgmtDevice(tt.deviceAddress)
+			if tt.err {
+				assert.NotNil(t, err)
+			} else {
+				assert.Nil(t, err)
+				assert.Equal(t, tt.busName, busName)
+				assert.Equal(t, tt.devName, devName)
+			}
+		})
+	}
+}