I'm not keen on storing a load of information with a lease on exactly
what option information was returned the last time the lease was
renewed. That's a big overhead when there are lots of leases and goes
directly against the design decision that such information gets send out
to an external process and then deleted internally. (That's the
lease->extradata). I can't see this being popular with the openWRT
people if compiling and configuring Ubus support increases the memory
footprint of dnsmasq.
One compromise may be to make keeping the extra information a separate
configuration option.
The comments below don't refer to your patches, but more to the way UBus
support is done in general. Reviewing your code made me look at the
existing Ubus code and some of it seems a little strange.
The ubus code seems to hook into the log_packet() routine to send
information over ubus on a DHCPACK or DHCPRELEASE. It may be rather more
sensible to hook the DHCP lease database and send "lease-created" and
"lease-destroyed" messages instead. There's a perfect template for this,
since it's exactly what DBus does. Look in do_script_run() in lease.c
for calls to emit_dbus_signal(). Of course that's incompatible with any
existing uses of the Ubus interface, so that chance may have been missed.
At least I think the call to ubus should be in the body of dhcp_reply()
and not bounce through log_packet()
Cheers,
Simon.
On 02/07/2021 18:20, Eduardo AGUILAR wrote:
> Hello,
>
> I send a new version of the patch simplified. It is based on current master
> (9d806c51c25a23d64df607c8d7a88089745f963a).
>
> Brief explanation:
> This adds a new method for the dnsmasq ubus object 'leases', when calling
> this method it will return an ubus data structure with all the available
> leases stored.
>
> Apart from this, the DHCP options from the request packet are saved into a
> linked list to be sent in "dhcp.ack" event and in the 'leases' method.
>
> Perhaps some of these changes could be shared or refactored for other
> features that exist in dnsmasq (HAVE_SCRIPT, DBUS...).
>
> Best Regards,
> Eduardo.
>
>
>
>
> From: Eduardo AGUILAR <eduardo.aguilar_...@softathome.com>
> Sent: Monday, June 21, 2021 8:56 AM
> To: dnsmasq-discuss@lists.thekelleys.org.uk
> <dnsmasq-discuss@lists.thekelleys.org.uk>
> Subject: Re: [Dnsmasq-discuss] [PATCH] Expose more lease data through ubus
>
> Hi Petr,
>
> Thanks for the review.
>
> The patch adds a method that can be called through ubus "dnsmasq.leases()"
> which iterates the leases linked list and shows the available information on
> them. Then also extend the information in existing ubus events when a DHCP
> packet is processed. I will add more information on the patch commit
> commentary perhaps.
>
> You are right regarding the ACK options, perhaps I can derive the data in
> other way.
>
> Regarding the location of the ubus_event_bcast calls I also thought of
> implementing as you propose, but currently it is called inside the log_packet
> call. Perhaps getting the ubus_event_bcast out will simplify the patch too.
>
> I was also wondering that the variable "extradata" may be used for the same
> purpose, and if so, maybe it would make sense to remake the patch using that
> variable instead.
>
> I will make these changes and submit a new patch based on master.
>
> Best Regards,
> Eduardo.
>
> From: Dnsmasq-discuss <dnsmasq-discuss-boun...@lists.thekelleys.org.uk> on
> behalf of Petr Menšík <pemen...@redhat.com>
> Sent: Tuesday, June 15, 2021 4:26 PM
> To: dnsmasq-discuss@lists.thekelleys.org.uk
> <dnsmasq-discuss@lists.thekelleys.org.uk>
> Subject: Re: [Dnsmasq-discuss] [PATCH] Expose more lease data through ubus
>
> Hi Eduardo,
> can you rebase it to master branch?
> Do you have any readme or summary, how that feature would be used?
> My first note is it adds HAVE_UBUS define to lease structure, but it seems
> more or less unrelated to ubus itself. I could imagine similar mechanism
> might be interesting also for d-bus interface eventually.
> Why does it log ACK options? Can they be different for known host? I think
> logging network tag would help, but otherwise sent options should be constant
> according to configuration. Should it store those flags again?
> First problem is it always allocates extra info for the lease. I think it
> should try it only when --enable-ubus is used in configuration. I would move
> separate net member into ubus_extra_info structure. And move its defintion
> outside lease structure, use in struct lease {} just:
> struct ubus_extra_info *ubus_extra_info;
> I would use functions to free and reset ubus_extra_info inside ubus.c. It
> might not need to know structure definition of ubus_extra_info. Avoid complex
> ubus_delete_options() and free in lease.c, move it to function called
> ubus_free_extra_info(lease->ubus_extra_info);
> Move implementation details to ubus, since those details are only used there.
> I don't like extending log_packet by lease, just because only 3 types are
> logged with details.
> Why is used (string ? string : NULL) ?? use just string. It is either
> non-NULL or NULL anyway. I don't like assigning xid in packet_log function.
> It should only log, not change the records also. Is DHCPREQUEST handled by
> if's, but not supplied with lease intentional? I would expect options from
> request to be the most important to log.
> I would use it like this, since it should not log every message:
> log_packet("DHCPREQUEST", ...);
> #ifdef HAVE_UBUS
> ubus_event_bcast("dhcp.request", daemon->namebuff,
> inet_ntoa(&mess->yiaddr), NULL, iface_name, lease);
> #endif
> Just might thoughts after quick patch review.
> Cheers,
> Petr
> On 6/11/21 11:59 AM, Eduardo AGUILAR wrote:
> Hi everyone,
>
> I tried emailing Simon directly but he seems absent.
>
> I send a patch we are using at Soft at Home to retrieve more information
> about leases using ubus. We would like to have this upstreamed if possible.
>
> We are maintaining these changes at
> https://gitlab.com/soft.at.home/forks/dnsmasq/-/tree/v2.85_ubus_patch
>
> Please let me know about any feedback you may have.
>
> Best Regards,
> Eduardo.
>
>
>
> _______________________________________________
> Dnsmasq-discuss mailing list
> Dnsmasq-discuss@lists.thekelleys.org.uk
> https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
>
>
> _______________________________________________
> Dnsmasq-discuss mailing list
> Dnsmasq-discuss@lists.thekelleys.org.uk
> https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
>
_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
