This series seems reasonable to me. I just have some comments on
implementation details.
On 02/24/2013 09:00 PM, Doug Goldstein wrote:
Allow the bridge helper to take a config directory rather than having to
specify every file in the directory manually via an include statement.
Signed-off-by: Doug Goldstein <car...@cardoe.com>
CC: Anthony Liguori <aligu...@us.ibm.com>
CC: Richa Marwaha <rmar...@linux.vnet.ibm.com>
CC: Corey Bryant <cor...@linux.vnet.ibm.com>
TO: qemu-devel@nongnu.org
---
qemu-bridge-helper.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 55 insertions(+)
diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
index 287bfd5..b8771a3 100644
--- a/qemu-bridge-helper.c
+++ b/qemu-bridge-helper.c
@@ -16,6 +16,7 @@
#include "config-host.h"
#include <stdio.h>
+#include <dirent.h>
#include <errno.h>
#include <fcntl.h>
#include <unistd.h>
@@ -70,6 +71,26 @@ static void usage(void)
"Usage: qemu-bridge-helper [--use-vnet] --br=bridge
--fd=unixfd\n");
}
+static int filter_bridge_conf_dir(const struct dirent *entry)
+{
+ ssize_t name_pos;
+
+ /* We want to check the last 5 bytes for '.conf' */
+ name_pos = strlen(entry->d_name) - 6;
a.conf sets name_pos=0..
+
+ /* We need the file to at least be called 'a.conf' to make
+ * sense of this.
+ */
+ if (name_pos < 1)
+ return 0;
..so if the name is a.conf we'd return 0 here which means it would be
skipped. That's not right.
+
+ /* If the file didn't end in '.conf', skip it */
+ if (strcmp(".conf", entry->d_name + name_pos))
+ return 0;
+
+ return 1;
+}
+
I think you can simplify this function to something like:
static int is_filter_bridge_conf_dir(const struct dirent *entry)
{
size_t len = strlen(entry->d_name);
if (len > 5 &&
strcmp(".conf", &entry->d_name[len-5]) == 0) {
return 1;
}
return 0;
}
static int parse_acl_file(const char *filename, ACLList *acl_list)
{
FILE *f;
@@ -84,6 +105,9 @@ static int parse_acl_file(const char *filename, ACLList
*acl_list)
while (fgets(line, sizeof(line), f) != NULL) {
char *ptr = line;
char *cmd, *arg, *argend;
+ struct dirent **include_list = NULL;
+ int i, include_count;
+ char *conf_file;
while (isspace(*ptr)) {
ptr++;
@@ -137,6 +161,37 @@ static int parse_acl_file(const char *filename, ACLList
*acl_list)
snprintf(acl_rule->iface, IFNAMSIZ, "%s", arg);
}
QSIMPLEQ_INSERT_TAIL(acl_list, acl_rule, entry);
+ } else if (strcmp(cmd, "includedir") == 0) {
+ include_count = scandir(arg, &include_list,
+ filter_bridge_conf_dir, NULL);
+ if (include_count < 0) {
+ fprintf(stderr, "Unable to retrieve conf files from '%s':
%s\n",
+ arg, strerror(errno));
+ fclose(f);
+ return -1;
+ }
+
+ for (i = 0; i < include_count; i++) {
+ if (asprintf(&conf_file, "%s/%s", arg,
+ include_list[i]->d_name) < 0) {
+ fprintf(stderr, "Failed to allocate memory for "
+ "file path: %s/%s\n",
+ arg, include_list[i]->d_name);
+ fclose(f);
+ errno = ENOMEM;
+ return -1;
The new failure paths in this patch have memory leaks that are fixed in
patch 2. It might be simpler if you just merge patch 1 and 2.
+ }
+
+ parse_acl_file(conf_file, acl_list);
+
+ free(conf_file);
+ free(include_list[i]);
+ include_list[i] = NULL;
+ }
+ free(include_list);
+ include_list = NULL;
+ include_count = 0;
+
} else if (strcmp(cmd, "include") == 0) {
/* ignore errors */
parse_acl_file(arg, acl_list);
--
Regards,
Corey Bryant
