Only take into account processes that have our plugin loaded

installer/src/installer-lib/process.cpp

 #include <stdexcept>
 #include <functional>
 #include <wctype.h>
 // <thread> is C++11, but implemented in VS2012
 #include <thread>
 
 #include "process.h"
 
 //-------------------------------------------------------
 // Windows_Handle
@@ skipping 16 matching lines @@
   validate_handle() ;
 }
 
 void Windows_Handle::validate_handle()
 {
   if ( handle == INVALID_HANDLE_VALUE )
   {
     // TODO: This code really deserves use of a proper exception class that packages 
     // Windows API errors consistently.
     char tmp[256];
-        sprintf(tmp, "Invalid handle. Last error: %d", GetLastError());

[Wladimir Palant, 2014/03/28 07:30:17]

sprintf() is deprecated, please use sprintf_s() instead.

Also a nit: Please use spaces for indentation.

+    sprintf_s(tmp, "Invalid handle. Last error: %d", GetLastError());
     throw std::runtime_error( tmp ) ;
   }
 }
 
 //-------------------------------------------------------
 // process_by_name_CI
 //-------------------------------------------------------
 process_by_name_CI::process_by_name_CI( const wchar_t * name )
   : name( name ), length( wcslen( name ) )
 {}
 
 bool process_by_name_CI::operator()( const PROCESSENTRY32W & process )
 {
   return 0 == wcsncmpi( process.szExeFile, name, length ) ;
 }
 
 
-//-------------------------------------------------------
-// process_by_any_exe_with_any_module
-//-------------------------------------------------------

[Wladimir Palant, 2014/03/28 07:30:17]

I guess I am missing the point of having of this comment - what value does it
add?

 bool process_by_any_exe_with_any_module::operator()( const PROCESSENTRY32W & process )
 {
-  if ( processNames.find( process.szExeFile ) != processNames.end() )
-  {
-      if (moduleNames.empty())
-                  return true;
-
-          ModulesSnapshot ms( process.th32ProcessID );
-          MODULEENTRY32* me = ms.begin();
-          while ( me != ms.end() )
-          {
-                  if (moduleNames.find( me->szModule ) != moduleNames.end())
-                  {
-                          return true;
-                  }
-                  me = ms.next();
-          }

[Wladimir Palant, 2014/03/28 07:30:17]

Nit: Please use spaces for indentation.

+  if (processNames.find(process.szExeFile) != processNames.end())
+  {
+    if (moduleNames.empty())
+      return true;
+
+    ModulesSnapshot ms(process.th32ProcessID);
+    MODULEENTRY32W* me = ms.first();
+    while (me != NULL)
+    {
+      if (moduleNames.find(me->szModule) != moduleNames.end())
+      {
+        return true;
+      }
+      me = ms.next();
+    }
   }
   return false;
 }
 
 //-------------------------------------------------------
 // wcscmpi
 //-------------------------------------------------------
 int wcscmpi( const wchar_t * s1, const wchar_t * s2 )
 {
   // Note: Equality of character sequences is case-insensitive in all predicates below.
@@ skipping 74 matching lines @@
 
 //-------------------------------------------------------
 // Snapshot
 //-------------------------------------------------------
 Snapshot::Snapshot()
   : handle( ::CreateToolhelp32Snapshot( TH32CS_SNAPPROCESS, 0 ) )
 {
   process.dwSize = sizeof( PROCESSENTRY32W ) ;
 }
 
-PROCESSENTRY32W * Snapshot::begin()
-{
-  return ::Process32FirstW( handle, & process ) ? ( & process ) : 0 ;
+PROCESSENTRY32W * Snapshot::first()
+{
+  return ::Process32FirstW(handle, &process) ? (&process) : 0;
 }
 
 PROCESSENTRY32W * Snapshot::next()
 {
-  return ::Process32NextW( handle, & process ) ? ( & process ) : 0 ;
+  return ::Process32NextW(handle, &process) ? (&process) : 0;
 }
 
 void Snapshot::refresh()
 {
-  handle = ::CreateToolhelp32Snapshot( TH32CS_SNAPPROCESS, 0 ) ;
+  handle = ::CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
 }
 
 
 //-------------------------------------------------------
 // ModulesSnapshot
 //-------------------------------------------------------
 ModulesSnapshot::ModulesSnapshot(DWORD processId)
-  : handle( ::CreateToolhelp32Snapshot( TH32CS_SNAPMODULE | TH32CS_SNAPMODULE32, processId ) )

[Wladimir Palant, 2014/03/28 07:30:17]

Style nit: That's rather unusual style in this file - spaces after opening
parenthesis and before closing parenthesis, usually we don't do that in our
code. Please remove them in your changes, it needs to be fixed for the rest of
the file as well eventually.

-{
-  module.dwSize = sizeof( MODULEENTRY32 ) ;
-}
-
-MODULEENTRY32W * ModulesSnapshot::begin()
-{
-  return ::Module32FirstW( handle, & module ) ? ( & module ) : 0 ;
+  : handle(::CreateToolhelp32Snapshot(TH32CS_SNAPMODULE | TH32CS_SNAPMODULE32, processId))
+{
+  module.dwSize = sizeof(MODULEENTRY32);
+}
+
+MODULEENTRY32W * ModulesSnapshot::first()
+{
+  return ::Module32FirstW(handle, &module) ? (&module) : 0;
 }
 
 MODULEENTRY32W * ModulesSnapshot::next()
 {
-  return ::Module32NextW( handle, & module ) ? ( & module ) : 0 ;
+  return ::Module32NextW(handle, &module) ? (&module) : 0;
 }

[Wladimir Palant, 2014/03/28 07:30:17]

This fakes a C++ iterator interface but does so incompletely - the iterator
should be a reference, next() and end() have to be methods of the iterator. IMHO
in that case it is better to not even pretend being an iterator. These methods
can be called first() and next() and the caller can simply check whether it gets
NULL.

[Eric, 2014/03/28 12:06:00]

Yep. It really ought be a proper iterator class, but that can't be done. It's a
wrapper around a Windows system call with state implicitly embedded in a handle.
There's another one just like it the Snapshot class, which uses the same system
call to retrieve all processes in the system; this one retrieves all modules for
a process.
I would have done exactly that had it been possible. The very short explanation:
iterators must be copyable.
Using the iterator naming convention makes the loops like just like other ones
in C++. It's more indicative when reading the code what's going on, assuming one
is used to reading C++ code.

[Oleksandr, 2014/03/28 12:48:35]

I actually like the way it is implemented. The iterator in this case is "handle"
and begin(), next() and end() are all there and can be considered methods of the
"handle". I would like to leave it as is. Can you please give an example of why
this isn't a good approach?

On 2014/03/28 07:30:17, Wladimir Palant wrote:

[Wladimir Palant, 2014/03/28 13:55:24]

This is simply misleading, one looks at the loop and expects a real iterator.
The difference is only obvious at the second glance, this invites overlooking
issues or making mistakes when using this interface in own code.

[Oleksandr, 2014/03/28 14:28:16]

Fixed in Patchset 4
On 2014/03/28 13:55:24, Wladimir Palant wrote:

 
 
 //-------------------------------------------------------
 // send_message, send_endsession_messages
 //-------------------------------------------------------
 /**
  * Default process exit wait time (per message) 5000 ms
  *
  * 5 seconds is time that the system will wait before it considers a process non-responsive.
  */
@@ skipping 235 matching lines @@
       if ( ! is_running() )
       {
         return true ;
       }
     }
     // Assert is_running()
   }
   // No control path leaves the for-loop.
 } ;