Code review (24.03.2011):

Coordinator
Mar 24, 2011 at 12:58 PM
Edited Mar 24, 2011 at 1:06 PM

Reviewer: Danil Ishkov.

Комментариев много и они хорошие. Это хорошо. Но всёравно код получился несколько запутанным. Что происходит в некоторых местах, я например, не понял. Поэтому код надо будет немного отрефакторить и посмотреть ещё раз.

Есть несколько замечаний и предложений.

 

 


 

 

SocketRedirection:

- Зачем нам пустой проект?

 

 



WinsockRedirectService:

 

- Удалить неиспользуеммый ReadMe.txt

- У нас есть исходники уже готового, развивающегося лога. Городить свой не стоит.

- Не стоит использовать аббревиатуры. Это я про Svc.

- Не стоит использовать TCHAR в приложении, которое будет работать с сетью. Используйте только юникодные версии строк и функций.

extern LPTSTR gSvcName; // service name (WinsockRedirectService.cpp)

extern TCHAR szLogFile[]; // name of log file (LogEvents.cpp)

 

- Глобальные переменные -- это не круто. Локализуйте как-нибудь их внутри классов, которые будут содержать функции, с ними работающие.

 В WinsockRedirectService.cpp:

LPTSTR gSvcName = TEXT("WinsockRedirectService");// service name

SERVICE_STATUS          gSvcStatus; // current service status

SERVICE_STATUS_HANDLE   gSvcStatusHandle; // service status handle of registered

 //  function SvcCtrlHandler

HANDLE                  gSvcStopEvent  = NULL; // is needed to terminate the thread

 //  in which SvcMain is running

BOOL gSvcRunning = false; // service running status

CRITICAL_SECTION gLogCS;

 В LogEvents.cpp:

TCHAR szLogFile [MAX_PATH];// name of log file

 

- Для инидикации ошибки в C++ есть исключения. Возвращать BOOL не надо.

BOOL DefineLogFile ()

 

- Переоткрывать лог для каждой записи -- не круто.

VOID WriteLog ( __in TCHAR* pFile, __in LPTSTR pMsg)

{ // write error or other information into log file

 ::EnterCriticalSection (&gLogCS);

 try {

 SYSTEMTIME tm; ::GetLocalTime (&tm);

 FILE* fLogFile;

  _tfopen_s (&fLogFile, szLogFile, TEXT("a"));

 _ftprintf_s (fLogFile, TEXT("%02d/%02d/%04d, %02d:%02d:%02d\n    %s\n"), tm.wMonth, tm.wDay, tm.wYear, tm.wHour, tm.wMinute, tm.wSecond, pMsg);

 fclose (fLogFile);

  } catch(...) {}

 ::LeaveCriticalSection (&gLogCS);}

 

- Catch(...) прячет проблемму. Т.е. проблемма останется, но узнать о ней ты уже не сможешь.

- Решили использовать венгерскую натацию -- будьте последовательными: gSvcName говорит нам о том, что он глобальный а szLogFile -- нет. А лучше венгерскую нотацию не использовать вообще. Но это на ваш вкус, конечно.

- Лучше избавиться от синхронизации на слипах и перейти к синхронизации на примитивах синхронизации, вроде eventов

 Sleep(5000); // !!! wait until SvcStartControl() ends

 // TO_DO: main actions to perform

 while(gSvcRunning)

 {

 HandleLogEvent (TEXT("SvcExecutionThread running..."));

 Sleep(5000);

 }

 return 0;

 

- Для линковки с внешними либами лучше использовать настройки проекта а не

#pragma comment(lib, "advapi32.lib")

 

- Фигурные скобки код только украшают. Можно добавить их в ифах и в этом свитче:  

switch(dwCtrl)   

{      

 case SERVICE_CONTROL_STOP:   

 case SERVICE_CONTROL_SHUTDOWN:        

 SvcUpdateStatus(SERVICE_STOP_PENDING, NO_ERROR, 0);
         // Signal the service to stop.

  gSvcRunning = false;        

 SetEvent(gSvcStopEvent);        

 SvcUpdateStatus(gSvcStatus.dwCurrentState, NO_ERROR, 0);                 

 return;       

 default:         

 break;  

}

- Надо перенести #include <windows.h> #include <tchar.h> #include <strsafe.h> и #include <strsafe.h> в stdafx.h из цпп-шников. (Вообще это касается всех инклудов, внешних по отношению к проекту)

 



 

PackUnpackArgs:

-Стоит подумать, как сериализовать указатели равные  NULL если они встретятся в структуре. М.б. стоит сериализовать указатели в два приема: вначале записываем структуру в виде POD типа, потом прохоимся по указателям и заменяем их значения на смещения относительно начала буффера до данных, на которые должен ссылаться указатель. Затем по этому смещению записываем аналогичным образом блок данных, на которые ссылается указатель

-Вообще, надо будет ещё раз обсудить перспективу использования готовой бустовой синхронизации.

-Надо изменить тип буффера на std::vector<unsigned char> будет гораздо удобнее.

-C-style приведения типа. Надо как-то по-аккуратнее с ними. Лучше используйте приведения типов в стиле C++.

-Нигде нет проверок указателей на ноль (т.е. внутри структуры могут быть нулевые указатели, и тогда мы закрешимся). Мне кажется, эта проверка должна быть в функциях getarg и addarg

-Не надо передавать в функции void* в качестве указателя на буффер. Надо использовать unsigned char* для буффера в C-style

-Кроме того, надо подумать, как это всё будет распаковываться на x64 будучи запакованным на x32 и наоборот. Сейчас задача поддерживать x64 не стоит, но это только потому, что это тестовый проект. Думать об этом надо.

-Закомментированные printf надо убрать

-Где эта память высвобождается??? 

void* datatosend=new BYTE[100];

- Зачем так спрятано объявление указателя *p?

void* datatosend=new BYTE[100],*p;

Coordinator
Mar 24, 2011 at 1:05 PM

Ребята, почитайте о RAII - например тут http://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization

постарайтесь в будущем применять полученные знания и избегать "голых" ресурсов.

А в общем и целом - молодцы!