macsec setting
ClosedPublic

Authored by pranavgade on Thu, Dec 6, 12:37 PM.

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.
pranavgade created this revision.Thu, Dec 6, 12:37 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptThu, Dec 6, 12:37 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
pranavgade requested review of this revision.Thu, Dec 6, 12:37 PM

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

pranavgade updated this revision to Diff 46958.Thu, Dec 6, 2:21 PM

rebased on master

jgrulich added inline comments.Thu, Dec 6, 2:53 PM
autotests/settings/macsecsettingtest.cpp
30

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

src/settings/macsecsetting.cpp
27

Same wrong NM version as in the test.

312

Flags should be inserted to the map all the time.

src/settings/macsecsetting.h
59

Can be turned into an enum.

71

Can be turned into an enum.

src/settings/setting.cpp
34

Again not true.

pranavgade added inline comments.Thu, Dec 6, 4:48 PM
autotests/settings/macsecsettingtest.cpp
30

I tried to get the version from here:

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

src/settings/macsecsetting.h
59

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

pranavgade updated this revision to Diff 46975.Thu, Dec 6, 5:13 PM
pranavgade marked 3 inline comments as done.
pranavgade added inline comments.
src/settings/macsecsetting.h
59

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

jgrulich added inline comments.Fri, Dec 7, 11:02 AM
autotests/settings/macsecsettingtest.cpp
30

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.

src/settings/macsecsetting.h
59

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.

pranavgade updated this revision to Diff 47021.Fri, Dec 7, 11:27 AM
pranavgade marked 8 inline comments as done.
This comment was removed by jgrulich.
jgrulich added inline comments.Fri, Dec 7, 12:50 PM
src/settings/macsecsetting.cpp
50

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

295

mode() > Macsec::Psk

311

validation() != Macsec::Strict

src/settings/macsecsetting.h
51

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.

71

const QString &mkaCak

74

const QString &mkaCkn

80

const QString &parent

pranavgade updated this revision to Diff 47027.Fri, Dec 7, 1:00 PM
pranavgade marked 7 inline comments as done.
jgrulich accepted this revision.Fri, Dec 7, 1:10 PM
This revision is now accepted and ready to land.Fri, Dec 7, 1:10 PM
This revision was automatically updated to reflect the committed changes.