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