[Qemu-devel] [PATCHv2 0/2] bridge helper: includedir conf arg

[Qemu-devel] [PATCHv2 0/2] bridge helper: includedir conf arg

[Doug Goldstein]

The goal is to support an 'includedir' to include all files within a
directory specified in the bridge.conf file. The rationale is to allow
libvirt to be able to configure interfaces to for use by unprivileged
users by just simply generating a new configuration file to the directory.

Change from v1:
- Reversed patch order to make the series clearer
- Integrated review changes from Corey Bryant
- Integrated review changes from Stefan Hajnoczi

Doug Goldstein (2):
  bridge helper: unified error cleanup for parse_acl_file
  bridge helper: support conf dirs

 qemu-bridge-helper.c | 75 +++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 66 insertions(+), 9 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCHv2 1/2] bridge helper: unified error cleanup for parse_acl_file

[Doug Goldstein]

Handle errors and cleanup from the error in a unified place for
parse_acl_file().

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 | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
index 287bfd5..ee67740 100644
--- a/qemu-bridge-helper.c
+++ b/qemu-bridge-helper.c
@@ -74,11 +74,12 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
 {
     FILE *f;
     char line[4096];
+    int ret = -EINVAL;
     ACLRule *acl_rule;
 
     f = fopen(filename, "r");
     if (f == NULL) {
-        return -1;
+        return -errno;
     }
 
     while (fgets(line, sizeof(line), f) != NULL) {
@@ -102,9 +103,8 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
 
         if (arg == NULL) {
             fprintf(stderr, "Invalid config line:\n  %s\n", line);
-            fclose(f);
-            errno = EINVAL;
-            return -1;
+            ret = -EINVAL;
+            goto failure;
         }
 
         *arg = 0;
@@ -142,15 +142,17 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
             parse_acl_file(arg, acl_list);
         } else {
             fprintf(stderr, "Unknown command `%s'\n", cmd);
-            fclose(f);
-            errno = EINVAL;
-            return -1;
+            ret = -EINVAL;
+            goto failure;
         }
     }
 
+    ret = 0;
+
+failure:
     fclose(f);
 
-    return 0;
+    return ret;
 }
 
 static bool has_vnet_hdr(int fd)
@@ -272,7 +274,7 @@ int main(int argc, char **argv)
 
     /* parse default acl file */
     QSIMPLEQ_INIT(&acl_list);
