all: encode platform in type constants #1689
Conversation
7379a07 to
c38959e
Compare
|
c38959e to
3843dfc
Compare
| } | ||
|
|
||
| // AppendInstructions decodes [Instruction] from r and appends them to insns. | ||
| func AppendInstructions(insns Instructions, r io.Reader, bo binary.ByteOrder, platform string) (Instructions, error) { |
There was a problem hiding this comment.
Any strong reason for this change? Could this become Instructions.Append() instead of a free function? I agree it makes more sense than Unmarshal() with the accompanying replacement semantic.
There was a problem hiding this comment.
The common operation is to encode into a new slice. With a free function that is:
AppendInstructions(nil, ...)
As a method it requires an extra line:
var insns Instructions
insns.Append(...)
| @@ -104,6 +105,16 @@ const ( | |||
| Arena | |||
| ) | |||
|
|
|||
| // MapTypeForPlatform returns a platform specific map type. | |||
| func MapTypeForPlatform(plat string, typ uint32) (MapType, error) { | |||
There was a problem hiding this comment.
This is pure personal taste, but I find this a bit unusual API. If I understand correctly, exernal callers can use this to create MapSpecs for a given target based on a well-known map type (e.g. give me whatever Hash Windows supports), but also to check if ebpf-go supports the Windows equivalent of a given map type (e.g. PerCPUCGroupStorage.On(platform.Windows) will fail). This could be useful for creating portable apps if such a thing ever makes sense.
What about making this a method instead? It allows for more succinct/expressive uses:
t, err := HashOfMaps.On(platform.Windows)(or HashOfMaps.For(platform.Windows))
compared to
t, err := MapTypeForPlatform(platforn.Windows, HashOfMaps)Initially, I thought of this as t, err := HashOfMaps.Platform(platforn.Windows), but that stutters when used with the internal platform constants.
There was a problem hiding this comment.
That's not how it's intended to be used at the moment. It solves the problem of what to do if a new map type is introduced which the library doesn't have a constant for (yet). Then you can do:
mapType, err := MapTypeForPlatform("windows", 0xfeed)
// use mapType in calls that expect MapType
Remember that HashOfMaps on linux does not have the same numerical value as HashOfMaps on windows.
Linux and Windows share the concept of program types, map types and so on. For example, XDP is supported on both platforms. Unfortunately the constant values used by both platforms are different. On Linux, XDP has value 0x6 while on Windows it is 0x1. This problem extends to a CollectionSpec parsed from an ELF. Because of the different values a Windows PerCPUHash aliases with a Linux ProgramArray. This causes all sorts of mayhem that is hard to avoid without making too many changes to the code base. Introduce the notion of a supported platform. External users only see this as a string, like "linux". Internally each platform has a corresponding tag which is used to encode the platform into a constant. This means that there are distinct constants for each platform. Instead of overloading the XDP constant we will introduce WindowsXDP. This will allow the ELF reader to remain platform independent and makes generating stringers a lot easier. The value of the platform tag is chosen so that on Linux the constant values do not change and are identical with the value in upstream headers. On Windows the constants have a fixed (but implementation defined) offset. The downside of this approach is that Windows constants will all require a prefix of some sort to distinguish them from Linux ones. This is probably most onerous for map types, because those tend to be created manually more frequently than programs, for example. We can add some behind the scenes translation of Linux map types to Windows ones if need be. Another downside is that constant values are now limited to 28 bits since we steal the top 4 bits to store the platform. The constants we're applying this to are all sequentially numbered from 0, so this is hopefully enough. Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
Linux and Windows share the concept of program types, map types and so on. For example, XDP is supported on both platforms. Unfortunately the constant values used by both platforms are different. On Linux, XDP has value 0x6 while on Windows it is 0x1.
This problem extends to a CollectionSpec parsed from an ELF. Because of the different values a Windows PerCPUHash aliases with a Linux ProgramArray. This causes all sorts of mayhem that is hard to avoid without making too many changes to the code base.
Introduce the notion of a supported platform. External users only see this as a string, like "linux". Internally each platform has a corresponding tag which is used to encode the platform into a constant. This means that there are distinct constants for each platform. Instead of overloading the XDP constant we will introduce WindowsXDP. This will allow the ELF reader to remain platform independent and makes generating stringers a lot easier.
The value of the platform tag is chosen so that on Linux the constant values do not change and are identical with the value in upstream headers. On Windows the constants have a fixed (but implementation defined) offset.
The downside of this approach is that Windows constants will all require a prefix of some sort to distinguish them from Linux ones. This is probably most onerous for map types, because those tend to be created manually more frequently than programs, for example. We can add some behind the scenes translation of Linux map types to Windows ones if need be.
Another downside is that constant values are now limited to 28 bits since we steal the top 4 bits to store the platform. The constants we're applying this to are all sequentially numbered from 0, so this is hopefully enough.