Added DCB settings
ClosedPublic

Authored by pranavgade on Dec 8 2018, 10:18 AM.

Details

Summary

Added missing DCB(Data Center Bridging) settings according to :
https://developer.gnome.org/NetworkManager/stable/settings-dcb.html

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
There are a very large number of changes, so older changes are hidden. Show Older Changes
jgrulich added inline comments.Dec 10 2018, 7:33 AM
src/settings/dcbsetting.cpp
31

You can initialize also all the lists here, because they have default values as well.

197

If you initialize the list at the beginning, you will not need to have this check and also do this weird thing below. Simply have the first check and otherwise you can do just d->priorityFlowControl[userPriority] = enabled.

212

Check just if userPriority is < 8.

224

Same as for setPriorityFlowControl()

239

Same as for priorityFlowControl()

251

Same here.

266

Same here.

277

Here.

294

Here.

304

Also here.

320

Also here.

330

Also here.

347

And last one.

356

Remove comments.

393

You can directly assign whole UintList. Same for the rest of options below.

427

Remove comments.

428

Shouldn't it save appFcoeMode() instead?

447

You can probably insert flags all the time.

464

You can directly push the list there. Same for the lists below.

src/settings/dcbsetting.h
49

You are missing Q_DECLARE_FLAGS()

83

Here it would probably make more sense to accept second parameter as bool.

95

Make second parameter as bool.

src/settings/dcbsetting_p.h
46

You can use our "UintList" defined data type for these.

pranavgade updated this revision to Diff 47243.Dec 10 2018, 8:24 AM
pranavgade marked 7 inline comments as done.
pranavgade marked an inline comment as done.
pranavgade added inline comments.
src/settings/dcbsetting.cpp
31

how do i do that?
i am getting this error:
error: class ‘NetworkManager::DcbSettingPrivate’ does not have any field named ‘setPriorityFlowControl’

197

I think it is better to have this as a safety measure, and initialising so many values manually wold take 8*6=48 lines in the beginning of the file

212

that may lead to a malloc assertion error sometimes, and thus a crash which is confusing to understand

393

I get a type casting error

427

oops, i was debugging it

428

yeah, i uploaded some unfinished code

464

How?

src/settings/dcbsetting.h
49

I tried to do it in the same way as in ipv6 settings, so should i add Q_DECLARE_FLAGS() ?

