Re: [libvirt] [PATCH RESEND] Add support for <option> tag in network config

My original response to the patch and to Michal's review is at the end of this message for historical purposes. I had written it up, made some small changes to the patch to address problems I found, and pushed it, but somehow managed to never hit "Send" on the email :-( In the meantime, I read Eric's review of my followup patch that added support for a "force" attribute: https://www.redhat.com/archives/libvir-list/2013-February/msg01413.html Eric's self-debate here got me thinking more seriously about the future implications of this original "numeric-only" <option> patch, and of something that has been becoming more apparent to me as I've been taking a whack at implementing support for named options on top of it. In particular, the "value" of a dhcp option can be one of several types, and in some cases it can be a repeated list (especially of IP addresses, but there are a few that are lists of numbers). As it stands, we treat "value" as an opaque string, and just pass it through to dnsmasq as-is (except for checking for embedded spaces and \). For example, this: <option number='6' value='1.2.3.4,5.6.7.8'/> results in the following line in dnsmasq.conf: dhcp-option=6,1.2.3.4,5.6.7.8 That works, but if we ever decide we want to do a better job of validating the value (rather than just relying on dnsmasq), then we arrive at a situation where the data that has been parsed out of the XML must be further subdivided and parsed to get at the individual values in the list. I can recall at least twice in recent memory that I dinged a patch during review for exactly this reason. On the other hand, I don't want to over-engineer this problem so much that we never push *anything*. Without even arriving at a decision about this, I'm now thinking that maybe we should revert the earlier <option> patch until after the release, and re-push it after the named-option support is done (potentially with some changes).

