D17425: dcb settings

[Jan Grulich]

jgrulich added inline comments.

INLINE COMMENTS

> dcbsetting.cpp:31
> +    , appFipPriority(-1)
> +    , appIscsiPriority(-1)
> +{ }

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

> dcbsetting.cpp:197
> +    if (userPriority < 8) {
> +        if (userPriority < d->priorityFlowControl.size()) {
> +            d->priorityFlowControl.replace(userPriority, enabled);

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.

> dcbsetting.cpp:212
> +
> +    if (userPriority < d->priorityFlowControl.size() && userPriority < 8) {
> +        return d->priorityFlowControl.value(userPriority);

Check just if userPriority is < 8.

> dcbsetting.cpp:224
> +    if (userPriority < 8) {
> +        if (userPriority < d->priorityBandwidth.size()) {
> +            d->priorityBandwidth.replace(userPriority, bandwidthPercent);

Same as for setPriorityFlowControl()

> dcbsetting.cpp:239
> +
> +    if (userPriority < d->priorityBandwidth.size() && userPriority < 8) {
> +        return d->priorityBandwidth.value(userPriority);

Same as for priorityFlowControl()

> dcbsetting.cpp:251
> +    if (groupId < 8) {
> +        if (groupId < d->priorityGroupBandwidth.size()) {
> +            d->priorityGroupBandwidth.replace(groupId, bandwidthPercent);

Same here.

> dcbsetting.cpp:266
> +
> +    if (groupId < d->priorityGroupBandwidth.size() && groupId < 8) {
> +        return d->priorityGroupBandwidth.value(groupId);

Same here.

> dcbsetting.cpp:277
> +
> +    if (userPriority < 8) {
> +        if (userPriority < d->priorityGroupId.size()) {

Here.

> dcbsetting.cpp:294
> +    if (userPriority < d->priorityGroupId.size() && userPriority < 8) {
> +        return d->priorityGroupId.value(userPriority);
> +    } else {

Here.

> dcbsetting.cpp:304
> +
> +    if (userPriority < 8) {
> +        if (userPriority < d->priorityStrictBandwidth.size()) {

Also here.

> dcbsetting.cpp:320
> +
> +    if (userPriority < d->priorityStrictBandwidth.size() && userPriority < 8) {
> +        return d->priorityStrictBandwidth.value(userPriority);

Also here.

> dcbsetting.cpp:330
> +    Q_D(DcbSetting);
> +
> +    if (userPriority < 8) {

Also here.

> dcbsetting.cpp:347
> +
> +    if (userPriority < d->priorityTrafficClass.size() && userPriority < 8) {
> +        return d->priorityTrafficClass.value(userPriority);

And last one.

> dcbsetting.cpp:356
> +{
> +    // if (setting.contains(QLatin1String(NM_SETTING_DCB_APP_FCOE_MODE))) {
> +        setAppFcoeMode(setting.value(QLatin1String(NM_SETTING_DCB_APP_FCOE_MODE)).toString());

Remove comments.

> dcbsetting.cpp:393
> +    if (setting.contains(QLatin1String(NM_SETTING_DCB_PRIORITY_FLOW_CONTROL))) {
> +        QList<quint32> list = qdbus_cast<QList<quint32>>(setting.value(QLatin1String(NM_SETTING_DCB_PRIORITY_FLOW_CONTROL)));
> +        for (int i = 0; i < list.size(); ++i) setPriorityFlowControl(i, list.at(i));

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

> dcbsetting.cpp:427
> +
> +    // if (!appFcoeMode().isEmpty()) {
> +        setting.insert(QLatin1String(NM_SETTING_DCB_APP_FCOE_MODE), "vn2vn");

Remove comments.

> dcbsetting.cpp:428
> +    // if (!appFcoeMode().isEmpty()) {
> +        setting.insert(QLatin1String(NM_SETTING_DCB_APP_FCOE_MODE), "vn2vn");
> +    // }

Shouldn't it save appFcoeMode() instead?

> dcbsetting.cpp:447
> +
> +    if ((int)appFipFlags() >= 0) {
> +        setting.insert(QLatin1String(NM_SETTING_DCB_APP_FIP_FLAGS), (int)appFipFlags());

You can probably insert flags all the time.

> dcbsetting.cpp:464
> +    if (priorityFlowControl(7) > 0) {
> +        QList<quint32> list;
> +        for (int i = 0; i < 8; ++i) list.append(priorityFlowControl(i));

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

> dcbsetting.h:49
> +    };
> +
> +    DcbSetting();

You are missing Q_DECLARE_FLAGS()

> dcbsetting.h:83
> +
> +    void setPriorityFlowControl(quint32 userPriority, quint32 enabled);
> +    quint32 priorityFlowControl(quint32 userPriority) const;

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

> dcbsetting.h:95
> +
> +    void setPriorityStrictBandwidth(quint32 userPriority, quint32 strict);
> +    quint32 priorityStrictBandwidth(quint32 userPriority) const;

Make second parameter as bool.

> dcbsetting_p.h:46
> +    NetworkManager::DcbSetting::DcbFlags priorityFlowControlFlags;
> +    QList<quint32> priorityFlowControl;
> +    QList<quint32> priorityBandwidth;

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

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

To: pranavgade, jgrulich
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20181210/f5aa24ac/attachment-0001.html>