Code review (24.03.2011) (sha1=b2c939f04997 ):

Coordinator
Apr 13, 2011 at 1:28 PM

0. rdc_transport::Buffer -- давайте удалим этот тайпдеф на std::vector, и будем юзать собственно std::vector. Будет понятнее и вообще лучше.

1. Названия проектов абсолютно ни о чём не говорят. Проекты можно переиминовывать в студии по F2, если вы не знали.

2. В проекте Addin в большенстве файлов есть зловещие коментарии вида:

int ret = ioctlsocket(ioctlsocketIN.s_, ioctlsocketIN.cmd_, ioctlsocketIN.argp_);
// ??? ioctlsocketIn.s_ cannot be used!!!
// use like
// std::map gsockets;
// int ret = ioctlsocket(gsockets[ioctlsocketIN.s_], ioctlsocketIN.cmd_, ioctlsocketIN.argp_);


или

int ret = shutdown (shutdownIN.s_, shutdownIN.how_);
// ??? shutdownIN.s_

Это же что-то значит, да?

3. В том же проекте кучи закоментированого кода. Удалить.

4. Я так вижу, что вы предполагаете сделать ассинхронный транспорт. Вижу это малореальным для проекта, т.к. у ассинхронного транспорта возникнут ещё и неиллюзорные проблеммы с многопоточностью.
Сделайте синхронный SendToClient(out_buffer, &in_buffer), который будет блокировать до получения результатов с другой стороны, и будет складывать результат в in_buffer, а уже потом отдавать упарвление.

5. Если вы хотите подменять оригинальные функции на сервере на функции из проекта Addin, навроде accept_proxy, то обязательно надо их содержимое обернуть в try/catch, и в случае ошибки в транспорте(индикатором ошибки будет выбюрос эксепшена) будете возвращать какую-нибудь ни к чему не обязывающую ошибку, навроде юниксового E_AGAIN.

6. Так не делайте:

     if(acceptOUT.wsa_last_error_!=0)
          pWSASetLastError(acceptOUT.wsa_last_error_);
     else
     {
          memcpy(addr,acceptOUT.addr_,sizeof(sockaddr));
          *addrlen = *acceptOUT.addrlen_;
          //memcpy(addrlen,acceptOUT.addrlen_,sizeof(int));
     }
     
неужели две фигурные скобки это так долго?
     
     if(acceptOUT.wsa_last_error_!=0)
     {
          pWSASetLastError(acceptOUT.wsa_last_error_);
     }
     else
     {
          memcpy(addr,acceptOUT.addr_,sizeof(sockaddr));
          *addrlen = *acceptOUT.addrlen_;
          //memcpy(addrlen,acceptOUT.addrlen_,sizeof(int));
     }
     
7. в файле Addin.cpp: От чего этот костыльчик? Ловите std::exception&, врятли ошибётесь.
     catch(...)
     {
          // TODO:: add log
          assert(false);
          return false;
     }


8. Data_Transfer_Protocol -- Я так понял -- это тестовый проетк. Создайте для всех тестовых проектов какие-то фильтры верхнього уровня в студийном солюшене. Что бы я на них при ревью смотрел после боевого кода, а то запутывает немного.

9. InjectAPP\EnumWMI.h функция EnumProcesses_Wmi -- штоэта??? Функция длинной в 200 строк, которая смотрит на меня с экрана с явным гастрономическим интересом. Я испуган, правда. Разбейте её на что-то Сделайте с ней что-то.
Я так понимаю, что это какой-то тестовый код.

10. VirtualChannelTestApp\VirtualChannelTestApp.cpp:
Не надо так писать. Даже в тестовом примере.

     struct testReadHandler : public rdc_transport::IReadHandler
     {
          void OnRead(rdc_transport::Buffer* pBuffer) throw()
          {          
               try
               {
                    {
                         std::ostringstream iss;
                         iss << "Buffer(" << pBuffer->size() << "): ";
                         for (rdc_transport::Buffer::iterator i = pBuffer->begin();
                               i != pBuffer->end(); ++i)
                               iss << *i;
                         
                         LOG(plog::info) << iss.str().c_str();
                    }
                    
                    tmp_read_buffer.resize (pBuffer->size());
                    std::copy (pBuffer->begin(), pBuffer->end(), tmp_read_buffer.begin());
                    
                    LOG(plog::info) << "OnRead";
                    std::cout << "OnRead\n";
                    
                    ::SetEvent (read_event);
               }
               catch(std::exception& /*ex*/)
               {
                    assert(false);
               }
          }
          void OnReadFail(const char* what) throw()
          {
               LOG(plog::info) << "Server OnReadFail: " << what;
          }
     };