Any other opinions? =========================================================================== On 02/22/2013 04:37 AM, Michal Privoznik wrote: > On 21.02.2013 23:40, Pieter Hollants wrote: >> This patch adds support for a new <option>-Tag in the <dhcp> block of network configs, >> based on a subset of the fifth proposal by Laine Stump in the mailing list discussion at >> https://www.redhat.com/archives/libvir-list/2012-November/msg01054.html. Any such defined >> option will result in a dhcp-option=<number>,"<value>" statement in the generated dnsmasq >> configuration file. >> >> Currently, DHCP options can be specified by number only and there is no whitelisting or >> blacklisting of option numbers, which should probably be added. >> >> Signed-off-by: Pieter Hollants <pieter(a)hollants.com&gt; >> --- >> AUTHORS.in | 1 + >> docs/schemas/network.rng | 6 +++ >> docs/schemas/networkcommon.rng | 6 +++ >> src/conf/network_conf.c | 54 +++++++++++++++++++++ >> src/conf/network_conf.h | 10 ++++ >> src/network/bridge_driver.c | 10 ++++ >> tests/networkxml2xmlin/netboot-network.xml | 1 + >> tests/networkxml2xmlin/netboot-proxy-network.xml | 1 + >> tests/networkxml2xmlout/netboot-network.xml | 1 + >> tests/networkxml2xmlout/netboot-proxy-network.xml | 1 + >> 10 Dateien geändert, 91 Zeilen hinzugefügt(+) >> >> diff --git a/AUTHORS.in b/AUTHORS.in >> index 39fe68d..074185e 100644 >> --- a/AUTHORS.in >> +++ b/AUTHORS.in >> @@ -74,6 +74,7 @@ Michel Ponceau <michel.ponceau(a)bull.net&gt; >> Nobuhiro Itou <fj0873gn(a)aa.jp.fujitsu.com&gt; >> Pete Vetere <pvetere(a)redhat.com&gt; >> Philippe Berthault <philippe.berthault(a)Bull.net&gt; >> +Pieter Hollants <pieter(a)hollants.com&gt; >> Saori Fukuta <fukuta.saori(a)jp.fujitsu.com&gt; >> Shigeki Sakamoto <fj0588di(a)aa.jp.fujitsu.com&gt; >> Shuveb Hussain <shuveb(a)binarykarma.com&gt; > This is not necessary. AUTHORS file is now completely generated since v0.10.2-207-g7b21981. > >> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng >> index 09d7c73..a2ce1c9 100644 >> --- a/docs/schemas/network.rng >> +++ b/docs/schemas/network.rng >> @@ -293,6 +293,12 @@ >> </optional> >> </element> >> </optional> >> + <zeroOrMore> >> + <element name="option"> >> + <attribute name="number"><ref name="unsignedByte"/></attribute> >> + <attribute name="value"><text/></attribute> Actually it appears that it's legal to send some options with no value at all; this is even mentioned in dnsmasq documentation. So value needs to be optional in the RNG, and in the parsing/formatting. >> + </element> >> + </zeroOrMore> >> </element> >> </optional> >> </element> > Maybe it's my lack of experience, but there's no mapping from numbers defined in RFC2132 to human readable strings and vice versa in my brain. I think, <option name='router' value='192.168.0.1'/> or <option name='ntp-server' value='1.2.3.4'/> is more readable than their numbered alternatives <option number='3' .../> and <option number='42' .../>. I think we can all agree on that. However, there is no official standard alpha ID name for the options (although they're named in the rfc, it's with a multi-word section title, not a single identifier", and dnsmasq (at least) doesn't support names for all options. So we need to support specifying numeric options anyway, and that's a good start on full support. As a followup to this patch, hopefully one of us will have the ambition+time to make a patch that adds a "name" attribute to <option, with an enum listing all the named options supported by dnsmasq, and special validation of the contents of value for those we know more about. When a name is given, it would be checked against the list of known names, then transferred directly into the dnsmasq commandline if it was known (after doing the extra validation of the value) There is one other thing I think we need to add right away though - a "force" attribute - if set to "yes', the dhcp server would be told to always send that option, even if it wasn't requested (the default is to only send if requested); the way to do this with dnsmasq is to use "dhcp-option-force=..." instead of "dhcp-option=...". (That can also be a separate patch submitted after this one). > >> diff --git a/docs/schemas/networkcommon.rng b/docs/schemas/networkcommon.rng >> index 51ff759..3510928 100644 >> --- a/docs/schemas/networkcommon.rng >> +++ b/docs/schemas/networkcommon.rng >> @@ -173,6 +173,12 @@ >> </data> >> </define> >> >> + <define name='unsignedByte'> >> + <data type='integer'> >> + <param name="minInclusive">0</param> >> + <param name="maxInclusive">255</param> >> + </data> >> + </define> > No need to invent new type; uint8range from basictypes.rng is just fine. > >> <define name='unsignedShort'> >> <data type='integer'> >> <param name="minInclusive">0</param> >> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c >> index 3604ff7..9d84c7e 100644 >> --- a/src/conf/network_conf.c >> +++ b/src/conf/network_conf.c >> @@ -777,6 +777,45 @@ cleanup: >> } >> >> static int >> +virNetworkDHCPOptionDefParseXML(const char *networkName, >> + xmlNodePtr node, >> + virNetworkDHCPOptionDefPtr option) >> +{ >> + char *number = NULL; >> + int ret = -1; >> + >> + if (!(number = virXMLPropString(node, "number"))) { >> + virReportError(VIR_ERR_XML_ERROR, >> + _("Option definition in IPv4 network '%s' " >> + "must have number attribute"), >> + networkName); >> + goto cleanup; >> + } >> + if (number && >> + virStrToLong_ui(number, NULL, 10, &option->number) < 0) { >> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", >> + _("Cannot parse <option> 'number' attribute")); >> + goto cleanup; >> + } > After previous 'if' @number is either NULL in which case you goto cleanup; or is not NULL in which case the first part of condition is always true and thus can be dropped. Moreover, I think the error message can be tuned to give even more hint about its origin. Something like: ("Malformed <option> number attribute: %s", number); But that's not a show stopper. However, the error is not VIR_ERR_INTERNAL_ERROR, but VIR_ERR_XML_ERROR or VIR_ERR_XML_DETAIL. > >> + /* TODO: either whitelist numbers or blacklist numbers already occupied >> + * by other XML statements (eg. submask) */ I don't think that's very useful, since that list could (and hopefully will) change over time, and new libvirt must always accept any XML that was allowed by an older version of libvirt. I'm always in favor of having a single way to specify any given configuration, but in this case I don't really think we can. >> + if (!(option->value = virXMLPropString(node, "value"))) { >> + virReportError(VIR_ERR_XML_ERROR, >> + _("Option definition in IPv4 network '%s' " >> + "must have value attribute"), >> + networkName); See my comment above. Not finding a value string isn't an error, because it's legal (and in some cases useful) to specify an option with no associated value. (Unfortunately there is no way to differentiate between "value not found" and "out of memory while trying to strdup the value we found", so we have to simply eliminate error checking here. >> + goto cleanup; >> + } >> + >> + ret = 0; >> + >> +cleanup: >> + VIR_FREE(number); >> + return ret; >> +} >> + >> + >> +static int >> virNetworkDHCPDefParseXML(const char *networkName, >> xmlNodePtr node, >> virNetworkIpDefPtr def) >> @@ -837,6 +876,17 @@ virNetworkDHCPDefParseXML(const char *networkName, >> def->bootfile = file; >> def->bootserver = inaddr; >> VIR_FREE(server); >> + } else if (cur->type == XML_ELEMENT_NODE && >> + xmlStrEqual(cur->name, BAD_CAST "option")) { >> + if (VIR_REALLOC_N(def->options, def->noptions + 1) < 0) { >> + virReportOOMError(); >> + return -1; >> + } >> + if (virNetworkDHCPOptionDefParseXML(networkName, cur, >> + &def->options[def->noptions])) { >> + return -1; >> + } >> + def->noptions++; >> } >> >> cur = cur->next; >> @@ -2045,6 +2095,10 @@ virNetworkIpDefFormat(virBufferPtr buf, >> virBufferAddLit(buf, "/>\n"); >> >> } >> + for (ii = 0 ; ii < def->noptions ; ii++) { >> + virBufferAsprintf(buf, "<option number='%u' value='%s' />\n", I actually don't like having the extra space before the /> here. We're inconsistent about that in our code, but I *think* we tended more toward eliminating that space in the recent past. In addition to that, since the value could contain *anything*, we need to format it into xml with virBufferEscapeString(). >> + def->options[ii].number, def->options[ii].value); >> + } >> >> virBufferAdjustIndent(buf, -2); >> virBufferAddLit(buf, "</dhcp>\n"); >> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h >> index 4c634ed..14f852a 100644 >> --- a/src/conf/network_conf.h >> +++ b/src/conf/network_conf.h >> @@ -77,6 +77,13 @@ struct _virNetworkDHCPHostDef { >> virSocketAddr ip; >> }; >> >> +typedef struct _virNetworkDHCPOptionDef virNetworkDHCPOptionDef; >> +typedef virNetworkDHCPOptionDef *virNetworkDHCPOptionDefPtr; >> +struct _virNetworkDHCPOptionDef { >> + unsigned int number; >> + char *value; >> +}; >> + >> typedef struct _virNetworkDNSTxtDef virNetworkDNSTxtDef; >> typedef virNetworkDNSTxtDef *virNetworkDNSTxtDefPtr; >> struct _virNetworkDNSTxtDef { >> @@ -139,6 +146,9 @@ struct _virNetworkIpDef { >> char *tftproot; >> char *bootfile; >> virSocketAddr bootserver; >> + >> + size_t noptions; /* Zero or more additional dhcp options */ >> + virNetworkDHCPOptionDefPtr options; >> }; >> >> typedef struct _virNetworkForwardIfDef virNetworkForwardIfDef; >> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c >> index c834f83..c7c0a9e 100644 >> --- a/src/network/bridge_driver.c >> +++ b/src/network/bridge_driver.c >> @@ -926,6 +926,15 @@ networkDnsmasqConfContents(virNetworkObjPtr network, >> virBufferAsprintf(&configbuf, "dhcp-boot=%s\n", ipdef->bootfile); >> } >> } >> + >> + /* Any additional DHCP options? */ >> + if (ipdef->noptions > 0) { No need for the "if (ipdef->noptions > 0) - that's taken care of by the for loop already. >> + for (r = 0 ; r < ipdef->noptions ; r++) { >> + virBufferAsprintf(&configbuf, "dhcp-option=%u,\"%s\"\n", All of the dnsmasq.conf examples I've seen (with the one exception of the 'option 252="\n"' that you used in the test data) do *not* quote the value unless it contains an embedded space (or an escaped character, as in the case of "\n"). To make the generated conf file as close to the examples as possible, I'm changing this to *not* include quotes unless there is an embedded space or "\". (Through experimentation I found that \n with no quotes yielded the two character string 0x5c 0x6E, while "\n" quoted resulted in sending a single newline character) I just sent mail to the dnsmasq list asking if the quotes are necessary in any other case, and will submit a relevant patch if so. >> + ipdef->options[r].number, >> + ipdef->options[r].value); We've done no sanity checking on the contents of value. Is there any danger of a rogue metacharacter in dnsmasq.conf causing problems? I seem to recall asking that question before and being told "no", and I'm going to continue assuming that for now, but I still think it would be good practice to do some basic validation of the value if we can determine an all-encompassing valid character set that is something smaller than "everything" :-) >> + } >> + } >> } >> ipdef = (ipdef == ipv6def) ? NULL : ipv6def; >> } >> @@ -959,6 +968,7 @@ networkDnsmasqConfContents(virNetworkObjPtr network, >> virBufferAsprintf(&configbuf, "addn-hosts=%s\n", >> dctx->addnhostsfile->path); >> >> + > This looks odd. > >> /* Are we doing RA instead of radvd? */ >> if (DNSMASQ_RA_SUPPORT(caps)) { >> if (ipv6def) >> diff --git a/tests/networkxml2xmlin/netboot-network.xml b/tests/networkxml2xmlin/netboot-network.xml >> index ed75663..4de8976 100644 >> --- a/tests/networkxml2xmlin/netboot-network.xml >> +++ b/tests/networkxml2xmlin/netboot-network.xml >> @@ -9,6 +9,7 @@ >> <dhcp> >> <range start="192.168.122.2" end="192.168.122.254" /> >> <bootp file="pxeboot.img" /> >> + <option number="252" value="\n" /> >> </dhcp> >> </ip> >> </network> >> diff --git a/tests/networkxml2xmlin/netboot-proxy-network.xml b/tests/networkxml2xmlin/netboot-proxy-network.xml >> index ecb6738..4c5c480 100644 >> --- a/tests/networkxml2xmlin/netboot-proxy-network.xml >> +++ b/tests/networkxml2xmlin/netboot-proxy-network.xml >> @@ -8,6 +8,7 @@ >> <dhcp> >> <range start="192.168.122.2" end="192.168.122.254" /> >> <bootp file="pxeboot.img" server="10.20.30.40" /> >> + <option number="252" value="\n" /> >> </dhcp> >> </ip> >> </network> >> diff --git a/tests/networkxml2xmlout/netboot-network.xml b/tests/networkxml2xmlout/netboot-network.xml >> index b8a4d99..c3ea95a 100644 >> --- a/tests/networkxml2xmlout/netboot-network.xml >> +++ b/tests/networkxml2xmlout/netboot-network.xml >> @@ -9,6 +9,7 @@ >> <dhcp> >> <range start='192.168.122.2' end='192.168.122.254' /> >> <bootp file='pxeboot.img' /> >> + <option number='252' value='\n' /> >> </dhcp> >> </ip> >> </network> >> diff --git a/tests/networkxml2xmlout/netboot-proxy-network.xml b/tests/networkxml2xmlout/netboot-proxy-network.xml >> index e11c50b..f5f2c0d 100644 >> --- a/tests/networkxml2xmlout/netboot-proxy-network.xml >> +++ b/tests/networkxml2xmlout/netboot-proxy-network.xml >> @@ -8,6 +8,7 @@ >> <dhcp> >> <range start='192.168.122.2' end='192.168.122.254' /> >> <bootp file='pxeboot.img' server='10.20.30.40' /> >> + <option number='252' value='\n' /> >> </dhcp> >> </ip> >> </network> >> > Maybe we should add test to networkxml2conf test as well, because the tests you are introducing make sure we don't break XML. However, we need to make sure we produce the right config file for dnsmasq. Something like: I Agree. > > diff --git a/tests/networkxml2confdata/netboot-network.conf b/tests/networkxml2confdata/netboot-network.conf > index b6f3c23..62fab51 100644 > --- a/tests/networkxml2confdata/netboot-network.conf > +++ b/tests/networkxml2confdata/netboot-network.conf > @@ -17,6 +17,7 @@ dhcp-no-override > enable-tftp > tftp-root=/var/lib/tftproot > dhcp-boot=pxeboot.img > +dhcp-option=252,"blah" > dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases > dhcp-lease-max=253 > dhcp-hostsfile=/var/lib/libvirt/dnsmasq/netboot.hostsfile > diff --git a/tests/networkxml2confdata/netboot-network.xml b/tests/networkxml2confdata/netboot-network.xml > index b8a4d99..24ede16 100644 > --- a/tests/networkxml2confdata/netboot-network.xml > +++ b/tests/networkxml2confdata/netboot-network.xml > @@ -9,6 +9,7 @@ > <dhcp> > <range start='192.168.122.2' end='192.168.122.254' /> > <bootp file='pxeboot.img' /> > + <option number='252' value='blah'/> > </dhcp> > </ip> > </network> > > > Overall status, I'm tentative to give ACK. However, I'd like to hear one more opinion on the correctness of XML schema. Elder libvirt developers remember times when XML schema change needed ACK from at least one of Daniels :) Well, I'm usually hesitant about any xml addition for fear of getting it wrong, but since it was me who originally suggested this schema, and there was no negative comment about it on the list at the time, I'm giving my ACK (even though I'm not lucky enough to be named Daniel :-)). There is still work to be done: 1) recognize the "force" attribute. 1) recognize the "name" attribute, and validate value accordingly 2) deal with ipv6 options (this currently only works for ipv4) 3) properly deal with vendor-specific options 4) We may need some tweaking wrt quote marks around the values. but I think those can be done as followups. I've made the few changes you and I pointed out (including the addition of options to a couple of networkxml2conf test cases) and pushed the result. -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list