From 37978889109f86b467f90d15324e36d6021d2a6b Mon Sep 17 00:00:00 2001 From: Matthias Frei Date: Tue, 27 Jun 2023 11:49:51 +0200 Subject: [PATCH] feedback: order struct members, add TextUn/Marshal, fix SVC.String format, add FormatHostPort, use MustParseHost --- pkg/addr/addr.go | 18 ++++++++ pkg/addr/addr_test.go | 44 ++++++++++++++++++-- pkg/addr/host.go | 2 +- pkg/addr/host_test.go | 19 ++++++++- pkg/addr/svc.go | 14 +++---- pkg/addr/svc_test.go | 18 ++++++++ pkg/snet/svcaddr_test.go | 2 +- tools/braccept/cases/bfd.go | 17 ++++---- tools/braccept/cases/child_to_child_xover.go | 5 +-- tools/braccept/cases/child_to_internal.go | 13 +++--- tools/braccept/cases/child_to_parent.go | 5 +-- 11 files changed, 120 insertions(+), 37 deletions(-) diff --git a/pkg/addr/addr.go b/pkg/addr/addr.go index 450fc783ca..4467f54a42 100644 --- a/pkg/addr/addr.go +++ b/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 // // [-,]:. @@ -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 +// +// [-,]:. +// +// 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) +} diff --git a/pkg/addr/addr_test.go b/pkg/addr/addr_test.go index 586a8efd98..169fb7fe95 100644 --- a/pkg/addr/addr_test.go +++ b/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: + 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: } 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: + 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: } 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) + } }) } } diff --git a/pkg/addr/host.go b/pkg/addr/host.go index 804c4f693e..d6f157e54c 100644 --- a/pkg/addr/host.go +++ b/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, diff --git a/pkg/addr/host_test.go b/pkg/addr/host_test.go index f7b8095261..d8da6eada0 100644 --- a/pkg/addr/host_test.go +++ b/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: "", 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{ "", diff --git a/pkg/addr/svc.go b/pkg/addr/svc.go index 19a80cd125..f47e14ba27 100644 --- a/pkg/addr/svc.go +++ b/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()". 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)) } } diff --git a/pkg/addr/svc_test.go b/pkg/addr/svc_test.go index 8a7db0fe41..1be0e932e8 100644 --- a/pkg/addr/svc_test.go +++ b/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) + }) + } +} diff --git a/pkg/snet/svcaddr_test.go b/pkg/snet/svcaddr_test.go index 138ec02040..4521128b9e 100644 --- a/pkg/snet/svcaddr_test.go +++ b/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 { diff --git a/tools/braccept/cases/bfd.go b/tools/braccept/cases/bfd.go index e6ed2d3814..e178869caa 100644 --- a/tools/braccept/cases/bfd.go +++ b/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) } diff --git a/tools/braccept/cases/child_to_child_xover.go b/tools/braccept/cases/child_to_child_xover.go index 958cb310e8..74ec549e90 100644 --- a/tools/braccept/cases/child_to_child_xover.go +++ b/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) } diff --git a/tools/braccept/cases/child_to_internal.go b/tools/braccept/cases/child_to_internal.go index 64e1bada75..88d5fcdcb9 100644 --- a/tools/braccept/cases/child_to_internal.go +++ b/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) } diff --git a/tools/braccept/cases/child_to_parent.go b/tools/braccept/cases/child_to_parent.go index 7c2ac02553..fb6f24092f 100644 --- a/tools/braccept/cases/child_to_parent.go +++ b/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) }