feedback: order struct members, add TextUn/Marshal, fix SVC.String fo…

…rmat, add FormatHostPort, use MustParseHost

pkg/addr/addr.go

@@ -71,6 +71,14 @@ func (a *Addr) Set(s string) error {
 	return nil
 }
 
+func (a Addr) MarshalText() ([]byte, error) {
+	return []byte(a.String()), nil
+}
+
+func (a *Addr) UnmarshalText(b []byte) error {
+	return a.Set(string(b))
+}
+
 // ParseAddrPort parses s as a SCION address with a port, in the format
 //
 //	[<ISD>-<AS>,<Host>]:<Port>.
@@ -97,3 +105,13 @@ func ParseAddrPort(s string) (Addr, uint16, error) {
 	}
 	return a, uint16(p), nil
 }
+
+// FormatAddrPort formats an Addr with a port to the format
+//
+//	[<ISD>-<AS>,<Host>]:<Port>.
+//
+// EXPERIMENTAL: This API is experimental. It may be changed to a String()
+// function an a combined AddrPort type instead.
+func FormatAddrPort(a Addr, port uint16) string {
+	return fmt.Sprintf("[%s]:%d", a, port)
+}

pkg/addr/addr_test.go

@@ -15,8 +15,11 @@
 package addr_test
 
 import (
+	"encoding"
+	"flag"
 	"fmt"
 	"net/netip"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -27,14 +30,14 @@ import (
 
 func ExampleParseAddr() {
 	a, err := addr.ParseAddr("6-ffaa:0:123,198.51.100.1")
-	fmt.Printf("ia: %v, host: %v, err: %v\n", a.IA, a.Host, err)
-	// Output: ia: 6-ffaa:0:123, host: 198.51.100.1, err: <nil>
+	fmt.Printf("ia: %v, host type: %v, host: %v, err: %v\n", a.IA, a.Host.Type(), a.Host, err)
+	// Output: ia: 6-ffaa:0:123, host type: IP, host: 198.51.100.1, err: <nil>
 }
 
 func ExampleParseAddr_svc() {
 	a, err := addr.ParseAddr("6-ffaa:0:123,CS")
-	fmt.Printf("host type: %v, err: %v\n", a.Host.Type(), err)
-	// Output: host type: SVC, err: <nil>
+	fmt.Printf("ia: %v, host type: %v, host: %v, err: %v\n", a.IA, a.Host.Type(), a.Host, err)
+	// Output: ia: 6-ffaa:0:123, host type: SVC, host: CS, err: <nil>
 }
 
 func TestParseAddr(t *testing.T) {
@@ -53,6 +56,20 @@ func TestParseAddr(t *testing.T) {
 			_, err := addr.ParseAddr(s)
 			assert.Error(t, err)
 		})
+		t.Run("unmarshal "+s, func(t *testing.T) {
+			var a addr.Addr
+			var u encoding.TextUnmarshaler = &a
+			err := u.UnmarshalText([]byte(s))
+			assert.Error(t, err)
+			assert.Equal(t, addr.Addr{}, a)
+		})
+		t.Run("set "+s, func(t *testing.T) {
+			var a addr.Addr
+			var v flag.Value = &a
+			err := v.Set(s)
+			assert.Error(t, err)
+			assert.Equal(t, addr.Addr{}, a)
+		})
 	}
 
 	valid := map[string]addr.Addr{
@@ -97,6 +114,20 @@ func TestParseAddr(t *testing.T) {
 			require.NoError(t, err)
 			assert.Equal(t, expected, a)
 		})
+		t.Run("unmarshal "+s, func(t *testing.T) {
+			var a addr.Addr
+			var u encoding.TextUnmarshaler = &a
+			err := u.UnmarshalText([]byte(s))
+			require.NoError(t, err)
+			assert.Equal(t, expected, a)
+		})
+		t.Run("set "+s, func(t *testing.T) {
+			var a addr.Addr
+			var v flag.Value = &a
+			err := v.Set(s)
+			require.NoError(t, err)
+			assert.Equal(t, expected, a)
+		})
 	}
 }
 
