D17381: macsec setting

2018-12-07 Thread Jan Grulich 
This revision was automatically updated to reflect the committed changes.
Closed by commit R282:b5a906ba5730: macsec setting (authored by pranavgade, 
committed by jgrulich).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D17381?vs=47027=47028#toc

REPOSITORY
  R282 NetworkManagerQt

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17381?vs=47027=47028

REVISION DETAIL
  https://phabricator.kde.org/D17381

AFFECTED FILES
  autotests/settings/CMakeLists.txt
  autotests/settings/macsecsettingtest.cpp
  autotests/settings/macsecsettingtest.h
  src/CMakeLists.txt
  src/settings/macsecsetting.cpp
  src/settings/macsecsetting.h
  src/settings/macsecsetting_p.h
  src/settings/setting.cpp
  src/settings/setting.h

To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17381: macsec setting

2018-12-07 Thread Jan Grulich 
jgrulich accepted this revision.
This revision is now accepted and ready to land.

REVISION DETAIL
  https://phabricator.kde.org/D17381

To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17381: macsec setting

2018-12-07 Thread Pranav Gade 
pranavgade updated this revision to Diff 47027.
pranavgade marked 7 inline comments as done.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17381?vs=47021=47027

REVISION DETAIL
  https://phabricator.kde.org/D17381

AFFECTED FILES
  autotests/settings/CMakeLists.txt
  autotests/settings/macsecsettingtest.cpp
  autotests/settings/macsecsettingtest.h
  src/CMakeLists.txt
  src/settings/macsecsetting.cpp
  src/settings/macsecsetting.h
  src/settings/macsecsetting_p.h
  src/settings/setting.cpp
  src/settings/setting.h

To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17381: macsec setting

2018-12-07 Thread Jan Grulich 
jgrulich added inline comments.

INLINE COMMENTS

> macsecsetting.cpp:49
> +, sendSci(true)
> +, validation(NetworkManager::MacsecSetting::Disable)
> +{ }

Isn't default validation 2, which means "strict" in your case.

