qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Corey Bryant
Subject: Re: [Qemu-devel] [PATCHv2 1/2] bridge helper: unified error cleanup for parse_acl_file
Date: Mon, 04 Mar 2013 14:04:21 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130219 Thunderbird/17.0.3



On 03/04/2013 01:53 PM, Doug Goldstein wrote:
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.


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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]