@@ -180,6 +211,11 @@ func TestParseAddrPort(t *testing.T) {
 			require.NoError(t, err)
 			assert.Equal(t, addr.Addr{IA: expected.IA, Host: expected.Host}, a)
 			assert.Equal(t, expected.Port, port)
+
+			fmted := addr.FormatAddrPort(a, port)
+			if strings.Index(s, "]:0") < 0 { // skip cases where port has leading 0s
+				assert.Equal(t, s, fmted)
+			}
 		})
 	}
 }

pkg/addr/host.go

@@ -49,9 +49,9 @@ func (t HostAddrType) String() string {
 //
 // The zero value is a valid object with Host{}.Type() == HostTypeNone.
 type Host struct {
-	t   HostAddrType
 	ip  netip.Addr
 	svc SVC
+	t   HostAddrType
 }
 
 // ParseHost parses s as either a service address or an IP address,

pkg/addr/host_test.go

@@ -17,6 +17,8 @@ package addr_test
 import (
 	"fmt"
 	"net/netip"
+	"reflect"
+	"runtime"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -58,10 +60,25 @@ func ExampleHost() {
 	// h: "<None>", h.Type(): "None", h == addr.Host{}: true
 	// h: "::1", h.Type(): "IP", h.IP().Is4(): false
 	// h: "198.51.100.1", h.Type(): "IP", h.IP().Is4(): true
-	// h: "CS A (0x0002)", h.Type(): "SVC", h.SVC().IsMulticast(): false
+	// h: "CS", h.Type(): "SVC", h.SVC().IsMulticast(): false
 	// has SvcCS: true, has SvcDS: false
 }
 
+func TestHostStructSize(t *testing.T) {
+	if runtime.GOARCH != `amd64` {
+		t.SkipNow()
+	}
+	ipv6 := 16
+	zonePtr := 8
+	svc := 2
+	typ := 1
+	padding := 5
+	expected := ipv6 + zonePtr + svc + typ + padding
+
+	sizeofHost := int(reflect.TypeOf(addr.Host{}).Size())
+	assert.Equal(t, expected, sizeofHost)
+}
+
 func TestParseHost(t *testing.T) {
 	invalid := []string{
 		"",

pkg/addr/svc.go

@@ -82,18 +82,16 @@ func (h SVC) Multicast() SVC {
 	return h | SVCMcast
 }
 
-// XXX(matzf): change this to the format accepted by ParseSVC?
 func (h SVC) String() string {
-	name := h.BaseString()
-	cast := 'A'
+	s := h.BaseString()
 	if h.IsMulticast() {
-		cast = 'M'
+		s += "_M"
 	}
-	return fmt.Sprintf("%v %c (0x%04x)", name, cast, uint16(h))
+	return s
 }
 
-// BaseString returns the upper case name of the service. For hosts or unrecognized services, it
-// returns UNKNOWN.
+// BaseString returns the upper case name of the service. For unrecognized services, it
+// returns "SVC(<Hex-Value>)".
 func (h SVC) BaseString() string {
 	switch h.Base() {
 	case SvcDS:
@@ -103,6 +101,6 @@ func (h SVC) BaseString() string {
 	case SvcWildcard:
 		return "Wildcard"
 	default:
-		return "UNKNOWN"
+		return fmt.Sprintf("SVC:0x%04x", uint16(h))
 	}
 }

pkg/addr/svc_test.go

@@ -58,3 +58,21 @@ func TestParseSVC(t *testing.T) {
 		})
 	}
 }
+
+func TestSVCString(t *testing.T) {
+	cases := map[addr.SVC]string{
+		addr.SVC(0xABC):              "SVC:0x0abc",
+		addr.SvcCS:                   "CS",
+		addr.SvcCS.Multicast():       "CS_M",
+		addr.SvcDS:                   "DS",
+		addr.SvcDS.Multicast():       "DS_M",
+		addr.SvcWildcard:             "Wildcard",
+		addr.SvcWildcard.Multicast(): "Wildcard_M",
+	}
+	for svc, expected := range cases {
+		t.Run(expected, func(t *testing.T) {
+			actual := svc.String()
+			assert.Equal(t, expected, actual)
+		})
+	}
+}

pkg/snet/svcaddr_test.go

@@ -36,7 +36,7 @@ func TestSVCAddrString(t *testing.T) {
 	}{
 		"nil": {
 			input: &snet.SVCAddr{},
-			want:  "0-0,UNKNOWN A (0x0000)",
+			want:  "0-0,SVC:0x0000",
 		},
 	}
 	for n, tc := range tests {

tools/braccept/cases/bfd.go

@@ -17,7 +17,6 @@ package cases
 import (
 	"hash"
 	"net"
-	"net/netip"
 	"path/filepath"
 
 	"github.com/google/gopacket"
@@ -103,11 +102,11 @@ func ExternalBFD(artifactsDir string, mac hash.Hash) runner.Case {
 		DstIA:        localIA,
 		SrcIA:        remoteIA,
 	}
-	err := scionL.SetSrcAddr(addr.HostIP(netip.MustParseAddr("192.168.13.3")))
+	err := scionL.SetSrcAddr(addr.MustParseHost("192.168.13.3"))
 	if err != nil {
 		panic(err)
 	}
-	err = scionL.SetDstAddr(addr.HostIP(netip.MustParseAddr("192.168.13.2")))
+	err = scionL.SetDstAddr(addr.MustParseHost("192.168.13.2"))
 	if err != nil {
 		panic(err)
 	}
@@ -135,11 +134,11 @@ func ExternalBFD(artifactsDir string, mac hash.Hash) runner.Case {
 	udp.SrcPort, udp.DstPort = udp.DstPort, udp.SrcPort
 	scionL.DstIA = remoteIA
 	scionL.SrcIA = localIA
-	err = scionL.SetSrcAddr(addr.HostIP(netip.MustParseAddr("192.168.13.2")))
+	err = scionL.SetSrcAddr(addr.MustParseHost("192.168.13.2"))
 	if err != nil {
 		panic(err)
 	}
-	err = scionL.SetDstAddr(addr.HostIP(netip.MustParseAddr("192.168.13.3")))
+	err = scionL.SetDstAddr(addr.MustParseHost("192.168.13.3"))
 	if err != nil {
 		panic(err)
 	}
@@ -199,11 +198,11 @@ func InternalBFD(artifactsDir string, mac hash.Hash) runner.Case {
 		SrcIA:        localIA,
 		DstIA:        localIA,
 	}
-	err := scionL.SetSrcAddr(addr.HostIP(netip.MustParseAddr("192.168.0.13")))
+	err := scionL.SetSrcAddr(addr.MustParseHost("192.168.0.13"))
 	if err != nil {
 		panic(err)
 	}
-	err = scionL.SetDstAddr(addr.HostIP(netip.MustParseAddr("192.168.0.11")))
+	err = scionL.SetDstAddr(addr.MustParseHost("192.168.0.11"))
 	if err != nil {
 		panic(err)
 	}
@@ -229,11 +228,11 @@ func InternalBFD(artifactsDir string, mac hash.Hash) runner.Case {
 	ip.SrcIP = net.IP{192, 168, 0, 11}
 	ip.DstIP = net.IP{192, 168, 0, 13}
 	udp.SrcPort, udp.DstPort = udp.DstPort, udp.SrcPort
-	err = scionL.SetSrcAddr(addr.HostIP(netip.MustParseAddr("192.168.0.11")))
+	err = scionL.SetSrcAddr(addr.MustParseHost("192.168.0.11"))
 	if err != nil {
 		panic(err)
 	}
-	err = scionL.SetDstAddr(addr.HostIP(netip.MustParseAddr("192.168.0.13")))
+	err = scionL.SetDstAddr(addr.MustParseHost("192.168.0.13"))
 	if err != nil {
 		panic(err)
 	}

tools/braccept/cases/child_to_child_xover.go

@@ -17,7 +17,6 @@ package cases
 import (
 	"hash"
 	"net"
-	"net/netip"
 	"path/filepath"
 	"time"
 
@@ -109,10 +108,10 @@ func ChildToChildXover(artifactsDir string, mac hash.Hash) runner.Case {
 		Path:         sp,
 	}
 
-	if err := scionL.SetSrcAddr(addr.HostIP(netip.MustParseAddr("172.16.5.1"))); err != nil {
+	if err := scionL.SetSrcAddr(addr.MustParseHost("172.16.5.1")); err != nil {
 		panic(err)
 	}
-	if err := scionL.SetDstAddr(addr.HostIP(netip.MustParseAddr("174.16.4.1"))); err != nil {
+	if err := scionL.SetDstAddr(addr.MustParseHost("174.16.4.1")); err != nil {
 		panic(err)
 	}
 

tools/braccept/cases/child_to_internal.go

@@ -17,7 +17,6 @@ package cases
 import (
 	"hash"
 	"net"
-	"net/netip"
 	"path/filepath"
 	"time"
 
@@ -98,10 +97,10 @@ func ChildToInternalHost(artifactsDir string, mac hash.Hash) runner.Case {
 		DstIA:        xtest.MustParseIA("1-ff00:0:1"),
 		Path:         sp,
 	}
-	if err := scionL.SetSrcAddr(addr.HostIP(netip.MustParseAddr("172.16.4.1"))); err != nil {
+	if err := scionL.SetSrcAddr(addr.MustParseHost("172.16.4.1")); err != nil {
 		panic(err)
 	}
-	if err := scionL.SetDstAddr(addr.HostIP(netip.MustParseAddr("192.168.0.51"))); err != nil {
+	if err := scionL.SetDstAddr(addr.MustParseHost("192.168.0.51")); err != nil {
 		panic(err)
 	}
 
@@ -205,10 +204,10 @@ func ChildToInternalHostShortcut(artifactsDir string, mac hash.Hash) runner.Case
 		DstIA:        xtest.MustParseIA("1-ff00:0:1"),
 		Path:         sp,
 	}
-	if err := scionL.SetSrcAddr(addr.HostIP(netip.MustParseAddr("172.16.4.1"))); err != nil {
+	if err := scionL.SetSrcAddr(addr.MustParseHost("172.16.4.1")); err != nil {
 		panic(err)
 	}
-	if err := scionL.SetDstAddr(addr.HostIP(netip.MustParseAddr("192.168.0.51"))); err != nil {
+	if err := scionL.SetDstAddr(addr.MustParseHost("192.168.0.51")); err != nil {
 		panic(err)
 	}
 
@@ -323,10 +322,10 @@ func ChildToInternalParent(artifactsDir string, mac hash.Hash) runner.Case {
 		DstIA:        xtest.MustParseIA("1-ff00:0:9"),
 		Path:         sp,
 	}
-	if err := scionL.SetSrcAddr(addr.HostIP(netip.MustParseAddr("172.16.4.1"))); err != nil {
+	if err := scionL.SetSrcAddr(addr.MustParseHost("172.16.4.1")); err != nil {
 		panic(err)
 	}
-	if err := scionL.SetDstAddr(addr.HostIP(netip.MustParseAddr("172.16.9.1"))); err != nil {
+	if err := scionL.SetDstAddr(addr.MustParseHost("172.16.9.1")); err != nil {
 		panic(err)
 	}
 

tools/braccept/cases/child_to_parent.go

@@ -17,7 +17,6 @@ package cases
 import (
 	"hash"
 	"net"
-	"net/netip"
 	"path/filepath"
 	"time"
 
@@ -106,10 +105,10 @@ func ChildToParent(artifactsDir string, mac hash.Hash) runner.Case {
 		DstIA:        xtest.MustParseIA("1-ff00:0:3"),
 		Path:         sp,
 	}
-	if err := scionL.SetSrcAddr(addr.HostIP(netip.MustParseAddr("172.16.4.1"))); err != nil {
+	if err := scionL.SetSrcAddr(addr.MustParseHost("172.16.4.1")); err != nil {
 		panic(err)
 	}
-	if err := scionL.SetDstAddr(addr.HostIP(netip.MustParseAddr("174.16.3.1"))); err != nil {
+	if err := scionL.SetDstAddr(addr.MustParseHost("174.16.3.1")); err != nil {
 		panic(err)
 	}