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

Repository
R282 NetworkManagerQt
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
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
32

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

198

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.

213

Check just if userPriority is < 8.

225

Same as for setPriorityFlowControl()

240

Same as for priorityFlowControl()

252

Same here.

267

Same here.

278

Here.

295

Here.

305

Also here.

321

Also here.

331

Also here.

348

And last one.

357

Remove comments.

394

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

428

Remove comments.

429

Shouldn't it save appFcoeMode() instead?

448

You can probably insert flags all the time.

465

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

src/settings/dcbsetting.h
50

You are missing Q_DECLARE_FLAGS()

84

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

96

Make second parameter as bool.

src/settings/dcbsetting_p.h
47

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
32

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

198

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

213

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

394

I get a type casting error

428

oops, i was debugging it

429

yeah, i uploaded some unfinished code

465

How?

src/settings/dcbsetting.h
50

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
32

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

54

Not this way.

198

It should simple by just:

Q_D(DcbSetting)

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

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

394

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

465

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
465

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

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

Return false.

302

Return false.

pranavgade added inline comments.Dec 10 2018, 10:51 AM
src/settings/dcbsetting.cpp
60

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

Yes please.

pranavgade added inline comments.Dec 10 2018, 10:58 AM
src/settings/dcbsetting.cpp
60

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

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

const UintList &list

95

const UintList &list

100

const UintList &list

105

const UintList &list

110

const UintList &list

115

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

Why? This type is defined in generictypes.h.

src/settings/dcbsetting.h
31

Again, this is defined in generictypes.h.

50

Yes, add Q_DECLARE_FLAGS.

src/settings/dcbsetting_p.h
28

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

Include not needed.

src/settings/dcbsetting.cpp
221

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

255

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

289

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

323

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

357

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

391

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

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

You don't print priorityBandwith, priororityGroupBandwidth and others.

src/settings/dcbsetting.h
44

Rename to DcbFlagType

125

Please also add Q_DECLARE_OPERATORS_FOR_FLAGS(DcbSetting::DcbFlags)

src/settings/setting.h
97

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.