Re: [libvirt] [PATCH 0/2] option to disable default gateway in IPv6 RA

On 06/29/2016 10:17 AM, Maxim Perevedentsev wrote: Nice detective work! It's good that somebody is actually using IPv6 and finding things like this. Since I'm in a bit of a rush today and this isn't a detailed nitpicky review anyway, I'm just going to put all my comments here in one place. * I understand why you would want it defaulted to set the default route (and that you probably got the idea for the name from the existing local variable in virNetworkDefParseXml()[1], but I have a great dislike for double-negative attributes as they tend to confuse me (and others too I hope :-). I would rather than the option was "ipv6DefRoute" and defaulted to "yes" (this takes a bit more setup in the code, but is worth it). * Beyond that, I think it would make more sense to have the option defined in the <ip> element for the IPv6 address rather than at the toplevel (I know there is already an option called "ipv6" at the toplevel, but that is a special case because it's telling what to do wrt IPv6 when there *aren't any* ipv6 <ip> elements in the network definition). A question: would it be possible to set multiple IPv6 addresses, and mark one of them as the default? If so, how would that be configured? * When you're checking for whether or not dnsmasq is able to support the option you're using, you base this on a dnsnasq version number. Is there any chance that the necessary info could be learned from the output of dnsmasq --help? Would it be adequate to just check for the presence of the string "--ra-param=" in the help output? This is already done to check for dnsmasq's use of SO_BINDTODEVICE - see dnsmasqCapsSetFromBuffer(). I'm guessing you based your addition on the existing code for DNSMASQ_DHCPv6_SUPPORT() and DNSMASQ_RA_SUPPORT(), but I think those were probably put in before the patches that added parsing of --help output to learn dnsmasq capabilities. * the split between the two patches is a bit odd compared to the way we normally add new features exposed in XML. Usually there would be 2 or 3 of the following, in the given order: 1) any new low level utilities needed (repeat as necessary), e.g. a patch adding a capabilities bit for the ability of dnsmasq to perform the needed function (it's not needed this time, but following this you could have patches adding new functionality in the utility functions for dnsmasq). 2) add the new config option to the schema, docs, parser/formatter, and xml2xml test cases (in a single patch) 3) add the code in the (in this case) network driver that examines the new config and capabilities flags, and if applicable calls the new low level utility to implement the config (in this case, there isn't a new low level function, you just add an extra argument to the dnsmasq commandline). [1] Although the internal variable is called "ipv6nogwStr" it is used to "enable guest to guest IPv6 communication" and is publicly named simply "ipv6", so there is no double negative visible to the outside world.