On Mittwoch, 9. März 2022 13:40:32 CET Arne Schwabe wrote: > Am 09.03.22 um 00:06 schrieb Heiko Hund: > > +bool dns_server_priority_parse(long *priority, const char *str, bool > > +[...] > > +void show_dns_options(const struct dns_options *o); > > These new functions are missing doxygen comments. We generally have the > doxygen comments in the header files rather than in the c files.
Okay, I'll move the comments into the header file. > > +--dns args > > + Client DNS configuration to be used with the connection. > > This man page section sounds like all options are supported on all > platforms while currently that is not the case. An explicit warning etc > that says that some options like port, dnssec, doh etc are not supported > on certain platforms should be included and also what happens if they > are not. Noted. What happens will likely be determined when we have the implementation(s) that actually take this option and try to apply it. Think it is too early to add this info to the manual, as it may change. Some ideas exist, but they have to prove themselves and survive public discussion. So far I have the idea that options will be ignored if they are not supported, and split DNS is turned into all DNS redirected to the pushed server(s) if they are unsupported. Again, this is very theoretical at this point and may change once facing reality. > > The ``--dns`` option will eventually obsolete the ``--dhcp-option`` > > directive.> > > + Until then it will add configuration at the places ``--dhcp-option`` > > puts it + and will override it if both are given. > > Is dhcp-option ignored when dns is present by newer client or are they > mixed? This is a bit vague. Will clarify. > > + long priority; > > why a type that is 32 bit on some platforms and 64 bit on others? Either > go with just int if 32 bit is enough or use long long or int64_t if we > need 64 but long is a weird type. int8_t would be enough, just need an extra few bits for the conversation from text. It is a long because strtol(3) returns a long, and I went with that. Not sure if casting after checks will gain much. If we decide to change this I'd rather use int8_t however, or a future somebody will ponder why the extra bits there, when the priority is really a 8 bit value. IMHO it is not worth the extra code, that's why I went with long. Compilers might align an 8 bit value anyways. > > + name_ok &= openvpn_snprintf(env_name, sizeof(env_name), > > "dns_server_%d_address4", i); > bitwise and with a bool feels wrong. We should use && instead of & for > boolean logic. Since there is no &&= operator, I thought it's more readable with &=. The result is the same because of the way bool is defined. I'll convert it to logical operators for the v2 and then we'll see if it looks and feels better. > > + enum dns_security dnssec; > > + enum dns_server_transport transport; > > + char *sni; > > +}; > > That should be probably be a const char* instead of a modifable char* Right. > > gc_free(&o->gc); > > > > + gc_free(&o->dns_options.gc); > > having an extra gc in this way feels a bit strange. Since this is the > only time it is initialised you can just as well just use o->gc instead. Actually no. The --dns option is a bit special as it can partially replace local setting with pushed ones (i.e. override single dns servers and others not). The contained gc is used to allocate (and free) pulled options. If we'd use o->gc as well, the pulled options wouldn't be freed for a potentially long time, increasing the memory footprint over time. > > + struct gc_arena dns_gc = gc_new();; > > extra ; Eagle eyes! =) > > + /* Free DNS options and reset them to pre-pull state */ > > + gc_free(&o->dns_options.gc); > > There is an additional free for the gc here that has no matching > gc_init. If your goal is to have a gc for the life time of prepull etc, > then it is better to put it there as that is easier to see its lifetime > than to have it only for DNS where the lifetime/ownership is not so clear. The accompanying gc_init() is with the one for o->gc. Don't see how moving the gc into struct options_pre_connect would change anything substantially. The gc_init would still be done in init_options() then. > I see also that this code makes no attempt to remove/refactor the old > DHCP code. The only thing I see is the tuntap_options_copy_dns code. > What the supposed way forward here? That also raises the question about > platform specific code. Does that code now need two implementations? One > for installing options from the dhcp structs and one from the DNS struct? > > Some idea how this is going to go forward would be good. There are several ideas. Instead of having a #ifdef hell, like for other platform specific code, I would prefer some sort of "dns plugin / script" way, where platform specific code could reside in individual units and linked / called on a on-demand basis. However I want the community to discuss and bless an approach before implementing it. Later. For now this is the first step to get openvpn 2 and 3 on a common basis for (pushing) dns options. dhcp-options will fade then, once we have proper --dns code in. If it turns out some of the dhcp-options are worthwhile keeping, I think it is best to introduce new specialized (Windows) options to handle them. Cheers, Heiko _______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
