Code review request: STP protocol patch. PLvision company

[GoogleCodeExporter]

Ostinato sources cloned 01.28.2015. 

Linux 3.11.0-26-generic #45~precise1-Ubuntu SMP Tue Jul 15 04:02:35 UTC 2014 
x86_64 x86_64 x86_64 GNU/Linux

Code review request:

https://developers%40plvision.eu@code.google.com/r/developers-stp-plvision/ 

Original issue reported on code.google.com by developers@plvision.eu on 28 Jan 2015 at 3:06

[GoogleCodeExporter]

Original comment by pstav...@gmail.com on 15 Feb 2015 at 1:46

• Changed state: Accepted

[GoogleCodeExporter]

Original comment by pstav...@gmail.com on 2 Mar 2015 at 3:37

• Changed state: Started

[GoogleCodeExporter]

* Generic
  - I assume RSTP/MSTP and other variants/extensions of STP are not supported with current code; any plans for future support for them? At the very least, you should make sure that your code is extendable in the future for the same.
  - For new files the Copyright line should include only the current year 2014, not 2010
  - The copyright is by Marchuk S., but you also have the line "This module is developed by PLVision <developers@plvision.eu>" - please use one name only (either Marchuk S. or developers@plvision.eu)
  - I'm not very familiar with the STP protocol and hence as such I haven't reviewed the code for correctness from a protocol spec perspective; I tried reading the spec but it takes too much time, delays the code review feedback and is not scalable for me to read every spec for every protocol submitted - so I'm just going to assume that you have done things according to spec
  - Once your code is in the main repository, I hope you would be available to debug and fix any bugs reported in your code 
  - Coding Guidelines - variable names are not camelCase (see http://code.google.com/p/ostinato/wiki/CodingGuidelines#Naming_Conventions) especially in the new files stp.cpp, stpconfig.cpp
* ostproto.pro
  - OK
* ostprotogui.pro
  - OK
* pdmlreader.cpp
  - please maintain alphabetic order for
    - #include "stppdml.h"
    - factory_.insert()
* protocol.proto
  - OK
* protocolmanager.cpp
  - OK
* protocolwidgetfactory.cpp
  - OK
* stp.cpp
  - Delete protocolIdType() function - it is not required
  - Delete frameFieldCount() function - it is not doing anything extra over the base class
  - fieldData()
    - Shorten FieldName string of the following fields to 'XXX_id' instead of 'XXX_identifier'
        - stp_protocol_identifier
        - stp_version_identifier
        - stp_root_identifier
        - stp_bridge_identifier
        - stp_port_identifier
    - Why does 'FieldValue' attrib for the following fields return string instead of integer? Typically this should always return an integer unless there is some special case.
        - stp_protocol_identifier
        - stp_bpdu_type
        - stp_root_identifier 
        - stp_bridge_identifier 
        - stp_port_identifier 
    - Should the FieldName for stp_flags be just "Flags" instead of "BPDU Flags" ?
    - stp_root_identifier - it might be better to keep 'FieldName' as just 'Root Identifier' and in FieldTextValue identify the prio/mac like this - "Priority: XXX, mac: XXX"; also in FieldTextValue shouldn't the QString().arg() be %1 and %2 instead of %1 and %3?
    - stp_root_path_cost - fix indentation under case FieldFrameValue
    - stp_bridge_identifier  - FieldName/FieldTextValue similar comments to stp_root_identifer above
    - stp_message_age/stp_max_age/hello_time/forward_delay - for FieldTextValue please print the unit of the time - if the unit for these timers as per the spec if not a straightforward human understandable unit like ms or seconds and something like 1/10 of second etc., print the actual value and a converted value to ahuman understandable unit e.g. something like print ("%u (%u.%03u s)", actualValue, valueInSeconds, remainingValueInMilliseconds) where milliseconds is 3 digit wide (0 - 999) and zero padded.
  - setFieldData()
    - Once you fix the 'FieldValue' attrib for the fields identified above, you will need to change the code for those fields in this function also to remove the string conversion
    - Please add the if(isOk) check to all the fields where it is not checked
* stp.h
  - shorten stp_*_identifier to stp_*_id
* stp.proto
  - rename the following (to shorten them and be consistent with remaining fields)
    - protocol_identifier to protocol_id
    - protocol_version_identifeir to protocol_version_id
  - should we add is_overide_* corresponding to protocol_identifier?
* stp.ui
  - remove the horizontal spacers between the two columns of widgets
  - reset the layout margins to their default values
* stpconfig.cpp
  - reUint64 validates a uint32 or uint64? The name suggests uint64 but it is applied to root_path_cost which is a uint32
  - use #define or const for the flag bits instead of hard coding 0 and 7
  - storeWidget()
    - default value for param base in toUInt() is base 10, so you can skip passing BASE_DEC to it.
* stpconfig.h
  - remove the "private slots:" statement since the class doesn't have any
* stppdml.cpp
  - why do we need special 'unknown' handling for root_id and bridge_id? Can't you use stp.root.hw/stp.root.prio and stp.bridge.hw/stp.bridge.prio - is it because in Ostinato both fields are combined into a single field?
  - unknownFieldHandler() - pos is already passed in as a param, you don't to extract it again from attributes
* stppdml.h
  - please fix the copyright notice at the top of the file to be consistent with other files
  - I assume the commented params in unknownFieldHandler was done to avoid unused parameter warnings; in that case isn't it enough to comment it only in the .cpp and not in the .h?

When you commit any changes based on the above feedback, please do not manually 
merge any updates from the main repo - so that the diff only shows changes 
relevant to this feature only. If you want to update your repo to the latest 
from the main repo - just use hg pull -u.

Original comment by pstav...@gmail.com on 24 Mar 2015 at 2:30

• Changed state: Reviewed