Page 1 of 1

Check correctness of my code, please

PostPosted: 27. Jul 2013, 14:04
by LastGayInChechnya
I want to write DLL for taking screenshots of virtualized Windows. Host OS is Windows too. By the way, is it possible to replace TakeScreenShotToArray by TakeScreenShot for performance? However, performance is not a necessity. The beauty of the code also was not very attractive. The main requirement is hassle-free. Please look for errors very carefully. This code is assigned the top responsibility.

Code: Select all   Expand viewCollapse view
#include <wtypes.h>
#include "VirtualBox.h"
#include "VBApi.h"

// GLOBAL VARIABLES
IVirtualBoxClient *CLIENT = NULL;
IDisplay *DISPLAY = NULL;
SAFEARRAY *BUFFER = NULL;
int PREVIOUS_INDEX = 0;

// Find running virtual machine. It's name coded by index.
// Returns pointer to display interface, NULL if something goes wrong.
IDisplay* findVM (int index) {
   HRESULT rc;
   if (!CLIENT) {
      rc = CoCreateInstance(CLSID_VirtualBoxClient, NULL, CLSCTX_INPROC_SERVER,IID_IVirtualBoxClient, (void**)&CLIENT);
      if FAILED(rc) return NULL;
   }

   IVirtualBox* box;
   rc = CLIENT->get_VirtualBox(&box);
   if FAILED(rc) return NULL;

   ISession* session;
   rc = CLIENT->get_Session(&session);
   if FAILED(rc) return NULL;

   IMachine *machine = NULL;
   BSTR name;
   // The code breaks all the ideological doctrine of programming, but it dosn't matter.
   // If you have a idea, please implement it without explanations, because I'm ignoramus in C++ (the DLL will be used from the exotic scripting language).
   switch (index) {
      case 1: name = SysAllocString(L"Win2000"); break;
      case 2: name = SysAllocString(L"WinXP"); break;
      case 3: name = SysAllocString(L"WinVista"); break;
      case 4: name = SysAllocString(L"Win7"); break;
      default: return NULL;
   }
   rc = box->FindMachine(name, &machine);
   SysFreeString(name);
   if FAILED(rc) return NULL;
   
   MachineState state = MachineState_Null;
   rc = machine->get_State(&state);
   if (FAILED(rc) || (state != MachineState_Running)) return NULL;
   
   rc = machine->LockMachine(session, LockType_Shared);
   if FAILED(rc) return NULL;
   
   IConsole* console;
   rc = session->get_Console(&console);
   if FAILED(rc) return NULL;
   
   IDisplay *display;
   rc = console->get_Display(&display);
   if FAILED(rc) return NULL;
   
   rc = session->UnlockMachine();
   if FAILED(rc) return NULL;

   return display;
}

// Returns a pointer to the screenshot.
SAFEARRAY* __stdcall takeScreen(int index) {
   if (index <= 0) return NULL;

   if (index != PREVIOUS_INDEX) {
      DISPLAY = findVM(index);
      PREVIOUS_INDEX = 0; // if findVM fails, then it to be repeated at the next call of takeScreen.
      if (!DISPLAY) return NULL;
      PREVIOUS_INDEX = index;
   }
   if (BUFFER) {
      SafeArrayDestroy(BUFFER);
      BUFFER = NULL;
   }
   HRESULT rc = DISPLAY->TakeScreenShotToArray(0, 1024, 768, &BUFFER);
   if FAILED(rc) return NULL;

   return BUFFER;
}

BOOL WINAPI DllMain(HINSTANCE hinstDLL, DWORD fdwReason,LPVOID lpvReserved) {
   switch(fdwReason) {
   case DLL_PROCESS_ATTACH:
      CoInitialize(NULL);
      break;
   case DLL_PROCESS_DETACH:
      if (CLIENT) {
         CLIENT->Release();
         CLIENT = NULL;
      }
      if (BUFFER) {
      SafeArrayDestroy(BUFFER);
      BUFFER = NULL;
      }
   }
   return TRUE;
}

Re: Check correctness of my code, please

PostPosted: 27. Jul 2013, 14:52
by noteirak
Seems like you unlock the VM before getting the screenshot info, I am not sure that would work. I am not even sure you actually need a lock for the screenshot command.
TakeScreenShot has the best performance AFAIK.

Overall, the code seems right but I'm not a C/C++ expert, so I can't say much more.
Do you actually have issue with the code? or is it running fine?

Re: Check correctness of my code, please

PostPosted: 27. Jul 2013, 16:09
by LastGayInChechnya
noteirak wrote:Do you actually have issue with the code? or is it running fine?


Freelancer performed this work. He tested it, causing it works. I also tested it, but that does not reassure me. At first glance, all is well. Unfortunately, C++ is very easy to create a program with the unpredictable behavior. Perhaps some errors will occur only once a year (hypothetically). So I decided to simplify this program. However, the changes are insignificant and do not relate to the interaction with VirtualBox API. The main difference: previously, the program examined the list of machines and looking for one that works already and has the desired resolution; now it checks the defined machine and not worry about the resolution. In my situation, it is easier to set up VirtualBox a certain way (strict names of machines, only one resolution), than make a universal program. So I'm ready to new simplifications.

Re: Check correctness of my code, please

PostPosted: 27. Jul 2013, 16:16
by noteirak
It's simple enough that if it works now, and always has been since you modified it, there is no reason for it to break. I don't use the XPCOM C++ bind so I can't test it out myself.
the only change I would see is the TakeSnapshot method for better performance, but that is about it.

Re: Check correctness of my code, please

PostPosted: 27. Jul 2013, 17:28
by LastGayInChechnya
noteirak wrote:It's simple enough that if it works now, and always has been since you modified it, there is no reason for it to break.


Apparently, I'm a perfectionist and paranoid. %) But you calm me down a bit. :) Thanks. Maybe someone else will confirm your words, it would have strengthened my spirit.

the only change I would see is the TakeSnapshot method for better performance, but that is about it.


I asked the freelancer, why he did not do so, and he said it is impossible in Windows. In any case, the performance is not crucial. Screenshots will be made once a minute.

P.S.

noteirak wrote:Seems like you unlock the VM before getting the screenshot info, I am not sure that would work. I am not even sure you actually need a lock for the screenshot command.


Take a closer look! Locking is used for catching console and display, not for taking screenshot.

Re: Check correctness of my code, please

PostPosted: 27. Jul 2013, 18:41
by noteirak
LastGayInChechnya wrote:Locking is used for catching console and display, not for taking screenshot.

Indeed, I was just under the impression you had to keep that lock until you're done taking the screenshot. I guess not since your code works :)