summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorAlan Dunn <amdunn@google.com>2016-02-24 08:12:25 -0800
committerSam Mendoza-Jonas <sam@mendozajonas.com>2016-03-15 14:10:22 +1100
commit939660528bf1568c55b6dcf982cc9020c1dbcdd2 (patch)
tree7163c3684434f644537ea253b95305fe9b2ec466
parent6e5cd61065181460be8152a73dfc79f94ecd27fe (diff)
downloadtalos-petitboot-939660528bf1568c55b6dcf982cc9020c1dbcdd2.tar.gz
talos-petitboot-939660528bf1568c55b6dcf982cc9020c1dbcdd2.zip
Change parser interface to allow stat
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@google.com> Signed-off-by: Sam Mendoza-Jonas <sam@mendozajonas.com>
-rw-r--r--discover/grub2/builtins.c33
-rw-r--r--discover/parser.c30
-rw-r--r--discover/parser.h20
-rw-r--r--test/parser/Makefile.am1
-rw-r--r--test/parser/parser-test.h5
-rw-r--r--test/parser/test-grub2-test-file-ops.c63
-rw-r--r--test/parser/utils.c29
7 files changed, 149 insertions, 32 deletions
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;
OpenPOWER on IntegriCloud