Re: [patch libteam 2/3] teamd: lacp: allow user to set LACP System ID via config (runner.system_id)

[Jiri Pirko]

Wed, Sep 19, 2018 at 05:27:25PM CEST, [e-mail] wrote:
> Normally lacp runner is using Aggreagtor's MAC for LACP System ID
> to be carried in LACPDUs.
> Make this value configurable by the user, so lacp runner will send
> custom LACP System ID for all actor ports of the team device.
> Signed-off-by: Alexander Petrovskiy
>
> man/teamd.conf.5          |  3 +++
> teamd/teamd_runner_lacp.c | 48 ++++++++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 48 insertions(+), 3 deletions(-)
>
> diff --git a/man/teamd.conf.5 b/man/teamd.conf.5
> index a252042..8862a4f 100644
> --- a/man/teamd.conf.5
> +++ b/man/teamd.conf.5
> @@ -227,6 +227,9 @@ Same as for load balance runner.
> .BR "runner.tx_balancer.balancing_interval " (int)
> Same as for load balance runner.
> .TP
> +.BR "runner.system_id " (string)
> +Desired System ID. Usual MAC address format is accepted.
> +.TP
> .BR "runner.sys_prio " (int)
> System priority, value can be 0 (en 65535.
> .RS 7
> diff --git a/teamd/teamd_runner_lacp.c b/teamd/teamd_runner_lacp.c
> index 7b8f0a7..c068028 100644
> --- a/teamd/teamd_runner_lacp.c
> +++ b/teamd/teamd_runner_lacp.c
> @@ -142,7 +142,9 @@ struct lacp {
> #define		LACP_CFG_DFLT_MIN_PORTS 1
>    	enum lacp_agg_select_policy agg_select_policy;
> #define		LACP_CFG_DFLT_AGG_SELECT_POLICY LACP_AGG_SELECT_LACP_PRIO
> } cfg;
> bool system_id_explicit;
> uint8_t system_id[ETH_ALEN]; /* ID */;
> } cfg;
> struct teamd_balancer *tb;
>
> };
>
> @@ -245,10 +247,35 @@ found:
>    return 0;
> }
>
> +static int lacp_parse_system_hwaddr(uint8_t *system, const char *system_str)
> +{
> int err;
> char *hwaddr;
> unsigned int hwaddr_len;

Reverse christmas tree ordering please:
******
****
**
> err = teamd_parse_hwaddr(system_str, &hwaddr, &hwaddr_len);
> if (err) {
> teamd_log_err("Failed to parse hardware address for system id.");
> return err;
> }
> if (hwaddr_len != ETH_ALEN) {
> teamd_log_err("Passed hardware address for system id has incorrect length (%d), expected: (%d).",
> 	hwaddr_len, ETH_ALEN);

This is not aligned correctly. Should be:
    	teamd_log_err("Passed hardware address for system id has incorrect length (%d), expected: (%d).",
    		      hwaddr_len, ETH_ALEN);
> err = -EINVAL;
> goto free_hwaddr;
> }
> memcpy(system, hwaddr, hwaddr_len);
>
> +free_hwaddr:
> free(hwaddr);
> return err;
>
> +}
>
> static int lacp_load_config(struct teamd_context *ctx, struct lacp *lacp)
> {
>    int err;
>    int tmp;
>
> const char *system_id;
> const char *agg_select_policy_name;
>
> err = teamd_config_bool_get(ctx, &lacp->cfg.active, "$.runner.active");
>
> @@ -256,6 +283,18 @@ static int lacp_load_config(struct teamd_context *ctx, struct lacp *lacp)
>    	lacp->cfg.active =  LACP_CFG_DFLT_ACTIVE;
>    teamd_log_dbg("Using active "%d".", lacp->cfg.active);
> err = teamd_config_string_get(ctx, &system_id, "$.runner.system_id");
> if (!err) {
> err = lacp_parse_system_hwaddr(lacp->cfg.system_id, system_id);
> if (err) {
> 	teamd_log_err("\"system_id\" value is invalid.");
> 	return -EINVAL;
> }
> lacp->cfg.system_id_explicit = true;
> }
> teamd_log_dbg("Using explicit system_id "%d", system_id value "%s".",
> lacp->cfg.system_id_explicit, system_id);

This log should reside withing the previous "if" boundaries.
Also, lacp->cfg.system_id_explicit is bool. I believe this a is
forgotten part from previous version.
> err = teamd_config_int_get(ctx, &tmp, "$.runner.sys_prio");
>  if (err) {
>   lacp->cfg.sys_prio = LACP_CFG_DFLT_SYS_PRIO;
>
> @@ -891,7 +930,10 @@ static void lacp_port_actor_system_update(struct lacp_port *lacp_port)
> {
>    struct lacpdu_info *actor = &lacp_port->actor;
> memcpy(actor->system, lacp_port->ctx->hwaddr, ETH_ALEN);
> if (lacp_port->lacp->cfg.system_id_explicit)
> memcpy(actor->system, lacp_port->lacp->cfg.system_id, ETH_ALEN);
> else
> memcpy(actor->system, lacp_port->ctx->hwaddr, ETH_ALEN);
>
> }
>
> static void lacp_port_actor_init(struct lacp_port *lacp_port)
> @@ -1046,7 +1088,7 @@ static int lacpdu_send(struct lacp_port *lacp_port)
>    ll_slow = ll_my;
>    memcpy(ll_slow.sll_addr, slow_addr, ll_slow.sll_halen);
> memcpy(lacp_port->actor.system, lacp_port->ctx->hwaddr, ETH_ALEN);
>
> lacp_port_actor_system_update(lacp_port);
>
> hwaddr = team_get_ifinfo_orig_hwaddr(lacp_port->tdport->team_ifinfo);
> hwaddr_len = team_get_ifinfo_orig_hwaddr_len(lacp_port->tdport->team_ifinfo);
>
> -- 
> 2.6.4