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 <address@hidden> 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 <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.

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