From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:34651) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RLEtD-0006xG-DR for qemu-devel@nongnu.org; Tue, 01 Nov 2011 10:00:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RLEt6-0006rF-GJ for qemu-devel@nongnu.org; Tue, 01 Nov 2011 10:00:31 -0400 Received: from e5.ny.us.ibm.com ([32.97.182.145]:55407) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RLEt6-0006qg-CR for qemu-devel@nongnu.org; Tue, 01 Nov 2011 10:00:24 -0400 Received: from /spool/local by e5.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 1 Nov 2011 09:54:11 -0400 Received: from d03av05.boulder.ibm.com (d03av05.boulder.ibm.com [9.17.195.85]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id pA1Dr0qv277332 for ; Tue, 1 Nov 2011 09:53:00 -0400 Received: from d03av05.boulder.ibm.com (loopback [127.0.0.1]) by d03av05.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id pA1DqxjF019909 for ; Tue, 1 Nov 2011 07:52:59 -0600 Message-ID: <4EAFF9B9.90401@linux.vnet.ibm.com> Date: Tue, 01 Nov 2011 09:52:57 -0400 From: Corey Bryant MIME-Version: 1.0 References: <1320086191-23641-1-git-send-email-coreyb@linux.vnet.ibm.com> <1320086191-23641-2-git-send-email-coreyb@linux.vnet.ibm.com> <20111101081514.GA27890@stefanha-thinkpad.localdomain> In-Reply-To: <20111101081514.GA27890@stefanha-thinkpad.localdomain> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 1/4] Add basic version of bridge helper List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: rmarwah@linux.vnet.ibm.com, aliguori@us.ibm.com, qemu-devel@nongnu.org On 11/01/2011 04:15 AM, Stefan Hajnoczi wrote: > On Mon, Oct 31, 2011 at 02:36:28PM -0400, Corey Bryant wrote: > A couple of nitpicks regarding error handling: > >> +static int has_vnet_hdr(int fd) >> +{ >> + unsigned int features = 0; >> + struct ifreq ifreq; >> + >> + if (ioctl(fd, TUNGETFEATURES,&features) == -1) { >> + return -errno; >> + } >> + >> + if (!(features& IFF_VNET_HDR)) { >> + return -ENOTSUP; >> + } >> + >> + if (ioctl(fd, TUNGETIFF,&ifreq) != -1 || errno != EBADFD) { >> + return -ENOTSUP; >> + } >> + >> + return 1; >> +} > > This function is strange, it looks like a boolean function but actually > only returns 1 or -errno. It is used incorrectly in main(). I suggest > changing the return value to bool and returning false on error. > Ah, good catch, this was a bug. And I agree that bool would work better. I'll fix this. >> + /* open a socket to use to control the network interfaces */ >> + ctlfd = socket(AF_INET, SOCK_STREAM, 0); >> + if (ctlfd == -1) { >> + fprintf(stderr, "failed to open control socket\n"); >> + ret = -errno; > > It's better to stash away errno before invoking other library functions. > man errno(3) says: > > "a function that succeeds is allowed to change errno" > > This means fprintf(3) could clobber errno. > > I suggest simply printing out errno with the error message and returning > exit code 1 (EXIT_FAILURE). The same applies for the other error exit > cases in main(). > I agree. I'll fix this. >> +cleanup: >> + >> + close(fd); >> + >> + close(ctlfd); > > ctlfd is an uninitialized variable if opening fd fails. We also never > close unixfd. > > I'd remove this cleanup code and just return without closing any file > descriptors - let the kernel do it. Ok, I'll do this. But I think I'll re-introduce the cleanup goto in patch 2/4 to free the simple queue memory. -- Regards, Corey > > Stefan >