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

cannot parse high u64 default values in the API files #118

Open
hardeker opened this issue Mar 22, 2023 · 3 comments
Open

cannot parse high u64 default values in the API files #118

hardeker opened this issue Mar 22, 2023 · 3 comments

Comments

@hardeker
Copy link

As Govpp doesn't know in advance the exact template of the json it parses, it uses a generic parsing with json.Unmarshal here
However, when unmarshalling generic json in Go, numbers are casted in float64. Now, if the Json contain uint64 numbers, we lose precision casting them as float64.

As we allow u64 types in the VPP API, we may end up with a wrong value after parsing the API. And casting float64 into uint64 depends on the hardware implementation.
In Govpp, the conversion happens here
Let's take a VPP API exmaple:

 [
    "u64",
    "value",
    {
        "default": 18446744073709551615
    }
],

If we generate Go bindings with this Json, we get:

  • u64,name=value,default=9223372036854775808 if we run it on x86 devices
  • u64,name=value,default=18446744073709551615 if we run it on arm devices
@ondrej-fabry ondrej-fabry self-assigned this Mar 22, 2023
@hardeker
Copy link
Author

I will have a look at other VPP bindings generators to see how they react to that.

@ondrej-fabry
Copy link
Member

ondrej-fabry commented Mar 28, 2023

I have played around a bit with this and so far it seems the library we are using to process JSON does not seem to be flexible for parsing numbers. There is MustGetUint64 method, but it does not return the correct number, most likely due incorrect conversions.

I also tried standard encoding/json to parse number with max uint64 value and I was able to properly parse it only when decoding the value directly into uint64 variable or decoding into json.Number (practically a string) and then using strconv.ParseUint to convert it into uint64, because the json.Number only has Float64 and Int64 methods for quick conversion.

I don't want to spend much more time than I already did, but I plan to come back to this issue later after I am done with other more urgent matters. The default option for field is not used anywhere in the GoVPP beside being set as struct tag in the generated bindings and also there is no case that I know of in the official VPP API that has the default set to max uint64.

Anyone is welcome to take a shot and open PR if you find a fix for this.

@ondrej-fabry ondrej-fabry removed their assignment Mar 28, 2023
@ondrej-fabry ondrej-fabry moved this from Backlog to Todo in GoVPP Development Mar 28, 2023
@hardeker
Copy link
Author

hardeker commented Apr 6, 2023

I just had a look at the Python bindings. The json library commonly used in Python does differentiate between int and float. If the number contains a dot, it is considered as a float, otherwise, it is considered as an int. Thus, the problem does not appear in Python.

@ondrej-fabry ondrej-fabry moved this from Todo to Blocked / On-Hold in GoVPP Development Apr 28, 2023
@ondrej-fabry ondrej-fabry added the generator Generator label Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Blocked / On-Hold
Development

No branches or pull requests

2 participants