Так ведь горазд понятнее:

struct testReadHandler :
      public rdc_transport::IReadHandler
{
     void OnRead(rdc_transport::Buffer* pBuffer) throw();

     void OnReadFail(const char* what) throw();
};

void testReadHandler::OnRead(rdc_transport::Buffer* pBuffer) throw()
{          
     try
     {
          std::ostringstream iss;

          iss << "Buffer(" << pBuffer->size() << "): ";

          for( rdc_transport::Buffer::iterator i = pBuffer->begin();
                i != pBuffer->end();
                ++i)
          {
               iss << *i;
          }

                         
          LOG(plog::info) << iss.str();


          tmp_read_buffer.resize(pBuffer->size());

          std::copy (pBuffer->begin(), pBuffer->end(), tmp_read_buffer.begin());

          LOG(plog::info) << "OnRead";
          std::cout << "OnRead\n";

          ::SetEvent (read_event);
     }
     catch(std::exception& /*ex*/)
     {
          assert(false);
     }
}

void OnReadFail(const char* what) throw()
{
     LOG(plog::info) << "Server OnReadFail: " << what;
}
     
Сразу становится заметно, что вы:
1. юзаете глобальную переменную tmp_read_buffer (без синхронизации?) в каллбеке, судя по названию
2. игнорируете информацию выброшеную в эксепшене.
3. странно именуете std::ostringstream iss вместо ожидаемого out_ss. Не увлекайтесь сокращениями.
4. итератор принято сокращать до it а индекс -- до i.


Кстате, конструкцию

          for( rdc_transport::Buffer::iterator i = pBuffer->begin();
                i != pBuffer->end();
                ++i)
          {
               iss << *i;
          }

можно записать как:

std::ostream_iterator out_string_stream_it(iss, "");
std::copy(pBuffer->begin(), pBuffer->end(), out_string_stream_it);

И ещё. Не используйте спецификации ексепшенов навроде void foo() throw().
В случае если эксепшен оттуда всё-таки вылетит, у вас будет непредсказуемое поведение.

И ещё ещё. Вы же знаете, что в релизной версии assert-ы не будут рботать?

Я ещё гляну код вместе с вами. Судя по всему, у вас некоторое сейчас отставание от графика. Будем что-то с ним делать

Coordinator
Apr 14, 2011 at 10:01 AM
Edited Apr 14, 2011 at 10:02 AM

Учел все замечания, которые касаются виртуальных каналов, прокси и стабов, кроме 4-го пункта

4. Я так вижу, что вы предполагаете сделать ассинхронный транспорт. Вижу это малореальным для проекта, т.к. у ассинхронного транспорта возникнут ещё и неиллюзорные проблеммы с многопоточностью.
Сделайте синхронный SendToClient(out_buffer, &in_buffer), который будет блокировать до получения результатов с другой стороны, и будет складывать результат в in_buffer, а уже потом отдавать упарвление. 

Проект VirtualChannelTestApp тестовый, замечание

1. юзаете глобальную переменную tmp_read_buffer (без синхронизации?) в каллбеке, судя по названию

я учту в проекте WinsockProxyLib.

Coordinator
Apr 14, 2011 at 9:32 PM
Привет.

2011/4/14 voynytskyyp <notifications@codeplex.com>:
> From: voynytskyyp
>
> Учел все замечания, которые касаются виртуальных каналов, прокси и стабов,
> кроме 4-го пункта
>
> 4. Я так вижу, что вы предполагаете сделать ассинхронный транспорт. Вижу это
> малореальным для проекта, т.к. у ассинхронного транспорта возникнут ещё и
> неиллюзорные проблеммы с многопоточностью.
> Сделайте синхронный SendToClient(out_buffer, &in_buffer), который будет
> блокировать до получения результатов с другой стороны, и будет складывать
> результат в in_buffer, а уже потом отдавать упарвление.
>

Ээээ, а что не так с четвёртым пунктом-то? Мы же это обсудили в среду
вроде? Или я что-то не понимаю?
Coordinator
Apr 15, 2011 at 9:53 AM

Привет, Данил.

проблема в том, что как нам быть с функцией WSAAsyncSelect, если мы сделаем синхронный транспорт.

Она ведь посылает асинхронные сообщения.