[Babel-users] Revised [PATCH 1/2] Allow to independently monitor events for neighbour, interface, route, xroute

Juliusz Chroboczek jch at irif.fr
Mon Aug 12 14:08:58 BST 2019


Please don't use tabs.

> +	    if (strcmp(token2, "route") == 0)

Don't use space after the if.

> +	else // monitor was detected but none of the specialties - monitoring everything.
> +	    c = -1;
>       }

It's going to treat "monitor typo" as just "monitor".  This doesn't make
sense to me.

> +#define CONFIG_ACTION_NEIGHBOUR_MONITOR 6

Wouldn't MONITOR_NEIGBOUR be more logical?


> +        case CONFIG_ACTION_INTERFACE_MONITOR:
> +	    set_flag(&s->monitor, MONITOR_INTERFACE);
> +            local_notify_all_interface_1(s);
> +            break;
...

I suggest defining a separate function (say, monitor_mask) that maps the
constant to the flag.  Perhaps just an array index.  Then add a flags
value to local_notify_all_1 that specifies what you want to notify, and
say something like:

    case CONFIG_ACTION_INTERFACE_MONITOR:
    case CONFIG_ACTION_ROUTE_MONITOR:
    case ...
      mask = monitor_mask(rc);
      s->monitor |= mask;
      local_notify_all_1(s, mask);
      break;
    case CONFIG_ACTION_INTERFACE_UNMONITOR:
    case CONFIG_ACTION_ROUTE_UNMONITOR:
      s->monitof &= ~monitor_mask(rc);
      break;


> -    int monitor;
> +    uint8_t monitor;

Please make this an unsigned int.  We don't use stdint types in Babel (I'm
an old man).

-- Juliusz



More information about the Babel-users mailing list