-    if (parse_acl_file(DEFAULT_ACL_FILE, &acl_list) == -1) {
+    if (parse_acl_file(DEFAULT_ACL_FILE, &acl_list) < 0) {
         fprintf(stderr, "failed to parse default acl file `%s'\n",
                 DEFAULT_ACL_FILE);
         ret = EXIT_FAILURE;
-- 
1.7.12.4

Re: [Qemu-devel] [PATCHv2 1/2] bridge helper: unified error cleanup for parse_acl_file

[Corey Bryant]

On 03/02/2013 01:58 AM, Doug Goldstein wrote:
> Handle errors and cleanup from the error in a unified place for
> parse_acl_file().
>
> 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 | 20 +++++++++++---------
>   1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
> index 287bfd5..ee67740 100644
> --- a/qemu-bridge-helper.c
> +++ b/qemu-bridge-helper.c
> @@ -74,11 +74,12 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
>   {
>       FILE *f;
>       char line[4096];
> +    int ret = -EINVAL;
>       ACLRule *acl_rule;
>
>       f = fopen(filename, "r");
>       if (f == NULL) {
> -        return -1;
> +        return -errno;
>       }
>
>       while (fgets(line, sizeof(line), f) != NULL) {
> @@ -102,9 +103,8 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
>
>           if (arg == NULL) {
>               fprintf(stderr, "Invalid config line:\n  %s\n", line);
> -            fclose(f);
> -            errno = EINVAL;
> -            return -1;
> +            ret = -EINVAL;
> +            goto failure;

I would stick with setting errno here rather than ret..

>           }
>
>           *arg = 0;
> @@ -142,15 +142,17 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
>               parse_acl_file(arg, acl_list);
>           } else {
>               fprintf(stderr, "Unknown command `%s'\n", cmd);
> -            fclose(f);
> -            errno = EINVAL;
> -            return -1;
> +            ret = -EINVAL;
> +            goto failure;

And do the same here..

>           }
>       }
>
> +    ret = 0;
> +
> +failure:
>       fclose(f);
>
> -    return 0;
> +    return ret;
>   }
>
>   static bool has_vnet_hdr(int fd)
> @@ -272,7 +274,7 @@ int main(int argc, char **argv)
>
>       /* parse default acl file */
>       QSIMPLEQ_INIT(&acl_list);
> -    if (parse_acl_file(DEFAULT_ACL_FILE, &acl_list) == -1) {
> +    if (parse_acl_file(DEFAULT_ACL_FILE, &acl_list) < 0) {
>           fprintf(stderr, "failed to parse default acl file `%s'\n",
>                   DEFAULT_ACL_FILE);

.. and then you can append strerror(errno) to this message, which I 
admit should have been here before you touched this code.  This will 
keep this error path consistent with many of the others in this file.

>           ret = EXIT_FAILURE;
>

-- 
Regards,
Corey Bryant

Re: [Qemu-devel] [PATCHv2 1/2] bridge helper: unified error cleanup for parse_acl_file

[Doug Goldstein]

On Mon, Mar 4, 2013 at 10:27 AM, Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
>
>
> On 03/02/2013 01:58 AM, Doug Goldstein wrote:
>>
>> Handle errors and cleanup from the error in a unified place for
>> parse_acl_file().
>>
>> 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 | 20 +++++++++++---------
>>   1 file changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
>> index 287bfd5..ee67740 100644
>> --- a/qemu-bridge-helper.c
>> +++ b/qemu-bridge-helper.c
>> @@ -74,11 +74,12 @@ static int parse_acl_file(const char *filename,
>> ACLList *acl_list)
>>   {
>>       FILE *f;
>>       char line[4096];
>> +    int ret = -EINVAL;
>>       ACLRule *acl_rule;
>>
>>       f = fopen(filename, "r");
>>       if (f == NULL) {
>> -        return -1;
>> +        return -errno;
>>       }
>>
>>       while (fgets(line, sizeof(line), f) != NULL) {
>> @@ -102,9 +103,8 @@ static int parse_acl_file(const char *filename,
>> ACLList *acl_list)
>>
>>           if (arg == NULL) {
>>               fprintf(stderr, "Invalid config line:\n  %s\n", line);
>> -            fclose(f);
>> -            errno = EINVAL;
>> -            return -1;
>> +            ret = -EINVAL;
>> +            goto failure;
>
>
> I would stick with setting errno here rather than ret..
>
>
>>           }
>>
>>           *arg = 0;
>> @@ -142,15 +142,17 @@ static int parse_acl_file(const char *filename,
>> ACLList *acl_list)
>>               parse_acl_file(arg, acl_list);
>>           } else {
>>               fprintf(stderr, "Unknown command `%s'\n", cmd);
>> -            fclose(f);
>> -            errno = EINVAL;
>> -            return -1;
>> +            ret = -EINVAL;
>> +            goto failure;
>
>
> And do the same here..
>
>
>>           }
>>       }
>>
>> +    ret = 0;
>> +
>> +failure:
>>       fclose(f);
>>
>> -    return 0;
>> +    return ret;
>>   }
>>
>>   static bool has_vnet_hdr(int fd)
>> @@ -272,7 +274,7 @@ int main(int argc, char **argv)
>>
>>       /* parse default acl file */
>>       QSIMPLEQ_INIT(&acl_list);
>> -    if (parse_acl_file(DEFAULT_ACL_FILE, &acl_list) == -1) {
>> +    if (parse_acl_file(DEFAULT_ACL_FILE, &acl_list) < 0) {
>>           fprintf(stderr, "failed to parse default acl file `%s'\n",
>>                   DEFAULT_ACL_FILE);
>
>
> .. and then you can append strerror(errno) to this message, which I admit
> should have been here before you touched this code.  This will keep this
> error path consistent with many of the others in this file.

Would you consider the return value then being passed on to
strerror()? Seems like it'd be a little bit safer from the stand point
of someone coming through and adding something new the the cleanup
case or any other cases which calls a glibc function which then blows
away errno.

-- 
Doug Goldstein

Re: [Qemu-devel] [PATCHv2 1/2] bridge helper: unified error cleanup for parse_acl_file

[Corey Bryant]

On 03/04/2013 01:53 PM, Doug Goldstein wrote:
> On Mon, Mar 4, 2013 at 10:27 AM, Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
>>
>>
>> On 03/02/2013 01:58 AM, Doug Goldstein wrote:
>>>
>>> Handle errors and cleanup from the error in a unified place for
>>> parse_acl_file().
>>>
>>> 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 | 20 +++++++++++---------
>>>    1 file changed, 11 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
>>> index 287bfd5..ee67740 100644
>>> --- a/qemu-bridge-helper.c
>>> +++ b/qemu-bridge-helper.c
>>> @@ -74,11 +74,12 @@ static int parse_acl_file(const char *filename,
>>> ACLList *acl_list)
>>>    {
>>>        FILE *f;
>>>        char line[4096];
>>> +    int ret = -EINVAL;
>>>        ACLRule *acl_rule;
>>>
>>>        f = fopen(filename, "r");
>>>        if (f == NULL) {
>>> -        return -1;
>>> +        return -errno;
>>>        }
>>>
>>>        while (fgets(line, sizeof(line), f) != NULL) {
>>> @@ -102,9 +103,8 @@ static int parse_acl_file(const char *filename,
>>> ACLList *acl_list)
>>>
>>>            if (arg == NULL) {
>>>                fprintf(stderr, "Invalid config line:\n  %s\n", line);
>>> -            fclose(f);
>>> -            errno = EINVAL;
>>> -            return -1;
>>> +            ret = -EINVAL;
>>> +            goto failure;
>>
>>
>> I would stick with setting errno here rather than ret..
>>
>>
>>>            }
>>>
>>>            *arg = 0;
>>> @@ -142,15 +142,17 @@ static int parse_acl_file(const char *filename,
>>> ACLList *acl_list)
>>>                parse_acl_file(arg, acl_list);
>>>            } else {
>>>                fprintf(stderr, "Unknown command `%s'\n", cmd);
>>> -            fclose(f);
>>> -            errno = EINVAL;
>>> -            return -1;
>>> +            ret = -EINVAL;
>>> +            goto failure;
>>
>>
>> And do the same here..
>>
>>
>>>            }
>>>        }
>>>
>>> +    ret = 0;
>>> +
>>> +failure:
>>>        fclose(f);
>>>
>>> -    return 0;
>>> +    return ret;
>>>    }
>>>
>>>    static bool has_vnet_hdr(int fd)
>>> @@ -272,7 +274,7 @@ int main(int argc, char **argv)
>>>
>>>        /* parse default acl file */
>>>        QSIMPLEQ_INIT(&acl_list);
>>> -    if (parse_acl_file(DEFAULT_ACL_FILE, &acl_list) == -1) {
>>> +    if (parse_acl_file(DEFAULT_ACL_FILE, &acl_list) < 0) {
>>>            fprintf(stderr, "failed to parse default acl file `%s'\n",
>>>                    DEFAULT_ACL_FILE);
>>
>>
>> .. and then you can append strerror(errno) to this message, which I admit
>> should have been here before you touched this code.  This will keep this
>> error path consistent with many of the others in this file.
>
> Would you consider the return value then being passed on to
> strerror()? Seems like it'd be a little bit safer from the stand point
> of someone coming through and adding something new the the cleanup
> case or any other cases which calls a glibc function which then blows
> away errno.
>

Yes that makes sense.  Or you could save and restore errno at the 
beginning and end of the cleanup path.  Either way.

-- 
Regards,
Corey Bryant

[Qemu-devel] [PATCHv2 2/2] bridge helper: support conf dirs

[Doug Goldstein]

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 ee67740..39d343c 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,12 +71,27 @@ 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 len = strlen(entry->d_name);
+
+    /* We only want files ending in .conf */
+    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;
     char line[4096];
     int ret = -EINVAL;
     ACLRule *acl_rule;
+    struct dirent **include_list = NULL;
+    int i, include_count = 0;
+    char *conf_file;
 
     f = fopen(filename, "r");
     if (f == NULL) {
@@ -137,6 +153,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, alphasort);
+            if (include_count < 0) {
+                ret = -errno;
+                fprintf(stderr, "Unable to retrieve conf files from '%s': %s\n",
+                        arg, strerror(errno));
+                goto failure;
+            }
+
+            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);
+                    ret = -ENOMEM;
+                    goto failure;
+                }
+
+                /* ignore errors like 'include' cmd */
+                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);
@@ -152,6 +199,14 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
 failure:
     fclose(f);
 
+    if (include_list) {
+        for (i = 0; i < include_count; i++) {
+            if (include_list[i])
+                free(include_list[i]);
+        }
+        free(include_list);
+    }
+
     return ret;
 }
 
-- 
1.7.12.4

Re: [Qemu-devel] [PATCHv2 2/2] bridge helper: support conf dirs

[Corey Bryant]

On 03/02/2013 01:58 AM, 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 ee67740..39d343c 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,12 +71,27 @@ 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 len = strlen(entry->d_name);
> +
> +    /* We only want files ending in .conf */
> +    if (len > 5 &&
> +            strcmp(".conf", &entry->d_name[len-5]) == 0)
> +        return 1;

QEMU prefers braces on single statement blocks.  Check out the 
CODING_STYLE file.  Also you'll want to run scripts/checkpatch.pl 
against your patches and make sure it's not flagging any issues.  It'll 
catch things like this.

> +
> +    return 0;
> +}
> +
>   static int parse_acl_file(const char *filename, ACLList *acl_list)
>   {
>       FILE *f;
>       char line[4096];
>       int ret = -EINVAL;
>       ACLRule *acl_rule;
> +    struct dirent **include_list = NULL;
> +    int i, include_count = 0;
> +    char *conf_file;
>
>       f = fopen(filename, "r");
>       if (f == NULL) {
> @@ -137,6 +153,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, alphasort);
> +            if (include_count < 0) {
> +                ret = -errno;
> +                fprintf(stderr, "Unable to retrieve conf files from '%s': %s\n",
> +                        arg, strerror(errno));
> +                goto failure;
> +            }
> +
> +            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);
> +                    ret = -ENOMEM;
> +                    goto failure;
> +                }
> +
> +                /* ignore errors like 'include' cmd */
> +                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);
> @@ -152,6 +199,14 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
>   failure:
>       fclose(f);
>
> +    if (include_list) {
> +        for (i = 0; i < include_count; i++) {
> +            if (include_list[i])
> +                free(include_list[i]);

Same comment here.

> +        }
> +        free(include_list);
> +    }
> +
>       return ret;
>   }
>

