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

[Frediano Ziglio]

> 
> 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
>

I think this was more to avoid exception considered like devil time ago.
I think a proper cleanup wanting to call run again would have to also
avoid dangling handles and pointer.
I think I'll use std::unique_ptr for the pointer and initialize the
handled in the constructor, sounds reasonable.
About the handle to the library I'll use GetModuleHandle... user32 is
always there...
 
> 
> 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);
>