Re: [PATCH] ndp: Don't send first periodic router advertisement right after guest connects

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 26 Nov 2024 15:22:38 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Nov 25, 2024 at 09:09:05AM +0100, Stefano Brivio wrote:
> >
> > [...]
> >
> > @@ -420,9 +420,13 @@ void ndp_timer(const struct ctx *c, const struct timespec *now)
> >  	interval = min_rtr_adv_interval +
> >  		random() % (max_rtr_adv_interval - min_rtr_adv_interval);
> >  
> > +	if (!next_ra)
> > +		goto first;  
> 
> I don't think avoiding re-indenting two lines is sufficient reason to
> introduce yet another goto, though..
> 
> > +
> >  	info("NDP: sending unsolicited RA, next in %llds", (long long)interval);
> >  
> >  	ndp_ra(c, &in6addr_ll_all_nodes);
> >  
> > +first:
> >  	next_ra = now->tv_sec + interval;
> >  }  

At the cost of one additional line (and zero non-blank lines):

--
$ git diff --patch --stat
 ndp.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/ndp.c b/ndp.c
index 37bf7a3..d1ba867 100644
--- a/ndp.c
+++ b/ndp.c
@@ -420,13 +420,12 @@ void ndp_timer(const struct ctx *c, const struct timespec *now)
        interval = min_rtr_adv_interval +
                random() % (max_rtr_adv_interval - min_rtr_adv_interval);
 
-       if (!next_ra)
-               goto first;
+       if (next_ra) {
+               info("NDP: sending unsolicited RA, next in %llds",
+                    (long long)interval);
 
-       info("NDP: sending unsolicited RA, next in %llds", (long long)interval);
-
-       ndp_ra(c, &in6addr_ll_all_nodes);
+               ndp_ra(c, &in6addr_ll_all_nodes);
+       }
 
-first:
        next_ra = now->tv_sec + interval;
 }
--

we get:

- clarity about the fact that 'next_ra' happens to be 0 on the *first*
  run (the label says it)

- clarity about the fact that it's a special case (it's a goto)

- no wrapped lines

-- 
@@ -420,13 +420,12 @@ void ndp_timer(const struct ctx *c, const struct timespec *now)
        interval = min_rtr_adv_interval +
                random() % (max_rtr_adv_interval - min_rtr_adv_interval);
 
-       if (!next_ra)
-               goto first;
+       if (next_ra) {
+               info("NDP: sending unsolicited RA, next in %llds",
+                    (long long)interval);
 
-       info("NDP: sending unsolicited RA, next in %llds", (long long)interval);
-
-       ndp_ra(c, &in6addr_ll_all_nodes);
+               ndp_ra(c, &in6addr_ll_all_nodes);
+       }
 
-first:
        next_ra = now->tv_sec + interval;
 }
--

we get:

- clarity about the fact that 'next_ra' happens to be 0 on the *first*
  run (the label says it)

- clarity about the fact that it's a special case (it's a goto)

- no wrapped lines

-- 
Stefano