On Fri, Sep 27, 2013 at 01:28:06PM -0700, Paul Marks wrote:

On Fri, Sep 27, 2013 at 1:16 AM, Hannes Frederic Sowa
[off-list ref] wrote:

quoted

On Wed, Sep 25, 2013 at 03:12:55PM -0700, Paul Marks wrote:

quoted

-                                     if (prefered_lft != ifp->prefered_lft) {

Wouldn't the easiest solution be to just drop this if and execute the two
lines below unconditionally?

Yes, that's also correct.  But is it not better to have simpler code
than shorter diffs?  Should we transliterate English to C, or think
about what the algorithm is actually doing?  The fact that this bug
has gone unnoticed provides some evidence that the code may have been
too complicated.

I don't care about the length of diffs or shorter code. I would favour
a transliteration here because it makes verification easier (at least
for me). The algorithm is not that complex and I guess the bug has been
unnoticed because nobody ran into problems and cared til now.

So, why not get rid of update_lft then?

quoted

quoted

+                             const u32 minimum_lft = min(
+                                     stored_lft, (u32)MIN_VALID_LIFETIME);
+                             valid_lft = max(valid_lft, minimum_lft);

Quick question: Don't we need a prefered_lft = min(preferred_lft, valid_lft)
here?

The invariant is (preferred_lft <= valid_lft), and valid_lft can only
get bigger, so I don't think there's a problem.

Ah, I got confused. Missed in the last case that it got tested earlier in the
function. Your code looks correct regarding every rule.

Acked-by: Hannes Frederic Sowa <redacted>

Thanks,

  Hannes