-- 
Regards,
Corey Bryant

Re: [Qemu-devel] [PATCHv2 2/2] bridge helper: support conf dirs

[Stefan Hajnoczi]

On Sat, Mar 02, 2013 at 12:58:48AM -0600, Doug Goldstein wrote:
> @@ -152,6 +199,14 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
>  failure:
>      fclose(f);
>  
> +    if (include_list) {
> +        for (i = 0; i < include_count; i++) {
> +            if (include_list[i])
> +                free(include_list[i]);

free(NULL) is a nop so the if isn't necessary - you can
free(include_list[i]) unconditionally.

[Qemu-devel] [PATCHv3 0/2] bridge helper: includedir conf arg

[Doug Goldstein]

The goal is to support an 'includedir' to include all files within a
directory specified in the bridge.conf file. The rationale is to allow
libvirt to be able to configure interfaces to for use by unprivileged
users by just simply generating a new configuration file to the directory.

Change from v2:
- Integrated review changes from Corey Bryant
- Integrated review changes from Stefan Hajnoczi

Change from v1:
- Reversed patch order to make the series clearer
- Integrated review changes from Corey Bryant
- Integrated review changes from Stefan Hajnoczi

Doug Goldstein (2):
  bridge helper: unified error cleanup for parse_acl_file
  bridge helper: support conf dirs

 qemu-bridge-helper.c | 83 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 71 insertions(+), 12 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCHv3 1/2] bridge helper: unified error cleanup for parse_acl_file

[Doug Goldstein]

Handle errors and cleanup from the error in a unified place for
parse_acl_file().

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 | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
index 287bfd5..95486e7 100644
--- a/qemu-bridge-helper.c
+++ b/qemu-bridge-helper.c
@@ -74,11 +74,12 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
 {
     FILE *f;
     char line[4096];
+    int ret = -EINVAL;
     ACLRule *acl_rule;
 
     f = fopen(filename, "r");
     if (f == NULL) {
-        return -1;
+        return -errno;
     }
 
     while (fgets(line, sizeof(line), f) != NULL) {
@@ -102,9 +103,8 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
 
         if (arg == NULL) {
             fprintf(stderr, "Invalid config line:\n  %s\n", line);
-            fclose(f);
-            errno = EINVAL;
-            return -1;
+            ret = -EINVAL;
+            goto failure;
         }
 
         *arg = 0;
@@ -142,15 +142,17 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
             parse_acl_file(arg, acl_list);
         } else {
             fprintf(stderr, "Unknown command `%s'\n", cmd);
-            fclose(f);
-            errno = EINVAL;
-            return -1;
+            ret = -EINVAL;
+            goto failure;
         }
     }
 
+    ret = 0;
+
+failure:
     fclose(f);
 
-    return 0;
+    return ret;
 }
 
 static bool has_vnet_hdr(int fd)
