Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sysenc: special case unmarshaling into []byte #1180

Merged
merged 2 commits into from Oct 20, 2023

Conversation

lmb
Copy link
Collaborator

@lmb lmb commented Oct 20, 2023

map: use t.Cleanup in createArray

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>

sysenc: special case unmarshaling into []byte

The library has somewhat complicated semantics when unmarshaling map keys
and values. It turns out that the following was supported
(but not documented) until the zero-allocation marshaling work broke it:

    var s []byte
   m.Lookup(key, &s)

The important thing is that the slice here is empty. The current code treats
this as "the key should be zero length" while originally we'd assign the
temporary buffer used by the syscall to s as a way to reduce allocations. In
hindsight this wasn't a great idea, but it is what it is.

Reintroduce the byte slice special case.

Fixes https://github.com/cilium/ebpf/issues/1175

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
@lmb lmb requested a review from rgo3 October 20, 2023 09:26
The library has somewhat complicated semantics when unmarshaling
map keys and values. It turns out that the following was supported
(but not documented) until the zero-allocation marshaling work
broke it:

    var s []byte
    m.Lookup(key, &s)

The important thing is that the slice here is empty. The current
code treats this as "the key should be zero length" while originally
we'd assign the temporary buffer used by the syscall to s as a way to
reduce allocations. In hindsight this wasn't a great idea, but it is
what it is.

Reintroduce the byte slice special case.

Fixes cilium#1175

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
@lmb lmb merged commit e83be32 into cilium:main Oct 20, 2023
12 checks passed
@lmb lmb deleted the zero-alloc-slice-fix branch October 20, 2023 14:51
@ti-mo
Copy link
Collaborator

ti-mo commented Oct 24, 2023

Interesting, but couldn't we have left it the way it was and explicitly returned an error? The old behaviour is a little surprising; you give it a pointer to a slice (allocated by you!) and the lib updates the pointer to point to a new slice? That's not ideal if you're expecting to read a value from the original slice's backing memory.

@lmb
Copy link
Collaborator Author

lmb commented Oct 24, 2023

I think we can still decide to do that, but it should be a deliberate choice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants