Re: [Dnsmasq-discuss] [PATCH v2] Change dhcp_release to use default address when no IP subnet matches
Petr's patch applied also. Simon. On 01/10/2019 15:24, Petr Mensik wrote: > Hi Brian and Simon, > > I have one objection here. I think the tool should never end without > sending the request or showing some error. There is still one case left, > where it would do nothing and still exit without error. I admit it would > be rare, as IPv4 address has to be missing on given device. Anyway, I > think showing error and returning error code does not hurt. > > Patch attached. > > Cheers, > Petr > > On 8/30/19 10:22 PM, Simon Kelley wrote: >> That looks fine. Patch applied. >> >> >> Cheers, >> >> Simon. >> >> >> >> On 28/08/2019 21:13, haleyb@gmail.com wrote: >>> From: Brian Haley >>> >>> Currently, dhcp_release will only send a 'fake' release >>> when the address given is in the same subnet as an IP >>> on the interface that was given. >>> >>> This doesn't work in an environment where dnsmasq is >>> managing leases for remote subnets via a DHCP relay, as >>> running dhcp_release locally will just cause it to >>> silently exit without doing anything, leaving the lease >>> in the database. >>> >>> Change it to use the default IP on the interface, as the >>> dnsmasq source code at src/dhcp.c does, if no matching subnet >>> IP is found, as a fall-back. This fixes an issue we are >>> seeing in certain Openstack deployments where we are using >>> dnsmasq to provision baremetal systems in a datacenter. >>> >>> While using Dbus might have seemed like an obvious solution, >>> because of our extensive use of network namespaces (which >>> Dbus doesn't support), this seemed like a better solution >>> than creating system.d policy files for each dnsmasq we >>> might spawn and using --enable-dbus=$id in order to isolate >>> messages to specific dnsmasq instances. >>> >>> Signed-off-by: Brian Haley >>> --- >>> contrib/lease-tools/dhcp_release.c | 12 +--- >>> 1 file changed, 9 insertions(+), 3 deletions(-) >>> >>> diff --git a/contrib/lease-tools/dhcp_release.c >>> b/contrib/lease-tools/dhcp_release.c >>> index 59883d4..c866cd9 100644 >>> --- a/contrib/lease-tools/dhcp_release.c >>> +++ b/contrib/lease-tools/dhcp_release.c >>> @@ -178,7 +178,7 @@ static int is_same_net(struct in_addr a, struct in_addr >>> b, struct in_addr mask) >>>return (a.s_addr & mask.s_addr) == (b.s_addr & mask.s_addr); >>> } >>> >>> -static struct in_addr find_interface(struct in_addr client, int fd, >>> unsigned int index) >>> +static struct in_addr find_interface(struct in_addr client, int fd, >>> unsigned int index, int ifrfd, struct ifreq *ifr) >>> { >>>struct sockaddr_nl addr; >>>struct nlmsghdr *h; >>> @@ -218,7 +218,13 @@ static struct in_addr find_interface(struct in_addr >>> client, int fd, unsigned int >>> >>>for (h = (struct nlmsghdr *)iov.iov_base; NLMSG_OK(h, (size_t)len); >>> h = NLMSG_NEXT(h, len)) >>> if (h->nlmsg_type == NLMSG_DONE) >>> - exit(0); >>> + { >>> + /* No match found, return first address as src/dhcp.c code does */ >>> + ifr->ifr_addr.sa_family = AF_INET; >>> + if (ioctl(ifrfd, SIOCGIFADDR, ifr) != -1) >>> + return ((struct sockaddr_in *)>ifr_addr)->sin_addr; >>> + exit(0); >>> + } >>> else if (h->nlmsg_type == RTM_NEWADDR) >>>{ >>> struct ifaddrmsg *ifa = NLMSG_DATA(h); >>> @@ -285,7 +291,7 @@ int main(int argc, char **argv) >>> } >>> >>>lease.s_addr = inet_addr(argv[2]); >>> - server = find_interface(lease, nl, if_nametoindex(argv[1])); >>> + server = find_interface(lease, nl, if_nametoindex(argv[1]), fd, ); >>> >>>memset(, 0, sizeof(packet)); >>> >>> >> >> >> ___ >> Dnsmasq-discuss mailing list >> Dnsmasq-discuss@lists.thekelleys.org.uk >> http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss >> > > > ___ > Dnsmasq-discuss mailing list > Dnsmasq-discuss@lists.thekelleys.org.uk > http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss > ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [PATCH v2] Change dhcp_release to use default address when no IP subnet matches
On 10/1/19 10:24 AM, Petr Mensik wrote: Hi Brian and Simon, I have one objection here. I think the tool should never end without sending the request or showing some error. There is still one case left, where it would do nothing and still exit without error. I admit it would be rare, as IPv4 address has to be missing on given device. Anyway, I think showing error and returning error code does not hurt. Patch attached. LGTM, thanks Petr. -Brian On 8/30/19 10:22 PM, Simon Kelley wrote: That looks fine. Patch applied. Cheers, Simon. On 28/08/2019 21:13, haleyb@gmail.com wrote: From: Brian Haley Currently, dhcp_release will only send a 'fake' release when the address given is in the same subnet as an IP on the interface that was given. This doesn't work in an environment where dnsmasq is managing leases for remote subnets via a DHCP relay, as running dhcp_release locally will just cause it to silently exit without doing anything, leaving the lease in the database. Change it to use the default IP on the interface, as the dnsmasq source code at src/dhcp.c does, if no matching subnet IP is found, as a fall-back. This fixes an issue we are seeing in certain Openstack deployments where we are using dnsmasq to provision baremetal systems in a datacenter. While using Dbus might have seemed like an obvious solution, because of our extensive use of network namespaces (which Dbus doesn't support), this seemed like a better solution than creating system.d policy files for each dnsmasq we might spawn and using --enable-dbus=$id in order to isolate messages to specific dnsmasq instances. Signed-off-by: Brian Haley --- contrib/lease-tools/dhcp_release.c | 12 +--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/contrib/lease-tools/dhcp_release.c b/contrib/lease-tools/dhcp_release.c index 59883d4..c866cd9 100644 --- a/contrib/lease-tools/dhcp_release.c +++ b/contrib/lease-tools/dhcp_release.c @@ -178,7 +178,7 @@ static int is_same_net(struct in_addr a, struct in_addr b, struct in_addr mask) return (a.s_addr & mask.s_addr) == (b.s_addr & mask.s_addr); } -static struct in_addr find_interface(struct in_addr client, int fd, unsigned int index) +static struct in_addr find_interface(struct in_addr client, int fd, unsigned int index, int ifrfd, struct ifreq *ifr) { struct sockaddr_nl addr; struct nlmsghdr *h; @@ -218,7 +218,13 @@ static struct in_addr find_interface(struct in_addr client, int fd, unsigned int for (h = (struct nlmsghdr *)iov.iov_base; NLMSG_OK(h, (size_t)len); h = NLMSG_NEXT(h, len)) if (h->nlmsg_type == NLMSG_DONE) - exit(0); + { + /* No match found, return first address as src/dhcp.c code does */ + ifr->ifr_addr.sa_family = AF_INET; + if (ioctl(ifrfd, SIOCGIFADDR, ifr) != -1) + return ((struct sockaddr_in *)>ifr_addr)->sin_addr; + exit(0); + } else if (h->nlmsg_type == RTM_NEWADDR) { struct ifaddrmsg *ifa = NLMSG_DATA(h); @@ -285,7 +291,7 @@ int main(int argc, char **argv) } lease.s_addr = inet_addr(argv[2]); - server = find_interface(lease, nl, if_nametoindex(argv[1])); + server = find_interface(lease, nl, if_nametoindex(argv[1]), fd, ); memset(, 0, sizeof(packet)); ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [PATCH v2] Change dhcp_release to use default address when no IP subnet matches
Hi Brian and Simon, I have one objection here. I think the tool should never end without sending the request or showing some error. There is still one case left, where it would do nothing and still exit without error. I admit it would be rare, as IPv4 address has to be missing on given device. Anyway, I think showing error and returning error code does not hurt. Patch attached. Cheers, Petr On 8/30/19 10:22 PM, Simon Kelley wrote: > That looks fine. Patch applied. > > > Cheers, > > Simon. > > > > On 28/08/2019 21:13, haleyb@gmail.com wrote: >> From: Brian Haley >> >> Currently, dhcp_release will only send a 'fake' release >> when the address given is in the same subnet as an IP >> on the interface that was given. >> >> This doesn't work in an environment where dnsmasq is >> managing leases for remote subnets via a DHCP relay, as >> running dhcp_release locally will just cause it to >> silently exit without doing anything, leaving the lease >> in the database. >> >> Change it to use the default IP on the interface, as the >> dnsmasq source code at src/dhcp.c does, if no matching subnet >> IP is found, as a fall-back. This fixes an issue we are >> seeing in certain Openstack deployments where we are using >> dnsmasq to provision baremetal systems in a datacenter. >> >> While using Dbus might have seemed like an obvious solution, >> because of our extensive use of network namespaces (which >> Dbus doesn't support), this seemed like a better solution >> than creating system.d policy files for each dnsmasq we >> might spawn and using --enable-dbus=$id in order to isolate >> messages to specific dnsmasq instances. >> >> Signed-off-by: Brian Haley >> --- >> contrib/lease-tools/dhcp_release.c | 12 +--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/contrib/lease-tools/dhcp_release.c >> b/contrib/lease-tools/dhcp_release.c >> index 59883d4..c866cd9 100644 >> --- a/contrib/lease-tools/dhcp_release.c >> +++ b/contrib/lease-tools/dhcp_release.c >> @@ -178,7 +178,7 @@ static int is_same_net(struct in_addr a, struct in_addr >> b, struct in_addr mask) >>return (a.s_addr & mask.s_addr) == (b.s_addr & mask.s_addr); >> } >> >> -static struct in_addr find_interface(struct in_addr client, int fd, >> unsigned int index) >> +static struct in_addr find_interface(struct in_addr client, int fd, >> unsigned int index, int ifrfd, struct ifreq *ifr) >> { >>struct sockaddr_nl addr; >>struct nlmsghdr *h; >> @@ -218,7 +218,13 @@ static struct in_addr find_interface(struct in_addr >> client, int fd, unsigned int >> >>for (h = (struct nlmsghdr *)iov.iov_base; NLMSG_OK(h, (size_t)len); h >> = NLMSG_NEXT(h, len)) >> if (h->nlmsg_type == NLMSG_DONE) >> - exit(0); >> + { >> +/* No match found, return first address as src/dhcp.c code does */ >> +ifr->ifr_addr.sa_family = AF_INET; >> +if (ioctl(ifrfd, SIOCGIFADDR, ifr) != -1) >> + return ((struct sockaddr_in *)>ifr_addr)->sin_addr; >> +exit(0); >> + } >> else if (h->nlmsg_type == RTM_NEWADDR) >>{ >> struct ifaddrmsg *ifa = NLMSG_DATA(h); >> @@ -285,7 +291,7 @@ int main(int argc, char **argv) >> } >> >>lease.s_addr = inet_addr(argv[2]); >> - server = find_interface(lease, nl, if_nametoindex(argv[1])); >> + server = find_interface(lease, nl, if_nametoindex(argv[1]), fd, ); >> >>memset(, 0, sizeof(packet)); >> >> > > > ___ > Dnsmasq-discuss mailing list > Dnsmasq-discuss@lists.thekelleys.org.uk > http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss > -- Petr Menšík Software Engineer Red Hat, http://www.redhat.com/ email: pemen...@redhat.com PGP: 65C6C973 >From 8fda4b4620ca2b23152ca805d14c7cde1083fe31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Men=C5=A1=C3=ADk?= Date: Tue, 1 Oct 2019 16:08:28 +0200 Subject: [PATCH] Report error on dhcp_release If no IPv4 address is present on given interface, the tool would not send any request. It would not report any error at the same time. Report error if request send failed. Signed-off-by: Petr Mensik --- contrib/lease-tools/dhcp_release.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/contrib/lease-tools/dhcp_release.c b/contrib/lease-tools/dhcp_release.c index c866cd9..30e77c6 100644 --- a/contrib/lease-tools/dhcp_release.c +++ b/contrib/lease-tools/dhcp_release.c @@ -223,7 +223,11 @@ static struct in_addr find_interface(struct in_addr client, int fd, unsigned int ifr->ifr_addr.sa_family = AF_INET; if (ioctl(ifrfd, SIOCGIFADDR, ifr) != -1) return ((struct sockaddr_in *)>ifr_addr)->sin_addr; - exit(0); + else + { + fprintf(stderr, "error: local IPv4 address not found\n"); + exit(1); + } } else if (h->nlmsg_type == RTM_NEWADDR) { -- 2.20.1 ___
Re: [Dnsmasq-discuss] [PATCH v2] Change dhcp_release to use default address when no IP subnet matches
That looks fine. Patch applied. Cheers, Simon. On 28/08/2019 21:13, haleyb@gmail.com wrote: > From: Brian Haley > > Currently, dhcp_release will only send a 'fake' release > when the address given is in the same subnet as an IP > on the interface that was given. > > This doesn't work in an environment where dnsmasq is > managing leases for remote subnets via a DHCP relay, as > running dhcp_release locally will just cause it to > silently exit without doing anything, leaving the lease > in the database. > > Change it to use the default IP on the interface, as the > dnsmasq source code at src/dhcp.c does, if no matching subnet > IP is found, as a fall-back. This fixes an issue we are > seeing in certain Openstack deployments where we are using > dnsmasq to provision baremetal systems in a datacenter. > > While using Dbus might have seemed like an obvious solution, > because of our extensive use of network namespaces (which > Dbus doesn't support), this seemed like a better solution > than creating system.d policy files for each dnsmasq we > might spawn and using --enable-dbus=$id in order to isolate > messages to specific dnsmasq instances. > > Signed-off-by: Brian Haley > --- > contrib/lease-tools/dhcp_release.c | 12 +--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/contrib/lease-tools/dhcp_release.c > b/contrib/lease-tools/dhcp_release.c > index 59883d4..c866cd9 100644 > --- a/contrib/lease-tools/dhcp_release.c > +++ b/contrib/lease-tools/dhcp_release.c > @@ -178,7 +178,7 @@ static int is_same_net(struct in_addr a, struct in_addr > b, struct in_addr mask) >return (a.s_addr & mask.s_addr) == (b.s_addr & mask.s_addr); > } > > -static struct in_addr find_interface(struct in_addr client, int fd, unsigned > int index) > +static struct in_addr find_interface(struct in_addr client, int fd, unsigned > int index, int ifrfd, struct ifreq *ifr) > { >struct sockaddr_nl addr; >struct nlmsghdr *h; > @@ -218,7 +218,13 @@ static struct in_addr find_interface(struct in_addr > client, int fd, unsigned int > >for (h = (struct nlmsghdr *)iov.iov_base; NLMSG_OK(h, (size_t)len); h > = NLMSG_NEXT(h, len)) > if (h->nlmsg_type == NLMSG_DONE) > - exit(0); > + { > + /* No match found, return first address as src/dhcp.c code does */ > + ifr->ifr_addr.sa_family = AF_INET; > + if (ioctl(ifrfd, SIOCGIFADDR, ifr) != -1) > + return ((struct sockaddr_in *)>ifr_addr)->sin_addr; > + exit(0); > + } > else if (h->nlmsg_type == RTM_NEWADDR) >{ > struct ifaddrmsg *ifa = NLMSG_DATA(h); > @@ -285,7 +291,7 @@ int main(int argc, char **argv) > } > >lease.s_addr = inet_addr(argv[2]); > - server = find_interface(lease, nl, if_nametoindex(argv[1])); > + server = find_interface(lease, nl, if_nametoindex(argv[1]), fd, ); > >memset(, 0, sizeof(packet)); > > ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
[Dnsmasq-discuss] [PATCH v2] Change dhcp_release to use default address when no IP subnet matches
From: Brian Haley Currently, dhcp_release will only send a 'fake' release when the address given is in the same subnet as an IP on the interface that was given. This doesn't work in an environment where dnsmasq is managing leases for remote subnets via a DHCP relay, as running dhcp_release locally will just cause it to silently exit without doing anything, leaving the lease in the database. Change it to use the default IP on the interface, as the dnsmasq source code at src/dhcp.c does, if no matching subnet IP is found, as a fall-back. This fixes an issue we are seeing in certain Openstack deployments where we are using dnsmasq to provision baremetal systems in a datacenter. While using Dbus might have seemed like an obvious solution, because of our extensive use of network namespaces (which Dbus doesn't support), this seemed like a better solution than creating system.d policy files for each dnsmasq we might spawn and using --enable-dbus=$id in order to isolate messages to specific dnsmasq instances. Signed-off-by: Brian Haley --- contrib/lease-tools/dhcp_release.c | 12 +--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/contrib/lease-tools/dhcp_release.c b/contrib/lease-tools/dhcp_release.c index 59883d4..c866cd9 100644 --- a/contrib/lease-tools/dhcp_release.c +++ b/contrib/lease-tools/dhcp_release.c @@ -178,7 +178,7 @@ static int is_same_net(struct in_addr a, struct in_addr b, struct in_addr mask) return (a.s_addr & mask.s_addr) == (b.s_addr & mask.s_addr); } -static struct in_addr find_interface(struct in_addr client, int fd, unsigned int index) +static struct in_addr find_interface(struct in_addr client, int fd, unsigned int index, int ifrfd, struct ifreq *ifr) { struct sockaddr_nl addr; struct nlmsghdr *h; @@ -218,7 +218,13 @@ static struct in_addr find_interface(struct in_addr client, int fd, unsigned int for (h = (struct nlmsghdr *)iov.iov_base; NLMSG_OK(h, (size_t)len); h = NLMSG_NEXT(h, len)) if (h->nlmsg_type == NLMSG_DONE) - exit(0); + { + /* No match found, return first address as src/dhcp.c code does */ + ifr->ifr_addr.sa_family = AF_INET; + if (ioctl(ifrfd, SIOCGIFADDR, ifr) != -1) + return ((struct sockaddr_in *)>ifr_addr)->sin_addr; + exit(0); + } else if (h->nlmsg_type == RTM_NEWADDR) { struct ifaddrmsg *ifa = NLMSG_DATA(h); @@ -285,7 +291,7 @@ int main(int argc, char **argv) } lease.s_addr = inet_addr(argv[2]); - server = find_interface(lease, nl, if_nametoindex(argv[1])); + server = find_interface(lease, nl, if_nametoindex(argv[1]), fd, ); memset(, 0, sizeof(packet)); -- 2.17.1 ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss