| Message ID | 1362207528-27804-2-git-send-email-cardoe@cardoe.com |
|---|---|
| State | New |
| Headers | show |
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. > ret = EXIT_FAILURE; >
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.
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.
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; } *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; } } + 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); ret = EXIT_FAILURE;
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(-)