Adding support to ipv4.route-metric and ipv6.route-metric
Details
Details
- Reviewers
jgrulich lvsouza - Commits
- R282:2bf92ff35886: Adding support to ipv*.route-metric
Compiled and tested
Diff Detail
Diff Detail
- Repository
- R282 NetworkManagerQt
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Sorry if something are missing, its my first pull request, let me know if any change are needed. I've tested and it's working fine.
Comment Actions
Looks good to me. If you can also remove this property from the TODO list that would be great.
Btw. there is the same property missing in IPv6Setting in case you would like to do the same for IPv6.
Comment Actions
Looks good, thank you so much.
| src/settings/ipv6setting.cpp | ||
|---|---|---|
| 412 | Missing space between if and bracket. | |
Comment Actions
The summary says this patch adds route metric support to IPv4 too, but no IPv4 file is touched by this patch. Have you missed the IPv4 changes?
| src/settings/ipv6setting.cpp | ||
|---|---|---|
| 412 | You could have added a comment stating that according to NetworkManager's documentation zero is not a possible value for IPv6's route metric (the value is automatically changed to 1024 when trying to set it to zero). | |
Comment Actions
Adding missing files and making possible to set route-metric as 0, as described in NetworkManager documentation.
Comment Actions
Added the missing files.
About your comment suggestion, I preferred to change the code and accept the value 0, so that it works as specified by the documentation. In the case of ipv6, value 0 is accepted, but will be coerced to 1024. For IPv4, zero is a regular value for the metric.
Comment Actions
Looks good to me now, it looked to me good even before, but didn't read carefully the documentation. Lamarque, do you see anything else?
| src/settings/ipv4setting.cpp | ||
|---|---|---|
| 455 | Missing space between if and bracket. | |