Re: [PATCH iproute2-next 4/9] dcb: app: modify dcb_app_table_remove_replaced() for dcb-rewr reuse

[Petr Machata <petrm@nvidia.com>]

Daniel Machon <daniel.machon@microchip.com> writes:

> When doing a replace command, entries are checked against selector and
> protocol. Rewrite requires the check to be against selector and
> priority.
>
> Modify the existing dcb_app_table_remove_replace function for dcb-rewr
> reuse, by using the newly introduced dcbnl attribute in the
> dcb_app_table struct.
>
> Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
> ---
>  dcb/dcb_app.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/dcb/dcb_app.c b/dcb/dcb_app.c
> index 9bb64f32e12e..23d6bb2a0013 100644
> --- a/dcb/dcb_app.c
> +++ b/dcb/dcb_app.c
> @@ -160,15 +160,27 @@ void dcb_app_table_remove_replaced(struct dcb_app_table *a,
>  		for (ib = 0; ib < b->n_apps; ib++) {
>  			const struct dcb_app *ab = &b->apps[ib];
>  
> -			if (aa->selector == ab->selector &&
> -			    aa->protocol == ab->protocol)
> -				present = true;
> -			else
> +			if (aa->selector != ab->selector)
>  				continue;
>  
> -			if (aa->priority == ab->priority) {
> -				found = true;
> -				break;
> +			if (a->attr == DCB_ATTR_IEEE_APP_TABLE) {
> +				if (aa->protocol == ab->protocol)
> +					present = true;
> +				else
> +					continue;
> +				if (aa->priority == ab->priority) {
> +					found = true;
> +					break;
> +				}
> +			} else {
> +				if (aa->priority == ab->priority)
> +					present = true;
> +				else
> +					continue;
> +				if (aa->protocol == ab->protocol) {
> +					found = true;
> +					break;
> +				}
>  			}
>  		}

Same point about the attribute dispatch. How about this? (Not tested
though.)

	static bool dcb_app_pid_eq(const struct dcb_app *aa, const struct dcb_app *ab)
	{
		return aa->selector == ab->selector &&
		       aa->protocol == ab->protocol;
	}

	static bool dcb_app_prio_eq(const struct dcb_app *aa, const struct dcb_app *ab)
	{
		return aa->selector == ab->selector &&
		       aa->priority == ab->priority;
	}

	static void __dcb_app_table_remove_replaced(struct dcb_app_table *a,
						    const struct dcb_app_table *b,
						    bool (*key_eq)(const struct dcb_app *aa,
								const struct dcb_app *ab),
						    bool (*val_eq)(const struct dcb_app *aa,
								const struct dcb_app *ab))
	{
		size_t ia, ja;
		size_t ib;

		for (ia = 0, ja = 0; ia < a->n_apps; ia++) {
			struct dcb_app *aa = &a->apps[ia];
			bool present = false;
			bool found = false;

			for (ib = 0; ib < b->n_apps; ib++) {
				const struct dcb_app *ab = &b->apps[ib];

				if (key_eq(aa, ab))
					present = true;
				else
					continue;

				if (val_eq(aa, ab)) {
					found = true;
					break;
				}
			}

			/* Entries that remain in A will be removed, so keep in the
                         * table only APP entries whose sel/pid is mentioned in B,
			 * but that do not have the full sel/pid/prio match.
			 */
			if (present && !found)
				a->apps[ja++] = *aa;
		}

		a->n_apps = ja;
	}

	void dcb_app_table_remove_replaced(struct dcb_app_table *a,
					const struct dcb_app_table *b)
	{
		__dcb_app_table_remove_replaced(a, b, dcb_app_pid_eq, dcb_app_prio_eq);
	}

	void dcb_rwr_table_remove_replaced(struct dcb_app_table *a,
					const struct dcb_app_table *b)
	{
		__dcb_app_table_remove_replaced(a, b, dcb_app_prio_eq, dcb_app_pid_eq);
	}

Alternatively have key / value extractor callbacks and compare those
instead of directly priority and protocol.

And actually now that I think about it more, a key_eq / get_key callback
is all we need. Instead of val_eq / get_val, we can just compare the
full app. We know the key matches already, so whatever it actually is,
it will not prevent the second match.

Dunno. I just don't want the attribute field become a polymorphic type
tag of the structure. DCB is using these callbacks quite a bit all over
the place, so code like this will be right at home.

I was actually looking at dcb_app_table_remove_existing(), which is
tantalizingly close to being a special case of the above where key_eq
just always returns true and val_eq compares all fields. But alas for
empty tables it would do the wrong thing.