[PATCH 0/3] network: Generate TFTP config regardless of DHCP
See 3/3 for explanation. Michal Prívozník (3): network: Initialize variables in networkDnsmasqConfContents() network: Separate DHCP config generator into a function network: Generate TFTP config regardless of DHCP src/network/bridge_driver.c | 262 ++++++++++-------- .../networkxml2confdata/netboot-network.conf | 4 +- tests/networkxml2confdata/netboot-tftp.conf | 13 + tests/networkxml2confdata/netboot-tftp.xml | 9 + tests/networkxml2conftest.c | 1 + tests/networkxml2xmlin/netboot-tftp.xml | 1 + tests/networkxml2xmlout/netboot-tftp.xml | 1 + tests/networkxml2xmltest.c | 1 + 8 files changed, 172 insertions(+), 120 deletions(-) create mode 100644 tests/networkxml2confdata/netboot-tftp.conf create mode 100644 tests/networkxml2confdata/netboot-tftp.xml create mode 120000 tests/networkxml2xmlin/netboot-tftp.xml create mode 120000 tests/networkxml2xmlout/netboot-tftp.xml -- 2.32.0
In networkDnsmasqConfContents() there's a for() loop which initializes some variables in its initialization block. This makes both the loop() statement and variable declaration block look needlessly ugly. Speaking of variable declaration, also move some variables which are used only within blocks into their respective blocks. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 7e5fab630b..2d8bebdd2f 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1057,16 +1057,14 @@ networkDnsmasqConfContents(virNetworkObj *obj, { virNetworkDef *def = virNetworkObjGetDef(obj); g_auto(virBuffer) configbuf = VIR_BUFFER_INITIALIZER; - int r; int nbleases = 0; size_t i; virNetworkDNSDef *dns = &def->dns; bool wantDNS = dns->enable != VIR_TRISTATE_BOOL_NO; - virNetworkIPDef *tmpipdef; - virNetworkIPDef *ipdef; - virNetworkIPDef *ipv4def; - virNetworkIPDef *ipv6def; - bool ipv6SLAAC; + virNetworkIPDef *ipdef = NULL; + virNetworkIPDef *ipv4def = NULL; + virNetworkIPDef *ipv6def = NULL; + bool ipv6SLAAC = false; *configstr = NULL; @@ -1186,6 +1184,8 @@ networkDnsmasqConfContents(virNetworkObj *obj, "interface=%s\n", def->bridge); } else { + virNetworkIPDef *tmpipdef = NULL; + virBufferAddLit(&configbuf, "bind-interfaces\n"); /* * --interface does not actually work with dnsmasq < 2.47, @@ -1312,9 +1312,7 @@ networkDnsmasqConfContents(virNetworkObj *obj, } /* Find the first dhcp for both IPv4 and IPv6 */ - for (i = 0, ipv4def = NULL, ipv6def = NULL, ipv6SLAAC = false; - (ipdef = virNetworkDefGetIPByIndex(def, AF_UNSPEC, i)); - i++) { + for (i = 0; (ipdef = virNetworkDefGetIPByIndex(def, AF_UNSPEC, i)); i++) { if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET)) { if (ipdef->nranges || ipdef->nhosts) { if (ipv4def) { @@ -1370,6 +1368,7 @@ networkDnsmasqConfContents(virNetworkObj *obj, while (ipdef) { int prefix; + int r; prefix = virNetworkIPDefPrefix(ipdef); if (prefix < 0) { -- 2.32.0
Generating configuration file for dnsmasq is done in networkDnsmasqConfContents() which is this big, self-contained function. Separate at least DHCP part into its own function for better readability. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 227 +++++++++++++++++++----------------- 1 file changed, 120 insertions(+), 107 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 2d8bebdd2f..0338ef502f 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1047,6 +1047,123 @@ networkDnsmasqConfLocalPTRs(virBuffer *buf, } +static int +networkDnsmasqConfDHCP(virBuffer *buf, + virNetworkIPDef *ipdef, + const char *bridge, + int *nbleases, + dnsmasqContext *dctx) +{ + int r; + int prefix; + + if (!ipdef) + return 0; + + prefix = virNetworkIPDefPrefix(ipdef); + if (prefix < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("bridge '%s' has an invalid prefix"), + bridge); + return -1; + } + for (r = 0; r < ipdef->nranges; r++) { + int thisRange; + virNetworkDHCPRangeDef range = ipdef->ranges[r]; + g_autofree char *leasetime = NULL; + g_autofree char *saddr = NULL; + g_autofree char *eaddr = NULL; + + if (!(saddr = virSocketAddrFormat(&range.addr.start)) || + !(eaddr = virSocketAddrFormat(&range.addr.end))) + return -1; + + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) { + virBufferAsprintf(buf, "dhcp-range=%s,%s,%d", + saddr, eaddr, prefix); + } else { + /* IPv4 - dnsmasq requires a netmask rather than prefix */ + virSocketAddr netmask; + g_autofree char *netmaskStr = NULL; + + if (virSocketAddrPrefixToNetmask(prefix, &netmask, AF_INET) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to translate bridge '%s' " + "prefix %d to netmask"), + bridge, prefix); + return -1; + } + + if (!(netmaskStr = virSocketAddrFormat(&netmask))) + return -1; + virBufferAsprintf(buf, "dhcp-range=%s,%s,%s", + saddr, eaddr, netmaskStr); + } + + if ((leasetime = networkBuildDnsmasqLeaseTime(range.lease))) + virBufferAsprintf(buf, ",%s", leasetime); + + virBufferAddLit(buf, "\n"); + + thisRange = virSocketAddrGetRange(&range.addr.start, + &range.addr.end, + &ipdef->address, + virNetworkIPDefPrefix(ipdef)); + if (thisRange < 0) + return -1; + *nbleases += thisRange; + } + + /* + * For static-only DHCP, i.e. with no range but at least one + * host element, we have to add a special --dhcp-range option + * to enable the service in dnsmasq. (this is for dhcp-hosts= + * support) + */ + if (!ipdef->nranges && ipdef->nhosts) { + g_autofree char *bridgeaddr = virSocketAddrFormat(&ipdef->address); + if (!bridgeaddr) + return -1; + virBufferAsprintf(buf, "dhcp-range=%s,static", + bridgeaddr); + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) + virBufferAsprintf(buf, ",%d", prefix); + virBufferAddLit(buf, "\n"); + } + + if (networkBuildDnsmasqDhcpHostsList(dctx, ipdef) < 0) + return -1; + + /* Note: the following is IPv4 only */ + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET)) { + if (ipdef->nranges || ipdef->nhosts) { + virBufferAddLit(buf, "dhcp-no-override\n"); + virBufferAddLit(buf, "dhcp-authoritative\n"); + } + + if (ipdef->tftproot) { + virBufferAddLit(buf, "enable-tftp\n"); + virBufferAsprintf(buf, "tftp-root=%s\n", ipdef->tftproot); + } + + if (ipdef->bootfile) { + if (VIR_SOCKET_ADDR_VALID(&ipdef->bootserver)) { + g_autofree char *bootserver = virSocketAddrFormat(&ipdef->bootserver); + + if (!bootserver) + return -1; + virBufferAsprintf(buf, "dhcp-boot=%s%s%s\n", + ipdef->bootfile, ",,", bootserver); + } else { + virBufferAsprintf(buf, "dhcp-boot=%s\n", ipdef->bootfile); + } + } + } + + return 0; +} + + int networkDnsmasqConfContents(virNetworkObj *obj, const char *pidfile, @@ -1364,113 +1481,9 @@ networkDnsmasqConfContents(virNetworkObj *obj, "on the same network interface."); } - ipdef = ipv4def ? ipv4def : ipv6def; - - while (ipdef) { - int prefix; - int r; - - prefix = virNetworkIPDefPrefix(ipdef); - if (prefix < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("bridge '%s' has an invalid prefix"), - def->bridge); - return -1; - } - for (r = 0; r < ipdef->nranges; r++) { - int thisRange; - virNetworkDHCPRangeDef range = ipdef->ranges[r]; - g_autofree char *leasetime = NULL; - g_autofree char *saddr = NULL; - g_autofree char *eaddr = NULL; - - if (!(saddr = virSocketAddrFormat(&range.addr.start)) || - !(eaddr = virSocketAddrFormat(&range.addr.end))) - return -1; - - if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) { - virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%d", - saddr, eaddr, prefix); - } else { - /* IPv4 - dnsmasq requires a netmask rather than prefix */ - virSocketAddr netmask; - g_autofree char *netmaskStr = NULL; - - if (virSocketAddrPrefixToNetmask(prefix, &netmask, AF_INET) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to translate bridge '%s' " - "prefix %d to netmask"), - def->bridge, prefix); - return -1; - } - - if (!(netmaskStr = virSocketAddrFormat(&netmask))) - return -1; - virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%s", - saddr, eaddr, netmaskStr); - } - - if ((leasetime = networkBuildDnsmasqLeaseTime(range.lease))) - virBufferAsprintf(&configbuf, ",%s", leasetime); - - virBufferAddLit(&configbuf, "\n"); - - thisRange = virSocketAddrGetRange(&range.addr.start, - &range.addr.end, - &ipdef->address, - virNetworkIPDefPrefix(ipdef)); - if (thisRange < 0) - return -1; - nbleases += thisRange; - } - - /* - * For static-only DHCP, i.e. with no range but at least one - * host element, we have to add a special --dhcp-range option - * to enable the service in dnsmasq. (this is for dhcp-hosts= - * support) - */ - if (!ipdef->nranges && ipdef->nhosts) { - g_autofree char *bridgeaddr = virSocketAddrFormat(&ipdef->address); - if (!bridgeaddr) - return -1; - virBufferAsprintf(&configbuf, "dhcp-range=%s,static", - bridgeaddr); - if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) - virBufferAsprintf(&configbuf, ",%d", prefix); - virBufferAddLit(&configbuf, "\n"); - } - - if (networkBuildDnsmasqDhcpHostsList(dctx, ipdef) < 0) - return -1; - - /* Note: the following is IPv4 only */ - if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET)) { - if (ipdef->nranges || ipdef->nhosts) { - virBufferAddLit(&configbuf, "dhcp-no-override\n"); - virBufferAddLit(&configbuf, "dhcp-authoritative\n"); - } - - if (ipdef->tftproot) { - virBufferAddLit(&configbuf, "enable-tftp\n"); - virBufferAsprintf(&configbuf, "tftp-root=%s\n", ipdef->tftproot); - } - - if (ipdef->bootfile) { - if (VIR_SOCKET_ADDR_VALID(&ipdef->bootserver)) { - g_autofree char *bootserver = virSocketAddrFormat(&ipdef->bootserver); - - if (!bootserver) - return -1; - virBufferAsprintf(&configbuf, "dhcp-boot=%s%s%s\n", - ipdef->bootfile, ",,", bootserver); - } else { - virBufferAsprintf(&configbuf, "dhcp-boot=%s\n", ipdef->bootfile); - } - } - } - ipdef = (ipdef == ipv6def) ? NULL : ipv6def; - } + if (networkDnsmasqConfDHCP(&configbuf, ipv4def, def->bridge, &nbleases, dctx) < 0 || + networkDnsmasqConfDHCP(&configbuf, ipv6def, def->bridge, &nbleases, dctx) < 0) + return -1; if (nbleases > 0) virBufferAsprintf(&configbuf, "dhcp-lease-max=%d\n", nbleases); -- 2.32.0
We already allow users to provide TFTP root path in network XML and not specify any DHCP. This makes sense, because dnsmasq is not only DHCP server but also TFTP server and users might have a DHCP server configured on their own, outside of libvirt's control and want just the TFTP part. By moving TFTP config generator out of DHCP generator and calling it for every IPv4 range, users can finally enable just TFTP. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2026765 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 30 ++++++++++++++----- .../networkxml2confdata/netboot-network.conf | 4 +-- tests/networkxml2confdata/netboot-tftp.conf | 13 ++++++++ tests/networkxml2confdata/netboot-tftp.xml | 9 ++++++ tests/networkxml2conftest.c | 1 + tests/networkxml2xmlin/netboot-tftp.xml | 1 + tests/networkxml2xmlout/netboot-tftp.xml | 1 + tests/networkxml2xmltest.c | 1 + 8 files changed, 50 insertions(+), 10 deletions(-) create mode 100644 tests/networkxml2confdata/netboot-tftp.conf create mode 100644 tests/networkxml2confdata/netboot-tftp.xml create mode 120000 tests/networkxml2xmlin/netboot-tftp.xml create mode 120000 tests/networkxml2xmlout/netboot-tftp.xml diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 0338ef502f..b26b44ac01 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1141,11 +1141,6 @@ networkDnsmasqConfDHCP(virBuffer *buf, virBufferAddLit(buf, "dhcp-authoritative\n"); } - if (ipdef->tftproot) { - virBufferAddLit(buf, "enable-tftp\n"); - virBufferAsprintf(buf, "tftp-root=%s\n", ipdef->tftproot); - } - if (ipdef->bootfile) { if (VIR_SOCKET_ADDR_VALID(&ipdef->bootserver)) { g_autofree char *bootserver = virSocketAddrFormat(&ipdef->bootserver); @@ -1164,6 +1159,22 @@ networkDnsmasqConfDHCP(virBuffer *buf, } +static void +networkDnsmasqConfTFTP(virBuffer *buf, + virNetworkIPDef *ipdef, + bool *enableTFTP) +{ + if (!ipdef->tftproot) + return; + + if (!*enableTFTP) { + virBufferAddLit(buf, "enable-tftp\n"); + *enableTFTP = true; + } + virBufferAsprintf(buf, "tftp-root=%s\n", ipdef->tftproot); +} + + int networkDnsmasqConfContents(virNetworkObj *obj, const char *pidfile, @@ -1182,6 +1193,7 @@ networkDnsmasqConfContents(virNetworkObj *obj, virNetworkIPDef *ipv4def = NULL; virNetworkIPDef *ipv6def = NULL; bool ipv6SLAAC = false; + bool enableTFTP = false; *configstr = NULL; @@ -1441,6 +1453,8 @@ networkDnsmasqConfContents(virNetworkObj *obj, ipv4def = ipdef; } } + + networkDnsmasqConfTFTP(&configbuf, ipdef, &enableTFTP); } if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) { if (ipdef->nranges || ipdef->nhosts) { @@ -1619,7 +1633,7 @@ networkStartDhcpDaemon(virNetworkDriverState *driver, i = 0; while ((ipdef = virNetworkDefGetIPByIndex(def, AF_UNSPEC, i))) { i++; - if (ipdef->nranges || ipdef->nhosts) + if (ipdef->nranges || ipdef->nhosts || ipdef->tftproot) needDnsmasq = true; } @@ -3667,7 +3681,7 @@ networkUpdate(virNetworkPtr net, for (i = 0; (ipdef = virNetworkDefGetIPByIndex(def, AF_INET, i)); i++) { - if (ipdef->nranges || ipdef->nhosts) { + if (ipdef->nranges || ipdef->nhosts || ipdef->tftproot) { oldDhcpActive = true; break; } @@ -3782,7 +3796,7 @@ networkUpdate(virNetworkPtr net, for (i = 0; (ipdef = virNetworkDefGetIPByIndex(def, AF_INET, i)); i++) { - if (ipdef->nranges || ipdef->nhosts) { + if (ipdef->nranges || ipdef->nhosts || ipdef->tftproot) { newDhcpActive = true; break; } diff --git a/tests/networkxml2confdata/netboot-network.conf b/tests/networkxml2confdata/netboot-network.conf index 99272b9d68..a429663f8b 100644 --- a/tests/networkxml2confdata/netboot-network.conf +++ b/tests/networkxml2confdata/netboot-network.conf @@ -10,11 +10,11 @@ expand-hosts except-interface=lo bind-interfaces listen-address=192.168.122.1 +enable-tftp +tftp-root=/var/lib/tftproot dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0 dhcp-no-override dhcp-authoritative -enable-tftp -tftp-root=/var/lib/tftproot dhcp-boot=pxeboot.img dhcp-lease-max=253 dhcp-hostsfile=/var/lib/libvirt/dnsmasq/netboot.hostsfile diff --git a/tests/networkxml2confdata/netboot-tftp.conf b/tests/networkxml2confdata/netboot-tftp.conf new file mode 100644 index 0000000000..45615f3c33 --- /dev/null +++ b/tests/networkxml2confdata/netboot-tftp.conf @@ -0,0 +1,13 @@ +##WARNING: THIS IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE +##OVERWRITTEN AND LOST. Changes to this configuration should be made using: +## virsh net-edit tftp-only +## or other application using the libvirt API. +## +## dnsmasq conf file created by libvirt +strict-order +except-interface=lo +bind-dynamic +interface=virbr0 +enable-tftp +tftp-root=/var/lib/tftproot +addn-hosts=/var/lib/libvirt/dnsmasq/tftp-only.addnhosts diff --git a/tests/networkxml2confdata/netboot-tftp.xml b/tests/networkxml2confdata/netboot-tftp.xml new file mode 100644 index 0000000000..297f5a7ba1 --- /dev/null +++ b/tests/networkxml2confdata/netboot-tftp.xml @@ -0,0 +1,9 @@ +<network> + <name>tftp-only</name> + <uuid>eb486e5c-4df5-42ee-ae4a-ad8557998d00</uuid> + <forward mode='nat'/> + <bridge name='virbr0' stp='off' delay='1'/> + <ip address='192.168.122.1' netmask='255.255.255.0'> + <tftp root='/var/lib/tftproot'/> + </ip> +</network> diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index b76d72793a..a19449ea2b 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -137,6 +137,7 @@ mymain(void) DO_TEST("isolated-network", restricted); DO_TEST("netboot-network", restricted); DO_TEST("netboot-proxy-network", restricted); + DO_TEST("netboot-tftp", full); DO_TEST("nat-network-dns-srv-record-minimal", restricted); DO_TEST("nat-network-name-with-quotes", restricted); DO_TEST("routed-network", full); diff --git a/tests/networkxml2xmlin/netboot-tftp.xml b/tests/networkxml2xmlin/netboot-tftp.xml new file mode 120000 index 0000000000..1487de558b --- /dev/null +++ b/tests/networkxml2xmlin/netboot-tftp.xml @@ -0,0 +1 @@ +../networkxml2confdata/netboot-tftp.xml \ No newline at end of file diff --git a/tests/networkxml2xmlout/netboot-tftp.xml b/tests/networkxml2xmlout/netboot-tftp.xml new file mode 120000 index 0000000000..1487de558b --- /dev/null +++ b/tests/networkxml2xmlout/netboot-tftp.xml @@ -0,0 +1 @@ +../networkxml2confdata/netboot-tftp.xml \ No newline at end of file diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c index ca24305ace..9e8d675a10 100644 --- a/tests/networkxml2xmltest.c +++ b/tests/networkxml2xmltest.c @@ -127,6 +127,7 @@ mymain(void) DO_TEST("nat-network"); DO_TEST("netboot-network"); DO_TEST("netboot-proxy-network"); + DO_TEST("netboot-tftp"); DO_TEST("nat-network-dns-txt-record"); DO_TEST("nat-network-dns-srv-record"); DO_TEST("nat-network-dns-srv-records"); -- 2.32.0
On 12/9/21 17:17, Michal Privoznik wrote:
See 3/3 for explanation.
Michal Prívozník (3): network: Initialize variables in networkDnsmasqConfContents() network: Separate DHCP config generator into a function network: Generate TFTP config regardless of DHCP
src/network/bridge_driver.c | 262 ++++++++++-------- .../networkxml2confdata/netboot-network.conf | 4 +- tests/networkxml2confdata/netboot-tftp.conf | 13 + tests/networkxml2confdata/netboot-tftp.xml | 9 + tests/networkxml2conftest.c | 1 + tests/networkxml2xmlin/netboot-tftp.xml | 1 + tests/networkxml2xmlout/netboot-tftp.xml | 1 + tests/networkxml2xmltest.c | 1 + 8 files changed, 172 insertions(+), 120 deletions(-) create mode 100644 tests/networkxml2confdata/netboot-tftp.conf create mode 100644 tests/networkxml2confdata/netboot-tftp.xml create mode 120000 tests/networkxml2xmlin/netboot-tftp.xml create mode 120000 tests/networkxml2xmlout/netboot-tftp.xml
ping? Michal
participants (2)
-
Michal Privoznik -
Michal Prívozník