[Spice-devel] [vdagent-win PATCH] log: avoid crash on failure in log_version

[Frediano Ziglio]

> 
> On Mon, Jul 24, 2017 at 07:19:45AM -0400, Frediano Ziglio wrote:
> > > 
> > > On Sun, Jul 23, 2017 at 10:37:05AM +0100, Frediano Ziglio wrote:
> > > > The "throw" statement alone are used to throw an already existing
> > > > exception however in log_version they were meant to raise an exception.
> > > > This would have caused a termination of the program.
> > > > 
> > > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > > ---
> > > >  common/vdlog.cpp | 17 +++++++++--------
> > > >  1 file changed, 9 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/common/vdlog.cpp b/common/vdlog.cpp
> > > > index c983127..0a4c816 100644
> > > > --- a/common/vdlog.cpp
> > > > +++ b/common/vdlog.cpp
> > > > @@ -79,33 +79,34 @@ void VDLog::printf(const char* format, ...)
> > > >  
> > > >  void log_version()
> > > >  {
> > > > -    DWORD handle;
> > > > -    TCHAR module_fname[MAX_PATH];
> > > > +    if (![]() -> bool {
> > > 
> > > I don't think a lambda is really useful/required here..
> > > 
> > 
> > Nothing is required. Is fixing a bug so the code is surely useful.
> > 
> > Counterproposal?
> 
> s/throw/throw std::runtime_error("something"), or have an actual
> function instead of the lambda.
> 
> > 
> > Some really radical can be
> > 
> > void log_version()
> > {
> >     vd_printf("%u.%u.%u.%u", RC_PRODUCTVERSION);
> 
> Yup, or that, but I'd use WINDOWS_PRODUCTVERSION
> 
> Christophe
> 

Already sent the patch.

RC_PRODUCTVERSION was already defined, I just reused that.
I agree "RC_PRODUCTVERSION" is not really understandable at the first
sight (why something related to resources?) but "WINDOWS" looks more
related to the OS. Considering that old code get the information from
resources that change and constant make sense. Maybe a comment for who
is going to read the code like

    // print same version as resource one
    vd_printf("%u.%u.%u.%u", RC_PRODUCTVERSION);

Frediano