I sent a v2 patch with all your nits fixed.
Thanks
On Mon, 2023-03-13 at 13:13 +0100, Jiri Pirko wrote:
Fri, Dec 23, 2022 at 07:56:33PM CET, otto.hollmann(a)suse.com wrote:
> Now, if multiple link watchers are used, link is up if any of the
> link-watchers reports the link up.
>
> Introduce new option "lw_policy" to change this behaviour. Possible
values
> are "any" and "all". If nothing specified, default value
"any" will be
> used and there will be no change in current behaviour. If value "all"
will
> be set, link will be up only if ALL the link-watchers report the link up.
In general, this looks fine.
Couple of nits below:
>
> Signed-off-by: Otto Hollmann <otto.hollmann(a)suse.com>
> ---
> .../activebackup_multi_lw_1.conf | 1 +
> teamd/teamd.c | 28 +++++++++++++++++++
> teamd/teamd.h | 1 +
> teamd/teamd_link_watch.c | 27 ++++++++++++++----
You are missing manpage edit here.
> 4 files changed, 52 insertions(+), 5 deletions(-)
>
> diff --git a/teamd/example_configs/activebackup_multi_lw_1.conf
> b/teamd/example_configs/activebackup_multi_lw_1.conf
> index c1645d2..c66e6d7 100644
> --- a/teamd/example_configs/activebackup_multi_lw_1.conf
> +++ b/teamd/example_configs/activebackup_multi_lw_1.conf
> @@ -1,6 +1,7 @@
> {
> "device": "team0",
> "runner": {"name": "activebackup"},
> + "lw_policy": "any",
We have "link_watch" with no abbr. Please name this
"link_watch_policy".
It's a bit longer, but consistent.
> "link_watch": [
> {
> "name": "arp_ping",
Please add nother example file with "all".
> diff --git a/teamd/teamd.c b/teamd/teamd.c
> index a89b702..a8a347f 100644
> --- a/teamd/teamd.c
> +++ b/teamd/teamd.c
> @@ -1805,6 +1805,30 @@ static int teamd_drop_privileges()
>
> #endif
>
> +static int teamd_get_lw_policy(struct teamd_context *ctx)
> +{
> + int err;
> + const char *lw_policy;
> +
> + err = teamd_config_string_get(ctx, &lw_policy,
"$.lw_policy");
> +
Empty line not needed here, remove.
> + if (!err) {
> + if (!strcmp(lw_policy, "all")) {
> + ctx->evaluate_all_watchers = true;
> + } else if (!strcmp(lw_policy, "any")) {
> + ctx->evaluate_all_watchers = false;
> + } else {
> + teamd_log_err("Unrecognized value for
lw_policy.");
> + teamd_log_err("Only \"any\" or
\"all\" are allowed
> but
> \"%s\" found in config.", lw_policy);
Hmm, this is an odd line break. Email client broken? Perhaps better to
use git-send-email instead.
> + return -EINVAL;
> + }
> + } else {
> + ctx->evaluate_all_watchers = false;
Struct is zeroed (see teamd_context_init()), why do you need to
have this assign?
> + teamd_log_dbg(ctx, "No lw_policy specified in config, using
> default value \"any\".");
> + }
> + return 0;
> +}
> +
> int main(int argc, char **argv)
> {
> enum teamd_exit_code ret = TEAMD_EXIT_FAILURE;
> @@ -1863,6 +1887,10 @@ int main(int argc, char **argv)
> if (err)
> goto config_free;
>
> + err = teamd_get_lw_policy(ctx);
> + if (err)
> + goto config_free;
> +
> err = teamd_set_default_pid_file(ctx);
> if (err)
> goto config_free;
> diff --git a/teamd/teamd.h b/teamd/teamd.h
> index 541d2a7..4ca40a1 100644
> --- a/teamd/teamd.h
> +++ b/teamd/teamd.h
> @@ -110,6 +110,7 @@ struct teamd_context {
> json_t * config_json;
> char * pid_file;
> char * team_devname;
> + bool evaluate_all_watchers;
I would rather see this amond the rest of the bools a bit above this.
Or do you have any reason to put it here?
> char * ident;
> char * argv0;
> struct team_handle * th;
> diff --git a/teamd/teamd_link_watch.c b/teamd/teamd_link_watch.c
> index cae6549..11f4697 100644
> --- a/teamd/teamd_link_watch.c
> +++ b/teamd/teamd_link_watch.c
> @@ -133,11 +133,28 @@ bool teamd_link_watch_port_up(struct teamd_context
> *ctx,
> if (!tdport)
> return true;
> link = true;
> - teamd_for_each_port_priv_by_creator(common_ppriv, tdport,
> - LW_PORT_PRIV_CREATOR_PRIV) {
> - link = common_ppriv->link_up;
> - if (link)
> - return link;
> + if (ctx->evaluate_all_watchers) {
> + /*
> + * If multiple link-watchers used at the same time,
> + * link is up if ALL of the link-watchers reports the link
> up.
> + */
> + teamd_for_each_port_priv_by_creator(common_ppriv, tdport,
> +
> LW_PORT_PRIV_CREATOR_PRIV) {
> + link = common_ppriv->link_up;
> + if (!link)
> + return link;
> + }
> + } else {
> + /*
> + * If multiple link-watchers used at the same time,
> + * link is up if ANY of the link-watchers reports the link
> up.
> + */
> + teamd_for_each_port_priv_by_creator(common_ppriv, tdport,
> +
> LW_PORT_PRIV_CREATOR_PRIV) {
> + link = common_ppriv->link_up;
> + if (link)
> + return link;
> + }
> }
> return link;
> }
> --
> 2.35.3
>
>
_______________________________________________
libteam mailing list -- libteam(a)lists.fedorahosted.org
To unsubscribe send an email to libteam-leave(a)lists.fedorahosted.org
Fedora Code of Conduct:
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines:
https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives:
https://lists.fedorahosted.org/archives/list/libteam@lists.fedorahosted.org
Do not reply to spam, report it:
https://pagure.io/fedora-infrastructure/new_issue