Review Request: Add option to configure IPv6 Privacy Extension

Discussion:

Lamarque Vieira Souza

2013-01-02 22:10:58 UTC

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108064/#review24518
-----------------------------------------------------------

libs/internals/settings/ipv6.cpp
<http://git.reviewboard.kde.org/r/108064/#comment18819>

I think you should create a enum like there Ipv6Setting::EnumMethod instead of using hardcoded integer values. That works like a documentation about what are the possible values for mPrivacy.

- Lamarque Vieira Souza

-----------------------------------------------------------
http://git.reviewboard.kde.org/r/108064/
-----------------------------------------------------------
(Updated Jan. 1, 2013, 4:28 p.m.)
Review request for Network Management.
Description
-------
This patch adds an option to the IPv6 configuration screen to change the IPv6 Privacy Extension configuration.
The option is implemented as documented at http://projects.gnome.org/NetworkManager/developers/api/09/ref-settings.html . However, that document says the default value is "-1", while it also says that only 0, 1 and 2 are valid for this option. Therefore, I chose 0 to be the default in the Knm::Ipv6Setting class, but the option is always put into the NM config map - even if it is zero.
This addresses bug 312305.
http://bugs.kde.org/show_bug.cgi?id=312305
Diffs
-----
backends/NetworkManager/settings/ipv6dbus.cpp 8c846ab
libs/internals/settings/ipv6.h 2eadf69
libs/internals/settings/ipv6.cpp 590274b
libs/ui/ipv6.ui 56a1248
libs/ui/ipv6widget.cpp 36b6885
Diff: http://git.reviewboard.kde.org/r/108064/diff/
Testing
-------
I verified that whatever I configure in the applet ends up in the /etc/NetworkManager/system-connections files.
I could not yet test whether the extensions are actually properly enabled and disabled because the network I am currently in does not use IPv6. However, next week I will be at university again where I should be able to test this.
Thanks,
Ralf Jung

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20130102/9e352a2f/attachment.html>

Lamarque Vieira Souza

2013-01-02 23:20:10 UTC

Permalink

libs/internals/settings/ipv6.cpp, line 14
<http://git.reviewboard.kde.org/r/108064/diff/1/?file=103610#file103610line14>
I think you should create a enum like there Ipv6Setting::EnumMethod instead of using hardcoded integer values. That works like a documentation about what are the possible values for mPrivacy.

Four-state, we can use "Unkwon" internally.

- Lamarque Vieira

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108064/#review24518
-----------------------------------------------------------

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20130102/08f266c8/attachment.html>

Lamarque Vieira Souza

2013-01-03 13:32:11 UTC

Permalink

I prefer to use Unknown instead of Undefined like in NetworkManager's source code to make it easier to search both source codes. In line 466 of the second link you use the -1 value directly instead of the enumerate. There are some set* methods where the parameters can be const, like in setMethod, setIgnoreAutoRoutes, setIgnoreAutoDns, setNeverDefault, setMayFail and setPrivacy. There are some mismatches in code style (for instance in some places you use "if () {" and in some other places "if () <line break> {". The former is the correct code style. In some places you use "while(){", there should be more white spaces in there, like "while () {". You should follow code style similar to [1]. You can use the astyle command line described in [1] to do most of the changes for you, just keep in mind that astyle breaks the canonical form of the SIGNAL() and SLOT() macros, you should use normalize [2] after using astyle to fix that. The rest of the patches are ok, good work.

[1] http://techbase.kde.org/Policies/Kdelibs_Coding_Style
[2] git://gitorious.org/qt/qtrepotools/util/normalize

- Lamarque Vieira

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108064/#review24518
-----------------------------------------------------------

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20130103/df0f8834/attachment.html>

Lamarque Vieira Souza

2013-01-03 21:38:26 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108064/#review24614
-----------------------------------------------------------

libs/internals/settings/ipv6.h
<http://git.reviewboard.kde.org/r/108064/#comment18877>

code style, use "} else {" here instead of "} <line break> else {"

libs/ui/ipv6widget.cpp
<http://git.reviewboard.kde.org/r/108064/#comment18878>

privacy can be const here.

libs/ui/ipv6widget.cpp
<http://git.reviewboard.kde.org/r/108064/#comment18876>

code style, use "} else {" here instead of "} <line break> else {"

libs/ui/ipv6widget.cpp
<http://git.reviewboard.kde.org/r/108064/#comment18875>

