[libvirt] [PATCH 0/2] network: add force attribute for dhcp options
The firs patch just fixes unexpected behavior in virNetworkDHCPOptionDefParseXML found when testing the second patch. The second patch adds the "force" attribute I mentioned in the review of the patch that added support for specifying dhcp options in network config. While documenting this new attribute, I noticed that I'd neglected to see that original patch didn't include documentation, so I added full documentation for <option> in order to have a place to add the documentation for "force='yes'". I'm also working on a way to allow specification of option names rather than numbers, but didn't know if I could get it in before the freeze, so I'm sending this patch now. Laine Stump (2): conf: use VIR_EXPAND instead of VIR_REALLOC in virNetworkDHCPDefParseXML network: add force attribute for dhcp options docs/formatnetwork.html.in | 21 ++++++++++++++++++++ src/conf/network_conf.c | 32 +++++++++++++++++++++--------- src/conf/network_conf.h | 3 ++- src/network/bridge_driver.c | 3 ++- tests/networkxml2confdata/nat-network.conf | 2 +- tests/networkxml2confdata/nat-network.xml | 2 +- 6 files changed, 50 insertions(+), 13 deletions(-) -- 1.7.11.7
VIR_REALLOC_N doesn't initialize the newly allocated memory to 0, which can result in unpleasant surprises for those who have become accustomed to the 0-initializing behavior of most libvirt memory allocation functions, and add a new field to a struct thinking that it will default to 0. --- (in particular, this patch assures that the upcoming "force" attribute in virNetworkDHCPOptionDef will be initialized to false.) src/conf/network_conf.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 34fd05a..b3e2858 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -820,29 +820,29 @@ virNetworkDHCPDefParseXML(const char *networkName, if (cur->type == XML_ELEMENT_NODE && xmlStrEqual(cur->name, BAD_CAST "range")) { - if (VIR_REALLOC_N(def->ranges, def->nranges + 1) < 0) { + if (VIR_EXPAND_N(def->ranges, def->nranges, 1) < 0) { virReportOOMError(); return -1; } if (virSocketAddrRangeParseXML(networkName, cur, - &def->ranges[def->nranges]) < 0) { + &def->ranges[def->nranges - 1]) < 0) { + def->nranges--; return -1; } - def->nranges++; } else if (cur->type == XML_ELEMENT_NODE && xmlStrEqual(cur->name, BAD_CAST "host")) { - if (VIR_REALLOC_N(def->hosts, def->nhosts + 1) < 0) { + if (VIR_EXPAND_N(def->hosts, def->nhosts, 1) < 0) { virReportOOMError(); return -1; } if (virNetworkDHCPHostDefParseXML(networkName, def, cur, - &def->hosts[def->nhosts], + &def->hosts[def->nhosts - 1], false) < 0) { + def->nhosts--; return -1; } - def->nhosts++; } else if (VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET) && cur->type == XML_ELEMENT_NODE && @@ -870,15 +870,15 @@ virNetworkDHCPDefParseXML(const char *networkName, 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) { + if (VIR_EXPAND_N(def->options, def->noptions, 1) < 0) { virReportOOMError(); return -1; } if (virNetworkDHCPOptionDefParseXML(networkName, cur, - &def->options[def->noptions])) { + &def->options[def->noptions - 1])) { + def->noptions--; return -1; } - def->noptions++; } cur = cur->next; -- 1.7.11.7
If a dhcp option has "force='yes'", the dhcp server will send that option back to every client regardless of whether or not the client requests that option (without "force='yes'", an option will only be sent to those clients that ask for it in their request packet). For example: <option number='40' value='libvirt' force='yes'/> This information is relayed to dnsmasq by using the "dhcp-option-force" option, rather than "dhcp-option". --- docs/formatnetwork.html.in | 21 +++++++++++++++++++++ src/conf/network_conf.c | 14 ++++++++++++++ src/conf/network_conf.h | 3 ++- src/network/bridge_driver.c | 3 ++- tests/networkxml2confdata/nat-network.conf | 2 +- tests/networkxml2confdata/nat-network.xml | 2 +- 6 files changed, 41 insertions(+), 4 deletions(-) diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index f7c483d..4b4c47b 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -557,6 +557,10 @@ <range start="192.168.122.100" end="192.168.122.254" /> <host mac="00:16:3e:77:e2:ed" name="foo.example.com" ip="192.168.122.10" /> <host mac="00:16:3e:3e:a9:1a" name="bar.example.com" ip="192.168.122.11" /> + <option number='252' value='\n'/> + <option number='4' value='192.168.122.11'/> + <option number='6' value='192.168.145.23'/> + <option number='23' value='64' force='yes'/> </dhcp> </ip> <ip family="ipv6" address="2001:db8:ca2:2::1" prefix="64" /> @@ -699,6 +703,23 @@ for all address ranges and statically assigned addresses.<span class="since">Since 0.7.1 (<code>server</code> since 0.7.3).</span> </dd> + <dt><code>option</code></dt> + <dd>The optional <code>option</code> element (which can + be repeated for multiple DHCP options) specifies + generic DHCP options (as defined in RFC 2132, or later + RFCs in some cases) that will be sent by the DHCP + server to requesting clients (IPv4 only). There are + three possible attributes: <code>number</code> is + mandatory and gives the standard option + number; <code>value</code> is optional and gives the + value of that option to be provided to the client + <code>force</code> is also optional, and defaults to + "no"; if <code>force</code> is "yes", the option will + be sent to all DHCP clients regardless of whether or + not the client requests it (usually only options + specifically requested by the client are sent in the + DHCP response).<span class="since">Since 1.0.3</span> + </dd> </dl> </dd> </dl> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index b3e2858..8434dc4 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -782,6 +782,7 @@ virNetworkDHCPOptionDefParseXML(const char *networkName, virNetworkDHCPOptionDefPtr option) { char *number = NULL; + char *force = NULL; int ret = -1; if (!(number = virXMLPropString(node, "number"))) { @@ -797,12 +798,25 @@ virNetworkDHCPOptionDefParseXML(const char *networkName, number); goto cleanup; } + option->value = virXMLPropString(node, "value"); + if ((force = virXMLPropString(node, "force"))) { + if (STRCASEEQ(force, "yes")) { + option->force = true; + } else if (STRCASENEQ(force, "no")) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid <option> force attribute %s " + "in network '%s'"), force, networkName); + goto cleanup; + } + } + ret = 0; cleanup: VIR_FREE(number); + VIR_FREE(force); return ret; } diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index e7a4f95..fd7a03c 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -1,7 +1,7 @@ /* * network_conf.h: network XML handling * - * Copyright (C) 2006-2008, 2012 Red Hat, Inc. + * Copyright (C) 2006-2013 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -75,6 +75,7 @@ typedef virNetworkDHCPOptionDef *virNetworkDHCPOptionDefPtr; struct _virNetworkDHCPOptionDef { unsigned int number; char *value; + bool force; }; typedef struct _virNetworkDNSTxtDef virNetworkDNSTxtDef; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 0c3f778..e66a55d 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -928,7 +928,8 @@ networkDnsmasqConfContents(virNetworkObjPtr network, } for (r = 0 ; r < ipdef->noptions ; r++) { - virBufferAsprintf(&configbuf, "dhcp-option=%u", + virBufferAsprintf(&configbuf, "dhcp-option%s=%u", + ipdef->options[r].force ? "-force" : "", ipdef->options[r].number); /* value is optional, and only needs quoting if it contains spaces */ if (ipdef->options[r].value) { diff --git a/tests/networkxml2confdata/nat-network.conf b/tests/networkxml2confdata/nat-network.conf index ee41e2a..2e855f4 100644 --- a/tests/networkxml2confdata/nat-network.conf +++ b/tests/networkxml2confdata/nat-network.conf @@ -14,7 +14,7 @@ dhcp-range=192.168.122.2,192.168.122.254 dhcp-no-override dhcp-option=42,192.168.122.20 dhcp-option=23,50 -dhcp-option=40,libvirt +dhcp-option-force=40,libvirt dhcp-option=252,"\n" dhcp-option=253," leading and trailing spaces " dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases diff --git a/tests/networkxml2confdata/nat-network.xml b/tests/networkxml2confdata/nat-network.xml index 22ec533..0062cb4 100644 --- a/tests/networkxml2confdata/nat-network.xml +++ b/tests/networkxml2confdata/nat-network.xml @@ -10,7 +10,7 @@ <host mac='00:16:3e:3e:a9:1a' name='b.example.com' ip='192.168.122.11' /> <option number='42' value='192.168.122.20'/> <option number='23' value='50'/> - <option number='40' value='libvirt'/> + <option number='40' value='libvirt' force='yes'/> <option number='252' value='\n'/> <option number='253' value=' leading and trailing spaces '/> </dhcp> -- 1.7.11.7
If a dhcp option has "force='yes'", the dhcp server will send that option back to every client regardless of whether or not the client requests that option (without "force='yes'", an option will only be sent to those clients that ask for it in their request packet). For example: <option number='40' value='libvirt' force='yes'/> This information is relayed to dnsmasq by using the "dhcp-option-force" option, rather than "dhcp-option". --- Changes from V1: * add forgotten addition to RNG * add forgotten addition to virNetworkIpDefFormat * add networkxml2xml test (which would have caught the above) docs/formatnetwork.html.in | 21 +++++++++++++++++++++ docs/schemas/network.rng | 8 ++++++++ src/conf/network_conf.c | 16 ++++++++++++++++ src/conf/network_conf.h | 3 ++- src/network/bridge_driver.c | 3 ++- tests/networkxml2confdata/nat-network.conf | 2 +- tests/networkxml2confdata/nat-network.xml | 2 +- tests/networkxml2xmlin/nat-network.xml | 5 +++++ tests/networkxml2xmlout/nat-network.xml | 5 +++++ 9 files changed, 61 insertions(+), 4 deletions(-) diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 41a83fa..97e3bd4 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -557,6 +557,10 @@ <range start="192.168.122.100" end="192.168.122.254" /> <host mac="00:16:3e:77:e2:ed" name="foo.example.com" ip="192.168.122.10" /> <host mac="00:16:3e:3e:a9:1a" name="bar.example.com" ip="192.168.122.11" /> + <option number='252' value='\n'/> + <option number='4' value='192.168.122.11'/> + <option number='6' value='192.168.145.23'/> + <option number='23' value='64' force='yes'/> </dhcp> </ip> <ip family="ipv6" address="2001:db8:ca2:2::1" prefix="64" /> @@ -699,6 +703,23 @@ for all address ranges and statically assigned addresses.<span class="since">Since 0.7.1 (<code>server</code> since 0.7.3).</span> </dd> + <dt><code>option</code></dt> + <dd>The optional <code>option</code> element (which can + be repeated for multiple DHCP options) specifies + generic DHCP options (as defined in RFC 2132, or later + RFCs in some cases) that will be sent by the DHCP + server to requesting clients (IPv4 only). There are + three possible attributes: <code>number</code> is + mandatory and gives the standard option + number; <code>value</code> is optional and gives the + value of that option to be provided to the client + <code>force</code> is also optional, and defaults to + "no"; if <code>force</code> is "yes", the option will + be sent to all DHCP clients regardless of whether or + not the client requests it (usually only options + specifically requested by the client are sent in the + DHCP response).<span class="since">Since 1.0.3</span> + </dd> </dl> </dd> </dl> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index da7d8ad..ed6964a 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -305,6 +305,14 @@ <element name="option"> <attribute name="number"><ref name="uint8range"/></attribute> <attribute name="value"><text/></attribute> + <optional> + <attribute name="force"> + <choice> + <value>yes</value> + <value>no</value> + </choice> + </attribute> + </optional> </element> </zeroOrMore> </element> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 8b57242..4d07bac 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -796,6 +796,7 @@ virNetworkDHCPOptionDefParseXML(const char *networkName, virNetworkDHCPOptionDefPtr option) { char *number = NULL; + char *force = NULL; int ret = -1; if (!(number = virXMLPropString(node, "number"))) { @@ -811,12 +812,25 @@ virNetworkDHCPOptionDefParseXML(const char *networkName, number); goto cleanup; } + option->value = virXMLPropString(node, "value"); + if ((force = virXMLPropString(node, "force"))) { + if (STRCASEEQ(force, "yes")) { + option->force = true; + } else if (STRCASENEQ(force, "no")) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid <option> force attribute %s " + "in network '%s'"), force, networkName); + goto cleanup; + } + } + ret = 0; cleanup: VIR_FREE(number); + VIR_FREE(force); return ret; } @@ -2233,6 +2247,8 @@ virNetworkIpDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<option number='%u'", def->options[ii].number); virBufferEscapeString(buf, " value='%s'", def->options[ii].value); + if (def->options[ii].force) + virBufferAddLit(buf, " force='yes'"); virBufferAddLit(buf, "/>\n"); } diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index c509915..93ea371 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -1,7 +1,7 @@ /* * network_conf.h: network XML handling * - * Copyright (C) 2006-2008, 2012 Red Hat, Inc. + * Copyright (C) 2006-2013 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -76,6 +76,7 @@ typedef virNetworkDHCPOptionDef *virNetworkDHCPOptionDefPtr; struct _virNetworkDHCPOptionDef { unsigned int number; char *value; + bool force; }; typedef struct _virNetworkDNSTxtDef virNetworkDNSTxtDef; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 0932cf8..3f271d9 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -929,7 +929,8 @@ networkDnsmasqConfContents(virNetworkObjPtr network, } for (r = 0 ; r < ipdef->noptions ; r++) { - virBufferAsprintf(&configbuf, "dhcp-option=%u", + virBufferAsprintf(&configbuf, "dhcp-option%s=%u", + ipdef->options[r].force ? "-force" : "", ipdef->options[r].number); /* value is optional, and only needs quoting if it contains spaces */ if (ipdef->options[r].value) { diff --git a/tests/networkxml2confdata/nat-network.conf b/tests/networkxml2confdata/nat-network.conf index ee41e2a..2e855f4 100644 --- a/tests/networkxml2confdata/nat-network.conf +++ b/tests/networkxml2confdata/nat-network.conf @@ -14,7 +14,7 @@ dhcp-range=192.168.122.2,192.168.122.254 dhcp-no-override dhcp-option=42,192.168.122.20 dhcp-option=23,50 -dhcp-option=40,libvirt +dhcp-option-force=40,libvirt dhcp-option=252,"\n" dhcp-option=253," leading and trailing spaces " dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases diff --git a/tests/networkxml2confdata/nat-network.xml b/tests/networkxml2confdata/nat-network.xml index 22ec533..0062cb4 100644 --- a/tests/networkxml2confdata/nat-network.xml +++ b/tests/networkxml2confdata/nat-network.xml @@ -10,7 +10,7 @@ <host mac='00:16:3e:3e:a9:1a' name='b.example.com' ip='192.168.122.11' /> <option number='42' value='192.168.122.20'/> <option number='23' value='50'/> - <option number='40' value='libvirt'/> + <option number='40' value='libvirt' force='yes'/> <option number='252' value='\n'/> <option number='253' value=' leading and trailing spaces '/> </dhcp> diff --git a/tests/networkxml2xmlin/nat-network.xml b/tests/networkxml2xmlin/nat-network.xml index 23f7fcb..d6a174b 100644 --- a/tests/networkxml2xmlin/nat-network.xml +++ b/tests/networkxml2xmlin/nat-network.xml @@ -8,6 +8,11 @@ <range start="192.168.122.2" end="192.168.122.254" /> <host mac="00:16:3e:77:e2:ed" name="a.example.com" ip="192.168.122.10" /> <host mac="00:16:3e:3e:a9:1a" name="b.example.com" ip="192.168.122.11" /> + <option number='42' value='192.168.122.20'/> + <option number='23' value='50'/> + <option number='40' value='libvirt' force='yes'/> + <option number='252' value='\n'/> + <option number='253' value=' leading and trailing spaces '/> </dhcp> </ip> <ip family="ipv4" address="192.168.123.1" netmask="255.255.255.0"> diff --git a/tests/networkxml2xmlout/nat-network.xml b/tests/networkxml2xmlout/nat-network.xml index 02d6849..55c051e 100644 --- a/tests/networkxml2xmlout/nat-network.xml +++ b/tests/networkxml2xmlout/nat-network.xml @@ -10,6 +10,11 @@ <range start='192.168.122.2' end='192.168.122.254' /> <host mac='00:16:3e:77:e2:ed' name='a.example.com' ip='192.168.122.10' /> <host mac='00:16:3e:3e:a9:1a' name='b.example.com' ip='192.168.122.11' /> + <option number='42' value='192.168.122.20'/> + <option number='23' value='50'/> + <option number='40' value='libvirt' force='yes'/> + <option number='252' value='\n'/> + <option number='253' value=' leading and trailing spaces '/> </dhcp> </ip> <ip family='ipv4' address='192.168.123.1' netmask='255.255.255.0'> -- 1.7.11.7
On 02/25/2013 11:30 AM, Laine Stump wrote:
If a dhcp option has "force='yes'", the dhcp server will send that option back to every client regardless of whether or not the client requests that option (without "force='yes'", an option will only be sent to those clients that ask for it in their request packet). For example:
<option number='40' value='libvirt' force='yes'/>
This information is relayed to dnsmasq by using the "dhcp-option-force" option, rather than "dhcp-option". --- Changes from V1: * add forgotten addition to RNG * add forgotten addition to virNetworkIpDefFormat * add networkxml2xml test (which would have caught the above)
I'm sure there are useful symbolic names for the various options, instead of just number='40'; but like you said, that can be added later as needed. It's borderline on whether to take this patch without support for option names, since it was submitted prior to freeze but not reviewed until after. It is cleaning up the feature of <option> parsing that was just barely applied prior to freeze, so that argues that it can be classed as rounding out an incomplete feature. Delaying until post-release would let you get option names in the same release, but then we might want to revert the <option> support that just went in. Meanwhile, since both this patch, and any future patch for adding option name support, deals with just XML additions, they can easily be backported by distros without a soname bump, regardless of whether they make this release or not; and since it is just XML additions, I don't see how it could cause regressions to existing domain XML. I'm probably 60:40 in favor of including this patch now, but if anyone else speaks clearly for or against taking it in 1.0.3, I can be swayed.
+ <dt><code>option</code></dt> + <dd>The optional <code>option</code> element (which can + be repeated for multiple DHCP options) specifies + generic DHCP options (as defined in RFC 2132, or later + RFCs in some cases) that will be sent by the DHCP + server to requesting clients (IPv4 only). There are + three possible attributes: <code>number</code> is + mandatory and gives the standard option + number; <code>value</code> is optional and gives the + value of that option to be provided to the client + <code>force</code> is also optional, and defaults to + "no"; if <code>force</code> is "yes", the option will + be sent to all DHCP clients regardless of whether or + not the client requests it (usually only options + specifically requested by the client are sent in the + DHCP response).<span class="since">Since 1.0.3</span>
We definitely want the docs for <option>, even if we defer the force= attribute until post-release.
@@ -811,12 +812,25 @@ virNetworkDHCPOptionDefParseXML(const char *networkName, number); goto cleanup; } + option->value = virXMLPropString(node, "value");
+ if ((force = virXMLPropString(node, "force"))) { + if (STRCASEEQ(force, "yes")) {
I don't know that we have done case-insensitive matching in many other places; but being liberal in what we accept doesn't hurt my feelings. On the other hand, the RNG rejects any uppercase variant. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 25.02.2013 05:37, Laine Stump wrote:
The firs patch just fixes unexpected behavior in virNetworkDHCPOptionDefParseXML found when testing the second patch.
The second patch adds the "force" attribute I mentioned in the review of the patch that added support for specifying dhcp options in network config. While documenting this new attribute, I noticed that I'd neglected to see that original patch didn't include documentation, so I added full documentation for <option> in order to have a place to add the documentation for "force='yes'".
I'm also working on a way to allow specification of option names rather than numbers, but didn't know if I could get it in before the freeze, so I'm sending this patch now.
Laine Stump (2): conf: use VIR_EXPAND instead of VIR_REALLOC in virNetworkDHCPDefParseXML network: add force attribute for dhcp options
docs/formatnetwork.html.in | 21 ++++++++++++++++++++ src/conf/network_conf.c | 32 +++++++++++++++++++++--------- src/conf/network_conf.h | 3 ++- src/network/bridge_driver.c | 3 ++- tests/networkxml2confdata/nat-network.conf | 2 +- tests/networkxml2confdata/nat-network.xml | 2 +- 6 files changed, 50 insertions(+), 13 deletions(-)
ACK to both patches. Michal
participants (3)
-
Eric Blake -
Laine Stump -
Michal Privoznik