Re: [PATCH nft 4/4] src: single cache_update() call to build cache before evaluation

[Phil Sutter <phil@nwl.cc>]

Hi,

On Wed, Jun 05, 2019 at 06:46:52PM +0200, Pablo Neira Ayuso wrote:
[...]
> diff --git a/src/cache.c b/src/cache.c
> new file mode 100644
> index 000000000000..89a884012a90
> --- /dev/null
> +++ b/src/cache.c
> @@ -0,0 +1,123 @@
> +/*
> + * Copyright (c) 2019 Pablo Neira Ayuso <pablo@netfilter.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <expression.h>
> +#include <statement.h>
> +#include <rule.h>
> +#include <erec.h>
> +#include <utils.h>
> +
> +static void evaluate_cache_add(struct cmd *cmd, unsigned int *completeness)
> +{
> +	switch (cmd->obj) {
> +	case CMD_OBJ_SETELEM:
> +	case CMD_OBJ_SET:
> +	case CMD_OBJ_CHAIN:
> +	case CMD_OBJ_FLOWTABLE:
> +		if (*completeness < cmd->op)
> +			*completeness = cmd->op;
> +		break;
> +	case CMD_OBJ_RULE:
> +		/* XXX index is set to zero unless this handle_merge() call is
> +		 * invoked, this handle_merge() call is done from the
> +		 * evaluation, which is too late.
> +		 */

Using the right handle was somehow tricky. IIRC, getting it right was
part of the work on v5 of my series. Maybe you were working around a bug
in my code?

> +		handle_merge(&cmd->rule->handle, &cmd->handle);
> +
> +		if (cmd->rule->handle.index.id &&
> +		    *completeness < CMD_LIST)
> +			*completeness = CMD_LIST;
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +static void evaluate_cache_del(struct cmd *cmd, unsigned int *completeness)
> +{
> +	switch (cmd->obj) {
> +	case CMD_OBJ_SETELEM:
> +		if (*completeness < cmd->op)
> +			*completeness = cmd->op;

This manual max() thing seems to be a common pattern. Maybe make these
functions return cmd->op (or the desired completeness) and handle the
max() check in caller?

[...]
> +int cache_evaluate(struct nft_ctx *nft, struct list_head *cmds)
> +{
> +	unsigned int completeness = CMD_INVALID;
> +	struct cmd *cmd;
> +
> +	list_for_each_entry(cmd, cmds, list) {
> +		switch (cmd->op) {
> +		case CMD_ADD:
> +		case CMD_INSERT:
> +		case CMD_REPLACE:
> +			if (nft_output_echo(&nft->output) &&
> +			    completeness < cmd->op)
> +				return cmd->op;

Is 'return' correct here? That would abort cmd list processing.

> +
> +			/* Fall through */
> +		case CMD_CREATE:
> +			evaluate_cache_add(cmd, &completeness);
> +			break;
> +		case CMD_DELETE:
> +			evaluate_cache_del(cmd, &completeness);
> +			break;
> +		case CMD_GET:
> +		case CMD_LIST:
> +		case CMD_RESET:
> +		case CMD_EXPORT:
> +		case CMD_MONITOR:
> +			if (completeness < cmd->op)
> +				completeness = cmd->op;
> +			break;
> +		case CMD_FLUSH:
> +			evaluate_cache_flush(cmd, &completeness);
> +			break;
> +		case CMD_RENAME:
> +			evaluate_cache_rename(cmd, &completeness);

As suggested above, I would do (also in the other cases):

			tmp = evaluate_cache_rename(cmd);

> +			break;
> +		case CMD_DESCRIBE:
> +		case CMD_IMPORT:
> +			break;
> +		default:
> +			break;
> +		}

And here:
		completeness = max(tmp, completeness);

> +	}
> +
> +	return completeness;
> +}

Cheers, Phil