Patchwork [2/2] bridge helper: unified error cleanup for parse_acl_file

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

Comments

Doug Goldstein - Feb. 25, 2013, 2 a.m.
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 | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)
Corey Bryant - Feb. 25, 2013, 10:41 p.m.
On 02/24/2013 09:00 PM, 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 | 32 +++++++++++++++++++++-----------
>   1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
> index b8771a3..d95e760 100644
> --- a/qemu-bridge-helper.c
> +++ b/qemu-bridge-helper.c
> @@ -96,6 +96,9 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
>       FILE *f;
>       char line[4096];
>       ACLRule *acl_rule;
> +    struct dirent **include_list = NULL;
> +    int i, include_count = 0;
> +    char *conf_file = NULL;
>
>       f = fopen(filename, "r");
>       if (f == NULL) {
> @@ -105,9 +108,6 @@ 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++;
> @@ -126,9 +126,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;
> +            goto cleanup;
>           }
>
>           *arg = 0;
> @@ -167,8 +166,7 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
>               if (include_count < 0) {
>                   fprintf(stderr, "Unable to retrieve conf files from '%s': %s\n",
>                           arg, strerror(errno));
> -                fclose(f);
> -                return -1;
> +                goto cleanup;
>               }
>
>               for (i = 0; i < include_count; i++) {
> @@ -177,9 +175,8 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
>                       fprintf(stderr, "Failed to allocate memory for "
>                               "file path: %s/%s\n",
>                               arg, include_list[i]->d_name);
> -                    fclose(f);
>                       errno = ENOMEM;
> -                    return -1;
> +                    goto cleanup;
>                   }
>
>                   parse_acl_file(conf_file, acl_list);
> @@ -197,15 +194,28 @@ 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;
> +            goto cleanup;
>           }
>       }
>
>       fclose(f);
>
>       return 0;
> +
> +cleanup:

Maybe change this label to "failure:" since it's only a failure path?

Also I think you want to save and restore errno in this path.  For 
example if fclose() fails it will overwrite errno=EINVAL or whatever you 
had set it to in the above failure paths.

> +
> +    fclose(f);
> +
> +    if (include_list) {
> +        for (i = 0; i < include_count; i++) {
> +            if (include_list[i])
> +                free(include_list[i]);
> +        }
> +        free(include_list);
> +    }
> +
> +    return -1;
>   }
>
>   static bool has_vnet_hdr(int fd)
>

Patch

diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
index b8771a3..d95e760 100644
--- a/qemu-bridge-helper.c
+++ b/qemu-bridge-helper.c
@@ -96,6 +96,9 @@  static int parse_acl_file(const char *filename, ACLList *acl_list)
     FILE *f;
     char line[4096];
     ACLRule *acl_rule;
+    struct dirent **include_list = NULL;
+    int i, include_count = 0;
+    char *conf_file = NULL;
 
     f = fopen(filename, "r");
     if (f == NULL) {
@@ -105,9 +108,6 @@  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++;
@@ -126,9 +126,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;
+            goto cleanup;
         }
 
         *arg = 0;
@@ -167,8 +166,7 @@  static int parse_acl_file(const char *filename, ACLList *acl_list)
             if (include_count < 0) {
                 fprintf(stderr, "Unable to retrieve conf files from '%s': %s\n",
                         arg, strerror(errno));
-                fclose(f);
-                return -1;
+                goto cleanup;
             }
 
             for (i = 0; i < include_count; i++) {
@@ -177,9 +175,8 @@  static int parse_acl_file(const char *filename, ACLList *acl_list)
                     fprintf(stderr, "Failed to allocate memory for "
                             "file path: %s/%s\n",
                             arg, include_list[i]->d_name);
-                    fclose(f);
                     errno = ENOMEM;
-                    return -1;
+                    goto cleanup;
                 }
 
                 parse_acl_file(conf_file, acl_list);
@@ -197,15 +194,28 @@  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;
+            goto cleanup;
         }
     }
 
     fclose(f);
 
     return 0;
+
+cleanup:
+    
+    fclose(f);
+
+    if (include_list) {
+        for (i = 0; i < include_count; i++) {
+            if (include_list[i])
+                free(include_list[i]);
+        }
+        free(include_list);
+    }
+
+    return -1;
 }
 
 static bool has_vnet_hdr(int fd)