xrootd: introduce a XRootD (un)marshal interface and generator #214

EgorMatirov · 2018-06-01T17:19:30Z

This helps to generate boilerplate code which is used for encoding
and decoding of the requests and the responses.

It's a bit involved version of the #210.
@sbinet, thank you very much for your help!

sbinet

just a first pass.
but it looks good already :)

sbinet · 2018-06-01T17:52:16Z

xrootd/client/login.go

@@ -7,23 +7,13 @@ package client // import "go-hep.org/x/hep/xrootd/client"
 import (
 	"context"

-	"go-hep.org/x/hep/xrootd/protocol"
 	"go-hep.org/x/hep/xrootd/protocol/login"
 )

 // Login initializes a server connection using username
 // and token which can be supplied by the previous redirection response.
 func (client *Client) Login(ctx context.Context, username string, token string) (login.Response, error) {


s/username string, token string/username, token string/

sbinet · 2018-06-01T17:54:53Z

xrootd/client/client.go

@@ -149,6 +150,16 @@ func (client *Client) consume(ctx context.Context) {
 	}
 }

+// Send sends the request to the server and stores the response inside the resp.
+func (client *Client) Send(ctx context.Context, resp interface{}, req protocol.Request) error {


couldn't we request that resp honours protocol.Response instead of the empty interface{} ?

with:

package protocol type Response interface { RespID() uint16 // just to prevent passing a Request instead of a Response (and vice versa) MarshalXrd UnmarshalXrd }

sbinet · 2018-06-01T17:56:23Z

xrootd/client/client.go

+		return err
+	}
+
+	return protocol.Unmarshal(serverResponse, resp)


then we could write:

return resp.UnmarshalXrd(data)

sbinet · 2018-06-01T17:56:41Z

xrootd/client/client.go

@@ -149,6 +150,16 @@ func (client *Client) consume(ctx context.Context) {
 	}
 }

