Implement SDR repo reservations and two-part reads (#66)

CONTRIBUTING.md

@@ -28,8 +28,8 @@ Be sure to add response operations to the `operationLayerTypes` map in this file
 If the request or response payload has any enum-style fields, e.g. `ChassisControl`, create a new type with constants for its possible values, then implement `fmt.Stringer` to make it print nicely.
 It is recommended not to embed any fields implementing `fmt.Stringer` in a layer, as this means it cannot be printed by `gopacket` (there is an issue [here](https://github.com/google/gopacket/issues/683)).
 
-Session-less commands should be added to the `SessionlessCommands` interface, and the response, if any, to the `sessionlessRspLayers` struct, then the appropriate decoders in `v(1|2)sessionless.go`.
-Commands that must be sent inside a session should be added to the `SessionCommands` interface, and the response, if any, to the `sessionRspLayers` struct, then the appropriate decoders in `v(1|2)session.go`.
+Session-less commands should be added to the `SessionlessCommands` interface, then the appropriate decoders in `v(1|2)sessionless.go`.
+Commands that must be sent inside a session should be added to the `SessionCommands` interface, then the appropriate decoders in `v(1|2)session.go`.
 Writing appropriate implementations for IPMI v1.5 and v2.0 in `v1session(less).go` and `v2session(less).go` respectively makes the command easy for users to execute.
 
 ### Examples

pkg/ipmi/completion_code.go

@@ -26,6 +26,11 @@ const (
 	CompletionCodeUnrecognisedCommand CompletionCode = 0xc1
 	CompletionCodeTimeout             CompletionCode = 0xc3
 
+	// CompletionCodeReservationCanceledOrInvalid means that either the
+	// requester's reservation has been canceled or the request's reservation
+	// ID is invalid.
+	CompletionCodeReservationCanceledOrInvalid CompletionCode = 0xc5
+
 	// CompletionCodeRequestTruncated means the request ended prematurely. Did
 	// you forget to add the final request data layer?
 	CompletionCodeRequestTruncated CompletionCode = 0xc6

pkg/ipmi/get_device_id_test.go

@@ -8,7 +8,7 @@ import (
 	"github.com/google/gopacket/layers"
 )
 
-func TestGetDecideIDRspDecodeFromBytes(t *testing.T) {
+func TestGetDeviceIDRspDecodeFromBytes(t *testing.T) {
 	tests := []struct {
 		in   []byte
 		want *GetDeviceIDRsp

pkg/ipmi/layer_types.go

@@ -257,4 +257,13 @@ var (
 			}),
 		},
 	)
+	LayerTypeReserveSDRRepositoryRsp = gopacket.RegisterLayerType(
+		1031,
+		gopacket.LayerTypeMetadata{
+			Name: "Reserve SDR Repository Response",
+			Decoder: layerexts.BuildDecoder(func() layerexts.LayerDecodingLayer {
+				return &ReserveSDRRepositoryRsp{}
+			}),
+		},
+	)
 )

pkg/ipmi/operation.go

@@ -86,6 +86,14 @@ var (
 		Function: NetworkFunctionStorageRsp,
 		Command:  0x20,
 	}
+	OperationReserveSDRRepositoryReq = Operation{
+		Function: NetworkFunctionStorageReq,
+		Command:  0x22,
+	}
+	OperationReserveSDRRepositoryRsp = Operation{
+		Function: NetworkFunctionStorageRsp,
+		Command:  0x22,
+	}
 	OperationGetSDRReq = Operation{
 		Function: NetworkFunctionStorageReq,
 		Command:  0x23,
@@ -135,6 +143,7 @@ var (
 		OperationGetChannelAuthenticationCapabilitiesRsp: LayerTypeGetChannelAuthenticationCapabilitiesRsp,
 		OperationSetSessionPrivilegeLevelRsp:             LayerTypeSetSessionPrivilegeLevelRsp,
 		OperationGetSDRRepositoryInfoRsp:                 LayerTypeGetSDRRepositoryInfoRsp,
+		OperationReserveSDRRepositoryRsp:                 LayerTypeReserveSDRRepositoryRsp,
 		OperationGetSDRRsp:                               LayerTypeGetSDRRsp,
 		OperationGetSensorReadingRsp:                     LayerTypeGetSensorReadingRsp,
 		OperationGetSessionInfoRsp:                       LayerTypeGetSessionInfoRsp,

pkg/ipmi/reserve_sdr_repository.go

@@ -0,0 +1,66 @@
+package ipmi
+
+import (
+	"encoding/binary"
+	"fmt"
+
+	"github.com/google/gopacket"
+	"github.com/google/gopacket/layers"
+)
+
+// ReserveSDRRepositoryRsp represents the response to a Reserve SDR Repository
+// command, specified in 33.11 of IPMI v2.0.
+type ReserveSDRRepositoryRsp struct {
+	layers.BaseLayer
+
+	ReservationID ReservationID
+}
+
+func (*ReserveSDRRepositoryRsp) LayerType() gopacket.LayerType {
+	return LayerTypeReserveSDRRepositoryRsp
+}
+
+func (r *ReserveSDRRepositoryRsp) CanDecode() gopacket.LayerClass {
+	return r.LayerType()
+}
+
+func (*ReserveSDRRepositoryRsp) NextLayerType() gopacket.LayerType {
+	return gopacket.LayerTypePayload
+}
+
+func (r *ReserveSDRRepositoryRsp) DecodeFromBytes(data []byte, df gopacket.DecodeFeedback) error {
+	if len(data) < 2 {
+		df.SetTruncated()
+		return fmt.Errorf("response must be at least 2 bytes, got %v", len(data))
+	}
+
+	r.BaseLayer.Contents = data[:2]
+	r.ReservationID = ReservationID(binary.LittleEndian.Uint16(data[0:2]))
+	return nil
+}
+
+type ReserveSDRRepositoryCmd struct {
+	Rsp ReserveSDRRepositoryRsp
+}
+
+// Name returns "Reserve SDR Repository".
+func (*ReserveSDRRepositoryCmd) Name() string {
+	return "Reserve SDR Repository"
+}
+
+// Operation returns &OperationReserveSDRRepositoryReq.
+func (*ReserveSDRRepositoryCmd) Operation() *Operation {
+	return &OperationReserveSDRRepositoryReq
+}
+
+func (c *ReserveSDRRepositoryCmd) RemoteLUN() LUN {
+	return LUNBMC
+}
+
+func (c *ReserveSDRRepositoryCmd) Request() gopacket.SerializableLayer {
+	return nil
+}
+
+func (c *ReserveSDRRepositoryCmd) Response() gopacket.DecodingLayer {
+	return &c.Rsp
+}

pkg/ipmi/reserve_sdr_repository_test.go

@@ -0,0 +1,32 @@
+package ipmi
+
+import (
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/gopacket"
+	"github.com/google/gopacket/layers"
+	"testing"
+)
+
+func TestReserveSDRRepositoryRspDecodeFomBytes(t *testing.T) {
+	tests := []struct {
+		in   []byte
+		want *ReserveSDRRepositoryRsp
+	}{
+		{
+			in: []byte{0x20, 0x58},
+			want: &ReserveSDRRepositoryRsp{
+				BaseLayer:     layers.BaseLayer{Contents: []byte{0x20, 0x58}},
+				ReservationID: 22560,
+			},
+		},
+	}
+	for _, test := range tests {
+		rsp := &ReserveSDRRepositoryRsp{}
+		if err := rsp.DecodeFromBytes(test.in, gopacket.NilDecodeFeedback); err != nil {
+			t.Errorf("unexpected error: %v", err)
+		}
+		if diff := cmp.Diff(test.want, rsp); diff != "" {
+			t.Errorf("decode %v = %v, want %v: %v", test.in, rsp, test.want, diff)
+		}
+	}
+}

pkg/ipmi/sdr.go

@@ -34,17 +34,18 @@ type SDR struct {
 	// to sensors.
 	Type RecordType
 
-	// There is a 1-byte length field containing the number of remaining bytes
-	// in the payload (i.e. after the header), so the max SDR size on the wire
-	// is 260 bytes. In practice, OEM records notwithstanding, it is unlikely to
-	// be >60.
+	// Length is the number of remaining bytes in the payload (i.e. after the header).
+	//
+	// This means the max SDR size on the wire is 260 bytes. In practice, OEM
+	// records notwithstanding, it is unlikely to be >60.
 	//
 	// If it weren't for this field, the limit for the whole SDR including
 	// header could theoretically be 255 + the max supported payload size (the
 	// SDR Repo Device commands provide no way to address subsequent sections
 	// for reading).
+	Length uint8
 
-	// payload contains the record key and body
+	// payload contains the record key and body.
 }
 
 func (*SDR) LayerType() gopacket.LayerType {
@@ -68,6 +69,7 @@ func (s *SDR) DecodeFromBytes(data []byte, df gopacket.DecodeFeedback) error {
 	s.ID = RecordID(binary.LittleEndian.Uint16(data[0:2]))
 	s.Version = bcd.Decode(data[2]&0xf)*10 + bcd.Decode(data[2]>>4)
 	s.Type = RecordType(data[3])
+	s.Length = uint8(data[4])
 
 	s.BaseLayer.Contents = data[:5]
 	s.BaseLayer.Payload = data[5:]

pkg/ipmi/sdr_test.go

@@ -38,6 +38,7 @@ func TestSDRDecodeFromBytes(t *testing.T) {
 				ID:      61455,
 				Version: 99,
 				Type:    RecordTypeFullSensor,
+				Length:  22,
 			},
 		},
 		{
@@ -61,6 +62,7 @@ func TestSDRDecodeFromBytes(t *testing.T) {
 				ID:      4080,
 				Version: 15,
 				Type:    RecordTypeCompactSensor,
+				Length:  32,
 			},
 		},
 	}

sdr_repository.go

@@ -11,6 +11,11 @@ import (
 	"github.com/google/gopacket"
 )
 
+const (
+	sdrHeaderLength = 5
+	sdrMaxLength    = 64
+)
+
 var (
 	errSDRRepositoryModified = errors.New(
 		"the SDR Repository was modified during enumeration")
@@ -59,12 +64,24 @@ func RetrieveSDRRepository(ctx context.Context, s Session) (SDRRepository, error
 
 // walkSDRs iterates over the SDR Repository. It is not concerned with the repo
 // changing behind its back.
+//
+// For each SDR, it starts by requesting the header and inspecting the type. If
+// it's a FullSensorRecord, it then requests the key fields and body. Otherwise,
+// it skips to the next SDR.
+// This is more expensive than reading the entire SDR at once, but it's
+// resilient to BMCs that return a malformed packet when the request's Length is
+// 0xff.
 func walkSDRs(ctx context.Context, s Session) (SDRRepository, error) {
 	repo := SDRRepository{} // we could set a size; it's a micro-optimisation
+	reserveSDRRepoCmdResp, err := s.ReserveSDRRepository(ctx)
+	if err != nil {
+		return nil, err
+	}
 	getSDRCmd := &ipmi.GetSDRCmd{
 		Req: ipmi.GetSDRReq{
-			RecordID: ipmi.RecordIDFirst,
-			Length:   0xff,
+			RecordID:      ipmi.RecordIDFirst,
+			Length:        sdrHeaderLength,                     // read header only
+			ReservationID: reserveSDRRepoCmdResp.ReservationID, // needed for partial reads
 		},
 	}
 
@@ -74,24 +91,52 @@ func walkSDRs(ctx context.Context, s Session) (SDRRepository, error) {
 	// duplicate it.
 	for getSDRCmd.Req.RecordID != ipmi.RecordIDLast {
 		if err := ValidateResponse(s.SendCommand(ctx, getSDRCmd)); err != nil {
-			// if we get a 0xca or 0xff, we need to implement reservations and
-			// partial reading - hopefully we'll be alright - yet to see a SDR
-			// >70 bytes long - they're specified as 64 after all.
 			return nil, err
 		}
-
-		packet := gopacket.NewPacket(getSDRCmd.Rsp.Payload, ipmi.LayerTypeSDR,
+		headerPacket := gopacket.NewPacket(getSDRCmd.Rsp.Payload, ipmi.LayerTypeSDR,
 			gopacket.DecodeOptions{
 				Lazy: true,
 				// we can't set NoCopy because we reuse getSDRCmd.Rsp
 			})
-		if packet == nil {
+		if headerPacket == nil {
 			return nil, fmt.Errorf("invalid SDR: %v", getSDRCmd)
 		}
-		if fsrLayer := packet.Layer(ipmi.LayerTypeFullSensorRecord); fsrLayer != nil {
+		headerLayer := headerPacket.Layer(ipmi.LayerTypeSDR)
+		if headerLayer == nil {
+			return nil, fmt.Errorf("packet is missing SDR layer: %v", getSDRCmd)
+		}
+		header := headerLayer.(*ipmi.SDR)
+
+		if header.Type == ipmi.RecordTypeFullSensor {
+			if header.Length > sdrMaxLength {
+				// SDR exceeds the specified max length, which means we need to implement
+				// partial reading. Hopefully we'll be alright - yet to see a SDR >70 bytes
+				// long - they're specified as 64 after all.
+				return nil, fmt.Errorf("SDR length %d exceeds max of %d bytes: %v",
+					header.Length, sdrMaxLength, getSDRCmd)
+			}
+
+			getSDRCmd.Req.Offset = sdrHeaderLength
+			getSDRCmd.Req.Length = header.Length
+			if err := ValidateResponse(s.SendCommand(ctx, getSDRCmd)); err != nil {
+				return nil, err
+			}
+			fsrPacket := gopacket.NewPacket(getSDRCmd.Rsp.Payload, ipmi.LayerTypeFullSensorRecord,
+				gopacket.DecodeOptions{Lazy: true})
+			if fsrPacket == nil {
+				return nil, fmt.Errorf("invalid Full Sensor Record: %v", getSDRCmd)
+			}
+			fsrLayer := fsrPacket.Layer(ipmi.LayerTypeFullSensorRecord)
+			if fsrLayer == nil {
+				return nil, fmt.Errorf("packet is missing Full Sensor Record layer: %v",
+					getSDRCmd)
+			}
 			repo[getSDRCmd.Req.RecordID] = fsrLayer.(*ipmi.FullSensorRecord)
 		}
+
 		getSDRCmd.Req.RecordID = getSDRCmd.Rsp.Next
+		getSDRCmd.Req.Offset = 0x00
+		getSDRCmd.Req.Length = sdrHeaderLength
 	}
 	return repo, nil
 }

session_commands.go

@@ -36,6 +36,12 @@ type SessionCommands interface {
 	// respectively.
 	GetSDRRepositoryInfo(context.Context) (*ipmi.GetSDRRepositoryInfoRsp, error)
 
+	// ReserveSDRRepository sets the requester as the present "owner" of the
+	// repository. The returned reservation ID must be included in requests that
+	// either delete or partially read/write an SDR.
+	// This is specified in 33.11 of IPMI v2.0.
+	ReserveSDRRepository(context.Context) (*ipmi.ReserveSDRRepositoryRsp, error)
+
 	// GetSensorReading retrieves the current value of a sensor, identified by
 	// its number. It is specified in 29.14 and 35.14 of IPMI v1.5 and 2.0
 	// respectively. Note, the raw value is in one of three formats, and is

v2session.go

@@ -271,6 +271,14 @@ func (s *V2Session) GetSDRRepositoryInfo(ctx context.Context) (*ipmi.GetSDRRepos
 	return &cmd.Rsp, nil
 }
 
+func (s *V2Session) ReserveSDRRepository(ctx context.Context) (*ipmi.ReserveSDRRepositoryRsp, error) {
+	cmd := &ipmi.ReserveSDRRepositoryCmd{}
+	if err := ValidateResponse(s.SendCommand(ctx, cmd)); err != nil {
+		return nil, err
+	}
+	return &cmd.Rsp, nil
+}
+
 func (s *V2Session) GetSensorReading(ctx context.Context, sensor uint8) (*ipmi.GetSensorReadingRsp, error) {
 	cmd := &ipmi.GetSensorReadingCmd{
 		Req: ipmi.GetSensorReadingReq{