you can reuse the privacy variable here instead of calling d->setting->privacy() again.

After those changes ship it.

- Lamarque Vieira Souza

Post by Lamarque Vieira Souza
-----------------------------------------------------------
http://git.reviewboard.kde.org/r/108064/
-----------------------------------------------------------
(Updated Jan. 3, 2013, 6:17 p.m.)
Review request for Network Management.
Description
-------
This patch adds an option to the IPv6 configuration screen to change the IPv6 Privacy Extension configuration.
The option is implemented as documented at http://projects.gnome.org/NetworkManager/developers/api/09/ref-settings.html . However, that document says the default value is "-1", while it also says that only 0, 1 and 2 are valid for this option. Therefore, I chose 0 to be the default in the Knm::Ipv6Setting class, but the option is always put into the NM config map - even if it is zero.
This addresses bug 312305.
http://bugs.kde.org/show_bug.cgi?id=312305
Diffs
-----
backends/NetworkManager/settings/ipv6dbus.cpp 8c846ab
libs/internals/settings/ipv6.h 2eadf69
libs/internals/settings/ipv6.cpp 590274b
libs/ui/ipv6.ui 56a1248
libs/ui/ipv6widget.cpp 36b6885
Diff: http://git.reviewboard.kde.org/r/108064/diff/
Testing
-------
I verified that whatever I configure in the applet ends up in the /etc/NetworkManager/system-connections files.
I could not yet test whether the extensions are actually properly enabled and disabled because the network I am currently in does not use IPv6. However, next week I will be at university again where I should be able to test this.
Thanks,
Ralf Jung

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20130103/8a37e443/attachment-0001.html>

Ralf Jung

2013-01-01 16:28:14 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108064/
-----------------------------------------------------------

Review request for Network Management.

Description
-------

This patch adds an option to the IPv6 configuration screen to change the IPv6 Privacy Extension configuration.

The option is implemented as documented at http://projects.gnome.org/NetworkManager/developers/api/09/ref-settings.html . However, that document says the default value is "-1", while it also says that only 0, 1 and 2 are valid for this option. Therefore, I chose 0 to be the default in the Knm::Ipv6Setting class, but the option is always put into the NM config map - even if it is zero.

This addresses bug 312305.
http://bugs.kde.org/show_bug.cgi?id=312305

Diffs
-----

backends/NetworkManager/settings/ipv6dbus.cpp 8c846ab
libs/internals/settings/ipv6.h 2eadf69
libs/internals/settings/ipv6.cpp 590274b
libs/ui/ipv6.ui 56a1248
libs/ui/ipv6widget.cpp 36b6885

Diff: http://git.reviewboard.kde.org/r/108064/diff/

Testing
-------

I verified that whatever I configure in the applet ends up in the /etc/NetworkManager/system-connections files.

I could not yet test whether the extensions are actually properly enabled and disabled because the network I am currently in does not use IPv6. However, next week I will be at university again where I should be able to test this.

Thanks,

Ralf Jung

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20130101/06f578fc/attachment-0001.html>

Ralf Jung

2013-01-01 16:47:03 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108064/#review24367
-----------------------------------------------------------

I just noticed (from looking at the NetworkManager source code) that -1 actually is a valid value for the "ip6-privacy" option. It means "unknown". However, if I understand the code applying that configuration properly, "unknown" and "disabled" are treated exactly the same. Please let me know how I should deal with that in the GUI.

- Ralf Jung

Post by Lamarque Vieira Souza
-----------------------------------------------------------
http://git.reviewboard.kde.org/r/108064/
-----------------------------------------------------------
(Updated Jan. 1, 2013, 4:28 p.m.)
Review request for Network Management.
Description
-------
This patch adds an option to the IPv6 configuration screen to change the IPv6 Privacy Extension configuration.
The option is implemented as documented at http://projects.gnome.org/NetworkManager/developers/api/09/ref-settings.html . However, that document says the default value is "-1", while it also says that only 0, 1 and 2 are valid for this option. Therefore, I chose 0 to be the default in the Knm::Ipv6Setting class, but the option is always put into the NM config map - even if it is zero.
This addresses bug 312305.
http://bugs.kde.org/show_bug.cgi?id=312305
Diffs
-----
backends/NetworkManager/settings/ipv6dbus.cpp 8c846ab
libs/internals/settings/ipv6.h 2eadf69
libs/internals/settings/ipv6.cpp 590274b
libs/ui/ipv6.ui 56a1248
libs/ui/ipv6widget.cpp 36b6885
Diff: http://git.reviewboard.kde.org/r/108064/diff/
Testing
-------
I verified that whatever I configure in the applet ends up in the /etc/NetworkManager/system-connections files.
I could not yet test whether the extensions are actually properly enabled and disabled because the network I am currently in does not use IPv6. However, next week I will be at university again where I should be able to test this.
Thanks,
Ralf Jung

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20130101/63da6585/attachment-0001.html>

