On Fri, Apr 09, 2021 at 10:28:21AM +0200, Martijn van Duren wrote:
> On Fri, 2021-04-09 at 09:47 +0200, Florian Obser wrote:
> > On Fri, Apr 09, 2021 at 09:41:24AM +0200, Florian Obser wrote:
> > > I think it would be better (and less ugly) to treat invalid values as
> > > if they had not been set. Could you pull the two checks up before
> > > if(renewal_time == 0) and do something like this:
> > >
> > > if (renewal_time >= rebinding_time) {
> > > log_warn(...)
> > > renewal_time = 0;
> > > }
> > > if (rebinding_time >= lease_time) {
> > > log_warn(...)
> > > rebinding_time = 0;
> > > }
> > >
> >
> > It might even be better to ignore both values if at least one is
> > invalid.
> >
> > if (renewal_time >= rebinding_time || rebinding_time >= lease_time) {
> > ...
> >
> > Since you are in there and have a device that causes trouble could you
> > give this a bit of thought? I'm currently swamped with other things
> > and won't have time until the evening.
> >
> Using your latter suggestion would loose information on what grounds the
> times are reset. If someone wants to do some proper work on their server
> side this is useful information. My diff below still works for me and
You took me too literally, what you have below is what I meant.
OK florian
> shows full information while still resetting both. I also changed the
> printf statement a little, since this reads a little better to me, but
> don´t mind changing it back if you feel strong about it.
>
> Since the lease time here is 24 hours I haven´t tested this yet with
> a renewal.
>
> Index: engine.c
> ===================================================================
> RCS file: /cvs/src/sbin/dhcpleased/engine.c,v
> retrieving revision 1.11
> diff -u -p -r1.11 engine.c
> --- engine.c 22 Mar 2021 15:34:07 -0000 1.11
> +++ engine.c 9 Apr 2021 08:26:37 -0000
> @@ -972,23 +972,24 @@ parse_dhcp(struct dhcpleased_iface *ifac
> }
>
> /* RFC 2131 4.4.5 */
> + /* Ignore invalid T1/T2 options */
> + if (renewal_time >= rebinding_time) {
> + log_warnx("%s: renewal_time(%u) >= rebinding_time(%u) "
> + "from %s: using defaults",
> + __func__, renewal_time, rebinding_time, from);
> + renewal_time = rebinding_time = 0;
> + } else if (rebinding_time >= lease_time) {
> + log_warnx("%s: rebinding_time(%u) >= lease_time(%u) "
> + "from %s: using defaults",
> + __func__, rebinding_time, lease_time, from);
> + renewal_time = rebinding_time = 0;
> + }
> +
> if(renewal_time == 0)
> renewal_time = lease_time / 2;
> if (rebinding_time == 0)
> rebinding_time = lease_time - (lease_time / 8);
>
> - if (renewal_time >= rebinding_time) {
> - log_warnx("%s: renewal_time >= rebinding_time "
> - "(%u >= %u) from %s", __func__, renewal_time,
> - rebinding_time, from);
> - return;
> - }
> - if (rebinding_time >= lease_time) {
> - log_warnx("%s: rebinding_time >= lease_time"
> - "(%u >= %u) from %s", __func__, rebinding_time,
> - lease_time, from);
> - return;
> - }
> clock_gettime(CLOCK_MONOTONIC, &iface->request_time);
> iface->server_identifier.s_addr = server_identifier.s_addr;
> iface->requested_ip.s_addr = dhcp_hdr->yiaddr.s_addr;
>
>
--
I'm not entirely sure you are real.