jgrulich added inline comments.Dec 10 2018, 8:38 AM
src/settings/dcbsetting.cpp
54 ↗(On Diff #47139)

Not this way.

31

something like priorityFlowControl({0, 0, 0, 0, 0, 0, 0, 0, 0}) should work, or not? Same for othe lists.

197

It should simple by just:

Q_D(DcbSetting)

if (userPriority < 8) {
   d->priorityFlowControl[userPriority] = enabled;
}
212

Not if you initialize it at the beginning as it should be.

393

You can have Q_D(DcbSetting) at the beginning of fromMap() function and then instead of setFoo() have d->foo = list.

464

Oh, right, you don't have methods for that. In that case you can again use Q_D(const DcbSetting) and push there d->priorityFlowControl and others.

pranavgade updated this revision to Diff 47253.Dec 10 2018, 9:21 AM
pranavgade marked 15 inline comments as done.
pranavgade added inline comments.
src/settings/dcbsetting.cpp
464

I can use Q_D in fromMap, but i get this error in toMap:

error: invalid conversion from ‘const NetworkManager::DcbSettingPrivate*’ to ‘NetworkManager::DcbSettingPrivate*’ [-fpermissive] Q_D(DcbSetting);

so, changed frommap, but keeping this as is.

pranavgade marked 12 inline comments as done.Dec 10 2018, 9:34 AM
jgrulich added inline comments.Dec 10 2018, 10:42 AM
src/settings/dcbsetting.cpp
60 ↗(On Diff #47139)

This is odd, maybe we should introduce getters and setters for the whole list, this would simplify it and you can use it also in fromMap() and toMap()

222 ↗(On Diff #47139)

Return false.

302 ↗(On Diff #47139)

Return false.

pranavgade added inline comments.Dec 10 2018, 10:51 AM
src/settings/dcbsetting.cpp
60 ↗(On Diff #47139)

Yeah, that would help.
Do you think I should do that?

pranavgade marked 2 inline comments as done.
jgrulich added inline comments.Dec 10 2018, 10:56 AM
src/settings/dcbsetting.cpp
60 ↗(On Diff #47139)

Yes please.

pranavgade added inline comments.Dec 10 2018, 10:58 AM
src/settings/dcbsetting.cpp
60 ↗(On Diff #47139)

Okay, im on it
But on the other hand, it will add 12 functions unnecessarily

pranavgade marked 4 inline comments as done.
jgrulich added inline comments.Dec 10 2018, 11:41 AM
src/settings/dcbsetting.h
107 ↗(On Diff #47139)

Move them please among the rest of methods, so all related methods are next to each other. Also use please "const UintList &list" and do not name the method as setFooList(), they can be just setFoo().

pranavgade marked an inline comment as done.
jgrulich added inline comments.Dec 10 2018, 12:05 PM
src/settings/dcbsetting.h
90 ↗(On Diff #47139)

const UintList &list

95 ↗(On Diff #47139)

const UintList &list

100 ↗(On Diff #47139)

const UintList &list

105 ↗(On Diff #47139)

const UintList &list

110 ↗(On Diff #47139)

const UintList &list

115 ↗(On Diff #47139)

const UintList &list

pranavgade updated this revision to Diff 47270.Dec 10 2018, 1:21 PM
pranavgade marked 6 inline comments as done.

Sorry, I had forgot about that.

jgrulich added inline comments.Dec 10 2018, 2:20 PM
autotests/settings/dcbsettingtest.cpp
31 ↗(On Diff #47139)

Why? This type is defined in generictypes.h.

src/settings/dcbsetting.h
31 ↗(On Diff #47139)

Again, this is defined in generictypes.h.

49

Yes, add Q_DECLARE_FLAGS.

src/settings/dcbsetting_p.h
28 ↗(On Diff #47139)

Here as well.

cfeck set the repository for this revision to R282 NetworkManagerQt.Dec 10 2018, 2:50 PM
pranavgade updated this revision to Diff 47279.Dec 10 2018, 2:57 PM
pranavgade marked 4 inline comments as done.
jgrulich added inline comments.Dec 10 2018, 3:08 PM
autotests/settings/dcbsettingtest.cpp
29 ↗(On Diff #47139)

Include not needed.

src/settings/dcbsetting.cpp
221 ↗(On Diff #47139)

You can remove the else branch and have just return false.

255 ↗(On Diff #47139)

You can remove the else branch and have just return 0.

289 ↗(On Diff #47139)

You can remove the else branch and have just return 0.

323 ↗(On Diff #47139)

You can remove the else branch and have just return 0.

357 ↗(On Diff #47139)

You can remove the else branch and have just return false.

391 ↗(On Diff #47139)

You can remove the else branch and have just return 0.

This was last round, otherwise it's ready to go, just fix the remaining issues. Thanks.

pranavgade updated this revision to Diff 47282.Dec 10 2018, 3:15 PM
pranavgade marked an inline comment as done.
pranavgade marked 7 inline comments as done.
cfeck set the repository for this revision to R282 NetworkManagerQt.Dec 10 2018, 3:25 PM
cfeck added a subscriber: cfeck.

Please keep the Repository settings. Additionally, please format the title and description according to https://community.kde.org/Infrastructure/Phabricator#Formatting_your_patch

pranavgade retitled this revision from dcb settings to Added DCB settings.Dec 10 2018, 3:30 PM
pranavgade edited the summary of this revision. (Show Details)
jgrulich added inline comments.Dec 11 2018, 9:50 AM
src/settings/dcbsetting.cpp
492 ↗(On Diff #47139)

I think all those should be added all the time, checking whether the last one is higher can sometime avoid adding them to the map when one of previous ones is set to non-default value.

528 ↗(On Diff #47139)

You don't print priorityBandwith, priororityGroupBandwidth and others.

src/settings/dcbsetting.h
44 ↗(On Diff #47139)

Rename to DcbFlagType

125 ↗(On Diff #47139)

Please also add Q_DECLARE_OPERATORS_FOR_FLAGS(DcbSetting::DcbFlags)

src/settings/setting.h
97 ↗(On Diff #47139)

Remove trailing spaces. They are not visible here, but once I apply this patch, I see them in KDevelop.

pranavgade marked 7 inline comments as done.
jgrulich accepted this revision.Dec 11 2018, 10:28 AM
This revision is now accepted and ready to land.Dec 11 2018, 10:28 AM
This revision was automatically updated to reflect the committed changes.