Lamarque Vieira Souza

2013-01-04 10:54:47 UTC

Permalink

Post by Lamarque Vieira Souza

Post by Ralf Jung
I just noticed (from looking at the NetworkManager source code) that -1 actually is a valid value for the "ip6-privacy" option. It means "unknown". However, if I understand the code applying that configuration properly, "unknown" and "disabled" are treated exactly the same. Please let me know how I should deal with that in the GUI.

How does nm-applet deal with that? I think calling "disabled" is better than "unknown" if they really behave the same.

- Lamarque Vieira

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108064/#review24367
-----------------------------------------------------------

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20130104/f7440d34/attachment-0001.html>

Ralf Jung

2013-01-06 22:35:30 UTC

Permalink

Post by Lamarque Vieira Souza

How does nm-applet deal with that? I think calling "disabled" is better than "unknown" if they really behave the same.

They call 0 "Disabled" and do not expose -1. If the config says -1, they show it as "Disabled". This is not yet committed though, I think [2].
(Sorry for not yet pushing this change, I was busy with university and will push it tomorrow)

[2] https://bugzilla.gnome.org/show_bug.cgi?id=633233

- Ralf

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108064/#review24367
-----------------------------------------------------------

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20130106/b3c95277/attachment.html>

Ralf Jung

2013-01-03 18:17:46 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108064/
-----------------------------------------------------------

(Updated Jan. 3, 2013, 6:17 p.m.)

Review request for Network Management.

Changes
-------

* Added enum for privacy setting
* Added some braces

Description
-------

This patch adds an option to the IPv6 configuration screen to change the IPv6 Privacy Extension configuration.

The option is implemented as documented at http://projects.gnome.org/NetworkManager/developers/api/09/ref-settings.html . However, that document says the default value is "-1", while it also says that only 0, 1 and 2 are valid for this option. Therefore, I chose 0 to be the default in the Knm::Ipv6Setting class, but the option is always put into the NM config map - even if it is zero.

This addresses bug 312305.
http://bugs.kde.org/show_bug.cgi?id=312305

Diffs (updated)
-----

backends/NetworkManager/settings/ipv6dbus.cpp 8c846ab
libs/internals/settings/ipv6.h 2eadf69
libs/internals/settings/ipv6.cpp 590274b
libs/ui/ipv6.ui 56a1248
libs/ui/ipv6widget.cpp 36b6885

Diff: http://git.reviewboard.kde.org/r/108064/diff/

Testing
-------

I verified that whatever I configure in the applet ends up in the /etc/NetworkManager/system-connections files.

I could not yet test whether the extensions are actually properly enabled and disabled because the network I am currently in does not use IPv6. However, next week I will be at university again where I should be able to test this.

Thanks,

Ralf Jung

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20130103/a042448c/attachment.html>

Commit Hook

2013-01-07 11:11:40 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108064/#review24891
-----------------------------------------------------------

This review has been submitted with commit b1bed1ebdc3ded10370cc084bb01a28a7e3432ef by Ralf Jung to branch nm09.

- Commit Hook

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20130107/3560446a/attachment.html>

Commit Hook

2013-01-07 18:18:48 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108064/#review24935
-----------------------------------------------------------

This review has been submitted with commit 402ad6f360aec052c5f48dfe68f40a1fc0ae9f17 by Lamarque V. Souza on behalf of Ralf Jung to branch master.

- Commit Hook

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20130107/bf2a37c5/attachment.html>

Ralf Jung

2013-01-02 23:04:03 UTC

Permalink

Post by Lamarque Vieira Souza

