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

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

On 03/04/2013 01:53 PM, Doug Goldstein wrote:
> On Mon, Mar 4, 2013 at 10:27 AM, Corey Bryant <coreyb@linux.vnet.ibm.com> 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 <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 | 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.
>

Yes that makes sense.  Or you could save and restore errno at the 
beginning and end of the cleanup path.  Either way.

-- 
Regards,
Corey Bryant