Message ID | 1361757620-23318-2-git-send-email-cardoe@cardoe.com |
---|---|
State | New |
Headers | show |
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 <cardoe@cardoe.com> > CC: Anthony Liguori <aliguori@us.ibm.com> > CC: Richa Marwaha <rmarwah@linux.vnet.ibm.com> > CC: Corey Bryant <coreyb@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); >
On Sun, Feb 24, 2013 at 08:00:19PM -0600, Doug Goldstein wrote: > @@ -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; These new variables could be declared... > 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) { ...here in the scope where they are used. > + include_count = scandir(arg, &include_list, > + filter_bridge_conf_dir, NULL); The POSIX scandir(3) spec and the Linux man page do not define what happens when the compar argument is NULL. You could use alphasort(3) here to make behavior clear.
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; + + /* We need the file to at least be called 'a.conf' to make + * sense of this. + */ + if (name_pos < 1) + return 0; + + /* If the file didn't end in '.conf', skip it */ + if (strcmp(".conf", entry->d_name + name_pos)) + return 0; + + return 1; +} + 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; + } + + 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);
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 <cardoe@cardoe.com> CC: Anthony Liguori <aliguori@us.ibm.com> CC: Richa Marwaha <rmarwah@linux.vnet.ibm.com> CC: Corey Bryant <coreyb@linux.vnet.ibm.com> TO: qemu-devel@nongnu.org --- qemu-bridge-helper.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)