+// Send sends the request to the server and stores the response inside the resp.
+func (client *Client) Send(ctx context.Context, resp interface{}, req protocol.Request) error {
+	serverResponse, err := client.call(ctx, req)


s/serverResponse/data/

sbinet · 2018-06-01T17:59:28Z

xrootd/client/protocol_mock_test.go

 		if err != nil {
-			cancel()
+			defer cancel()


is putting back defer a deliberate change? (or just a merge carried over by some previous CL?)

Sorry, looks like a strange merge from somewhere (Have no idea how it leaked here...).

sbinet · 2018-06-01T18:18:06Z

xrootd/protocol/protocol/protocol.go

-	RequestLevel RequestLevel
+// SetIsMeta sets whether this server has meta attribute.
+func (resp *Response) SetIsMeta(v bool) {
+	if v {


please use an early return to help the compiler inline calls of this method.

sbinet · 2018-06-01T18:18:17Z

xrootd/protocol/protocol/protocol.go

-)
+// SetIsProxy sets whether this server has proxy attribute.
+func (resp *Response) SetIsProxy(v bool) {
+	if v {


please use an early return to help the compiler inline calls of this method.

sbinet · 2018-06-01T18:18:30Z

xrootd/protocol/protocol/protocol.go

-		options |= ReturnSecurityRequirements
+// SetIsSupervisor sets whether this server has supervisor attribute.
+func (resp *Response) SetIsSupervisor(v bool) {
+	if v {


please use an early return to help the compiler inline calls of this method.

sbinet · 2018-06-01T18:18:41Z

xrootd/protocol/protocol/protocol.go

+
+// SetForceSecurity sets whether signing is forced.
+func (resp *Response) SetForceSecurity(v bool) {
+	if v {


please use an early return to help the compiler inline calls of this method.

sbinet · 2018-06-01T18:37:18Z

xrootd/protocol/protocol/protocol.go

+}
+
+// Response is a response for the `Protocol` request. See details in the xrootd protocol specification.
+type Response struct {


somehow, the SetIsXyz methods on this protocol.Response type feel a bit wrong to me: calling those won't impact back the server from which we got them.
I understand you introduced them so you could create a Response to test against in the Client tests.

Perhaps we could just leave the flags field of Response exported (so s/flags/Flags/) and re-export the IsXyz bitmasks ?
the protocol package isn't super crowded so it's fine to populate it somewhat.

that way, you could create a want value like so:

want := protocol.Response{ BinaryProtocolVersion: protocolVersion, Flags: protocol.IsManager | protocol.IsServer | protocol.IsMeta | protocol.IsProxy | protocol.IsSupervisor, SecurityLevel: xrdproto.Pedantic, SecurityOverrides: []xrdproto.SecurityOverride{{1, xrdproto.SignNeeded}}, }

I understand you introduced them so you could create a Response to test against in the Client tests.

was thinking about using them for the server implementation, not just tests. However, I don't argue about usage of Flags, it's fine too. :)

sbinet · 2018-06-02T07:04:30Z

xrootd/protocol/marshaling.go

+// UnmarshalXrd must copy the data if it wishes to retain the data after
+// returning.
+type Unmarshaler interface {
+	UnmarshalXrd(data []byte) (int, error)


ok... to address this annoying (to me, at least) issue of this extra int return value, what about modifying the (un)marshalers to something like:

type Marshaler interface { MarshalXrd(buf *Buffer) error } type Unmarshaler interface { UnmarshalXrd(buf *Buffer) error }

where Buffer is the union of the current Encoder and Decoder type.

that way, the current issue of unmarshaling protocol/protocol.Request:

// UnmarshalXrd implements xrootd/protocol.Unmarshaler func (o *Response) UnmarshalXrd(data []byte) (pos int, err error) { dec := xrdenc.NewDecoder(data) o.BinaryProtocolVersion = dec.ReadI32() o.flags = flags(dec.ReadI32()) if dec.Available() > 0 { dec.Skip(1) dec.Skip(1) o.SecurityVersion = dec.ReadU8() o.SecurityOptions = securityOptions(dec.ReadU8()) o.SecurityLevel = protocol.SecurityLevel(dec.ReadU8()) o.SecurityOverrides = make([]protocol.SecurityOverride, dec.ReadU8()) for i := 0; i < len(o.SecurityOverrides); i++ { n, err := o.SecurityOverrides[i].UnmarshalXrd(dec.Bytes()) if err != nil { return 0, err } dec.Skip(n) } } return dec.Pos(), err }

would become:

// UnmarshalXrd implements xrootd/protocol.Unmarshaler func (o *Response) UnmarshalXrd(buf *xrdproto.Buffer) error { o.BinaryProtocolVersion = buf.ReadI32() o.flags = flags(buf.ReadI32()) if buf.Len() > 0 { buf.Skip(1) buf.Skip(1) o.SecurityVersion = buf.ReadU8() o.SecurityOptions = securityOptions(buf.ReadU8()) o.SecurityLevel = protocol.SecurityLevel(buf.ReadU8()) o.SecurityOverrides = make([]protocol.SecurityOverride, buf.ReadU8()) for i := 0; i < len(o.SecurityOverrides); i++ { err := o.SecurityOverrides[i].UnmarshalXrd(buf) if err != nil { return err } } } return nil }

most notably, the dec.Skip(n) statement (which could easily be overlooked and fogotten) just disappeared.

what do you think?

What do you think about keeping Encoder and Decoder and passing them instead of xrdproto.Buffer? Because we don't need to perform any writing in UnmarshalXrd and any reading in MarshalXrd.
So it will look like:

type Marshaler interface { MarshalXrd(enc *xrdenc.Encoder) error } type Unmarshaler interface { UnmarshalXrd(dec *xrdenc.Decoder) error }

Then, what about renaming 'Decoder' into 'RBuffer' and 'Encoder' into 'WBuffer'?

Sounds good!

sbinet · 2018-06-02T07:06:11Z

xrootd/protocol/protocol/protocol.go

+	dec := xrdenc.NewDecoder(data)
+	o.BinaryProtocolVersion = dec.ReadI32()
+	o.flags = flags(dec.ReadI32())
+	if dec.Available() > 0 {


please invert the logic and apply an early return.

sbinet · 2018-06-02T07:08:16Z

xrootd/protocol/protocol/protocol.go

+func (o Response) MarshalXrd() (data []byte, err error) {
+	var enc xrdenc.Encoder
+	enc.WriteI32(o.BinaryProtocolVersion)
+	enc.WriteI32(int32(o.flags))


hum... why isn't the marshaling handling the optional writing part, symmetric to what is done on the unmarshaling side?

This helps to generate boilerplate code which is used for encoding and decoding of the requests and the responses.

sbinet

LGTM

EgorMatirov mentioned this pull request Jun 1, 2018

xrootd: add dirlist request #211

Merged

sbinet reviewed Jun 1, 2018

View reviewed changes

sbinet reviewed Jun 2, 2018

View reviewed changes

EgorMatirov force-pushed the xrootd-marshaler branch from f11712d to e9e54bd Compare June 2, 2018 11:33

xrootd: introduce a XRootD (un)marshal interface and generator

3600d05

This helps to generate boilerplate code which is used for encoding and decoding of the requests and the responses.

EgorMatirov force-pushed the xrootd-marshaler branch from e9e54bd to 3600d05 Compare June 2, 2018 13:00

EgorMatirov mentioned this pull request Jun 2, 2018

xrootd: add open request #215

Merged

sbinet approved these changes Jun 2, 2018

View reviewed changes

sbinet merged commit dfaefec into go-hep:master Jun 2, 2018

sbinet mentioned this pull request Jun 2, 2018

Xrootd marshaler #210

Closed

EgorMatirov deleted the xrootd-marshaler branch June 5, 2018 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xrootd: introduce a XRootD (un)marshal interface and generator #214

xrootd: introduce a XRootD (un)marshal interface and generator #214

EgorMatirov commented Jun 1, 2018 •

edited

sbinet left a comment

sbinet Jun 1, 2018

sbinet Jun 1, 2018 •

edited

sbinet Jun 1, 2018

sbinet Jun 1, 2018

sbinet Jun 1, 2018

EgorMatirov Jun 1, 2018

sbinet Jun 1, 2018

sbinet Jun 1, 2018

sbinet Jun 1, 2018

sbinet Jun 1, 2018

sbinet Jun 1, 2018

EgorMatirov Jun 1, 2018

sbinet Jun 2, 2018

EgorMatirov Jun 2, 2018

sbinet Jun 2, 2018

EgorMatirov Jun 2, 2018

sbinet Jun 2, 2018

sbinet Jun 2, 2018

sbinet left a comment

xrootd: introduce a XRootD (un)marshal interface and generator #214

xrootd: introduce a XRootD (un)marshal interface and generator #214

Conversation

EgorMatirov commented Jun 1, 2018 • edited

sbinet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbinet Jun 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbinet left a comment

Choose a reason for hiding this comment

EgorMatirov commented Jun 1, 2018 •

edited

sbinet Jun 1, 2018 •

edited