digitalocean / go-openvswitch

Go packages which enable interacting with Open vSwitch and related tools. Apache 2.0 Licensed.
Other
295 stars 95 forks source link

Fixed size of PortID in PortStat #59

Closed hwchiu closed 6 years ago

hwchiu commented 6 years ago

Hii.

The type of PortID in the PortStats is int and it's not a fixed length type. We will meet the problems if we want to write that PortStats struct to buffer via the binary.Write.

I think we can fix that problem by changing the type from int to int32 to make the whole structure fixed length.

What do you think ?

Thanks.

type PortStats struct {
    // PortID specifies the OVS port ID which this PortStats refers to.
    PortID int

    // Received and Transmitted contain information regarding the number
    // of received and transmitted packets, bytes, etc.
    // OVS stores all of these counters as uint64 values.
    Received    PortStatsReceive
    Transmitted PortStatsTransmit
}
mdlayher commented 6 years ago

I am hesitant to make this change because we have used this field for years internally as an unsized integer like this. What is the benefit of being able to convert the structure using binary.Write?

hwchiu commented 6 years ago

Hi. Thanks your reply. I build a gRPC server/client in my environment and the server will reply the PortStats to the client. Since I don't want to re-design a new structure like PortStats in my gRPC prototype, I use the type [][]byte to represent the data of PortStats and that's why I should use the binary.Write to convert the data from the well-known structure PortStats to a common type buffer. In this case, sine the int isn't a fixed length filed and I can't use the binary.Write to convert the data.

Thanks.

mdlayher commented 6 years ago

I can't say I'd recommend doing that. Generally with gRPC, you want to create explicit message types for each type of data you're passing; an opaque bytes or repeated bytes type says absolutely nothing about the contents. On top of that, what's the endianness of those opaque bytes?

That said, if Open vSwitch uses some kind of fixed size integer internally to represent the types, it might be more reasonable to make the change. I'm guessing it does. This package doesn't intend to provide any kind of structure size or layout guarantees though; it's pretty high-level.

hwchiu commented 6 years ago

Got it, Thanks.