[apparmor] [Patch] abstract out the directory walking routine
John Johansen
john.johansen at canonical.com
Fri Aug 17 04:47:00 UTC 2012
On 08/16/2012 04:48 PM, Steve Beattie wrote:
> On Thu, Aug 16, 2012 at 04:23:04PM -0700, John Johansen wrote:
>>> parser_misc.c as they're kind of intended to serve the same purpose;
>>> helper functions that don't fit in a specific location, but that's not
>>> a blocker for this patch.
>>>
>> actually my plan was to move things the other way as they got cleaned up.
>
> Yes, that's fine. I don't really care which way a merge goes, I just
> don't want two of them.
>
>>> I do wonder if it wouldn't be better to have three versions of this
>>> function, one as-is requiring @dir and @name to be non-NULL, and the
>>> other two for the single argument version of the functions (I suspect
>>> the one with @dir only would be called by the other two, but I haven't
>>> thought very hard about it). This would potentially be a little more
>>> defensive programming as a caller that meant to pass a non-NULL as one
>>> of the arguments but messed up for some reason would generate an error,
>>> and not be accepted with unexpected behavior.
>>>
>> Hrmmm I'll tinker with it, I am thinking that the others would basically
>> be wrapper fns.
>
> Right, my thought as well.
>
Alright here is what I came up with as a first pass,
its larger than I was expecting and I am tempted to just make the current
fn the internal __ fn, and make the 3 exported fns, just thin wrappers around
it, instead of trying to split out the individual bits of code like I have
---
diff --git a/parser/lib.c b/parser/lib.c
index c4a917b..f41b1d0 100644
--- a/parser/lib.c
+++ b/parser/lib.c
@@ -32,24 +32,13 @@
#include "parser.h"
/**
- * dirat_for_each: iterate over a directory calling cb for each entry
- * @dir: already opened directory (MAY BE NULL)
- * @name: name of the directory (MAY BE NULL)
+ * __dirat_for_each: iterate over a directory calling cb for each entry
+ * @d: already opened directory (NOT NULL)
+ * @dirent: dirent struct to use (NOT NULL)
* @data: data pointer to pass to the callback fn (MAY BE NULL)
* @cb: the callback to pass entry too (NOT NULL)
*
- * Iterate over the entries in a directory calling cb for each entry.
- * The directory to iterate is determined by a combination of @dir and
- * @name.
- *
- * IF @name is a relative path it is determine relative to at @dir if it
- * is specified, else it the lookup is done relative to the current
- * working directory.
- *
- * If @name is not specified then @dir is used as the directory to iterate
- * over.
- *
- * It is an error if both @name and @dir are null
+ * Iterate over the entries in @d calling cb for each entry.
*
* The cb function is called with the DIR in use and the name of the
* file in that directory. If the file is to be opened it should
@@ -57,47 +46,17 @@
*
* Returns: 0 on success, else -1 and errno is set to the error code
*/
-int dirat_for_each(DIR *dir, const char *name, void *data,
- int (* cb)(DIR *, const char *, struct stat *, void *))
+static int __dirat_for_each(DIR *d, struct dirent *dirent, void *data,
+ int (* cb)(DIR *, const char *, struct stat *, void *))
{
- struct dirent *dirent = NULL, *ent;
- DIR *d = NULL;
+ struct dirent *ent;
int error = 0;
- if (!cb || (!dir && !name)) {
+ if (!d || !dirent) {
errno = EINVAL;
return -1;
}
- if (dir && (!name || *name != '/')) {
- dirent = malloc(offsetof(struct dirent, d_name) +
- fpathconf(dirfd(dir), _PC_NAME_MAX) + 1);
- } else {
- dirent = malloc(offsetof(struct dirent, d_name) +
- pathconf(name, _PC_NAME_MAX) + 1);
- }
- if (!dirent) {
- PDEBUG("could not alloc dirent");
- return -1;
- }
-
- if (name) {
- if (dir && *name != '/') {
- int fd = openat(dirfd(dir), name, O_RDONLY);
- if (fd == -1)
- goto fail;
- d = fdopendir(fd);
- } else {
- d = opendir(name);
- }
- PDEBUG("Open dir '%s': %s\n", name, d ? "succeeded" : "failed");
- if (!(d))
- goto fail;
- } else { /* dir && !name */
- PDEBUG("Recieved cache directory\n");
- d = dir;
- }
-
for (error = readdir_r(d, dirent, &ent);
error == 0 && ent != NULL;
error = readdir_r(d, dirent, &ent)) {
@@ -109,27 +68,156 @@ int dirat_for_each(DIR *dir, const char *name, void *data,
if (fstatat(dirfd(d), ent->d_name, &my_stat, 0)) {
PDEBUG("stat failed for '%s'", name);
- goto fail;
+ return -1;
}
- if (cb(d, ent->d_name, &my_stat, data)) {
+ if ((error = cb(d, ent->d_name, &my_stat, data))) {
PDEBUG("dir_for_each callback failed\n");
- goto fail;
+ return error;
}
}
- if (d != dir)
+ return error;
+}
+
+/**
+ * dirpath_for_each: iterate over a directory calling cb for each entry
+ * @name: name of the directory (NOT NULL)
+ * @data: data pointer to pass to the callback fn (MAY BE NULL)
+ * @cb: the callback to pass entry too (NOT NULL)
+ *
+ * Iterate over the entries in @d calling cb for each entry.
+ *
+ * The cb function is called with the DIR in use and the name of the
+ * file in that directory. If the file is to be opened it should
+ * use the openat, fstatat, and related fns.
+ *
+ * Returns: 0 on success, else -1 and errno is set to the error code
+ */
+int dirpath_for_each(const char *name, void *data,
+ int (* cb)(DIR *, const char *, struct stat *, void *))
+{
+ struct dirent *dirent = NULL;
+ DIR *d = NULL;
+ int error = -1;
+
+ if (!name) {
+ errno = EINVAL;
+ return -1;
+ }
+
+ dirent = malloc(offsetof(struct dirent, d_name) +
+ pathconf(name, _PC_NAME_MAX) + 1);
+ if (!dirent) {
+ PDEBUG("could not alloc dirent");
+ return -1;
+ }
+
+ d = opendir(name);
+ PDEBUG("Open dir '%s': %s\n", name, d ? "succeeded" : "failed");
+ if (d) {
+ error = __dirat_for_each(d, dirent, data, cb);
closedir(d);
+ }
free(dirent);
return error;
+}
+
+
+/**
+ * dirat_for_each: iterate over a directory calling cb for each entry
+ * @dir: already opened directory (MAY BE NULL)
+ * @name: name of the directory (NOT NULL)
+ * @data: data pointer to pass to the callback fn (MAY BE NULL)
+ * @cb: the callback to pass entry too (NOT NULL)
+ *
+ * Iterate over the entries in the direntory @name in @dir calling cb
+ * for each entry.
+ *
+ * IF @name is an absolute path then @dir is ignored and may be NULL,
+ * else name is looked up relative to @dir
+ *
+ * The cb function is called with the DIR in use and the name of the
+ * file in that directory. If the file is to be opened it should
+ * use the openat, fstatat, and related fns.
+ *
+ * Returns: 0 on success, else -1 and errno is set to the error code
+ */
+int dirat_for_each(DIR *dir, const char *name, void *data,
+ int (* cb)(DIR *, const char *, struct stat *, void *))
+{
+ struct dirent *dirent = NULL;
+ DIR *d = NULL;
+ int error = -1;
-fail:
- error = errno;
- if (d && d != dir)
+ if (!name) {
+ errno = EINVAL;
+ return -1;
+ } else if (*name == '/') {
+ return dirpath_for_each(name, data, cb);
+ } else if (!dir) {
+ errno = EINVAL;
+ return -1;
+ }
+
+ dirent = malloc(offsetof(struct dirent, d_name) +
+ fpathconf(dirfd(dir), _PC_NAME_MAX) + 1);
+ if (!dirent) {
+ PDEBUG("could not alloc dirent");
+ return -1;
+ }
+
+ int fd = openat(dirfd(dir), name, O_RDONLY);
+ if (fd == -1)
+ goto out;
+ d = fdopendir(fd);
+ PDEBUG("Open dir '%s': %s\n", name, d ? "succeeded" : "failed");
+ if (d) {
+ error = __dirat_for_each(d, dirent, data, cb);
closedir(d);
+ }
+
+out:
+ free(dirent);
+
+ return error;
+}
+
+/**
+ * dir_for_each: iterate over a directory calling cb for each entry
+ * @d: already opened directory (NOT NULL)
+ * @data: data pointer to pass to the callback fn (MAY BE NULL)
+ * @cb: the callback to pass entry too (NOT NULL)
+ *
+ * Iterate over the entries in @d calling @cb for each entry.
+ *
+ * The cb function is called with the DIR in use and the name of the
+ * file in that directory. If the file is to be opened it should
+ * use the openat, fstatat, and related fns.
+ *
+ * Returns: 0 on success, else -1 and errno is set to the error code
+ */
+int dir_for_each(DIR *d, void *data,
+ int (* cb)(DIR *, const char *, struct stat *, void *))
+{
+ struct dirent *dirent = NULL;
+ int error = -1;
+
+ if (!d) {
+ errno = EINVAL;
+ return -1;
+ }
+
+ dirent = malloc(offsetof(struct dirent, d_name) +
+ fpathconf(dirfd(d), _PC_NAME_MAX) + 1);
+ if (!dirent) {
+ PDEBUG("could not alloc dirent");
+ return -1;
+ }
+
+ error = __dirat_for_each(d, dirent, data, cb);
free(dirent);
- errno = error;
- return -1;
+ return error;
}
diff --git a/parser/lib.h b/parser/lib.h
index efa9b9c..e286177 100644
--- a/parser/lib.h
+++ b/parser/lib.h
@@ -1,7 +1,11 @@
#ifndef __AA_LIB_H_
#define __AA_LIB_H_
+int dirpath_for_each(const char *name, void *data,
+ int (* cb)(DIR *, const char *, struct stat *, void *));
int dirat_for_each(DIR *dir, const char *name, void *data,
int (* cb)(DIR *, const char *, struct stat *, void *));
+int dir_for_each(DIR *d, void *data,
+ int (* cb)(DIR *, const char *, struct stat *, void *));
#endif /* __AA_LIB_H_ */
diff --git a/parser/parser_lex.l b/parser/parser_lex.l
index 1258b01..a45a192 100644
--- a/parser/parser_lex.l
+++ b/parser/parser_lex.l
@@ -167,7 +167,7 @@ void include_filename(char *filename, int search)
struct cb_struct data = { fullpath, filename };
fclose(include_file);
include_file = NULL;
- if (dirat_for_each(NULL, fullpath, &data, include_dir_cb)) {
+ if (dirpath_for_each(fullpath, &data, include_dir_cb)) {
yyerror(_("Could not process include directory"
" '%s' in '%s'"), fullpath, filename);;
}
diff --git a/parser/parser_main.c b/parser/parser_main.c
index 1f56372..ede2181 100644
--- a/parser/parser_main.c
+++ b/parser/parser_main.c
@@ -766,7 +766,7 @@ static char *handle_features_dir(const char *filename, char **buffer, int size,
{
struct features_struct fst = { buffer, size, pos };
- if (dirat_for_each(NULL, filename, &fst, features_dir_cb)) {
+ if (dirpath_for_each(filename, &fst, features_dir_cb)) {
PDEBUG("Failed evaluating .features\n");
exit(1);
}
@@ -1183,7 +1183,7 @@ static int clear_cache_files(const char *path)
exit(1);
}
- error = dirat_for_each(NULL, cache, NULL, clear_cache_cb);
+ error = dirpath_for_each(cache, NULL, clear_cache_cb);
free(cache);
More information about the AppArmor
mailing list