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