@@ -238,7 +240,7 @@ int main(int argc, char **argv)
     ACLRule *acl_rule;
     ACLList acl_list;
     int access_allowed, access_denied;
-    int ret = EXIT_SUCCESS;
+    int ret;
 
 #ifdef CONFIG_LIBCAP
     /* if we're run from an suid binary, immediately drop privileges preserving
@@ -272,9 +274,10 @@ int main(int argc, char **argv)
 
     /* parse default acl file */
     QSIMPLEQ_INIT(&acl_list);
-    if (parse_acl_file(DEFAULT_ACL_FILE, &acl_list) == -1) {
-        fprintf(stderr, "failed to parse default acl file `%s'\n",
-                DEFAULT_ACL_FILE);
+    ret = parse_acl_file(DEFAULT_ACL_FILE, &acl_list);
+    if (ret < 0) {
+        fprintf(stderr, "failed to parse default acl file `%s': %s\n",
+                DEFAULT_ACL_FILE, strerror(ret));
         ret = EXIT_FAILURE;
         goto cleanup;
     }
@@ -416,6 +419,7 @@ int main(int argc, char **argv)
     /* ... */
 
     /* profit! */
+    ret = EXIT_SUCCESS;
 
 cleanup:
 
-- 
1.7.12.4

[Qemu-devel] [PATCHv3 2/2] bridge helper: support conf dirs

[Doug Goldstein]

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 95486e7..cebfcf8 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,12 +71,28 @@ 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 len = strlen(entry->d_name);
+
+    /* We only want files ending in .conf */
+    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;
     char line[4096];
     int ret = -EINVAL;
     ACLRule *acl_rule;
+    struct dirent **include_list = NULL;
+    int i, include_count = 0;
+    char *conf_file;
 
     f = fopen(filename, "r");
     if (f == NULL) {
@@ -137,6 +154,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, alphasort);
+            if (include_count < 0) {
+                ret = -errno;
+                fprintf(stderr, "Unable to retrieve conf files from '%s': %s\n",
+                        arg, strerror(errno));
+                goto failure;
+            }
+
+            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);
+                    ret = -ENOMEM;
+                    goto failure;
+                }
+
+                /* ignore errors like 'include' cmd */
+                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);
@@ -152,6 +200,13 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
 failure:
     fclose(f);
 
