>
> 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@xxxxxxxxxx>
> > ---
> > 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);
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel