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

LOC: fix float altitude value ingestion, gate size and precision values #3130

Merged
merged 4 commits into from
Sep 27, 2024

Conversation

systemcrash
Copy link
Contributor

@systemcrash systemcrash commented Sep 24, 2024

Previously, decimal portions of altitudes may have been truncated, resulting in, at worst, loss of up to 0.99m.

Altitude is still internally uint32, however (according to RFC standard).

We now gate the size and precision values, and handle those correctly.

Tested locally against Loopia. Result ✅

@systemcrash systemcrash marked this pull request as draft September 24, 2024 20:16
@hmoffatt
Copy link
Contributor

Thanks, looks good to me. I have added LOC for CLOUDNS in #3127. Right now I'm stripping the decimal portion in the provider else SetTargetLOCString fails in the scanf, so I'll need to revise this once this PR is merged.

@systemcrash systemcrash changed the title LOC: Reworked to ingest float altitude values correctly LOC: fix float altitude value ingestion, gate size and precision values Sep 25, 2024
@systemcrash systemcrash marked this pull request as ready for review September 25, 2024 01:00
@systemcrash
Copy link
Contributor Author

Well... that was fun. @hmoffatt go ahead and run some tests with the current code if you have the time. Otherwise I think I got everything.

@hmoffatt
Copy link
Contributor

On a related note, in the Javascript you can write non-integer values for the degrees/minutes fields, but the results are very strange - setting degrees to 1.2 corrupts the minutes field. I'm not sure if it's worth reporting or not.

@hmoffatt
Copy link
Contributor

Well... that was fun. @hmoffatt go ahead and run some tests with the current code if you have the time. Otherwise I think I got everything.

I tested my CLOUDNS changes with yours and the test suite passed.

@systemcrash
Copy link
Contributor Author

On a related note, in the Javascript you can write non-integer values for the degrees/minutes fields, but the results are very strange - setting degrees to 1.2 corrupts the minutes field. I'm not sure if it's worth reporting or not.

Can you give me an example? It's not impossible that weird values result, since there is not much error checking there.

Check the LOC builder helper functions:

https://docs.dnscontrol.org/language-reference/domain-modifiers/loc_builder_dd

@hmoffatt
Copy link
Contributor

Can you give me an example? It's not impossible that weird values result, since there is not much error checking there.

I have the following defined

LOC("foo", 1.2, 2, 3,     "N", 4,  5, 6.3,     "E", -10030,   0.2,    300,  400)

which causes

#1: + CREATE LOC foo.xyz.com 01 14 3.000 N 04 05 6.300 E -10030m 0.20m 300m 400m ttl=3600

At a glance maybe the 1.2 degrees turned into 1 degree 12 minutes, which was added to the 2 minutes I asked for. My input was nonsense, but if there was some way to reject it that wouldn't be a bad thing. But that's for another issue.

Previously, decimal portions of altitudes may have been truncated,
resulting in, at worst, loss of up to 0.99m.

Altitude is still internally uint32, however (according to RFC standard).
Sync also with helpers.js

Add test-cases to verify size and precision threshold values
@systemcrash
Copy link
Contributor Author

LOC() throws an error now if the appropriate values are not integers.

@hmoffatt
Copy link
Contributor

Looks good!

@tlimoncelli
Copy link
Contributor

The go code all looks good. TBH, I don't understand all the lat/long stuff, but if you say its good to merge, I'll merge it!

@systemcrash
Copy link
Contributor Author

As a whole, integer/float 32/64 bit discrimination stuff. The max size number 90,000,00 is a meters float value (or 90,000km in the standard), but it is converted to cm so 90,000,000 * 100 => 32 bit overflow. These values get packed into a uint8 - 4bits+4bits - which represents the mantissa and exponent. "This allows sizes from 0e0 (<1cm) to 9e9 (90,000km) to be expressed."

For wire formats, the standards are space conscious.

I'm happy with how things are here.

@tlimoncelli tlimoncelli mentioned this pull request Sep 27, 2024
@tlimoncelli
Copy link
Contributor

Great! Thanks, folks!

@tlimoncelli tlimoncelli merged commit d6d50fc into StackExchange:main Sep 27, 2024
22 checks passed
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.

3 participants