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<https://fra01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.com%2Fsoft.at.home%2Fforks%2Fdnsmasq%2F-%2Ftree%2Fv2.85_ubus_patch&data=04%7C01%7Ceduardo.aguilar_ext%40softathome.com%7Cc63606d7c9624651ad5208d9300eb508%7Caa10e044e4054c10835336b4d0cce511%7C0%7C0%7C637596061179056720%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=RWK3zcs7kkE6QZXmGRqZA5%2Fng15N11xgM6gykhbMQrY%3D&reserved=0>
Please let me know about any feedback you may have.
Best Regards,
Eduardo.
_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk<mailto:Dnsmasq-discuss@lists.thekelleys.org.uk>
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss<https://fra01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.thekelleys.org.uk%2Fcgi-bin%2Fmailman%2Flistinfo%2Fdnsmasq-discuss&data=04%7C01%7Ceduardo.aguilar_ext%40softathome.com%7Cc63606d7c9624651ad5208d9300eb508%7Caa10e044e4054c10835336b4d0cce511%7C0%7C0%7C637596061179056720%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=PLfw0Ypb51CjaNOXf%2F%2BNnif8N26MSU0AmYQgClRdcys%3D&reserved=0>
_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