Post by Lamarque Vieira Souza
-----------------------------------------------------------
http://git.reviewboard.kde.org/r/108064/
-----------------------------------------------------------
(Updated Jan. 1, 2013, 4:28 p.m.)
Review request for Network Management.
Description
-------
This patch adds an option to the IPv6 configuration screen to change the IPv6 Privacy Extension configuration.
The option is implemented as documented at http://projects.gnome.org/NetworkManager/developers/api/09/ref-settings.html . However, that document says the default value is "-1", while it also says that only 0, 1 and 2 are valid for this option. Therefore, I chose 0 to be the default in the Knm::Ipv6Setting class, but the option is always put into the NM config map - even if it is zero.
This addresses bug 312305.
http://bugs.kde.org/show_bug.cgi?id=312305
Diffs
-----
backends/NetworkManager/settings/ipv6dbus.cpp 8c846ab
libs/internals/settings/ipv6.h 2eadf69
libs/internals/settings/ipv6.cpp 590274b
libs/ui/ipv6.ui 56a1248
libs/ui/ipv6widget.cpp 36b6885
Diff: http://git.reviewboard.kde.org/r/108064/diff/
Testing
-------
I verified that whatever I configure in the applet ends up in the /etc/NetworkManager/system-connections files.
I could not yet test whether the extensions are actually properly enabled and disabled because the network I am currently in does not use IPv6. However, next week I will be at university again where I should be able to test this.
Thanks,
Ralf Jung

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20130102/cca839fd/attachment-0001.html>

Jan Grulich

2013-01-03 13:06:55 UTC

Permalink

Take a look at my implementation of IPv6Privacy [1] [2]

[1] http://quickgit.kde.org/?p=libnm-qt.git&a=blob&h=0f7719078556e8654a009803c61cc1904e886990&hb=971ec1ff8dd66a144c79c2ac61e5d483e90b1630&f=settings%2Fipv6.h
[2] http://quickgit.kde.org/?p=libnm-qt.git&a=blob&h=b2568805fc907b605faa34ecb7d4822f393acfe1&hb=971ec1ff8dd66a144c79c2ac61e5d483e90b1630&f=settings%2Fipv6.cpp

- Jan

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108064/#review24518
-----------------------------------------------------------

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20130103/3b4fed9b/attachment-0001.html>

Jan Grulich

2013-01-03 13:58:20 UTC

Permalink

Okay, will do.
Would you prefer the enum to be three-state (like in the patch proposed for the Gnome applet [1]), or four-state including "Unknown", like in libnm-util? The behaviour of "Unknown" used to differ from "Disabled", but this was changed [2] and now I can not find any indication for a difference in behaviour in the sources. As I said above, the docs do not mention "Unknown" at all... but it is the default value.
[1] http://bugzilla-attachments.gnome.org/attachment.cgi?id=217377
[2] http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=af0eb9e7adf0732921ebe927408a41a36f56f72e
Four-state, we can use "Unkwon" internally.
Take a look at my implementation of IPv6Privacy [1] [2]
[1] http://quickgit.kde.org/?p=libnm-qt.git&a=blob&h=0f7719078556e8654a009803c61cc1904e886990&hb=971ec1ff8dd66a144c79c2ac61e5d483e90b1630&f=settings%2Fipv6.h
[2] http://quickgit.kde.org/?p=libnm-qt.git&a=blob&h=b2568805fc907b605faa34ecb7d4822f393acfe1&hb=971ec1ff8dd66a144c79c2ac61e5d483e90b1630&f=settings%2Fipv6.cpp
I prefer to use Unknown instead of Undefined like in NetworkManager's source code to make it easier to search both source codes. In line 466 of the second link you use the -1 value directly instead of the enumerate. There are some set* methods where the parameters can be const, like in setMethod, setIgnoreAutoRoutes, setIgnoreAutoDns, setNeverDefault, setMayFail and setPrivacy. There are some mismatches in code style (for instance in some places you use "if () {" and in some other places "if () <line break> {". The former is the correct code style. In some places you use "while(){", there should be more white spaces in there, like "while () {". You should follow code style similar to [1]. You can use the astyle command line described in [1] to do most of the changes for you, just keep in mind that astyle breaks the canonical form of the SIGNAL() and SLOT() macros, you should use normalize [2] after using astyle to fix that. The rest of the patches are ok, good work.
[1] http://techbase.kde.org/Policies/Kdelibs_Coding_Style
[2] git://gitorious.org/qt/qtrepotools/util/normalize

The problem with comparison to -1 was fixed after I send these links here. Some problems with coding style was due to fact that I copied this part from networkmanagement and forgot to fix that. Anyway, I just wanted to help to Ralf Jung and my work is not part of this review :). Thanks for the short review

- Jan

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108064/#review24518
-----------------------------------------------------------

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20130103/d410b0e5/attachment.html>