GATEWAYS-4422 - Add DumpPortMappings API to retrieve ofport and MAC address for tap interfaces #141

yrkurapati · 2025-11-14T22:25:53Z

• Adds new DumpPortMappings method to OpenFlowService for retrieving port mappings
• Parses ovs-ofctl show output to extract ofport numbers and MAC addresses
• Filters results to include only tap interfaces and LOCAL port
• Returns map of interface names to PortMapping structs containing ofport and MAC
• Includes comprehensive test coverage with 5 test cases

…faces

do-msingh · 2025-11-17T21:36:29Z

ovs/openflow.go

+// PortMapping contains the ofport number and MAC address for an interface.
+type PortMapping struct {
+	// OfPort specifies the OpenFlow port number.
+	OfPort int


Suggest ofPort to maintain consistency. Ditto for macAddress

do-msingh · 2025-11-17T22:00:23Z

ovs/openflow.go

+	mappings := make(map[string]*PortMapping)
+	scanner := bufio.NewScanner(bytes.NewReader(out))
+
+	for scanner.Scan() {


Could you follow semantics similar to parseEach() and define your own parseEachPort() or something like that, instead of regexp?

showPrefix =[]byte("OFPT_FEATURES_REPLY")

do-msingh · 2025-11-17T22:01:35Z

ovs/openflow.go

 	"errors"
 	"fmt"
 	"io"
+	"regexp"


These may not be needed if you switch to parseEach semantics.

do-msingh · 2025-11-17T22:02:17Z

ovs/openflow.go

+	// portMappingPattern matches port lines from 'ovs-ofctl show' output.
+	// Pattern matches: " 7(interface1): addr:fe:4f:76:09:88:2b"
+	// or " 65534(LOCAL): addr:fe:4f:76:09:88:2b"
+	portMappingPattern = regexp.MustCompile(`^\s*(\d+)\(([^)]+)\):\s*addr:([a-fA-F0-9:]+)`)


Should be able to remove this.

do-msingh · 2025-11-17T22:04:40Z

ovs/openflow_test.go

+	}
+}
+
+func TestClientOpenFlowDumpPortMappingsOnlyLOCAL(t *testing.T) {


Instead of a test case specifically for LOCAL, this should be a generic one to test for a specified interface index

do-msingh · 2025-11-17T22:11:53Z

ovs/openflow.go


+// DumpPortMappings retrieves port mappings (ofport and MAC address) for all
+// interfaces from the specified bridge. The output is parsed from 'ovs-ofctl show' command.
+func (o *OpenFlowService) DumpPortMappings(bridge string) (map[string]*PortMapping, error) {


I would like to add one variant of this function in addition to this function. It should also be possible using another function to return the PortMapping for a single interface index provided as an input. Also, see my comment in the test section - you can modify the test accordingly.

- Added JSON tags (ofPort, macAddress) to PortMapping struct for consistency - Refactored to use parseEachPort() semantics instead of regex, following parseEach() pattern - Removed unused regexp import and portMappingPattern variable - Updated test to be generic table-driven instead of LOCAL-specific - Added DumpPortMapping (singular) function for single interface lookup - Fixed some error handling consistency, add MAC/ofport validation, and eliminate code duplication

do-msingh · 2025-11-17T23:27:28Z

ovs/openflow.go

+	// Find the opening parenthesis
+	openParen := strings.IndexByte(s, '(')
+	if openParen == -1 {
+		return fmt.Errorf("invalid port mapping format: missing opening parenthesis")


It should be sufficient to indicate invalid format for both open & close paren error cases without being too specific.

I will simplify the error handling - any malformed line would return the generic ‘invalid port mapping format’ message, so we don’t over-specify which character is missing.

do-msingh · 2025-11-17T23:31:07Z

ovs/openflow.go

+	}
+
+	// Find the closing parenthesis
+	closeParen := strings.IndexByte(s[openParen:], ')')


Why not look up in s, like the openParen case? Then you won't have to add the index.

Good point - that would make both ( and ) searched directly in the full string, so thers's no extra index math anymore

do-msingh · 2025-11-17T23:34:19Z

ovs/openflow.go

+
+	// Find "addr:" after the closing parenthesis
+	addrPrefix := ": addr:"
+	addrIdx := strings.Index(s[closeParen:], addrPrefix)


Similar comment as above

do-msingh · 2025-11-17T23:52:48Z

ovs/openflow.go

+	mappings := make(map[string]*PortMapping)
+	err = parseEachPort(out, showPrefix, func(line []byte) error {
+		// Check if line matches port format: " 7(interface1): addr:..."
+		if !bytes.Contains(line, []byte("(")) || !bytes.Contains(line, []byte("): addr:")) {


Why are all these checks needed? L445 call to UnmarshalText will do all this anyway, no?

Noted. I will plan to remove them.
After UnmarshalText succeeds, we slice s[openParen+1 : closeParen] without rechecking openParen/closeParen (openflow.go, lines 432-435).
Is it acceptable to trust UnmarshalText (since it already checks parentheses), or do you want a defensive guard to skip unexpected lines?

I don't think an additional check is necessary. This is a pretty custom function meant only for this purpose. There won't be any other callers for this variant of UnmarshalText. UnmarshalText is supposed to do the heavy lifting including parsing etc and returning the parsed values.

Understood, I will remove the redundant checks.

do-msingh · 2025-11-17T23:53:46Z

ovs/openflow.go

+// PortMapping contains the ofport number and MAC address for an interface.
+type PortMapping struct {
+	// OfPort specifies the OpenFlow port number.
+	OfPort int `json:"ofPort"`


Why json encodings? I was suggesting changing the var name from OfPort to ofPort.

I initially interpreted your “ofPort/macAddress” note as wanting camelCase JSON keys, so I added tags instead of renaming the exported fields. Will make the suggested changes.

Current code uses ofPort/macAddress (lowercase) in PortMapping (openflow.go, lines 75-82). This breaks external callers who expect PortMapping.OfPort.

Could you confirm if we were supposed to only rename the exported fields to OfPort/MACAddress, or actually hide them? Should we revert to exported fields and have camelCase in comments/docs instead?

Was suggesting a similar case as the type structures at L62 and L70. Since this is a new type definition, I am curious which external callers (other than test) it breaks.
If test isn't able to work with this case, we can ignore this comment.

- Removeed JSON tags and use unexported fields (ofPort, macAddress) following L62-66 pattern - Removed redundant format checks in callback (UnmarshalText handles all validation) - Fixed string slicing to search full string for closeParen and addrIdx - Consolidated error messages to generic 'invalid port mapping format'

do-msingh · 2025-11-18T01:59:10Z

ovs/openflow.go

+			return nil
+		}
+
+		// Extract interface name from the validated line for map key


You can move parsing of interface name to UnmarshalText as well. You may need to declare another structure to accomplish this e.g.

type PortAttr struct { // ofPort specifies the OpenFlow port number. ofPort int // macAddress specifies the MAC address of the interface. macAddress string } type PortMapping struct { portAttr PortAttr ifName string }

Add DumpPortMappings API to retrieve ofport and MAC address for inter…

7a2a409

…faces

yrkurapati force-pushed the GATEWAYS-4422-implement-go-openvswitch-api-for-ofport-interface-mapping branch from 24eb418 to 7a2a409 Compare November 14, 2025 23:11

yrkurapati marked this pull request as ready for review November 17, 2025 18:41

do-msingh reviewed Nov 17, 2025

View reviewed changes

yrkurapati requested a review from do-msingh November 18, 2025 01:56

do-msingh reviewed Nov 18, 2025

View reviewed changes

GATEWAYS-4422 - Add DumpPortMappings API to retrieve ofport and MAC address for tap interfaces #141

Are you sure you want to change the base?

GATEWAYS-4422 - Add DumpPortMappings API to retrieve ofport and MAC address for tap interfaces #141

Uh oh!

Conversation

yrkurapati commented Nov 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yrkurapati Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yrkurapati Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yrkurapati Nov 18, 2025 •

edited

Loading

yrkurapati Nov 18, 2025 •

edited

Loading