From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:42633) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UCagp-0006Mb-9F for qemu-devel@nongnu.org; Mon, 04 Mar 2013 14:04:51 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UCagl-0004PF-BE for qemu-devel@nongnu.org; Mon, 04 Mar 2013 14:04:47 -0500 Received: from e39.co.us.ibm.com ([32.97.110.160]:59212) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UCagl-0004P9-3u for qemu-devel@nongnu.org; Mon, 04 Mar 2013 14:04:43 -0500 Received: from /spool/local by e39.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 4 Mar 2013 12:04:41 -0700 Received: from d03relay01.boulder.ibm.com (d03relay01.boulder.ibm.com [9.17.195.226]) by d03dlp01.boulder.ibm.com (Postfix) with ESMTP id 37F511FF004C for ; Mon, 4 Mar 2013 11:59:46 -0700 (MST) Received: from d03av03.boulder.ibm.com (d03av03.boulder.ibm.com [9.17.195.169]) by d03relay01.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r24J4OsO050972 for ; Mon, 4 Mar 2013 12:04:25 -0700 Received: from d03av03.boulder.ibm.com (loopback [127.0.0.1]) by d03av03.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r24J4NS2022131 for ; Mon, 4 Mar 2013 12:04:24 -0700 Message-ID: <5134F035.4020307@linux.vnet.ibm.com> Date: Mon, 04 Mar 2013 14:04:21 -0500 From: Corey Bryant MIME-Version: 1.0 References: <1361757620-23318-1-git-send-email-cardoe@cardoe.com> <1362207528-27804-1-git-send-email-cardoe@cardoe.com> <1362207528-27804-2-git-send-email-cardoe@cardoe.com> <5134CB81.5090502@linux.vnet.ibm.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCHv2 1/2] bridge helper: unified error cleanup for parse_acl_file List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Doug Goldstein Cc: Richa Marwaha , Anthony Liguori , qemu-devel@nongnu.org On 03/04/2013 01:53 PM, Doug Goldstein wrote: > On Mon, Mar 4, 2013 at 10:27 AM, Corey Bryant 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 >>> CC: Anthony Liguori >>> CC: Richa Marwaha >>> CC: Corey Bryant >>> 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