lacp protocol patch. PLvision company

[GoogleCodeExporter]

Ostinato sources cloned 09.17.2014. 
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-lacp-plvision/

Original issue reported on code.google.com by developers@plvision.eu on 17 Sep 2014 at 4:07

[GoogleCodeExporter]

This is not a defect but unfortunately we don't see ability to define issue 
type.
This is issue of type "Review Request".

Original comment by developers@plvision.eu on 17 Sep 2014 at 4:11

[GoogleCodeExporter]

Original comment by pstav...@gmail.com on 18 Sep 2014 at 3:50

• Changed state: Accepted
• Added labels: Type-Review
• Removed labels: Type-Defect

[GoogleCodeExporter]

Please find my review comments on the LACP code. Some of the comments deal with 
coding conventions which are not explicitly listed anywhere as of now. Ideally 
this should have been available so that contributors can take care of this 
while they code rather than later - my apologies. I'll put up a wiki page on 
coding guidelines shortly.

General
 * Code doesn't compile with Qt4.3 (minimum Qt version supported)
   * QWidget::setInputMethod was added in Qt4.6 - you may use #ifdef QT_VERSION to protect such code so that the enhanced functionality is available to those who have 4.6 or higher, but make sure you degrade gracefully and don't break anything functional for older versions!
 * For all the new protocol specific files added, as per the project's contribution policy, you retain the copyright - so please replace "Copyright (C) <years> Srivats P." with your name; as a consequence, the "This module is developed by" line is not required.
 * Break all lines exceeding 80 columns into multiple lines
 * Please remove all the "See AbstractProtocol::XXX() for more info" comments that were present in the sample.cpp that you used as template for your code - these comments were intended for you as a developer using the template file and are no longer needed.

lacp.proto
 * why use 212 as the 'protocol number' for lacp instead of 209 - the next available number in the 2xx range?

lacp.h
 * LACP frame format - change comment to specify field width is in bytes not bits
 * enum lacpfield
   - why do you need to number intermediate enums?
   - rename the is_XXX enums to is_override_XXX to better reflect their usage

lacp.cpp
 * why do you need protocolIdType() to return protocolIdIp? Shouldn't it be protocolIdNone which the base class returns so you don't need to overload it
 * you don't need frameFieldCount() - remove it
 * fieldData()
    * There are no bit fields in LACP, so you can omit 'case FieldBitSize' for all fields
    * lacp_subtype/lacp_version: you can skip "LACP" from the FieldName
    * actor/partner/collector/terminator tlv_type/length: wouldn't it better to display the value in decimal instead of hex for FieldTextValue
    * actor/partner/collector/terminator reserved: wouldn't it better to return type of QByteArray for FieldValue (and retain QString for FieldTextValue)
 * protocolFrameSize()/isProtocolFrameValueVariable()/isProtocolFrameSizeVariable()/protocolFrameVariableCount(): remove these functions since the base class does the same thing

lacp.ui
 * please look at compacting the UI dialog for a smaller minimum size. You can juggle around with the layout, the label text and size of the line-edits.

lacpconfig.cpp
 * LacpConfigForm(): you are using the same validator allocated on the heap for multiple QLinEdits; I can't remember about the ownership of validators and who will destroy it - a quick look at the Qt manual is also not conclusive - please dig a little and make sure there's no memory leak or duplicate free caused by this usage
 *  loadWidget()/storeWidget(): please reformat the calls to fieldData() and setFieldData() as used in sampleconfig.cpp - makes it more readable and easier to look for potential problems

lacppdml.h
 * options_ seems to be unused - remove?

pdmlfileformat.cpp/pdmlreader.cpp
 * can you elaborate on why change in these files are needed? These files are used by all protocols, so one needs to be very careful in making any changes here as it might cause regression issues for other protocols

Original comment by pstav...@gmail.com on 2 Oct 2014 at 2:49

• Changed state: Started

[GoogleCodeExporter]

Question: Code doesn't compile with Qt4.3 (minimum Qt version supported)
Answer: not reproduced

Question: QWidget::setInputMethod was added in Qt4.6 - you may use #ifdef 
QT_VERSION to protect such code so that the enhanced functionality is available 
to those who have 4.6 or higher, but make sure you degrade gracefully and don't 
break anything functional for older versions!
Answer: We don't use setInputMethod function in my code. Could you please point 
us into the place.

