Hi,

On Tue, Dec 29, 2015 at 06:03:04PM +0200, Lev Stipakov wrote:
> * Use adapter name instead of index on WinXP - sadly XP does not support 
> indexes
> * Write Windows version to log
> * Send it with peer-info as IV_PLAT_VER

Overall, I'm fine with the patch - thanks a lot.  

I've built release/2.3 + patch on ubuntu 14.04 and tested on XP SP3, and
it nicely worked - sending IV_PLAT_VER=5.1 ("WIN_XP"), and using interface
name again.

A few small details (half of them already discussed on IRC):

 - please send IV_PLAT_VER only if the client requested this (push-peer-info)
   (this is slightly more sensitive information, comparable to library
   versions, and when introducing library versions in IV_SSL, we decided
   to not send such information by default)

 - and please send a patch of the windows version bits for master ("all but
   the changes against route.c and tun.c" :) )

 - src/compat/compat-versionhelpers.h needs to be listed "somewhere"
   (src/compat/Makefile.am libcompat_la_SOURCES=... maybe?) so that
   "make dist" will include it into the generated .tar.gz

>  #elif defined (WIN32)
>  
> -  struct buffer out = alloc_buf_gc (64, &gc);
> -  buf_printf (&out, "interface=%d", tt->adapter_index );
> -  device = buf_bptr(&out);
> +  if (win32_version_info() != WIN_XP)
> +    {
> +     struct buffer out = alloc_buf_gc (64, &gc);
> +     buf_printf (&out, "interface=%d", tt->adapter_index );
> +     device = buf_bptr(&out);
> +    }

the indentation on these is off - they are indented with a full <tab>
but our funny 2.3 convention requires *six* spaces here...  (sorry for
being a pain... trying to be consistent)

Same thing for delete_route_ipv6().

Functionally, this all looks perfectly fine (I only tested on XP, but
as the rest is just existing code, it will "obviously" work for the
non-XP case).

[..]
> +int
> +win32_version_info()
> +{
> +    if (!IsWindowsXPOrGreater())
> +    {
> +        msg (M_FATAL, "Error: Windows version must be XP or greater.");
> +    }
> +
> +    if (!IsWindowsVistaOrGreater())
> +    {
> +        return WIN_XP;
> +    }

I'm not sure if this is the canonical best version to deal with it or
not, but I don't particularily *want* to get myself involved into this.

If someone else thinks this needs improvement (like, working Win10
detection :-) ) I'm all open to accept additional patches on top of it,
but to solve our immediate needs, this is good enough for me and does 
not *need* changes.

thanks, and waiting for v2 :-)

gert

-- 
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             g...@greenie.muc.de
fax: +49-89-35655025                        g...@net.informatik.tu-muenchen.de

Attachment: signature.asc
Description: PGP signature

Reply via email to