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 <address@hidden>
> CC: Anthony Liguori <address@hidden>
> CC: Richa Marwaha <address@hidden>
> CC: Corey Bryant <address@hidden>
> TO: address@hidden
> ---
>   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