Skip to content

Conversation

@talmakion
Copy link
Contributor

Change summary

  • Adding support for hex, octal and binary integers (decimal-only floats)
  • Defaults retain current parameter and parsing behaviour
  • Any radix option overrides default assumption of "--decimal"
  • Radix options are inclusive together

Some peculiarities & further thoughts:

  • Since number_of_string is used to parse other parameters, selected radices and prefixes must be observed for parameters. I've overridden both range and the not-value handlers to always permit decimal notation for convenience - this could be removed or extended to allow all supported radices, easily enough
  • I've completely ignored hex floats and just used integer handling for any non-decimals
  • There could be an --any-radix or --any-base flag as well, I've left this out
  • I had intended to make an --assume-<radix> type option for values missing a prefix - but this can likely be dealt with in conf XML with less fuss
  • My use of OCaml or formatting likely isn't entirely idiomatic - it's been a decade or 2

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • Other (please describe):

Related Task(s)

Related PR(s)

How to test / Smoketest result

  • Firewall, BGP, policy, LPBR and PBR seem to use a lot of numeric validation, those smoketests pass without issue
  • Floating point validation still works as before, and rejects attempts at non-decimal floats (only used in a couple of spots in the conf-tree)
  • Relative values, +/-, work in all radices allowed as expected (only used in one spot in conf)

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

* Adding support for hex, octal and binary integers (decimal-only floats)
* Defaults retain current parameter and parsing behaviour
* Any radix option overrides default assumption of "--decimal"
* Radix options are inclusive together
* As number_of_string is used for parameters, param parsing is overridden
  to always allow un-prefixed decimals, for convenience.
@dmbaturin
Copy link
Member

Let's collect all locations where we may need that:

  • GRE ethertype.
  • DHCP options?
  • QoS ?

@talmakion
Copy link
Contributor Author

Places where it looks like 0x notation is already used:

  • GRE ethertype
  • Tunnel interface traffic class
  • IPv6 flowlabels

There's a bunch of un-prefixed use of hexadecimal that wouldn't go well with this validator and should stay regex. Packing OpenConnect's [a-fA-F0-9]{20,10000} keys into an integer is a bit of a stretch.

Potential uses:

  • connmarks and fwmarks - admittedly less useful here, since VyOS doesn't expose bitmask matches
  • Bridge firewalls match a handful of well-known ethertypes by name, there's currently no way for ad-hoc matches to be made - very niche
  • L2 QoS protocol is a decimal match against ethertype
  • DSCP & ToS may be considered bitfields, but are usually well-known values in practice that are already aliased

I've just noticed how many different places refer to ethertype in wildly different ways tbh. Admittedly the same syntax doesn't work the same way everywhere, in the underlying.

Copy link
Contributor

@jestabro jestabro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks reasonable to me. All smoketests pass on image with PR.

Copy link
Member

@dmbaturin dmbaturin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The need is real and the implementation seems good. Let's give it a try.

@dmbaturin dmbaturin merged commit d93ec62 into vyos:current Aug 4, 2025
@github-actions
Copy link

github-actions bot commented Aug 4, 2025


Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

@dmbaturin
Copy link
Member

Thanks, @talmakion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants