Hi
Thanks for the review.
On Fri, Sep 25, 2020 at 5:24 AM Lev Stipakov <lstipa...@gmail.com> wrote:
> Hi,
>
> > Note: this will set the domain twice if both v4 and v6 DNS
> > servers are defined. It cant hurt, but could be avoided by
> > making the domain setting a separate call from the DNS
> > server setting.
>
> I think we should do that - there is no reason to make expensive call
> twice:
>
Separating the two is easy, but not sure how hard it would be to
avoid calling it twice. With the ipv6-only feature, ipv4 setting is no
longer guaranteed and elsewhere in the code we do some tasks twice: once
for v4 and once for v6 instead of keeping a tab.
I'll take a closer look.
>
> $ time wmic nicconfig where \(InterfaceIndex=38\) call SetDNSDomain
> vpntest.lev
> Executing
> (\\LAPTOP-4L3N7KFS\ROOT\CIMV2:Win32_NetworkAdapterConfiguration.Index=17)->SetDNSDomain()
> Method execution successful.
> Out Parameters:
> instance of __PARAMETERS
> {
> ReturnValue = 0;
> };
>
> real 0m0,236s
> user 0m0,000s
> sys 0m0,015s
Besides, DOMAIN could also be set by DHCP - there is no point
> to do it via service in this case.
>
If DHCP is in use, do_dns_service() will not get called and the DOMAIN
setting will not be delegated to the service even with this patch.
> > + strncpy(dns.domains, tt->options.domain, _countof(dns.domains));
> > + /* truncation of domain name is not checked as it cant happen
> > + * with 512 bytes room in dns.domains.
> > + */
>
> Mix of tabs and whitespaces.
>
Will fix.
>
> > + /* comma separated list must be enclosed in parenthesis */
>
> We don't pass lists yet, is this for future SearchDomains support?
>
Yes, kind of...
I have been playing with this for a while and was motivated because wmic
does support SetDNSSuffixSearchOrder. But works only for the global
setting, not per interface -- it errors if a where clause is added.
I left the code in there as it may get supported in future. Also it took
awhile for me to figure out what format is needed for lists vs single
objects from the command line.
>
> > + if (msg->domains[0]) {
> > + err = SetDNSDomain(wide_name, msg->domains, lists);
> > + }
>
> msg->domains comes from (unprivileged) client and might
> not be NULL terminated. Shall we do something like
>
> msg->domains[sizeof(msg->domains) - 1] = '\0';
>
> Same for interface_t::name. Or am I missing something?
>
My mistake. Will fix. Also our own safer strncpynt instead of strncpy in
do_dns_service().
Thanks,
Selva
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
