COLIBRI: Basic types needed for initial request payload

[juagargi]

Adds types that will be used later in the colibri store, as well as in the requests, responses, etc.

---

This change is 

[oncilla]

Reviewed 2 of 3 files at r1.
Reviewable status: 2 of 3 files reviewed, 11 unresolved discussions (waiting on @juagargi)

---

go/lib/colibri/types.go, line 15 at r1 (raw file):

// limitations under the License.

package colibri

consider moving the types here to a sub package reservation. I think that would lead to more readable package API form a callers point of view:

e.g

reservation.SegmentID
reservation.E2EID
reservation.BandwithClass
reservation.Index
reservation.LatencyClass

---

go/lib/colibri/types.go, line 25 at r1 (raw file):

// SegmentReservationID identifies a COLIBRI segment reservation. The suffix differentiates
// reservations for the same AS.
type SegmentReservationID struct {

maybe just SegmentID ?

---

go/lib/colibri/types.go, line 25 at r1 (raw file):

// SegmentReservationID identifies a COLIBRI segment reservation. The suffix differentiates
// reservations for the same AS.
type SegmentReservationID struct {

This could just be a byte slice

type SegmentID []byte

func (id SegmentID) AS() addr.AS {
    	high := common.Order.Uint16(id[:2])
	low := common.Order.Uint32(id[2:])
	return (addr.AS(high) << (4 * 8)) | addr.AS(low)
}

func (id SegmentID) Suffix() uint32 {
  return common.Order.Uint32(ud[6:])
}

---

go/lib/colibri/types.go, line 33 at r1 (raw file):

// SegmentReservationIDFromRaw constructs a SegmentReservationID parsing a raw buffer.
func SegmentReservationIDFromRaw(raw common.RawBytes) (

use []byte

---

go/lib/colibri/types.go, line 47 at r1 (raw file):

}

func (id *SegmentReservationID) Write(raw common.RawBytes) error {

No need for this, if it is a byte slice.
you can just call copy

---

go/lib/colibri/types.go, line 60 at r1 (raw file):

// E2EReservationID identifies a COLIBRI E2E reservation. The suffix is different for each
// reservation for any given AS.
type E2EReservationID struct {

E2EID?

Also, same comment regarding just being []byte

---

go/lib/colibri/types.go, line 68 at r1 (raw file):

// E2EReservationIDFromRaw constructs an E2EReservationID parsing a buffer.
func E2EReservationIDFromRaw(raw common.RawBytes) (*E2EReservationID, error) {

just use []byte

---

go/lib/colibri/types.go, line 91 at r1 (raw file):

// Tick represents a slice of time of 4 seconds.
type Tick uint32

This should mention how you get from UNIX time to tick

---

go/lib/colibri/types.go, line 135 at r1 (raw file):

// the different COLIBRI path types.
const (
	DownPath PathType = iota

I think we should start with UnknownPath = iota to catch unset values.
We need 3 bits anyway.

---

go/lib/colibri/types.go, line 200 at r1 (raw file):

// Len returns the length in bytes when serialized.
func (f InfoField) Len() int {

I don't think that is needed?

---

go/lib/colibri/types.go, line 205 at r1 (raw file):

// InfoFieldFromRaw builds an InfoField from the InfoFieldLen bytes buffer.
func InfoFieldFromRaw(raw common.RawBytes) (*InfoField, error) {

use []byte

---

go/lib/colibri/types_test.go, line 27 at r1 (raw file):

func TestSegmentReservationIDFromRaw(t *testing.T) {
	raw := xtest.MustParseHexString("ffaa00001101facecafe")

cute :)

[oncilla]

Reviewed 1 of 3 files at r1.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @juagargi)

[juagargi]

Reviewable status: 0 of 3 files reviewed, 11 unresolved discussions (waiting on @oncilla)

---

go/lib/colibri/types.go, line 15 at r1 (raw file):

Previously, Oncilla wrote…

consider moving the types here to a sub package reservation. I think that would lead to more readable package API form a callers point of view:

e.g

reservation.SegmentID
reservation.E2EID
reservation.BandwithClass
reservation.Index
reservation.LatencyClass

There is another PR not yet created that introduces some types in go/cs/reservation (https://github.com/juagargi/scion/tree/colibri.request_payload/go/cs/reservation).
I think that that branch will die anyways, so I am moving these to reservation as suggested.

---

go/lib/colibri/types.go, line 25 at r1 (raw file):

Previously, Oncilla wrote…

maybe just SegmentID ?

Done.

---

go/lib/colibri/types.go, line 25 at r1 (raw file):

Previously, Oncilla wrote…

This could just be a byte slice

type SegmentID []byte

func (id SegmentID) AS() addr.AS {
    	high := common.Order.Uint16(id[:2])
	low := common.Order.Uint32(id[2:])
	return (addr.AS(high) << (4 * 8)) | addr.AS(low)
}

func (id SegmentID) Suffix() uint32 {
  return common.Order.Uint32(ud[6:])
}

I tried, but I saw it doesn't reduce code or complexity (removes code in Write() and FromRaw(), adds code AS() and Suffix()). Also there is the risk of calling SegmentID{}.AS() and panic (empty value is not safe).

---

go/lib/colibri/types.go, line 33 at r1 (raw file):

Previously, Oncilla wrote…

use []byte

Done.

---

go/lib/colibri/types.go, line 47 at r1 (raw file):

Previously, Oncilla wrote…

No need for this, if it is a byte slice.
you can just call copy

See comment above.

---

go/lib/colibri/types.go, line 60 at r1 (raw file):

Previously, Oncilla wrote…

E2EID?

Also, same comment regarding just being []byte

Done.

---

go/lib/colibri/types.go, line 68 at r1 (raw file):

Previously, Oncilla wrote…

just use []byte

Done.

---

go/lib/colibri/types.go, line 91 at r1 (raw file):

Previously, Oncilla wrote…

This should mention how you get from UNIX time to tick

Done.

---

go/lib/colibri/types.go, line 135 at r1 (raw file):

Previously, Oncilla wrote…

I think we should start with UnknownPath = iota to catch unset values.
We need 3 bits anyway.

Done. PTAL also at Validate()

---

go/lib/colibri/types.go, line 200 at r1 (raw file):

Previously, Oncilla wrote…

I don't think that is needed?

Done.

---

go/lib/colibri/types.go, line 205 at r1 (raw file):

Previously, Oncilla wrote…

use []byte

Done.

---

go/lib/colibri/types_test.go, line 27 at r1 (raw file):

Previously, Oncilla wrote…

cute :)

:)

[oncilla]

Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @juagargi and @oncilla)

---

go/lib/colibri/BUILD.bazel, line 20 at r1 (raw file):

    embed = [":go_default_library"],
    deps = [
        "//go/lib/common:go_default_library",

💯

---

go/lib/colibri/types.go, line 25 at r1 (raw file):

Previously, juagargi (Juan A. García Pardo Giménez de los Galanes) wrote…

I tried, but I saw it doesn't reduce code or complexity (removes code in Write() and FromRaw(), adds code AS() and Suffix()). Also there is the risk of calling SegmentID{}.AS() and panic (empty value is not safe).

You can also make it [10]byte.

Write is a bit of an odd concept to me. It aims to reduce allocations, but then is defined on a pointer type, thus might even force a heap escape depending on how it is called.

Also, it does exactly the opposite of what io.Writer does.
If you really want to use the "write" pattern, you should rename it to read and implement the https://golang.org/pkg/io/#Reader interface.

[juagargi]

Reviewed 1 of 3 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)

---

go/lib/colibri/types.go, line 25 at r1 (raw file):

Previously, Oncilla wrote…

You can also make it [10]byte.

Write is a bit of an odd concept to me. It aims to reduce allocations, but then is defined on a pointer type, thus might even force a heap escape depending on how it is called.

Also, it does exactly the opposite of what io.Writer does.
If you really want to use the "write" pattern, you should rename it to read and implement the https://golang.org/pkg/io/#Reader interface.

I agree it's very confusing; I just wanted to be consistent with e.g. addr.IA.Write. I would like to follow the convention that scionproto uses/will use, whichever it is. Can you point me to one type in scionproto using e.g. the Read method?

[oncilla]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @juagargi)

---

go/lib/colibri/types.go, line 25 at r1 (raw file):

Previously, juagargi (Juan A. García Pardo Giménez de los Galanes) wrote…

I agree it's very confusing; I just wanted to be consistent with e.g. addr.IA.Write. I would like to follow the convention that scionproto uses/will use, whichever it is. Can you point me to one type in scionproto using e.g. the Read method?

I cannot, because the code base is full of bad examples :)

You can be the first to start the movement towards idiomatic go ❤️

[juagargi]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)

---

go/lib/colibri/types.go, line 25 at r1 (raw file):

Previously, Oncilla wrote…

I cannot, because the code base is full of bad examples :)

You can be the first to start the movement towards idiomatic go ❤️

Done.

[oncilla]

Reviewed 3 of 3 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @juagargi)

---

go/lib/colibri/reservation/types.go, line 236 at r3 (raw file):

	b[5] = byte(f.RLC)
	b[6] = byte(f.Idx<<4) | uint8(f.PathType)
	// b[7] is padding

zero fill padding

also create a test where this padding is not zero, and checks that it is set to zero

[juagargi]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)

---

go/lib/colibri/reservation/types.go, line 236 at r3 (raw file):

Previously, Oncilla wrote…

zero fill padding

also create a test where this padding is not zero, and checks that it is set to zero

Done.

[oncilla]

Reviewed 2 of 2 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved