[Dnsmasq-discuss] [PATCH 2/2] Add D-Bus methods to add or remove a lease from the internal database.

[Simon Kelley]

On 05/06/15 11:47, Nicolas Cavallari wrote:
> On 04/06/2015 23:12, Simon Kelley wrote:
>> Long delay, I've returned to this.
>>
>> The many parameters seem a bit ugly (I'm no Dbus expert, so I may be
>> wrong), especially having to includes is_temporary and IAID in DHCPv4
>> leases. One solution to this might be to have seperate AddDhcp4Lease and
>>  AddDhcp6Lease methods.
> 
> I considered this, but this will likely add more code.  Either the two
> AddDhcp4Lease and AddDhcp6Lease are implemented separately, which
> double the code size, or they are handled by the same code with more
> conditionals.  I would prefer this solution over the others.
> 
> But since D-Bus is only an RPC, there are few simple solutions to pass
> data in an extensible way...
> 
>> Another option may be to remove the is_temporary flag and only allow
>> non-temporary leases to be created this way. Temporary leases are for
>> random addresses (like privacy addresses in SLAAC) so there may be no
>> need to be able to create them via this route.
> 
> This will only remove one argument ... and not much code.
> 
>> Final suggestion, which is more radical: just have one argument, which
>> is a string, and looks like a line in the leases file. It would be easy
>> to pass that to the parsing code in src/lease.c, saving much code.
> 
> I expect AddDhcpLease to also be able to extend or update an existing
> lease.  The code in src/lease.c does not handle this case correctly
> since it assumes the lease database is empty.
> 
>> Scripting may be easier too.
> 
> I don't see many cases where a lease script invocation calls
> AddDhcpLease on the same server for the same lease.  The lease script
> probably need to parse its input anyway.
> 
> To me, the most obstacle to simple scripting in the lease script is
> that AddDhcpLease also triggers another lease script invocation.

That's all true. You've thought about this more than I have. I'm happy
to go with your code.

> 
> I think that users which both uses the lease script and AddDhcpLease
> are probably doing something a bit advanced and must implement some
> not easily scriptable logic anyway.
> 
>> A request, would it be possible to have suitable updates to
>> dbus/DBus-interface in the patch, whatever scheme is finally done?
> 
> What updates to the D-Bus interface are you thinking of ? Since D-Bus
> messages are not extensible, it will be hard to update without
> breaking it for existing users.

Sorry, I wasn't clear. dbus/DBus-interface is a file in the dnsmasq tree
which documents the DBus methods. If you could add the new methods to
that and send the patch, that would 1) save me some time. 2) be more
likely correct, since you won't be reverse-engineering the new code like
I would be.


Cheers,


Simon.

>