+    if (include_list) {
+        for (i = 0; i < include_count; i++) {
+            free(include_list[i]);
+        }
+        free(include_list);
+    }
+
     return ret;
 }
 
-- 
1.7.12.4

Re: [Qemu-devel] [PATCHv3 2/2] bridge helper: support conf dirs

[Blue Swirl]

On Thu, Mar 7, 2013 at 6:32 AM, Doug Goldstein <cardoe@cardoe.com> 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 95486e7..cebfcf8 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,12 +71,28 @@ 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 len = strlen(entry->d_name);
> +
> +    /* We only want files ending in .conf */
> +    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;
>      char line[4096];
>      int ret = -EINVAL;
>      ACLRule *acl_rule;
> +    struct dirent **include_list = NULL;
> +    int i, include_count = 0;
> +    char *conf_file;
>
>      f = fopen(filename, "r");
>      if (f == NULL) {
> @@ -137,6 +154,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, alphasort);
> +            if (include_count < 0) {
> +                ret = -errno;
> +                fprintf(stderr, "Unable to retrieve conf files from '%s': %s\n",
> +                        arg, strerror(errno));
> +                goto failure;
> +            }
> +
> +            for (i = 0; i < include_count; i++) {
> +                if (asprintf(&conf_file, "%s/%s", arg,

Please use g_strdup_printf() and g_free() instead. This will make the
check go away too since it will not fail.

> +                             include_list[i]->d_name) < 0) {
> +                    fprintf(stderr, "Failed to allocate memory for "
> +                            "file path: %s/%s\n",
> +                            arg, include_list[i]->d_name);
> +                    ret = -ENOMEM;
> +                    goto failure;
> +                }
> +
> +                /* ignore errors like 'include' cmd */
> +                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);
> @@ -152,6 +200,13 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
>  failure:
>      fclose(f);
>
> +    if (include_list) {

This check is somewhat redundant since the for loop below will also do
nothing for cases where include_count is either zero (freed) or < 0
(failure).

> +        for (i = 0; i < include_count; i++) {
> +            free(include_list[i]);
> +        }
> +        free(include_list);
> +    }
> +
>      return ret;
>  }
>
> --
> 1.7.12.4
>
>

