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 <[email protected]>
> + *
> + * 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
