Hi,
Thanks for the review and comments. A quick reply below, will send a v2
later.
On Mon, Oct 1, 2018 at 7:11 AM Lev Stipakov <lstipa...@gmail.com> wrote:
> Hi,
>
> Thanks, I tested on Windows 10 with Visual Studio build and works as
> expected.
>
> A few nitpicks:
>
> + if (!WriteFile(pipe, &dhcp, sizeof(dhcp), &len, NULL)
>> + || !ReadFile(pipe, &ack, sizeof(ack), &len, NULL))
>> + {
>> + msg(M_WARN, "TUN: could not talk to service: %s [%lu]",
>> + strerror_win32(GetLastError(), &gc), GetLastError());
>> + goto out;
>> + }
>>
>
> A very similar code (communicate with service) is used in
> do_address_service and do_dns6_service, so
> how about factoring that code into separate method?
>
Sounds like a good idea. As this affects a number of unrelated contexts,
will follow up with a refactoring patch.
>
>
>> + /* Path of netsh */
>> + int n = GetSystemDirectory(argv0, MAX_PATH);
>> + if (n > 0 && n < MAX_PATH) /* got system directory */
>> + {
>> + wcsncat(argv0, L"\\netsh.exe", MAX_PATH - n - 1);
>> + }
>> + else
>> + {
>> + wcsncpy(argv0, L"C:\\Windows\\system32\\netsh.exe", MAX_PATH);
>> + }
>>
>
>
Same as above (code in netsh_dns_cmd).
>
Will do in v2.
>
>
>
>> + /* max cmdline length in wchars -- include room for if index */
>> + size_t ncmdline = wcslen(fmt) + 10 + 1;
>>
>
> Maybe comment that 10 is a max string length of int type and 1 is a nul
> terminator (do we need it here?).
>
Extra room for NUL is strictly not necessary as there is space in the
format specifier "%d" etc. Still, I think its better to have an explicit +1.
Selva
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