Re: [Qemu-devel] [PATCHv3 0/2] bridge helper: includedir conf arg

[Stefan Hajnoczi]

On Thu, Mar 07, 2013 at 12:32:08AM -0600, Doug Goldstein wrote:
> The goal is to support an 'includedir' to include all files within a
> directory specified in the bridge.conf file. The rationale is to allow
> libvirt to be able to configure interfaces to for use by unprivileged
> users by just simply generating a new configuration file to the directory.
> 
> Change from v2:
> - Integrated review changes from Corey Bryant
> - Integrated review changes from Stefan Hajnoczi
> 
> Change from v1:
> - Reversed patch order to make the series clearer
> - Integrated review changes from Corey Bryant
> - Integrated review changes from Stefan Hajnoczi
> 
> Doug Goldstein (2):
>   bridge helper: unified error cleanup for parse_acl_file
>   bridge helper: support conf dirs
> 
>  qemu-bridge-helper.c | 83 ++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 71 insertions(+), 12 deletions(-)
> 
> -- 
> 1.7.12.4
> 
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCHv3 0/2] bridge helper: includedir conf arg

[Corey Bryant]

On 03/07/2013 01:32 AM, Doug Goldstein wrote:
> The goal is to support an 'includedir' to include all files within a
> directory specified in the bridge.conf file. The rationale is to allow
> libvirt to be able to configure interfaces to for use by unprivileged
> users by just simply generating a new configuration file to the directory.
>
> Change from v2:
> - Integrated review changes from Corey Bryant
> - Integrated review changes from Stefan Hajnoczi
>
> Change from v1:
> - Reversed patch order to make the series clearer
> - Integrated review changes from Corey Bryant
> - Integrated review changes from Stefan Hajnoczi
>
> Doug Goldstein (2):
>    bridge helper: unified error cleanup for parse_acl_file
>    bridge helper: support conf dirs
>
>   qemu-bridge-helper.c | 83 ++++++++++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 71 insertions(+), 12 deletions(-)
>

