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

[Corey Bryant]

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


--
Regards,
Corey Bryant