Review Request: Add option to configure IPv6 Privacy Extension

[Jan Grulich]

> On Jan. 2, 2013, 10:11 p.m., Lamarque Vieira Souza wrote:
> > 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.
> 
> Ralf Jung wrote:
>     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
> 
> Lamarque Vieira Souza wrote:
>     Four-state, we can use "Unkwon" internally.
> 
> Jan Grulich wrote:
>     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
> 
> Lamarque Vieira Souza wrote:
>     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
-----------------------------------------------------------


On Jan. 1, 2013, 4:28 p.m., Ralf Jung wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> 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/20130103/d410b0e5/attachment.html>