Patchwork [1/2] bridge helper: support conf dirs

login
register
mail settings
Submitter Doug Goldstein
Date Feb. 25, 2013, 2 a.m.
Message ID <1361757620-23318-2-git-send-email-cardoe@cardoe.com>
Download mbox | patch
Permalink /patch/222817/
State New
Headers show

Comments

Doug Goldstein - Feb. 25, 2013, 2 a.m.
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(+)
Corey Bryant - Feb. 25, 2013, 10:40 p.m.
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);
>
Stefan Hajnoczi - Feb. 26, 2013, 4:05 p.m.
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.

Patch

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);