> macsecsetting.cpp:294
> +
> +if (mode() > 0) {
> +setting.insert(QLatin1String(NM_SETTING_MACSEC_MODE), (int)mode());

mode() > Macsec::Psk

> macsecsetting.cpp:310
> +
> +if (validation() >= 0) {
> +setting.insert(QLatin1String(NM_SETTING_MACSEC_VALIDATION), 
> (int)validation());

validation() != Macsec::Strict

> macsecsetting.h:50
> +
> +enum Mode{
> +Psk = NM_SETTING_MACSEC_MODE_PSK,

In this case you don't need to define the defines above, just list both enums 
without assigned values, or you can just assign 0 to the first one to make sure 
it starts from 0.

> macsecsetting.h:70
> +
> +void setMkaCak(QString mkaCak);
> +QString mkaCak() const;

const QString 

> macsecsetting.h:73
> +
> +void setMkaCkn(QString mkaCkn);
> +QString mkaCkn() const;

const QString 

> macsecsetting.h:79
> +
> +void setParent(QString parent);
> +QString parent() const;

const QString 

REVISION DETAIL
  https://phabricator.kde.org/D17381

To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17381: macsec setting

2018-12-07 Thread Jan Grulich 
jgrulich added a comment.


  Please update full diff.

REVISION DETAIL
  https://phabricator.kde.org/D17381

To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17381: macsec setting

2018-12-07 Thread Pranav Gade 
pranavgade updated this revision to Diff 47021.
pranavgade marked 8 inline comments as done.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17381?vs=46975=47021

REVISION DETAIL
  https://phabricator.kde.org/D17381

AFFECTED FILES
  autotests/settings/CMakeLists.txt
  autotests/settings/macsecsettingtest.cpp
  autotests/settings/macsecsettingtest.h
  src/CMakeLists.txt
  src/settings/macsecsetting.cpp
  src/settings/macsecsetting.h
  src/settings/macsecsetting_p.h
  src/settings/setting.cpp
  src/settings/setting.h

To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17381: macsec setting

2018-12-07 Thread Jan Grulich 
jgrulich added inline comments.

INLINE COMMENTS

> pranavgade wrote in macsecsettingtest.cpp:29
> I tried to get the version from here: 
> F6459587: Screenshot from 2018-12-06 22-13-34.png 
> 
> 
> Is that incorrect?
> If so, from where do I check the version?

That is probably incorrect, the easiest way how to search for this is to clone 
NetworkManager and checkout to a branch you want to check and grep the define 
you are looking for.

So for example in my cloned NM, I go to "nm-1-6" branch and do "grep -ir 
NM_SETTING_MACSEC_MODE"  and see that this property is defined there. You 
should check all NM versions so if it's defined in NM 1.6, I test also whether 
it is in NM 1.4. I usually start checking from the middle, which means "nm-1-8" 
branch and go to newer or older based on the result.

> pranavgade wrote in macsecsetting.h:58
> From where can I get the possible values? Because I cannot find them here: 
> https://developer.gnome.org/NetworkManager/stable/settings-macsec.html

You can get it when you clone NetworkManager and go to the header file where 
these properties are defined. In your case it is in 
libnm-core/nm-setting-macsec.h.

REVISION DETAIL
  https://phabricator.kde.org/D17381

To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17381: macsec setting

2018-12-06 Thread Pranav Gade 
pranavgade added inline comments.

INLINE COMMENTS

> pranavgade wrote in macsecsetting.h:58
> From where can I get the possible values? Because I cannot find them here: 
> https://developer.gnome.org/NetworkManager/stable/settings-macsec.html

(I mean the default values to use in the defines)

REVISION DETAIL
  https://phabricator.kde.org/D17381

To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17381: macsec setting

2018-12-06 Thread Pranav Gade 
pranavgade updated this revision to Diff 46975.
pranavgade marked 3 inline comments as done.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17381?vs=46958=46975

REVISION DETAIL
  https://phabricator.kde.org/D17381

AFFECTED FILES
  autotests/settings/CMakeLists.txt
  autotests/settings/macsecsettingtest.cpp
  autotests/settings/macsecsettingtest.h
  src/CMakeLists.txt
  src/settings/macsecsetting.cpp
  src/settings/macsecsetting.h
  src/settings/macsecsetting_p.h
  src/settings/setting.cpp
  src/settings/setting.h

To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17381: macsec setting

2018-12-06 Thread Pranav Gade 
pranavgade added inline comments.

INLINE COMMENTS

> jgrulich wrote in macsecsettingtest.cpp:29
> This required version is not true, please verify it properly, I'm not going 
> to do it every time :).

I tried to get the version from here: 
F6459587: Screenshot from 2018-12-06 22-13-34.png 


Is that incorrect?
If so, from where do I check the version?

> jgrulich wrote in macsecsetting.h:58
> Can be turned into an enum.

From where can I get the possible values? Because I cannot find them here: 
https://developer.gnome.org/NetworkManager/stable/settings-macsec.html

REVISION DETAIL
  https://phabricator.kde.org/D17381

To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17381: macsec setting

2018-12-06 Thread Jan Grulich 
jgrulich added inline comments.

INLINE COMMENTS

> macsecsettingtest.cpp:29
> +
> +#if !NM_CHECK_VERSION(1, 12, 0)
> +#define NM_SETTING_MACSEC_PARENT   "parent"

This required version is not true, please verify it properly, I'm not going to 
do it every time :).

> macsecsetting.cpp:26
> +
> +#if !NM_CHECK_VERSION(1, 12, 0)
> +#define NM_SETTING_MACSEC_SETTING_NAME "macsec"

Same wrong NM version as in the test.

> macsecsetting.cpp:311
> +
> +if (mkaCakFlags()) {
> +setting.insert(QLatin1String(NM_SETTING_MACSEC_MKA_CAK_FLAGS), 
> (int)mkaCakFlags());

Flags should be inserted to the map all the time.

> macsecsetting.h:58
> +
> +void setMode(qint32 mode);
> +qint32 mode() const;

Can be turned into an enum.

> macsecsetting.h:70
> +
> +void setValidation(qint32 validation);
> +qint32 validation() const;

Can be turned into an enum.

> setting.cpp:34
>  
> +#if !NM_CHECK_VERSION(1, 12, 0)
> +#define NM_SETTING_MACSEC_SETTING_NAME "macsec"

Again not true.

REVISION DETAIL
  https://phabricator.kde.org/D17381

To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17381: macsec setting

2018-12-06 Thread Pranav Gade 
pranavgade updated this revision to Diff 46958.
pranavgade added a comment.


  rebased on master

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17381?vs=46953=46958

REVISION DETAIL
  https://phabricator.kde.org/D17381

AFFECTED FILES
  autotests/settings/CMakeLists.txt
  autotests/settings/macsecsettingtest.cpp
  autotests/settings/macsecsettingtest.h
  src/CMakeLists.txt
  src/settings/macsecsetting.cpp
  src/settings/macsecsetting.h
  src/settings/macsecsetting_p.h
  src/settings/setting.cpp
  src/settings/setting.h

To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17381: macsec setting

2018-12-06 Thread Jan Grulich 
jgrulich added a comment.


  Rebase this change on top of your previous change, this will not apply.

REPOSITORY
  R282 NetworkManagerQt

REVISION DETAIL
  https://phabricator.kde.org/D17381

To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17381: macsec setting

2018-12-06 Thread Pranav Gade 
pranavgade created this revision.
pranavgade added a reviewer: jgrulich.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
pranavgade requested review of this revision.

REVISION SUMMARY
  Added macsec setting according to:
  https://developer.gnome.org/NetworkManager/stable/settings-macsec.html

REPOSITORY
  R282 NetworkManagerQt

REVISION DETAIL
  https://phabricator.kde.org/D17381

AFFECTED FILES
  autotests/settings/CMakeLists.txt
  autotests/settings/macsecsettingtest.cpp
  autotests/settings/macsecsettingtest.h
  src/CMakeLists.txt
  src/settings/macsecsetting.cpp
  src/settings/macsecsetting.h
  src/settings/macsecsetting_p.h
  src/settings/setting.cpp
  src/settings/setting.h

To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns