MDU某產品OMCI模塊代碼質量現狀分析

時間 2019-11-26

標籤 mdu 產品 omci 模塊代碼質量現狀分析简体版

原文原文鏈接

說明

本文參考MDU系列某產品OMCI模塊現有代碼，提取若干實例以說明目前的代碼質量，亦可做爲甄別不良代碼的參考。數據庫

本文旨在就事論事，而非否認前人(沒有前人的努力也難有後人的進步)。但願以史爲鑑，不破不立，最終產出高質量的代碼。編程

一質量現狀

不考慮業務實現，現有的OMCI模塊代碼質量不甚理想。不管是理解上手、修改擴展和測試排障，能夠用舉步維艱形容。尤爲是二層通道計算相關代碼，堪比令史前動物沒法自拔的「焦油坑」。安全

本節將不考慮流程設計，僅就函數粒度列舉目前存在的較爲突出的代碼質量問題。數據結構

1.1 巨型函數

經過Source Monitor度量代碼複雜度，以下圖所示：數據庫設計

可見，相比業界推薦的5如下，OMCI模塊代碼複雜度很是之高。ide

複雜度最高(157)的函數位於Omci_me_layer2_data_path.c二層通道計算文件，選取其中一段函數調用鏈分析：函數

→OMCI_Me_Layer2_Calculate_TCI_List(複雜度：67，行數：282)性能

→OMCI_Me_Layer2_Calculate_TCI_According_VlanRuleList(複雜度：28，行數：182)測試

該函數對擴展Vlan存在遞歸調用。this

→OMCI_Me_Layer2_Compare_TCI(複雜度：6，行數：38)

該函數位於三級while循環各級內，以下(原循環77行，爲突出重點已刪除次要細節)：

更具震撼性的是，OMCI_Me_Layer2_Compare_TCI調用下面三個巨型函數：

→→OMCI_Me_Layer2_Compare_EthType(複雜度：26，行數：90)

→→OMCI_Me_Layer2_Compare_Vid(複雜度：149，行數：450)

→→OMCI_Me_Layer2_Compare_Pri(複雜度：157，行數：468)

以上僅示出調用鏈上的關鍵函數，且調用鏈自身僅隸屬於通道計算的一環。評估其全局複雜度，這樣的代碼其實是不可維護的。

此外，當Compare處理耗時超過直接Set時，將嚴重違背設計本意。

事實上，諸如此類的巨型函數在模塊內隨處可見。單論函數行數，目前發現的最巨型的函數參見Sub_SNGetMeValueFromAdapter(複雜度：59，行數：538行)。

1.2 設計缺陷

代碼中存在較多設計欠佳之處，在此舉例若干。

 1 /**********************************************************************
 2 * 函數名稱：    Omci_List_Pop_Front
 3 * 功能描述：    從鏈表頭部彈出一個節點
 4 * 輸入參數：    OMCI_LIST *pList        鏈表指針
 5 * 輸出參數：    void *pOutData          鏈表節點的數據
 6 * 返 回 值：                   
 7 ***********************************************************************/
 8 INT32U Omci_List_Pop_Front(OMCI_LIST *pList, void *pNodeData)
 9 {
10     OMCI_LIST_NODE *pHeadNode = NULL;
11 
12     if (pList == NULL || pNodeData == NULL)
13     {
14         return OMCI_FUNC_RETURN_FAILURE;
15     }
16 
17     pHeadNode = pList->pHead;
18     if (NULL != pHeadNode)
19     {
20         if (NULL != pHeadNode->pNodeData)
21         {
22             memcpy(pNodeData, pHeadNode->pNodeData, pList->dwNodeDataSize);
23         }
24         else
25         {
26             return OMCI_FUNC_RETURN_FAILURE;
27         }
28     }
29     else
30     {
31         return OMCI_FUNC_RETURN_FAILURE;
32     }
33 
34     Omci_List_Remove(pList, pHeadNode);
35 
36     return OMCI_FUNC_RETURN_SUCCESS;
37 }

Omci_List_Pop_Front

Omci_List_Pop_Front函數和Omci_List_Push_Back函數成對出現，其代碼風格在OMCI模塊內屬於比較優秀的。此處僅分析其設計缺陷：

1）從函數名稱看其功能應爲彈出棧元素，從「功能描述」看應爲隊列行爲，從函數體看既不是棧也不是隊列；

2）函數兼具取值和刪除功能，但一般取值時不該刪除，刪除時無需取值。

上述問題直接致使Omci_List_Pop_Front函數接近於廢棄，無處可用。

其實咱們須要的不是Omci_List_Pop_Front函數，而是Locate或稱Query函數。基於後者，刪除鏈表節點時先Locate後Remove；而不是目前由調用者遍歷鏈表後調用Omci_List_Remove，從而將鏈表細節暴露在外。

後來在XGPON代碼裏新增了Omci_List_Query函數，定義以下：

 1 OMCI_LIST_NODE* Omci_List_Query(OMCI_LIST *pList, ...)
 2 {
 3     OMCI_LIST_NODE_KEY  aKeyGroup[MAX_LIST_NODE_KEYS_NUM];
 4     OMCI_LIST_NODE *pNode=NULL;
 5     INT8U *pData=NULL, *pKeyValue=NULL;
 6     INT8U ucKeyNum=0, i;
 7     INT32U iKeyOffset=0, iKeyLen=0;
 8     VA_LIST tArgList;
 9 
10     if(NULL==pList)
11         return NULL;
12     memset((INT8U*)aKeyGroup, 0, sizeof(OMCI_LIST_NODE_KEY)*MAX_LIST_NODE_KEYS_NUM);
13     VA_START(tArgList, pList);
14     while(TRUE)
15     {
16         pKeyValue=VA_ARG(tArgList, INT8U*);
17         if(LIST_END==pKeyValue)
18             break;
19         iKeyOffset=VA_ARG(tArgList, INT32U);
20         iKeyLen=VA_ARG(tArgList, INT32U);
21         if(0==iKeyLen)
22         {
23             VA_END(tArgList);
24             return NULL;
25         }
26         if(ucKeyNum>=MAX_LIST_NODE_KEYS_NUM)
27         {
28             VA_END(tArgList);
29             return NULL;
30         }
31         aKeyGroup[ucKeyNum].pKeyValue=pKeyValue;
32         aKeyGroup[ucKeyNum].iKeyOffset=iKeyOffset;
33         aKeyGroup[ucKeyNum++].iKeyLen=iKeyLen;
34     }
35     VA_END(tArgList);
36 
37     pNode=Omci_List_First(pList);
38     while(NULL!=pNode)
39     {
40         pData=(INT8U*)pNode->pNodeData;
41         for(i=0; i<ucKeyNum; i++)
42         {
43             if(0!=memcmp(&pData[aKeyGroup[i].iKeyOffset], aKeyGroup[i].pKeyValue, aKeyGroup[i].iKeyLen))
44                 break;
45         }
46         if(i>=ucKeyNum)
47         {
48             break;
49         }
50         pNode=pNode->pNext;
51     }
52     return pNode;
53 }

Omci_List_Query

使用示例以下：

1 OMCI_LIST_NODE* pGemPortNode = Omci_List_Query(g_pIgmpUpAction, &tIGMPVlanAction, 0, sizeof(OMCI_ADAPTER_MulticastPort_vlan_EX), LIST_END);
2 if(NULL != pGemPortNode)
3 {
4      Omci_List_Remove(g_pIgmpUpAction, pGemPortNode);
5 }

Omci_List_Query Usage

可見，該函數有效地隱藏了鏈表細節，但變參函數易用性和可讀性比較差(注意原函數沒有註釋)。對比下面的查找函數：

 1 /**********************************************************************
 2 * 函數名稱： OmciLocateListNode
 3 * 功能描述： 查找鏈表首個與pData知足函數fCompareNode斷定關係的結點
 4 * 輸入參數： T_OMCI_LIST* pList           :鏈表指針
 5 *            VOID* pData                  :待比較數據指針
 6 *            CompareNodeFunc fCompareNode :比較回調函數指針
 7 * 輸出參數： NA
 8 * 返 回 值： T_OMCI_LIST_NODE* 鏈表結點指針(未找到時返回NULL)
 9 ***********************************************************************/
10 T_OMCI_LIST_NODE* OmciLocateListNode(T_OMCI_LIST *pList, VOID *pData, CompareNodeFunc fCompareNode)
11 {
12     CHECK_TRIPLE_POINTER(pList, pData, fCompareNode, NULL);
13     CHECK_SINGLE_POINTER(pList->pHead, NULL);
14     CHECK_SINGLE_POINTER(pList->pHead->pNext, NULL);
15 
16     T_OMCI_LIST_NODE *pListNode = pList->pHead->pNext;
17     while(pListNode != pList->pHead)
18     {
19         if(0 == fCompareNode(pListNode->pNodeData, pData, pList->dwNodeDataSize))
20             return pListNode;
21 
22         pListNode = pListNode->pNext;
23     }
24 
25     return NULL;
26 }

View Code

兩個函數由不一樣做者分別實現，互不知情。但對比之下，易用性和可讀性可窺一斑。

1.3 編碼缺陷

舉例以下：

 1 INT8U ONUMACDRV_get_omci(ONUMAC_OMCI* vpOut , INT8U* vpIn)
 2 {
 3     //static INT16U wTci;
 4     INT8U * buffer ;
 5     INT32U  index, crc_result = 0, crc_trans = 0;
 6     char    omcilog[300] = {0};
 7 
 8     buffer = ( INT8U * ) OMCI_MALLOC ( OMCI_MESSAGE_LENGTH ) ;
 9     if ( NULL == buffer )
10     {
11         OmciPrint_error("[%s]OMCI OMCI_MALLOC buffer is NULL!\n\r", FUNCTION_NAME);
12         return OMCI_FUNC_RETURN_FAILURE;
13     }
14     memmove(buffer,vpIn,OMCI_MESSAGE_LENGTH);
15 
16     /*升級過程當中，若出現crc出錯時，是須要應答OLT的，故須要繼續取值*/
17     vpOut->omci_header.wtranscorrelationid =  (buffer [ 0 ]<<8 ) | (buffer [ 1 ]);
18     vpOut->omci_header.ucar = (buffer [ 2 ]& 0x40)>>6;
19     vpOut->omci_header.ucak = (buffer [ 2 ]& 0x20)>>5;
20     vpOut->omci_header.ucmsgtype =  buffer [ 2 ]& 0x1f;
21     vpOut->omci_header.ucdevid =  buffer [ 3 ];
22     vpOut->omci_header.wmeclass =  (buffer [ 4 ]<< 8) | (buffer [ 5 ]);
23     vpOut->omci_header.wmeid = (buffer [ 6 ]<< 8) | (buffer [ 7 ]);
24 
25     /*crc 校驗*/
26     crc_result = OMCI_crc32(buffer,OMCI_CRC_SIZE);
27     crc_trans =( buffer [ 44 ] << 24) |(buffer [ 45] << 16)|(buffer [ 46] << 8)|(buffer [ 47]);
28     if(crc_result != crc_trans)
29     {
30         OmciPrint_error("[%s]ZTE----OMCI CRC error!crc_trans = %ud,crc_result = %ud \n\r", FUNCTION_NAME, crc_trans,crc_result);
31         OMCI_FREE ( buffer ) ;
32         return OMCI_FUNC_RETURN_PARAMETER_ERROR;
33     }
34 
35     for(index = 0;index < 32; index++)
36     {
37         vpOut->auccontent[index] = buffer [index+8];
38     }
39 
40     if((0!=(dwPrintSwitch & 0x00000001))||(0!=(dwPrintSwitch & OMCI_PRINT_SWITCH_OMCIFRAME)))
41     {
42         memset(&omcilog[0],0,300);
43         if(vpOut->omci_header.ucmsgtype == OMCIMETYPE_MIB_UPLOAD_NEXT)
44             sprintf(&omcilog[strlen(omcilog)],"Recv MIB upload next Msg, TCI=%d, SequenceNum=%d",
45                     vpOut->omci_header.wtranscorrelationid, (vpOut->auccontent[0]<<8) | vpOut->auccontent[1]);
46         else
47             sprintf(&omcilog[strlen(omcilog)],"Recv omci Msg, MsgType=%d, MeClass=%d, MeId=0x%04x, TCI=%d",
48                     vpOut->omci_header.ucmsgtype,vpOut->omci_header.wmeclass,vpOut->omci_header.wmeid, vpOut->omci_header.wtranscorrelationid);
49         for(index = 0;index < OMCI_MESSAGE_LENGTH; index++)
50         {
51             if( 0 == (index%16))
52                 sprintf(&omcilog[strlen(omcilog)],"\n\r");
53             if( 8 == index)
54                 sprintf(&omcilog[strlen(omcilog)],"[");
55             if( 40 == index)
56                 sprintf(&omcilog[strlen(omcilog)],"]");
57             if( 24 == index)
58                 sprintf(&omcilog[strlen(omcilog)]," ");
59             sprintf(&omcilog[strlen(omcilog)],"%02x ",buffer[index]);
60         }
61         sprintf(&omcilog[strlen(omcilog)],"\n\r");
62         if(0!=(dwPrintSwitch & 0x00000001))
63         {
64             omci_printTime();
65             /* using this function just for it is compatible with old print switchs */
66             omcidebug_print("%s",omcilog);
67         }
68         else
69         {
70             OmciPrint_omciframe("%s", omcilog);
71         }
72     }
73 
74     OMCI_FREE ( buffer ) ;
75     return OMCI_FUNC_RETURN_SUCCESS;
76 }

ONUMACDRV_get_omci

ONUMACDRV_get_omci函數對接收到的OMCI幀進行解析和受控打印。此處不考慮單一職責的設計問題，僅分析其編碼缺陷：

1）OMCI幀定長爲32字節，沒有必要申請動態內存；

2）omcilog[strlen(omcilog)]致使大量毫無必要的字符串長度計算。

上述兩個問題極大地拖慢該函數執行速度，並直接致使某產品在打開OMCI幀打印時沒法註冊上線。ONUMACDRV_set_omci函數存在一樣問題。

關於strlen的冗餘計算，再舉相似一例：

 1 /***************************************************************
 2  * Function: Omci_IPv4_Int_to_String
 3  * Description: 將IP地址從INT32U型轉換成點分十進制的字符串
 4  * Input: 
 5  * Output: 
 6  * Returns: 點分十進制的字符串表示的IP地址
 7  ***************************************************************/
 8 INT8U *Omci_IPv4_Int_to_String(INT32U dwInt)
 9 {
10     INT8U ucByte,ucPos;
11     static INT8U azStr[16];
12     memset(azStr,0,16);
13 
14     ucPos = strlen(azStr);
15     sprintf(&azStr[ucPos],"%d.",(INT8U)((dwInt>>24)&0x000000FF));
16 
17     ucPos = strlen(azStr);
18     sprintf(&azStr[ucPos],"%d.",(INT8U)((dwInt>>16)&0x000000FF));
19 
20     ucPos = strlen(azStr);
21     sprintf(&azStr[ucPos],"%d.",(INT8U)((dwInt>>8)&0x000000FF));
22 
23     ucPos = strlen(azStr);
24     sprintf(&azStr[ucPos],"%d",(INT8U)(dwInt&0x000000FF));
25 
26     return azStr;
27 }

Omci_IPv4_Int_to_String

對比其另外一實現：

 1 /* IPv4 Format: 0x0a285b09 ----> "10.40.91.9" or "0a.28.5b.09" */
 2 /* **************************Usage**************************** */
 3 /* INT32S dwIp = 0x0a285b09; CHAR aIpAddr[32] = {0};           */
 4 /* HexIpv4ToStrIp(dwIp, aIpAddr, sizeof(aIpAddr));             */
 5 #define STR_IPV4_MAX_SIZE        (INT8U)sizeof("255.255.255.255")
 6 INT32S HexIpv4ToStrIp(INT32S dwIp, CHAR *pStrIp, INT16U wSize)
 7 { 
 8     if((NULL == pStrIp)    || (wSize < STR_IPV4_MAX_SIZE))
 9     {
10         return -1;
11     }
12     
13     sprintf(pStrIp, "%d.%d.%d.%d", (dwIp & 0xFF000000) >> 24, (dwIp & 0x00FF0000) >> 16,
14         (dwIp & 0x0000FF00) >> 8, dwIp & 0x000000FF);
15     /* Use "%02x.%02x.%02x.%02x" to form "0a.28.5b.09" */    
16 
17     return 0;
18 }

HexIpv4ToStrIp

1.4 重複造輪

重複造輪主要體如今對於相同或類似的功能，存在多種實現方式。

例如，經過Get-Next命令從表裏逐條獲取記錄時，應執行相同的操做。代碼裏實現以下：

函數索引表裏的函數實現(綠色)幾乎各不相同，代表未作好足夠的抽象。

另外一處明顯的重複出如今Sub_InsertPerformanceDb/Sub_InsertPerfInfoDb、Sub_DeletePerformanceDb/ Sub_DeletePerfInfoDb等，對於性能統計歷史庫和性能統計信息庫定義了幾乎相同的兩套操做函數。一樣，這裏體現了抽象和提取能力的欠缺。

固然，上述重複性相比SNdbplat.c文件裏各實體操做數據庫的諸多函數，簡直不值一提。

重複造輪還表如今OMCI函數狀態返回值多處定義，違反了DRY(Don’t Repeat Yourself)原則。列舉部分紅功返回值以下：

1 OMCI_RET_OK
2 OMCI_RET_SUCCESS
3 R_OA_OK
4 AGENT_OPER_SUCCESS
5 OMCI_LIST_OPER_SUCCESS
6 OMCI_FUNC_RETURN_SUCCESS
7 …………

Return Code

其實，將函數返回狀態定義爲統一的枚舉變量不是更好嗎？

1.5 引用混亂

引用混亂主要體如今頭文件嵌套或交叉引用，違反了編譯防火牆的原則。舉例以下：

#include "omci_common_function.h"

→#include "omci_me_layer2_vlan.h"

→#include "omcifunc.h"

→

以上僅示出引用鏈上的關鍵頭文件，這些多重引用的頭文件不只拖慢編譯和連接速度，並且經常引起莫名其妙的錯誤。例如，曾發現頭文件A中將OP_SET定義爲宏，頭文件B中將OP_SET定義爲枚舉值；而某源文件所必須包含的頭文件展轉包含了頭文件A和B，因而……

對於獨立功能的代碼(如鏈表操做等)，頭文件嵌套引用的作法還會削弱其獨立性(使用者可能不得不拷貝沒必要要的文件)。

固然，頭文件嵌套引用能夠「簡化」編碼，如使用者可能只需在.c源文件裏#include 「XXXCommon.h」。但這無異於飲鴆止渴，除了縱容使用者的懶惰和不求甚解外別無他用！

所以咱們約定：在頭文件內儘可能不引用其餘頭文件，而在源文件內引用所需的頭文件。

此外，不加區分地將以太網、組播、語音、升級等相關宏和數據結構揉爲一體，造就了1570行的巨型頭文件：OmciComm.h！

注意該頭文件的名稱後綴(Comm)，您彷佛已意識到這是個被廣泛包含的你們夥？

1.6 命名隨意

詞不達意的變量名自不待說，先就函數命名的隨意性舉例若干：

 1 void Sub_SNAddmulticastoperationsprofile(DB_ONUMAC_OMCI * in,SN_OPERATE_OUT * out)
 2 {
 3     WORD  wNum, i = 0;
 4     BYTE  ucResult = 0;
 5     BYTE * pRecord = DB_POINTER_NULL;
 6     SN_MULCAST_OPERATIONS_PROFILE   MulCastOp;
 7 
 8     SN_MULCAST_OPERATIONS_PROFILE* pMulCastOpRecord = DB_POINTER_NULL;
 9 
10     MIB_OMCI_INDEX  ont_data_index;
11 
12     OmciPrint_debug("[LHP]Enter Sub_SNAddmulticastoperationsprofile MeClass %u MeId %u \n\r",in->index.wMeClass,in->index.wMeID);
13     g_wMulticastOperationsProfileDbHandle = _GetDBHandle(MULTICASTOPERATIONSPROFILE_DB);
14 
15     //... ...
16 }

Sub_SNAddmulticastoperationsprofile

示例函數不管函數名仍是局部變量名(毫無用處的全局DB句柄)長度都有點過度。然而更過度的示例來自下面的函數：

1 void Sub_SNQuerymulticastmulticastsubscriberconfiginfoForMultopProfilePT(INT16U Meid,SN_MULTOPPRO_MULTSUBS_POINT_QUERY_OUT * out,int MaxCount)

Sub_SNQuerymulticastmulticastsubscriberconfiginfoForMultopProfilePT

該函數名足足有67個字符！代碼做者彷佛「刻意」將multicast重複兩遍，並在函數名末尾鬼使神差地使用了縮寫：MultopProfile也許意指multicastoperationsprofile，但PT做何解呢？該函數在完成其既定功能的同時，還順帶考驗讀者的耐心和智商。

看過「大腹便便」型的函數名後，再來賞析「霧裏看花」型的函數名：

1 INT32U OA_manage_configure_flows(void)
2 {
3     OA_manage_compare_flows();
4     OA_manage_delete_flows();
5     OA_manage_modify_flows();
6     OA_manage_add_flows();
7 
8     return R_OA_OK;
9 }

OA_manage_configure_flows

據遠古的傳說講，compare、delete、modify、add的操做對象彷佛不是同一條(或全部條)flow……

1.7 接口晦澀

對外提供的接口應有詳細的文檔說明和註釋，且文檔應隨接口代碼發佈(最好置於同一目錄)。接口函數應有詳盡的函數簽名，相同層次的函數應有相同的行爲模式。此外，接口在封裝內部實現的同時，應對外提供足夠的透明性。沒法探知內部狀態的接口，不管易用性或安全性都將大打折扣。

此處就R73內存數據庫接口加以說明：

1）從上研移植來的R73內存數據庫設計文檔已不可知。代碼中也未提供數據庫視圖、關鍵結構/參數說明。接口函數簽名信息量太少，直接將函數名或參數名拆分做爲註釋。

1 /*--------------------------------------------------------------
2 function name: _CreateDB
3 para: dbName     = db name
4 desc: create db
5 udate history:
6       XXX write code on 2002-03-04
7 --------------------------------------------------------------*/
8 WORD _CreateDB( BYTE *dbName )

_CreateDB

2）插入(_Insert)、刪除(_Delete)、修改(_Update)接口從邏輯而言屬於相同層次的函數，但其返回值模式並不相同，即：

_Insert成功時返回0，不然返回非0正數；
_Delete/_Update成功時返回非0正數，不然返回0或負數。

首先，成功時返回非0正數違反編程習慣，默認約定0應表示成功。其次，三個接口返回模式不一樣，會增長使用者出錯的可能性。

3）修改(_Update)接口內含大量校驗處理，且失敗時均返回-1。加之接口未提供錯誤打印(這本是良好的設計風格)，致使使用者在調用失敗時很難定位緣由，而不得不手工添加調試打印：

 1     if ((dbHandle<1)||(dbHandle>=mm_db_num)||(keyPtr==NULL)||(indexNo>INDEX_NUM-1))
 2     {
 3         GeneralErrorHandler();
 4         return -1;
 5     }
 6     if(SafetyCheck(WRITE_OP,dbHandle)) return -1;
 7     curRecNum=GetRecNum(dbHandle);
 8     if (curRecNum==0) 
 9     {
10         FreeLock(WRITE_OP,dbHandle);
11         return -1;
12     }