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

[Christophe Fergeau]

On Mon, Jul 24, 2017 at 08:19:00AM -0400, Frediano Ziglio wrote:
> > 
> > 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);

Most important thing to mention in a comment is that RC_PRODUCTVERSION is defined as
$major, $minor, $microdigit, 0.
I checked that we get warnings from the compiler if we pass too many/not
enough args.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170724/889dbc38/attachment.sig>