On 21.02.2013 23:40, Pieter Hollants wrote:
This patch adds support for a new <option>-Tag in the
<dhcp> block of network configs,
based on a subset of the fifth proposal by Laine Stump in the mailing list discussion at
https://www.redhat.com/archives/libvir-list/2012-November/msg01054.html. Any such
defined
option will result in a dhcp-option=<number>,"<value>" statement in
the generated dnsmasq
configuration file.
Currently, DHCP options can be specified by number only and there is no whitelisting or
blacklisting of option numbers, which should probably be added.
Signed-off-by: Pieter Hollants <pieter(a)hollants.com>
---
AUTHORS.in | 1 +
docs/schemas/network.rng | 6 +++
docs/schemas/networkcommon.rng | 6 +++
src/conf/network_conf.c | 54 +++++++++++++++++++++
src/conf/network_conf.h | 10 ++++
src/network/bridge_driver.c | 10 ++++
tests/networkxml2xmlin/netboot-network.xml | 1 +
tests/networkxml2xmlin/netboot-proxy-network.xml | 1 +
tests/networkxml2xmlout/netboot-network.xml | 1 +
tests/networkxml2xmlout/netboot-proxy-network.xml | 1 +
10 Dateien geändert, 91 Zeilen hinzugefügt(+)
diff --git a/AUTHORS.in b/AUTHORS.in
index 39fe68d..074185e 100644
--- a/AUTHORS.in
+++ b/AUTHORS.in
@@ -74,6 +74,7 @@ Michel Ponceau <michel.ponceau(a)bull.net>
Nobuhiro Itou <fj0873gn(a)aa.jp.fujitsu.com>
Pete Vetere <pvetere(a)redhat.com>
Philippe Berthault <philippe.berthault(a)Bull.net>
+Pieter Hollants <pieter(a)hollants.com>
Saori Fukuta <fukuta.saori(a)jp.fujitsu.com>
Shigeki Sakamoto <fj0588di(a)aa.jp.fujitsu.com>
Shuveb Hussain <shuveb(a)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