Fix nil input to starting key of batch lookup

[christarazi]

Batch lookup requires the caller to provide the starting key
(startKey) from which the lookup begins. In order to perform a batch
lookup from the very beginning of the map, it is expected to pass a nil
startKey. Due to the type of startKey being interface{}, this
means that it's possible for the value of startKey to have a concrete
type T, but contain a nil value.

For example, let's say the concrete type T is an *int.

func dumpAll[T comparable](m *Map, startKey, key *T) (count int, err error) {
    ...
    m.batchLookup(..., startKey, ...)
    ...
}

dumpAll(m, nil, new(int))

The first call to dumpAll() must pass nil to startKey. When it
reaches (*Map).batchLookup(), startKey becomes a non-nil
interface{} with an underlying concrete type of *int, whose value
is nil.

This is problematic because startKey != nil evaluates to true and the
call to marshalMapSyscallInput() expects startKey to contain a
non-nil value.

This commit fixes this by checking whether the underlying value of the
interface contains a non-nil value.

Signed-off-by: Chris Tarazi chris@isovalent.com

[christarazi]

cc @ti-mo @lmb

[lmb]

Hmm, what about Map.NextKey and other similar APIs? That also currently only accepts a plain nil and not an interface one. There are probably lots of other places where we'd need to make this change for consistency. I agree that nil vs typed nil is confusing, but sprinkling reflect.ValueOf checks throughout the code base is pretty brittle as well.

Could you work around this issue in your generic function by defining nextKey to be any instead of T? Then the default value is bare nil. Later on you assign to nextKey from your array of keys, which ends up doing the implicit boxing for you.

[christarazi]

Hmm, what about Map.NextKey and other similar APIs? That also currently only accepts a plain nil and not an interface one. There are probably lots of other places where we'd need to make this change for consistency. I agree that nil vs typed nil is confusing, but sprinkling reflect.ValueOf checks throughout the code base is pretty brittle as well.

I'm happy to expand this PR to be consist everywhere else.

Could you work around this issue in your generic function by defining nextKey to be any instead of T? Then the default value is bare nil. Later on you assign to nextKey from your array of keys, which ends up doing the implicit boxing for you.

The problem with this suggestion is that it riddles the code with interface{} / any and does not allow you to use generics.

sprinkling reflect.ValueOf checks throughout the code base

By doing this though, it allows users to use both interface{} / any or generics, and doesn't force them into one paradigm. It's also not so bad because it's just one extra check. Reflection is already being used in these functions.

Supporting generics at the cost of one extra function call to reflect.ValueOf doesn't seem to be too consequential.

[lmb]

The problem with this suggestion is that it riddles the code with interface{} / any and does not allow you to use generics.

I don't fully understand how this prevents using generics. I guess you mean "makes calling the API from generic code cumbersome"? From your description it sounds like there is an error returned by the method, but its probably cryptic?

Maybe you have a link to the code you want to be able to write?

Supporting generics at the cost of one extra function call to reflect.ValueOf doesn't seem to be too consequential.

The cost isn't in the overhead of reflect.ValueOf but in auditing the code to find all the places it is needed and maintaining these semantics in the future. My biggest worry is that for consistency we'd need to handle nil vs typed nil for more any arguments. All of this any business together with how we marshal and unmarshal map and key values is already really complicated (recent example #1180) so I'm reluctant to make it even more subtle.

[christarazi]

@lmb The example code I'm writing is cilium/cilium#28183.

See function (m *Map) Count(). The nil parameter that's being passed to countBatch() is the parameter that's causing issues that I submitted this PR here for.

[christarazi]

JFYI, I've managed to workaround this issue by having the prev parameter be of type interface{}. Other parameters are of type T which makes the type assertion a bit less cumbersome, than if all parameters were interface{} and didn't use generics.

[lmb]

JFYI, I've managed to workaround this issue by having the prev parameter be of type interface{}.

Nice!

For what it's worth, this change might still be worth it, provided we only have to do this check for NextKey and Batch lookups. @ti-mo any opinions on whether we should tackle nil vs interface nil? What else would we have to take into account?

You also mentioned that prevKey isn't really a key but more of an opaque handle of sorts? Does it make sense to rename the parameter or document this behaviour?

[christarazi]

You also mentioned that prevKey isn't really a key but more of an opaque handle of sorts? Does it make sense to rename the parameter or document this behaviour?

Yes, I will file a PR that documents the behavior and consider a rename this week.

[christarazi]

@lmb @ti-mo #1223

[christarazi]

Superseded by #1223