[libvirt] [PATCH RESEND] Add support for <option> tag in network config
Michal Privoznik
mprivozn at redhat.com
Fri Feb 22 09:37:36 UTC 2013
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 at 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 at bull.net>
> Nobuhiro Itou <fj0873gn at aa.jp.fujitsu.com>
> Pete Vetere <pvetere at redhat.com>
> Philippe Berthault <philippe.berthault at Bull.net>
> +Pieter Hollants <pieter at hollants.com>
> Saori Fukuta <fukuta.saori at jp.fujitsu.com>
> Shigeki Sakamoto <fj0588di at aa.jp.fujitsu.com>
> Shuveb Hussain <shuveb at 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
More information about the libvir-list
mailing list