datasource: Allow setting smaller static strings

[mauriciovasquezbernal]

There are cases when an operator needs to change the value of a static string (coming from ebpf for instance). This commit ipt improves the logic to allow the operator calling the PutString() method with a string that is smaller than the buffer size.

---

I found this issue / limitation when trying to update the name field in wasm in #2823. That PR implements a workaround for it, but IMO it needs to be handled directly in the accessor.

@flyth any opinions on this one?

[flyth]

Hmm...

I think we need to add errors to the Put* functions to do these kind of things, otherwise it could lead to (from the developer) unexpected behavior (e.g. some Puts would work, some wouldn't (if the string is too long)). To be fair, it's already the case if you'd try to write a string with the exact length of the field vs. another length right now.

IMHO this isn't best practice in the first place (editing a statically sized string), but if we allow it, we should at least make sure it's reported if it didn't succeed.

[mauriciovasquezbernal]

I think we need to add errors to the Put* functions to do these kind of things, otherwise it could lead to (from the developer) unexpected behavior (e.g. some Puts would work, some wouldn't (if the string is too long)). To be fair, it's already the case if you'd try to write a string with the exact length of the field vs. another length right now.

I totally agree. What do you think about the "Get()" methods? Should we return errors instead of default value when something goes wrong?

IMHO this isn't best practice in the first place (editing a statically sized string), but if we allow it, we should at least make sure it's reported if it didn't succeed.

I don't see any reason why we should block this. I feel there are use cases where this is valuable.

[flyth]

I think we need to add errors to the Put* functions to do these kind of things, otherwise it could lead to (from the developer) unexpected behavior (e.g. some Puts would work, some wouldn't (if the string is too long)). To be fair, it's already the case if you'd try to write a string with the exact length of the field vs. another length right now.

I totally agree. What do you think about the "Get()" methods? Should we return errors instead of default value when something goes wrong?

Yeah, let's do it - better now than later. I was a bit worried about returning two values, because it might not be compatible with all languages that would compile to WASM - but I think it's better to have a clean implementation on the host and let the guest APIs worry about getting it right there.

IMHO this isn't best practice in the first place (editing a statically sized string), but if we allow it, we should at least make sure it's reported if it didn't succeed.

I don't see any reason why we should block this. I feel there are use cases where this is valuable.

I agree, we shouldn't block this.

[mauriciovasquezbernal]

Updated the PR to return errors. The code is not as clean as before, but we need it.

[flyth]

Thanks for addressing the errors - it's indeed not as clean looking as before, but - well - I guess cleaner working. 🤷

[flyth]

Do you think there are cases when it makes sense to read a Uint8 from something that's larger than 1 byte without specifying an additional offset? Otherwise maybe it's cleaner to just return ErrInvalidFieldLength and check for len(b) != 1. Also for the other types.

(Usually I'm not into restricting users from doing weird stuff, but here I don't know...)

[mauriciovasquezbernal]

Added that check as well. At this point, I'm wondering if we should just check the type of the field instead... (but I'll hold that discussion for a bit).

[flyth]

LGTM, thanks for cleaning this up!

[flyth]

Nice!

[mauriciovasquezbernal]

I took me quite few iterations to reach this! Glad to see you liked it!