Reviewed-by: Corey Bryant <coreyb@linux.vnet.ibm.com>

-- 
Regards,
Corey Bryant

[Qemu-devel] [PATCH v3 0/2] bridge helper: includedir conf arg

[Doug Goldstein]

The goal is to support an 'includedir' to include all files within a
directory specified in the bridge.conf file. The rationale is to allow
libvirt to be able to configure interfaces to for use by unprivileged
users by just simply generating a new configuration file to the directory.

Change from v3:
- Integreated review changes from Blue Swirl

Change from v2:
- Integrated review changes from Corey Bryant
- Integrated review changes from Stefan Hajnoczi

Change from v1:
- Reversed patch order to make the series clearer
- Integrated review changes from Corey Bryant
- Integrated review changes from Stefan Hajnoczi

Doug Goldstein (2):
  bridge helper: unified error cleanup for parse_acl_file
  bridge helper: support conf dirs

 qemu-bridge-helper.c | 75 +++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 63 insertions(+), 12 deletions(-)

-- 
1.8.1.5

[Qemu-devel] [PATCH v3 1/2] bridge helper: unified error cleanup for parse_acl_file

[Doug Goldstein]

Handle errors and cleanup from the error in a unified place for
parse_acl_file().

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Corey Bryant <coreyb@linux.vnet.ibm.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 | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
index 287bfd5..95486e7 100644
--- a/qemu-bridge-helper.c
+++ b/qemu-bridge-helper.c
@@ -74,11 +74,12 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
 {
     FILE *f;
     char line[4096];
+    int ret = -EINVAL;
     ACLRule *acl_rule;
 
     f = fopen(filename, "r");
     if (f == NULL) {
-        return -1;
+        return -errno;
     }
 
     while (fgets(line, sizeof(line), f) != NULL) {
@@ -102,9 +103,8 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
 
         if (arg == NULL) {
             fprintf(stderr, "Invalid config line:\n  %s\n", line);
-            fclose(f);
-            errno = EINVAL;
-            return -1;
+            ret = -EINVAL;
+            goto failure;
         }
 
         *arg = 0;
@@ -142,15 +142,17 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
             parse_acl_file(arg, acl_list);
         } else {
             fprintf(stderr, "Unknown command `%s'\n", cmd);
-            fclose(f);
-            errno = EINVAL;
-            return -1;
+            ret = -EINVAL;
+            goto failure;
         }
     }
 
+    ret = 0;
+
+failure:
     fclose(f);
 
-    return 0;
+    return ret;
 }
 
 static bool has_vnet_hdr(int fd)
