Hi Ismo,
On Mon, Jun 05, 2017 at 06:12:32PM +0300, Ismo Puustinen wrote:
> If a string after "include" keyword points to a directory instead of a
> file, consider the directory to contain only nft rule files and try to
> load them all. This helps with a use case where services drop their own
> firewall configuration files into a directory and nft needs to include
> those without knowing the exact file names.
>
> File loading order from the include directory is not specified, so the
> files inside an include directory should not depend on each other.
>
> Fixes(Bug 1154 - Allow include statement to operate on directories and/or wildcards).
Just request for comestic changes, see below.
> Signed-off-by: Ismo Puustinen <ismo.puustinen@xxxxxxxxx>
> ---
> src/scanner.l | 116 +++++++++++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 95 insertions(+), 21 deletions(-)
>
> diff --git a/src/scanner.l b/src/scanner.l
> index c2c008d..19e12bc 100644
> --- a/src/scanner.l
> +++ b/src/scanner.l
> @@ -10,6 +10,8 @@
>
> %{
>
> +#include <dirent.h>
> +#include <libgen.h>
> #include <limits.h>
> #include <netinet/in.h>
> #include <arpa/inet.h>
> @@ -661,6 +663,82 @@ err:
> return -1;
> }
>
> +static int find_and_add_include_file(void *scanner, const char *filename,
> + const struct location *loc)
Probably shorter function: include_dentry() ?
And please, align second line to parens.
static int include_dentry(void *scanner, const char *entryname,
const struct location *loc)
^
> +{
> + DIR *directory = NULL;
No need to set this to NULL.
> + FILE *f = NULL;
Same thing?
> + struct error_record *erec;
> + struct parser_state *state = yyget_extra(scanner);
We prefer to order variable definitions in inverse line length, eg.
struct parser_state *state = yyget_extra(scanner);
struct error_record *erec;
DIR *directory;
FILE *f;
> + /* The file can be either a simple file or a directory which
> + * contains files. If it is a directory, assume that all files there
> + * should be included. */
Preferred coding style for comment:
/* The file can be either a simple file or a directory which
* contains files. If it is a directory, assume that all files there
* should be included.
*/
> +
> + directory = opendir(filename);
> +
> + if (directory == NULL && errno != ENOTDIR)
> + /* Could not access the directory or file, keep on searching. */
> + return 1;
We usually return negative on error, ie. -1.
> + else if (directory != NULL) {
No need for else? Given we return above.
> + /* A directory. */
Remove this comment...
Better place this code below in function so we save one level of
indent, for this function name I'd suggest include_directory().
> + struct dirent *de;
> +
> + if (!filename[0] || filename[strlen(filename)-1] != '/') {
> + erec = error(loc, "Include directory name \"%s\" does not end in '/'",
> + filename);
> + goto err;
> + }
> +
> + /* If the path is a directory, assume that all files there need
> + * to be included. */
> + while ((de = readdir(directory))) {
> + char dirbuf[PATH_MAX];
> +
> + snprintf(dirbuf, sizeof(dirbuf), "%s/%s", filename, de->d_name);
> +
> + if (strcmp(de->d_name, ".") == 0 || strcmp(de->d_name, "..") == 0)
> + continue;
> +
> + f = fopen(dirbuf, "r");
> +
No need for empty line.
> + if (f == NULL) {
> + erec = error(loc, "Could not open file \"%s\": %s\n",
> + dirbuf, strerror(errno));
> + goto err;
> + }
> +
> + erec = scanner_push_file(scanner, de->d_name, f, loc);
> + if (erec != NULL)
> + goto err;
> + }
> +
> + closedir(directory);
> + }
> + else {
We prefer this:
} else {
> + /* A simple include file. */
> + f = fopen(filename, "r");
> + if (f == NULL) {
> + erec = error(loc, "Could not open file \"%s\": %s\n",
> + filename, strerror(errno));
> + goto err;
> + }
> + erec = scanner_push_file(scanner, filename, f, loc);
> + if (erec != NULL)
> + goto err;
> + }
Wrap this code above in function, eg. include_file() ?
> +
> + return 0;
> +
> +err:
> + erec_queue(erec, state->msgs);
> +
> + if (directory)
> + closedir(directory);
> +
> + return -1;
> +}
> +
> static bool search_in_include_path(const char *filename)
> {
> return (strncmp(filename, "./", strlen("./")) != 0 &&
> @@ -671,40 +749,36 @@ static bool search_in_include_path(const char *filename)
> int scanner_include_file(void *scanner, const char *filename,
> const struct location *loc)
> {
> - struct parser_state *state = yyget_extra(scanner);
> - struct error_record *erec;
> char buf[PATH_MAX];
> - const char *name = buf;
> unsigned int i;
> - FILE *f;
> + int ret;
> + struct error_record *erec;
> + struct parser_state *state = yyget_extra(scanner);
>
> - f = NULL;
> if (search_in_include_path(filename)) {
> for (i = 0; i < INCLUDE_PATHS_MAX; i++) {
> if (include_paths[i] == NULL)
> break;
> snprintf(buf, sizeof(buf), "%s/%s",
> include_paths[i], filename);
> - f = fopen(buf, "r");
> - if (f != NULL)
> - break;
> +
> + ret = find_and_add_include_file(scanner, buf, loc);
> + if (ret == 0)
> + return 0;
> + else if (ret != 1)
> + /* error has been processed already */
> + return -1;
> }
> - } else {
> - f = fopen(filename, "r");
> - name = filename;
> }
> - if (f == NULL) {
> - erec = error(loc, "Could not open file \"%s\": %s",
> - filename, strerror(errno));
> - goto err;
> + else {
> + ret = find_and_add_include_file(scanner, filename, loc);
> + if (ret == 0)
> + return 0;
> + else if (ret != 1)
> + return -1;
^
wrong indent.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
