nixos/networkd: add missing options #44233
Conversation
|
Probably should be also backported. |
There was a problem hiding this comment.
This doesn't look to bad. I have not checked every option there is.
One thing that always was an issue in the past:
How do we keep that list updated? What was your process to add(&test) the missing fields?
Are you targeting the current systemd-networkd version from unstable or from the systemd master branch?
|
I suppose once it is complete, it can be updated by diffing |
jfrankenau
force-pushed
the
networkd-fix-options
branch
from
42df9cd
to
8d09510
Compare
|
Yes, I have been using the man pages and mostly been going through by hand and some XML queries on their sources using xq from the yq package. Diffing would be the best way to go when thinking about updating it. I have actually used the man pages from systemd 239 which means that there are some options which are not yet supported but not the other way around I think. I hope that 239 comes to NixOS in the near future anyway. Personally I have mixed feelings about replicating all those options in Nix. It is a tedious process and oftentimes not clear which NixOS option to use when looking at the systemd man pages, especially when systemd allows specifying an option more than once which needs to be abstracted into a list in Nix. Take for example |
|
Personally I by pass nixos's module system for networkd: https://github.com/Mic92/dotfiles/blob/master/nixos/network-configuration.nix#L185 |
|
I started working on v239 support NixOS/systemd#20 |
|
@jflanglois can you have a look if everything from #40573 is included in your PR? We could then close #40573… |
|
@flokli did you mean @jfrankenau? ;) |
|
yes, of course - github autocompletion gone wrong, sorry :-) |
|
No worries! |
jfrankenau
force-pushed
the
networkd-fix-options
branch
from
8d09510
to
b2f1790
Compare
|
@flokli, yes of course. I have added your changes. |
|
Did you test this with link definitions? It looks like you used |
|
@lopsided98, sorry. I blame auto completion for it. ;) |
|
Those same lines also break 32-bit Nix, because integers are 32 bits signed, so 4294967295 exceeds their maximum value. I'm not sure what to do about that. Do we really need to check the range of those options? |
|
Checking that it is an integer should be enough. |
|
@lopsided98: Ah, sorry, didn't see your comments as I've commented on the corresponding commit directly. |
|
See #45722 for a fix. |
Using a 64bit integer on 32bit systems will come with a bit of a performance overhead, but given that Nix doesn't use a lot of integers compared to other types, I think the overhead is neglicible also considering that 32bit systems are in decline. The biggest advantage however is that when we use a consistent integer size accross all platforms it's less likely that we miss things that we break due to that. One example would be: NixOS/nixpkgs#44233 On Hydra it will evaluate, because the evaluator runs on a 64bit machine, but when evaluating the same on a 32bit machine it will fail, so using 64bit integers should make that consistent. While the change of the type in value.hh is rather easy to do, we have a few more options available for doing the conversion in the lexer: * Via an #ifdef on the architecture and using strtol() or strtoll() accordingly depending on which architecture we are. For the #ifdef we would need another AX_COMPILE_CHECK_SIZEOF in configure.ac. * Using istringstream, which would involve copying the value. * As we're already using boost, lexical_cast might be a good idea. Spoiler: I went for the latter, first of all because lexical_cast does have an overload for const char* and second of all, because it doesn't involve copying around the input string. Also, it seems to come with a bigger overhead than boost::lexical_cast: https://www.boost.org/doc/libs/release/doc/html/boost_lexical_cast/performance.html The first method (still using strtol/strtoll) also wasn't something I pursued further, because it is also locale-aware which I doubt is what we want, given that the regex for int is [0-9]+. Signed-off-by: aszlig <aszlig@nix.build> Fixes: NixOS#2339
Using a 64bit integer on 32bit systems will come with a bit of a performance overhead, but given that Nix doesn't use a lot of integers compared to other types, I think the overhead is negligible also considering that 32bit systems are in decline. The biggest advantage however is that when we use a consistent integer size across all platforms it's less likely that we miss things that we break due to that. One example would be: NixOS/nixpkgs#44233 On Hydra it will evaluate, because the evaluator runs on a 64bit machine, but when evaluating the same on a 32bit machine it will fail, so using 64bit integers should make that consistent. While the change of the type in value.hh is rather easy to do, we have a few more options available for doing the conversion in the lexer: * Via an #ifdef on the architecture and using strtol() or strtoll() accordingly depending on which architecture we are. For the #ifdef we would need another AX_COMPILE_CHECK_SIZEOF in configure.ac. * Using istringstream, which would involve copying the value. * As we're already using boost, lexical_cast might be a good idea. Spoiler: I went for the latter, first of all because lexical_cast does have an overload for const char* and second of all, because it doesn't involve copying around the input string. Also, it seems to come with a bigger overhead than boost::lexical_cast: https://www.boost.org/doc/libs/release/doc/html/boost_lexical_cast/performance.html The first method (still using strtol/strtoll) also wasn't something I pursued further, because it is also locale-aware which I doubt is what we want, given that the regex for int is [0-9]+. Signed-off-by: aszlig <aszlig@nix.build> Fixes: NixOS#2339
Using a 64bit integer on 32bit systems will come with a bit of a performance overhead, but given that Nix doesn't use a lot of integers compared to other types, I think the overhead is negligible also considering that 32bit systems are in decline. The biggest advantage however is that when we use a consistent integer size across all platforms it's less likely that we miss things that we break due to that. One example would be: NixOS/nixpkgs#44233 On Hydra it will evaluate, because the evaluator runs on a 64bit machine, but when evaluating the same on a 32bit machine it will fail, so using 64bit integers should make that consistent. While the change of the type in value.hh is rather easy to do, we have a few more options available for doing the conversion in the lexer: * Via an #ifdef on the architecture and using strtol() or strtoll() accordingly depending on which architecture we are. For the #ifdef we would need another AX_COMPILE_CHECK_SIZEOF in configure.ac. * Using istringstream, which would involve copying the value. * As we're already using boost, lexical_cast might be a good idea. Spoiler: I went for the latter, first of all because lexical_cast does have an overload for const char* and second of all, because it doesn't involve copying around the input string. Also, because istringstream seems to come with a bigger overhead than boost::lexical_cast: https://www.boost.org/doc/libs/release/doc/html/boost_lexical_cast/performance.html The first method (still using strtol/strtoll) also wasn't something I pursued further, because it is also locale-aware which I doubt is what we want, given that the regex for int is [0-9]+. Signed-off-by: aszlig <aszlig@nix.build> Fixes: NixOS#2339
Motivation for this change
The networkd module misses quite a few options which I'd like to use.
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)