dhcpv6-ia: send ubus notifications for DHCPv6 lease events#393
dhcpv6-ia: send ubus notifications for DHCPv6 lease events#393kempniu wants to merge 1 commit intoopenwrt:masterfrom
Conversation
Send ubus notifications for DHCPv6 leases, similarly to DHCPv4 leases: - "dhcp.lease6" when a DHCPv6 lease is created or renewed, - "dhcp.expire6" when a DHCPv6 lease expires or is released. A separate "dhcp.release6" notification is not implemented because dhcpv6_ia_handle_IAs() treats DHCPv6 RELEASE messages as immediate expiry, making it impossible to distinguish release from expiry without further code changes. Signed-off-by: Michał Kępień <michal@isc.org>
|
|
||
| a->bound = true; | ||
| apply_lease(a, true); | ||
| ubus_bcast_dhcpv6_event("dhcp.lease6", iface->ifname, a); |
There was a problem hiding this comment.
Unlike what README.md suggests, DHCPv4 code emits a dhcp.lease4 notification both when a new lease is created and when an existing lease is renewed.
I used the same logic for DHCPv6. If distinct notifications for renewals would be preferred, dhcp.lease6 needs to be replaced with a different string here.
| case DHCPV6_MSG_RELEASE: | ||
| /* Immediately expire the lease */ | ||
| a->valid_until = now - 1; | ||
| break; |
There was a problem hiding this comment.
As mentioned in the commit message, DHCPv6 code uses different logic for RELEASE messages than DHCPv4 code.
For DHCPv4, the released lease is immediately freed, just after the relevant ubus notification is sent. This naturally enables distinguishing between a release and an expiry as these two events cause different code paths to be taken.
For DHCPv6, a RELEASE message does not cause the lease to be immediately freed, but rather marked for immediate expiry. This makes it impossible to distinguish between a release and an expiry from within valid_until_cb(). I suspect it is done this way to make logging work nicely later in dhcpv6_ia_handle_IAs().
Of course this can be addressed by adding extra flags to struct dhcpv6_lease that would be set appropriately in the relevant scenarios, but I refrained from doing that for now to keep the changes minimal.
|
|
||
| static inline | ||
| void ubus_bcast_dhcpv6_event(const char *type, const char *iface, | ||
| struct dhcpv6_lease *lease) |
There was a problem hiding this comment.
While I tried to make this a 1:1 counterpart of ubus_bcast_dhcpv4_event(), the struct dhcpv6_lease *lease parameter does not have the const qualifier because ubus_bcast_dhcpv6_event() calls odhcpd_enum_addr6(), which in turn passes that structure pointer to a callback that does not have const in its prototype. Therefore, omitting the const qualifier here seemed to be the most sane path forward; it does not mean that ubus_bcast_dhcpv6_event() modifies the lease structure in any way.
| } | ||
|
|
||
| static inline | ||
| void ubus_bcast_dhcpv6_event(const char *type, const char *iface, |
There was a problem hiding this comment.
I prioritized consistency with pre-existing code; I believe the const char *iface parameter is superfluous here as it can be extracted from the lease structure. Nevertheless, I stuck to the same approach as used for the DHCPv4 counterpart for now.
There was a problem hiding this comment.
Does compile warn about an unused parameter?
There was a problem hiding this comment.
It does not, because the iface parameter is used in the code as currently proposed:
void ubus_bcast_dhcpv6_event(const char *type, const char *iface,
struct dhcpv6_lease *lease)
{
...
blobmsg_add_string(&b, "interface", iface);
...As pointed out above, I did it this way because that's what ubus_bcast_dhcpv4_event() does; however, AFAICT, iface could be replaced with lease->iface->ifname in both ubus_bcast_dhcpv4_event() and ubus_bcast_dhcpv6_event().
| blobmsg_close_table(&b, a); | ||
| } | ||
|
|
||
| static void dhcpv6_blobmsg_ia_addr_short(_o_unused struct dhcpv6_lease *lease, struct in6_addr *addr, uint8_t prefix_len, |
There was a problem hiding this comment.
This is basically a shorter version of dhcpv6_blobmsg_ia_addr(), which is present just above. These two functions can definitely be refactored to share common code, or maybe the DHCPv6 notifications can just use the original function and be done with it. Nevertheless, I opted for only supplying a minimal set of information in the ubus notifications for DHCPv6, just like the DHCPv4 code does.
| | `dhcp.lease6` | `ip,name,interface` | A new DHCPv6 lease has been created | | ||
| | `dhcp.expire6`| `ip,name,interface` | A DHCPv6 lease has expired or has been released by a client | |
There was a problem hiding this comment.
It's hard to be consistent here:
dhcp.lease4is also emitted for renewals, not just for new lease assignments,- the descriptions of DHCPv4 notifications are technically not 100% complete as these may also contain DUID/IAID data.
Once again, I opted for consistency between the two protocols.
| | `dhcp.lease6` | `ip,name,interface` | A new DHCPv6 lease has been created | | ||
| | `dhcp.expire6`| `ip,name,interface` | A DHCPv6 lease has expired or has been released by a client | | ||
|
|
||
| These can be observed by running e.g. `ubus listen dhcp` on your OpenWrt device. |
There was a problem hiding this comment.
Pre-existing issue: this statement is not true.
odhcpd uses the object-specific ubus subscribe/notify mechanism, which is a different beast than its event system. Running ubus listen dhcp won't show any DHCP-related events; ubus subscribe dhcp will, assuming that odhcpd is started before that command is run.
|
@Noltari this looks decent |
2 participants
Send ubus notifications for DHCPv6 leases, similarly to DHCPv4 leases:
A separate "dhcp.release6" notification is not implemented because dhcpv6_ia_handle_IAs() treats DHCPv6 RELEASE messages as immediate expiry, making it impossible to distinguish release from expiry without further code changes.
This is the same feature as proposed in #369, but implemented from scratch for consistency with odhcpd's existing concepts and code.
I tested this PR on a small home network and it seems to behave as advertised for address leases (
IA_NA).I openly admit that I have not tested this PR with DHCPv6-PD clients (
IA_PD).