References: <20170504173605.8269-1-valdikss@...277...> From: ValdikSS Message-ID: <9cc5162b-d81b-9286-e715-e6c7d807385d@...277...> Date: Wed, 10 May 2017 19:08:27 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/alternative; boundary="------------96C8D10282057CD1BFE4F1AA" Content-Language: en-GB Subject: Re: [Openvpn-devel] [PATCH] Set a low interface metric for tap adapter when block-outside-dns is in use List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Selva Nair Cc: "openvpn-devel@lists.sourceforge.net" This is a multi-part message in MIME format. --------------96C8D10282057CD1BFE4F1AA Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 08.05.2017 17:47, Selva Nair wrote: > Hi, > > Please bear with me for making a few more comments. Please don't excuse. That's totally fine. > This close to final so only > a few minor issues. > > On Thu, May 4, 2017 at 1:36 PM, ValdikSS > wrote: > > Nitpicking here, but anyway a new version is required (see below), so > it would be nice if you wrap lines in commit message to something > like about 70 to 80 at most. Not sure whether we have a policy on this.. Done. > > > InitializeIpInterfaceEntry() is missing in all but very recent mingw32 versions > (their commit logs show it was added in early 2015) so we may need > to declare it in block_dns.c. I use Debian jessie (8.7) -- mingw gcc 4.9.1 and its > not there. I believe Samuli's build system uses an even older version. I don't know how to do that correctly. Can you please help me? > > Note that 64 bit build still succeeds with a warning but 32 bit will fail because > of stdcall. Yes, its stdcall though MSDN doesn't mention it as such. > > > This would be "Get" or "Return" not "Set" Done. > > > .. > > > > The current metric is queried and saved only for v4 while the metric is > set and restored for both v4 and v6. Isn't it necessary to also save the > metric for both independently? I'm not sure whether anyone really uses > different metric for v4 and v6 but Windows allows it. Done. > > > Here ">" should be ">=0", else automatic metric is not restored. Done. > > > Finally there are some trailing white space errors (6 places). This is a good > opportunity to fix those too. Done. Please check https://github.com/ValdikSS/openvpn-with-patches/commit/02058c8f043d2731011222a05686478a09e479ec Text vesion: https://github.com/ValdikSS/openvpn-with-patches/commit/02058c8f043d2731011222a05686478a09e479ec.patch > > Thanks, > > Selva > > > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > > > _______________________________________________ > Openvpn-devel mailing list > Openvpn-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openvpn-devel --------------96C8D10282057CD1BFE4F1AA Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 8bit On 08.05.2017 17:47, Selva Nair wrote:

Hi,

Please bear with me for making a few more comments.

Please don't excuse. That's totally fine.

This close to final so only
a few minor issues.

On Thu, May 4, 2017 at 1:36 PM, ValdikSS <valdikss@...277...> wrote:

Nitpicking here, but anyway a new version is required (see below), so

it would be nice if you wrap lines in commit message to something

like about 70 to 80 at most. Not sure whether we have a policy on this..

Done.

InitializeIpInterfaceEntry() is missing in all but very recent mingw32 versions

(their commit logs show it was added in early 2015) so we may need

to declare it in block_dns.c. I use Debian jessie (8.7) -- mingw gcc 4.9.1 and its

not there. I believe Samuli's build system uses an even older version.

I don't know how to do that correctly. Can you please help me?

Note that 64 bit build still succeeds with a warning but 32 bit will fail because

of stdcall. Yes, its stdcall though MSDN doesn't mention it as such.

This would be "Get" or "Return" not "Set"

Done.

..

The current metric is queried and saved only for v4 while the metric is

set and restored for both v4 and v6. Isn't it necessary to also save the

metric for both independently? I'm not sure whether anyone really uses
different metric for v4 and v6 but Windows allows it.

Done.

Here ">" should be ">=0", else automatic metric is not restored.

Done.

Finally there are some trailing white space errors (6 places). This is a good

opportunity to fix those too.

Done.

Please check
https://github.com/ValdikSS/openvpn-with-patches/commit/02058c8f043d2731011222a05686478a09e479ec

Text vesion:
https://github.com/ValdikSS/openvpn-with-patches/commit/02058c8f043d2731011222a05686478a09e479ec.patch

Thanks,

Selva

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

--------------96C8D10282057CD1BFE4F1AA--