@@ -238,7 +240,7 @@ int main(int argc, char **argv)
     ACLRule *acl_rule;
     ACLList acl_list;
     int access_allowed, access_denied;
-    int ret = EXIT_SUCCESS;
+    int ret;
 
 #ifdef CONFIG_LIBCAP
     /* if we're run from an suid binary, immediately drop privileges preserving
@@ -272,9 +274,10 @@ int main(int argc, char **argv)
 
     /* parse default acl file */
     QSIMPLEQ_INIT(&acl_list);
-    if (parse_acl_file(DEFAULT_ACL_FILE, &acl_list) == -1) {
-        fprintf(stderr, "failed to parse default acl file `%s'\n",
-                DEFAULT_ACL_FILE);
+    ret = parse_acl_file(DEFAULT_ACL_FILE, &acl_list);
+    if (ret < 0) {
+        fprintf(stderr, "failed to parse default acl file `%s': %s\n",
+                DEFAULT_ACL_FILE, strerror(ret));
         ret = EXIT_FAILURE;
         goto cleanup;
     }
@@ -416,6 +419,7 @@ int main(int argc, char **argv)
     /* ... */
 
     /* profit! */
+    ret = EXIT_SUCCESS;
 
 cleanup:
 
-- 
1.8.1.5

[Qemu-devel] [PATCH v3 2/2] bridge helper: support conf dirs

[Doug Goldstein]

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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Corey Bryant <coreyb@linux.vnet.ibm.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 | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
index 95486e7..b647848 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,12 +71,28 @@ 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 len = strlen(entry->d_name);
+
+    /* We only want files ending in .conf */
+    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;
     char line[4096];
     int ret = -EINVAL;
     ACLRule *acl_rule;
+    struct dirent **include_list = NULL;
+    int i, include_count = 0;
+    char *conf_file;
 
     f = fopen(filename, "r");
     if (f == NULL) {
@@ -137,6 +154,31 @@ 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, alphasort);
+            if (include_count < 0) {
+                ret = -errno;
+                fprintf(stderr, "Unable to retrieve conf files from '%s': %s\n",
+                        arg, strerror(errno));
+                goto failure;
+            }
+
+            for (i = 0; i < include_count; i++) {
+                conf_file = g_strdup_printf("%s/%s", arg,
+                                            include_list[i]->d_name);
+
+                /* ignore errors like 'include' cmd */
+                parse_acl_file(conf_file, acl_list);
+
+                g_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);
@@ -152,6 +194,11 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
 failure:
     fclose(f);
 
+    for (i = 0; i < include_count; i++) {
+        free(include_list[i]);
+    }
+    free(include_list);
+
     return ret;
 }
 
-- 
1.8.1.5

Re: [Qemu-devel] [PATCH v3 0/2] bridge helper: includedir conf arg

[Stefan Hajnoczi]

On Sun, Mar 17, 2013 at 11:17:19PM -0500, Doug Goldstein wrote:
> The goal is to support an 'includedir' to include all files within a
> directory specified in the bridge.conf file. The rationale is to allow
> libvirt to be able to configure interfaces to for use by unprivileged
> users by just simply generating a new configuration file to the directory.
> 
> Change from v3:
> - Integreated review changes from Blue Swirl
> 
> Change from v2:
> - Integrated review changes from Corey Bryant
> - Integrated review changes from Stefan Hajnoczi
> 
> Change from v1:
> - Reversed patch order to make the series clearer
> - Integrated review changes from Corey Bryant
> - Integrated review changes from Stefan Hajnoczi
> 
> Doug Goldstein (2):
>   bridge helper: unified error cleanup for parse_acl_file
>   bridge helper: support conf dirs
> 
>  qemu-bridge-helper.c | 75 +++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 63 insertions(+), 12 deletions(-)
> 
> -- 
> 1.8.1.5
> 
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>