[Spice-devel] [vdagent-win PATCH v3 08/10] Use destructor instead of cleanup function

[Jonathon Jongsma]

This works for how the code is currently implemented (where the VDAgent
object is destroyed immediately after run() returns). But what happens
if the code is changed so that we try to recover from a failure and
restart the agent by calling run() again on the same VDAgent object?
Since these variables are initialized within run() instead of within
the constructor, it seems better to free them upon exit from run() as
well.  Perhaps we could still take advantage of RAII to do the cleanup
with a helper class. Something like:

class VDAgentCleanup
{
public:
  VDAgentCleanup(VDagent&* agent) : vdagent(agent) {}
  ~VDAgentCleanup() { vdagent->cleanup(); }

private:
  VDAgent *vdagent;
};

...

bool VDAgent::run()
{
  VDAgentCleanup cleanup_on_return(this);
  ...
}


Or maybe there's no point trying to anticipate changes to how run() is
called since it will never happen? or maybe these variables could be
initialized in the constructor instead of the run() function?

Jonathon


On Fri, 2018-06-29 at 08:11 +0100, Frediano Ziglio wrote:
> More C++ style.
> Also avoids missing cleanup calls.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  vdagent/vdagent.cpp | 22 +++++-----------------
>  1 file changed, 5 insertions(+), 17 deletions(-)
> 
> diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
> index 5d9e7f0..b3e18d9 100644
> --- a/vdagent/vdagent.cpp
> +++ b/vdagent/vdagent.cpp
> @@ -115,7 +115,6 @@ private:
>      void load_display_setting();
>      bool send_announce_capabilities(bool request);
>      void cleanup_in_msg();
> -    void cleanup();
>      bool has_capability(unsigned int capability) const {
>          return VD_AGENT_HAS_CAPABILITY(_client_caps.begin(),
> _client_caps.size(),
>                                         capability);
> @@ -225,6 +224,11 @@ VDAgent::VDAgent()
>  
>  VDAgent::~VDAgent()
>  {
> +    FreeLibrary(_user_lib);
> +    CloseHandle(_stop_event);
> +    CloseHandle(_control_event);
> +    CloseHandle(_vio_serial);
> +    delete _desktop_layout;
>      delete _log;
>  }
>  
> @@ -282,14 +286,12 @@ bool VDAgent::run()
>              (PCLIPBOARD_OP)GetProcAddress(_user_lib,
> "RemoveClipboardFormatListener");
>          if (!_add_clipboard_listener || !_remove_clipboard_listener)
> {
>              vd_printf("GetProcAddress failed %lu", GetLastError());
> -            cleanup();
>              return false;
>          }
>      }
>      _control_event = CreateEvent(NULL, FALSE, FALSE, NULL);
>      if (!_control_event) {
>          vd_printf("CreateEvent() failed: %lu", GetLastError());
> -        cleanup();
>          return false;
>      }
>      _stop_event = OpenEvent(SYNCHRONIZE, FALSE,
> VD_AGENT_STOP_EVENT);
> @@ -298,7 +300,6 @@ bool VDAgent::run()
>      wcls.lpszClassName = VD_AGENT_WINCLASS_NAME;
>      if (!RegisterClass(&wcls)) {
>          vd_printf("RegisterClass() failed: %lu", GetLastError());
> -        cleanup();
>          return false;
>      }
>      _desktop_layout = new DesktopLayout();
> @@ -306,20 +307,17 @@ bool VDAgent::run()
>          vd_printf("No QXL devices!");
>      }
>      if (!init_vio_serial()) {
> -        cleanup();
>          return false;
>      }
>      if (!ReadFileEx(_vio_serial, _read_buf, sizeof(VDIChunk),
> &_read_overlapped, read_completion) &&
>              GetLastError() != ERROR_IO_PENDING) {
>          vd_printf("vio_serial read error %lu", GetLastError());
> -        cleanup();
>          return false;
>      }
>      _running = true;
>      event_thread = CreateThread(NULL, 0, event_thread_proc, this, 0,
> &event_thread_id);
>      if (!event_thread) {
>          vd_printf("CreateThread() failed: %lu", GetLastError());
> -        cleanup();
>          return false;
>      }
>      send_announce_capabilities(true);
> @@ -333,19 +331,9 @@ bool VDAgent::run()
>      }
>      vd_printf("Agent stopped");
>      CloseHandle(event_thread);
> -    cleanup();
>      return true;
>  }
>  
> -void VDAgent::cleanup()
> -{
> -    FreeLibrary(_user_lib);
> -    CloseHandle(_stop_event);
> -    CloseHandle(_control_event);
> -    CloseHandle(_vio_serial);
> -    delete _desktop_layout;
> -}
> -
>  void VDAgent::set_control_event(int control_command)
>  {
>      MutexLocker lock(_control_mutex);