[PATCH 1/1] Change parser interface to allow stat
Sam Mendoza-Jonas
sam at mendozajonas.com
Tue Mar 15 14:26:35 AEDT 2016
All looks good, and tests!
Merged as 9396605.
On Wed, Feb 24, 2016 at 08:12:25AM -0800, Alan Dunn wrote:
> Currently, the GRUB2 parser incorrectly reports "[ -f <path> ]" as
> false if the size of the file is above 1 MB. This patch changes the
> parser interface to allow stating files (with parser_stat_file). Then
> in the implementation of "[ -f <path> ]", we can use parser_stat_file
> instead of parser_request_file which has the size limitation. I
> eliminate parser_check_dir in lieu of this new interface, which has
> the side effect of making "[ -d <path> ]" work (the error code for
> stat was not checked correctly before).
>
> I add a basic test for the test file operations -f, -s, and -d (to
> show that my changes to test file operations do not break them) and
> minorly modify the test framework to ensure it has enough fidelity to
> cause the expected results. Unfortunately the test wouldn't have
> caught the issue with -d, since the test framework stubs out the
> parser interface itself. Nor can the test framework catch the initial
> problem with -f because the imposed limit is (transitively) in
> function parser_request_file.
>
> Note that -f and -d follow symlinks despite the fact that GRUB does
> not (see
> http://lists.gnu.org/archive/html/grub-devel/2016-02/msg00142.html
> discussing GRUB's behavior). This is not a change to Petitboot's
> behavior though.
>
> Tested:
> The test test-grub2-test-file-ops passes. I booted Petitboot against
> a GRUB snippet:
>
> status=success
>
> if [ ! -f /large_file -a $status = success ]
> then status=fail_large_file
> fi
> if [ ! -d /a_directory -a $status = success ]
> then status=fail_dir
> fi
>
> menuentry $status {
> linux /vmlinux
> }
>
> (after making /large_file a file of size > 1 MiB and /a_directory a
> directory) and the menuentry had title "success", as desired.
>
> Signed-off-by: Alan Dunn <amdunn at google.com>
> ---
> discover/grub2/builtins.c | 33 +++++++++++++-----
> discover/parser.c | 30 +++++++++-------
> discover/parser.h | 20 ++++++++---
> test/parser/Makefile.am | 1 +
> test/parser/parser-test.h | 5 +++
> test/parser/test-grub2-test-file-ops.c | 63 ++++++++++++++++++++++++++++++++++
> test/parser/utils.c | 29 ++++++++++++----
> 7 files changed, 149 insertions(+), 32 deletions(-)
> create mode 100644 test/parser/test-grub2-test-file-ops.c
>
> diff --git a/discover/grub2/builtins.c b/discover/grub2/builtins.c
> index 6ada2a6..8bff732 100644
> --- a/discover/grub2/builtins.c
> +++ b/discover/grub2/builtins.c
> @@ -124,43 +124,58 @@ static int builtin_search(struct grub2_script *script,
> return 0;
> }
>
> +/* Note that GRUB does not follow symlinks in evaluating all file
> + * tests but -s, unlike below. However, it seems like a bad idea to
> + * emulate GRUB's behavior (e.g., it would take extra work), so we
> + * implement the behavior that coreutils' test binary has. */
> static bool builtin_test_op_file(struct grub2_script *script, char op,
> const char *file)
> {
> bool result;
> - int len, rc;
> - char *buf;
> + int rc;
> + struct stat statbuf;
>
> - rc = parser_request_file(script->ctx, script->ctx->device,
> - file, &buf, &len);
> + rc = parser_stat_path(script->ctx, script->ctx->device,
> + file, &statbuf);
> if (rc)
> return false;
>
> switch (op) {
> case 's':
> /* -s: return true if file exists and has non-zero size */
> - result = len > 0;
> + result = statbuf.st_size > 0;
> break;
> case 'f':
> - /* -f: return true if file exists */
> - result = true;
> + /* -f: return true if file exists and is not a directory. This is
> + * different than the behavior of "test", but is what GRUB does
> + * (though note as above that we follow symlinks unlike GRUB). */
> + result = !S_ISDIR(statbuf.st_mode);
> break;
> default:
> result = false;
>
> }
>
> - talloc_free(buf);
> return result;
> }
>
> +/* See comment at builtin_test_op_file for differences between how
> + * GRUB implements file tests versus Petitboot's GRUB parser. */
> static bool builtin_test_op_dir(struct grub2_script *script, char op,
> const char *dir)
> {
> + int rc;
> + struct stat statbuf;
> +
> if (op != 'd')
> return false;
>
> - return parser_check_dir(script->ctx, script->ctx->device, dir) == 0;
> + rc = parser_stat_path(script->ctx, script->ctx->device, dir, &statbuf);
> + if (rc) {
> + return false;
> + }
> +
> + return S_ISDIR(statbuf.st_mode);
> }
>
> static bool builtin_test_op(struct grub2_script *script,
> diff --git a/discover/parser.c b/discover/parser.c
> index fbf31b2..5598f96 100644
> --- a/discover/parser.c
> +++ b/discover/parser.c
> @@ -1,8 +1,6 @@
>
> #include <fcntl.h>
> #include <stdlib.h>
> -#include <sys/types.h>
> -#include <sys/stat.h>
>
> #include "types/types.h"
> #include <file/file.h>
> @@ -49,24 +47,30 @@ int parser_request_file(struct discover_context *ctx,
> return rc;
> }
>
> -int parser_check_dir(struct discover_context *ctx,
> - struct discover_device *dev, const char *dirname)
> +int parser_stat_path(struct discover_context *ctx,
> + struct discover_device *dev, const char *path,
> + struct stat *statbuf)
> {
> - struct stat statbuf;
> - char *path;
> - int rc;
> + int rc = -1;
> + char *full_path;
>
> + /* we only support local files at present */
> if (!dev->mount_path)
> return -1;
>
> - path = local_path(ctx, dev, dirname);
> + full_path = local_path(ctx, dev, path);
>
> - rc = stat(path, &statbuf);
> - talloc_free(path);
> - if (!rc)
> - return -1;
> + rc = stat(full_path, statbuf);
> + if (rc) {
> + rc = -1;
> + goto out;
> + }
>
> - return S_ISDIR(statbuf.st_mode) ? 0 : -1;
> + rc = 0;
> +out:
> + talloc_free(full_path);
> +
> + return rc;
> }
>
> int parser_replace_file(struct discover_context *ctx,
> diff --git a/discover/parser.h b/discover/parser.h
> index e0e8dc6..fc165c5 100644
> --- a/discover/parser.h
> +++ b/discover/parser.h
> @@ -2,6 +2,9 @@
> #define _PARSER_H
>
> #include <stdbool.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
>
> #include "device-handler.h"
>
> @@ -51,8 +54,9 @@ int parse_user_event(struct discover_context *ctx, struct event *event);
> /* File IO functions for parsers; these should be the only interface that
> * parsers use to access a device's filesystem.
> *
> - * These are intended for small amounts of data, typically text configuration
> - * and state files.
> + * These are intended for small amounts of data, typically text
> + * configuration and state files. Note that parser_request_file,
> + * and parser_replace_file work only on non-directories.
> */
> int parser_request_file(struct discover_context *ctx,
> struct discover_device *dev, const char *filename,
> @@ -62,7 +66,15 @@ int parser_replace_file(struct discover_context *ctx,
> char *buf, int len);
> int parser_request_url(struct discover_context *ctx, struct pb_url *url,
> char **buf, int *len);
> -int parser_check_dir(struct discover_context *ctx,
> - struct discover_device *dev, const char *dirname);
> +/* parser_stat_path returns 0 if path can be stated on dev by the
> + * running user. Note that this function follows symlinks, like the
> + * stat system call. When the function returns 0, also fills in
> + * statbuf for the path. Returns non-zero on error. This function
> + * does not have the limitations on file size that the functions above
> + * do. Unlike some of the functions above, this function also works
> + * on directories. */
> +int parser_stat_path(struct discover_context *ctx,
> + struct discover_device *dev, const char *path,
> + struct stat *statbuf);
>
> #endif /* _PARSER_H */
> diff --git a/test/parser/Makefile.am b/test/parser/Makefile.am
> index dddb472..e4f9b9c 100644
> --- a/test/parser/Makefile.am
> +++ b/test/parser/Makefile.am
> @@ -37,6 +37,7 @@ parser_TESTS = \
> test/parser/test-grub2-sles-btrfs-snapshot \
> test/parser/test-grub2-lexer-error \
> test/parser/test-grub2-parser-error \
> + test/parser/test-grub2-test-file-ops \
> test/parser/test-kboot-single \
> test/parser/test-yaboot-empty \
> test/parser/test-yaboot-single \
> diff --git a/test/parser/parser-test.h b/test/parser/parser-test.h
> index 21b5b9c..383680f 100644
> --- a/test/parser/parser-test.h
> +++ b/test/parser/parser-test.h
> @@ -33,10 +33,15 @@ int test_run_parser(struct parser_test *test, const char *parser_name);
> void test_hotplug_device(struct parser_test *test, struct discover_device *dev);
> void test_remove_device(struct parser_test *test, struct discover_device *dev);
>
> +/* Note that the testing filesystem will only reflect files and
> + * directories that you explicitly add, so it is possible for a test
> + * to inconsistently believe that a file exists but that its parent
> + * directory does not. */
> void test_add_file_data(struct parser_test *test, struct discover_device *dev,
> const char *filename, const void *data, int size);
> void test_add_dir(struct parser_test *test, struct discover_device *dev,
> const char *dirname);
> +
> void test_set_event_source(struct parser_test *test);
> void test_set_event_param(struct event *event, const char *name,
> const char *value);
> diff --git a/test/parser/test-grub2-test-file-ops.c b/test/parser/test-grub2-test-file-ops.c
> new file mode 100644
> index 0000000..b614fc1
> --- /dev/null
> +++ b/test/parser/test-grub2-test-file-ops.c
> @@ -0,0 +1,63 @@
> +
> +#include "parser-test.h"
> +
> +#if 0 /* PARSER_EMBEDDED_CONFIG */
> +status=success
> +
> +if [ -f /file_that_does_not_exist -a $status = success ]
> +then status=fail_f_1
> +fi
> +if [ -f /dir -a $status = success ]
> +then status=fail_f_2
> +fi
> +if [ ! -f /empty_file -a $status = success ]
> +then status=fail_f_3
> +fi
> +
> +if [ -s /file_that_does_not_exist -a $status = success ]
> +then status=fail_s_1
> +fi
> +if [ ! -s /dir -a $status = success ]
> +then status=fail_s_2
> +fi
> +if [ -s /empty_file -a $status = success ]
> +then status=fail_s_3
> +fi
> +if [ ! -s /non_empty_file -a $status = success ]
> +then status=fail_s_4
> +fi
> +
> +if [ -d /file_that_does_not_exist -a $status = success ]
> +then status=fail_d_1
> +fi
> +if [ ! -d /dir -a $status = success ]
> +then status=fail_d_2
> +fi
> +if [ -d /empty_file -a $status = success ]
> +then status=fail_d_3
> +fi
> +
> +menuentry $status {
> + linux /vmlinux
> +}
> +#endif
> +
> +void run_test(struct parser_test *test)
> +{
> + struct discover_boot_option *opt;
> + struct discover_context *ctx;
> +
> + ctx = test->ctx;
> +
> + test_read_conf_embedded(test, "/grub2/grub.cfg");
> + test_add_file_data(test, ctx->device, "/empty_file", "", 0);
> + test_add_file_data(test, ctx->device, "/non_empty_file", "1", 1);
> + test_add_dir(test, ctx->device, "/dir");
> +
> + test_run_parser(test, "grub2");
> +
> + check_boot_option_count(ctx, 1);
> + opt = get_boot_option(ctx, 0);
> +
> + check_name(opt, "success");
> +}
> diff --git a/test/parser/utils.c b/test/parser/utils.c
> index 0050d13..2891969 100644
> --- a/test/parser/utils.c
> +++ b/test/parser/utils.c
> @@ -193,6 +193,9 @@ void test_add_dir(struct parser_test *test, struct discover_device *dev,
> file->type = TEST_DIR;
> file->dev = dev;
> file->name = dirname;
> + /* Pick a non-zero size for directories so that "[ -s <dir
> + * path> ]" sees that the file has non-zero size. */
> + file->size = 1;
> list_add(&test->files, &file->list);
> }
>
> @@ -241,20 +244,34 @@ int parser_request_file(struct discover_context *ctx,
> return -1;
> }
>
> -int parser_check_dir(struct discover_context *ctx,
> - struct discover_device *dev, const char *dirname)
> +int parser_stat_path(struct discover_context *ctx,
> + struct discover_device *dev, const char *path,
> + struct stat *statbuf)
> {
> struct parser_test *test = ctx->test_data;
> struct test_file *file;
>
> - printf("%s: %s\n", __func__, dirname);
> -
> list_for_each_entry(&test->files, file, list) {
> if (file->dev != dev)
> continue;
> - if (strcmp(file->name, dirname))
> + if (strcmp(file->name, path))
> continue;
> - return file->type == TEST_DIR ? 0 : -1;
> +
> + statbuf->st_size = (off_t)file->size;
> + switch (file->type) {
> + case TEST_FILE:
> + statbuf->st_mode = S_IFREG;
> + break;
> + case TEST_DIR:
> + statbuf->st_mode = S_IFDIR;
> + break;
> + default:
> + fprintf(stderr, "%s: bad test file mode %d!", __func__,
> + file->type);
> + exit(EXIT_FAILURE);
> + }
> +
> + return 0;
> }
>
> return -1;
> --
> 2.7.0.rc3.207.g0ac5344
>
> _______________________________________________
> Petitboot mailing list
> Petitboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/petitboot
More information about the Petitboot
mailing list