[libvirt] [PATCH RESEND] Add support for <option> tag in network config
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@hollants.com> --- 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@bull.net> Nobuhiro Itou <fj0873gn@aa.jp.fujitsu.com> Pete Vetere <pvetere@redhat.com> Philippe Berthault <philippe.berthault@Bull.net> +Pieter Hollants <pieter@hollants.com> Saori Fukuta <fukuta.saori@jp.fujitsu.com> Shigeki Sakamoto <fj0588di@aa.jp.fujitsu.com> Shuveb Hussain <shuveb@binarykarma.com> 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> + </element> + </zeroOrMore> </element> </optional> </element> 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> <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; + } + /* TODO: either whitelist numbers or blacklist numbers already occupied + * by other XML statements (eg. submask) */ + if (!(option->value = virXMLPropString(node, "value"))) { + virReportError(VIR_ERR_XML_ERROR, + _("Option definition in IPv4 network '%s' " + "must have value attribute"), + networkName); + 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", + 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) { + for (r = 0 ; r < ipdef->noptions ; r++) { + virBufferAsprintf(&configbuf, "dhcp-option=%u,\"%s\"\n", + ipdef->options[r].number, + ipdef->options[r].value); + } + } } ipdef = (ipdef == ipv6def) ? NULL : ipv6def; } @@ -959,6 +968,7 @@ networkDnsmasqConfContents(virNetworkObjPtr network, virBufferAsprintf(&configbuf, "addn-hosts=%s\n", dctx->addnhostsfile->path); + /* 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> -- 1.7.10.4
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@hollants.com> --- 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@bull.net> Nobuhiro Itou <fj0873gn@aa.jp.fujitsu.com> Pete Vetere <pvetere@redhat.com> Philippe Berthault <philippe.berthault@Bull.net> +Pieter Hollants <pieter@hollants.com> Saori Fukuta <fukuta.saori@jp.fujitsu.com> Shigeki Sakamoto <fj0588di@aa.jp.fujitsu.com> Shuveb Hussain <shuveb@binarykarma.com>
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> + </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' .../>.
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) */ + if (!(option->value = virXMLPropString(node, "value"))) { + virReportError(VIR_ERR_XML_ERROR, + _("Option definition in IPv4 network '%s' " + "must have value attribute"), + networkName); + 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", + 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) { + for (r = 0 ; r < ipdef->noptions ; r++) { + virBufferAsprintf(&configbuf, "dhcp-option=%u,\"%s\"\n", + ipdef->options[r].number, + ipdef->options[r].value); + } + } } 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: 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 :) Michal
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@hollants.com> --- 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@bull.net> Nobuhiro Itou <fj0873gn@aa.jp.fujitsu.com> Pete Vetere <pvetere@redhat.com> Philippe Berthault <philippe.berthault@Bull.net> +Pieter Hollants <pieter@hollants.com> Saori Fukuta <fukuta.saori@jp.fujitsu.com> Shigeki Sakamoto <fj0588di@aa.jp.fujitsu.com> Shuveb Hussain <shuveb@binarykarma.com> 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.
[adding Pieter, in case he didn't see the reply.] On 02/26/2013 11:05 AM, Laine Stump wrote:
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).
I also like the idea of reverting this change so that 1.0.3 doesn't leave us stuck with a half-baked design that we will later regret because we have to do back-compat support alongside a more friendly design with option names. Having a bit more time on our side to get the interface right in 1.0.4 feels like it will be beneficial in the long run. Pieter, can you chime in within the next 24 hours? If not, I'm okay if Laine does the revert before DV cuts rc2.
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@hollants.com> --- 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@bull.net> Nobuhiro Itou <fj0873gn@aa.jp.fujitsu.com> Pete Vetere <pvetere@redhat.com> Philippe Berthault <philippe.berthault@Bull.net> +Pieter Hollants <pieter@hollants.com> Saori Fukuta <fukuta.saori@jp.fujitsu.com> Shigeki Sakamoto <fj0588di@aa.jp.fujitsu.com> Shuveb Hussain <shuveb@binarykarma.com> 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/26/2013 01:21 PM, Eric Blake wrote:
[adding Pieter, in case he didn't see the reply.]
I'm starting to think there's a bug in Thunderbird - I pasted Pieter's address into a line that had "Bcc:" on it, then changed the Bcc to Cc; I specifically remember doing this after my recent experience with an attempted Cc to libvir-list ending up as a Bcc.
[Bcc problems] Ah, indeed, I never got some of those mails, just saw my original patch merged last Sunday and already wondered why nobody commented :) For brevity, I'll summarize the points we've been discussing. Forgive me if I oversee something important. - We're thinking about importing DHCP options as "first class citizens" into our XML language. - Issue: There are no official names in any RFC. - Possible solution: use what dnsmasq offers ("dnsmasq --help dhcp"). - Issue here: We do not want to depend on dnsmasq peculiariaties too much. Consider the situation where we choose a different DHCP server and that one uses different names. We'd have to convert from nameA to nameB. - Issue here: The selection made by dnsmasq could be called somewhat subjective. There is a great possibility that users need additional options, be it more rarely used, hardware-specific options for some access point, or the custom option range (224-252). - Possible solution: go with names for everything dnsmasq also has a name for and allow numbers for other DHCP options. - Issue here: If we take this route, we need to keep the names synchronous to dnsmasq. Eg. if dnsmasq learns a new option, we should add it to our XML schema as well for consistency reasons. - Possible solution: invent own names for further options. - Issue here: Not much utility and names won't be too intuitive. - Issue: whatever names we take, it hurts much more if in retrospect we chose the "wrong" ones than it does to allow <option>. So while allowing numbers at first sight looks like a possible trapdoor, I guess getting the names right is harder because it's sort of a decision "for eternity". - Issue: If we learn names, we might want to validate values just as well. - Issue: One could implement different depths of option validation. For example, according to RFC 4578 DHCP option 94 (Client network interface identifier) has a three octets-sized value and currently the first one can only be 1. Would we enforce this or just enforce three integer octets? - Issue: As a variation, some clients might not be 100% RFC-conformant and expect certain option values that we forbid because _we_ try to conform 100% to the appropriate RFC. - Issue: We would have to select for WHICH options we'd do validation. The underlying DHCP server, be it dnsmasq or something else in the future, is possibly not as restrictive as we are. The alternative approach "be as smart as dnsmasq is" will be hard to maintain. - Issue: Validation can't be done for the "private use" DHCP options. - Possible solution: Just allow anything for THESE. Unrelated to this discussion there was the idea to add a "force=" attribute. I don't think there's much argument about it, it's a good idea. Am 26.02.2013 19:05, schrieb Laine Stump:
On the other hand, I don't want to over-engineer this problem so much that we never push *anything*.
Yes, unless I missed something I think the bottom line of my summary above is that there would have to be a really good argument _against_ "number=xxx". Implementing "name=yyy" does not automatically mean that "number=xxx" is a bad thing to do. Most of all, it's a very pragmatic solution whereas "name=xxx" most probably requires some design decisions.
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?
I'm used to having to compile it myself anyway, so that'd be fine with me if we want to give it more thought. On the other hand, I don't see that we can do WITHOUT "number=..." unless we want to implement only a restricted subset of DHCP options known by name.
+ 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" :-)
dnsmasq (or any other DHCP server we support in the future) could use a "test config" mode that simply returns success or failure and that we could use when reading in XML fragments. That way, we wouldn't have to duplicate dnsmasq's work.
@@ -959,6 +968,7 @@ networkDnsmasqConfContents(virNetworkObjPtr network, virBufferAsprintf(&configbuf, "addn-hosts=%s\n", dctx->addnhostsfile->path);
+ This looks odd.
And it got overseen during the merge ;)
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 :-)).
I guess sometimes even after intensive discussions we just have to try and see if things work out. -- Pieter Hollants <pieter@hollants.com>
On 02/26/2013 03:38 PM, Pieter Hollants wrote:
[Bcc problems] Ah, indeed, I never got some of those mails, just saw my original patch merged last Sunday and already wondered why nobody commented :)
For brevity, I'll summarize the points we've been discussing. Forgive me if I oversee something important.
[very thorough summary snipped]
Am 26.02.2013 19:05, schrieb Laine Stump:
On the other hand, I don't want to over-engineer this problem so much that we never push *anything*.
Yes, unless I missed something I think the bottom line of my summary above is that there would have to be a really good argument _against_ "number=xxx". Implementing "name=yyy" does not automatically mean that "number=xxx" is a bad thing to do. Most of all, it's a very pragmatic solution whereas "name=xxx" most probably requires some design decisions.
Correct.
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?
I'm used to having to compile it myself anyway, so that'd be fine with me if we want to give it more thought.
On the other hand, I don't see that we can do WITHOUT "number=..." unless we want to implement only a restricted subset of DHCP options known by name.
Right. I haven't changed my mind about the "number" attribute, but since the "value" will be re-used when we have named (and more thoroughly parsed/validated) options, and since there are some options that take a *list* of values rather than a single value, if maybe we shouldn't do something like: <option number='6'> <value type='ipv4' data='1.2.3.4'/> <value type='ipv4' data='4.5.6.7'/> </option> or something similar. (on known options, the "type" would be implicit, and if specified would be required to match what was already known). Since that would create conflicts with what we'd decided / you implemented, and since we can't change anything once it's been in an official release, I figured the safest thing to do was revert the patch until after the release, giving us more time to ponder (I reverted it this morning). Definitely *something* will go in between 1.0.3 and 1.0.4, though! (BTW, thanks for posting a patch for this so quickly. It really helped to get me motivated about the issue.)
participants (4)
-
Eric Blake -
Laine Stump -
Michal Privoznik -
Pieter Hollants