Question:  * can you elaborate on why change in these files are needed? These 
files are used by all protocols, so one needs to be very careful in making any 
changes here as it might cause regression issues for other protocols
Answer: This was fix for open pcap/pdml bug. You can use this fix in common 
ostinato code for using new tshark version.

All other issues are fixed based on your comments. Please take a look last 
version of branch.

Original comment by developers@plvision.eu on 15 Oct 2014 at 1:51

[GoogleCodeExporter]

The new code didn't compile on Qt 4.3 - the main reason being that the .ui was 
created using a later version of Qt which had some new properties (e.g. 
inputMethodHint) that the older Qt does not support. I've removed those 
properties - in fact I've completely redesigned the layout so that there are no 
tabs and everything is nice and compact. There were other small changes also 
necessary to build on Qt 4.3.

Please apply the attached patch - changes4_3.patch and verify that the 
functionality isn't broken because of my ui redesign. 

There is one un-answered question from the previous list -
* Why does protocolIdType() return ProtocolIdEth? 

Another thing I'm not sure of is whether we would need to create a separate 
"slow protocols" protocol to demux LACP and other such slow protocols. Let's 
wait for the next slow protocol to be implemented and then rethink - for now, 
once the above is addressed, I should be able to commit the changes to the 
master repo

Original comment by pstav...@gmail.com on 23 Nov 2014 at 2:43

Attachments:

• changes4_3.patch

[GoogleCodeExporter]

Hello Srivats,

Finally we are ready with our commit.

Let's go step by step:

1. We checked your patch and it works perfect, but we still like our
example and we spend time to fix issue with QT4.3.
Now we fix the code and tested it - it should work on QT4.3 and QT4.8.

2. * Why does protocolIdType() return ProtocolIdEth?
 - Fixed

3. *"we would need to create a separate "slow protocols" protocol to demux
LACP and other such slow protocols."
As for us it is a good way to separate protocols by their "types", We hope
that someday Ostinato will support lot of protocols and potentially it
could be difficult to browse thru all of them without a tree structure.
Anyway, this is only your decision and if you really dislike such approach
and want to keep all protocols in a singe directory, we could fix it also
in next commit :)

Please check our new commit.

Regards,
Ihor Salamin, PLVISION LLC. <http://www.plvision.eu/>developers@plvision.eu

This message contains confidential information and trade secret
intended solely for the use of the named addressee. If you are not the
intended recipient, you should not read, use, disclose or reproduce
the content of this message. If you have received this message by
mistake, please notify the sender immediately. Any views or opinions
presented in this message are solely those of the author and do not
necessarily represent those of PLVISION LLC., unless otherwise stated
by the sender and duly authorized by the said companies.

Original comment by developers@plvision.eu on 17 Dec 2014 at 1:37

[GoogleCodeExporter]

Please take a look new changes in the 
https://developers%40plvision.eu@code.google.com/r/developers-lacp-plvision/

Original comment by developers@plvision.eu on 17 Dec 2014 at 1:38

[GoogleCodeExporter]

Ihor,

My apologies for the delay in reviewing the latest code.

Your latest code looks fine. Removal of multiple tabs (one per TLV) from the 
.ui and alignments are much better than what you had previously. However, when 
comparing what you have today with the .ui I had sent earlier - I find that 
your new version takes more horizontal and vertical space which leads to 
appearing of both scroll bars within the widget. The reason is the use of grid 
layouts, size of Label text and default size of LineEdits (too big for 1 or 2 
byte fields). Of course the streamconfig dialog box could be resized to remove 
the scroll bars, but I would like to keep it as small as possible. So, I would 
prefer that we go with my version of the .ui

Also, the protocolIdType() function is not yet fixed - you need to remove this 
function since LACP does not contain any protocolId field to demux payload. 
abstractprotocol.cpp comments for this function specify the following -

<snip>
The default implementation returns ProtocolIdNone. If a subclass has a
protocolId field it should return the appropriate value e.g. IP protocol
will return ProtocolIdIp, Ethernet will return ProtocolIdEth etc.
</snip>

Srivats

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