FDio / govpp

Go toolset for the VPP.
Apache License 2.0
199 stars 82 forks source link

Fix generated RPC client for Dump+Reply requests #64

Closed ondrej-fabry closed 2 years ago

ondrej-fabry commented 2 years ago

Intro

There are two types of dump requests in VPP API:

  1. Simple dump - send [dump] and receive [details, details, ..]
  2. Dump+Reply - send [dump] and received [details, details, .., reply]

Below is a code sample of generated code for Recv() method of RPC client for dumping output interfaces in nat44_ed VPP API:

https://github.com/FDio/govpp/blob/f77cad358519c63ce5a184765726fbb79b6f9be5/binapi/nat44_ed/nat44_ed_rpc.ba.go#L233-L250

Current problems

The generated code for the RPC client for Dump+Reply requests does not provide access to the *Reply message that is received as last message. This practically makes the generated code unusable for users that need the *Reply message values, e.g. reply message often contains a Cursor field where its value is used in the next request.

Another problem is that the Retval field from the *Reply is not converted to error.

Possible solutions

There are at least three alternative solutions that could be implemented to fix this.

1. SOLUTION

Add another method RecvReply() to dump client

When calling Recv() and receiving io.EOF error, the actual *Reply message received would be stored to a field of dump client and user would call RecvReply() (placeholder name) to receive it.

type RPCService_Nat44EdOutputInterfaceGetClient interface {
    Recv() (*Nat44EdOutputInterfaceDetails, error)
    RecvReply() *Nat44EdOutputInterfaceGetReply
    api.Stream
}

CONS:

2. SOLUTION

Define union-like type and return it instead of *Details

When calling Recv() and receiving io.EOF error, the return value would only have *Reply message set otherwise only *Details message would be set.

type Nat44EdOutputInterfaceGetX struct {
    Details *Nat44EdOutputInterfaceDetails
    Reply *Nat44EdOutputInterfaceGetReply
}

type RPCService_Nat44EdOutputInterfaceGetClient interface {
    Recv() (*Nat44EdOutputInterfaceGetX, error)
    api.Stream
}

CONS:

3. SOLUTION

Return 3 values from Recv()

When calling Recv() and receiving io.EOF error, the actual *Reply message received would be returned as second return value, otherwise only *Details or error would be non-nil.

type RPCService_Nat44EdOutputInterfaceGetClient interface {
    Recv() (*Nat44EdOutputInterfaceDetails, *Nat44EdOutputInterfaceGetReply, error)
    api.Stream
}

CONS:

I am personally for solution 3 as I do not see any disadvantages for it.


CC @edwarnicke @sknat

sknat commented 2 years ago

Agreed, solution 3 seems to be ideal among the three. The only drawback I can see is that it adds an extra return value and most users won't need it, but it's easy to ignore. Also that way it becomes a natural extension of RecvMsg()