From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44597) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T0xad-000847-PG for qemu-devel@nongnu.org; Mon, 13 Aug 2012 12:34:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T0xab-0001N6-Ee for qemu-devel@nongnu.org; Mon, 13 Aug 2012 12:34:03 -0400 Received: from e8.ny.us.ibm.com ([32.97.182.138]:43563) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T0xab-0001Mu-AY for qemu-devel@nongnu.org; Mon, 13 Aug 2012 12:34:01 -0400 Received: from /spool/local by e8.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 13 Aug 2012 12:33:59 -0400 Received: from d01relay02.pok.ibm.com (d01relay02.pok.ibm.com [9.56.227.234]) by d01dlp03.pok.ibm.com (Postfix) with ESMTP id 650CCC9006D for ; Mon, 13 Aug 2012 12:33:55 -0400 (EDT) Received: from d01av03.pok.ibm.com (d01av03.pok.ibm.com [9.56.224.217]) by d01relay02.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q7DGXsYv147304 for ; Mon, 13 Aug 2012 12:33:55 -0400 Received: from d01av03.pok.ibm.com (loopback [127.0.0.1]) by d01av03.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q7DGXssr005817 for ; Mon, 13 Aug 2012 13:33:54 -0300 Message-ID: <50292C70.6040903@linux.vnet.ibm.com> Date: Mon, 13 Aug 2012 12:33:52 -0400 From: Corey Bryant MIME-Version: 1.0 References: <1344690878-1555-1-git-send-email-coreyb@linux.vnet.ibm.com> <1344690878-1555-7-git-send-email-coreyb@linux.vnet.ibm.com> <50266C28.6070908@redhat.com> <502904C6.6050903@linux.vnet.ibm.com> <50292876.60808@redhat.com> In-Reply-To: <50292876.60808@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v9 6/7] block: Enable qemu_open/close to work with fd sets List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, aliguori@us.ibm.com, stefanha@linux.vnet.ibm.com, libvir-list@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, pbonzini@redhat.com On 08/13/2012 12:16 PM, Eric Blake wrote: > On 08/13/2012 07:44 AM, Corey Bryant wrote: >> I'll send a new version shortly with these updates also. >> > >>>> + >>>> + ret = monitor_fdset_dup_fd_add(fdset_id, dupfd); >>>> + if (ret == -1) { >>>> + close(dupfd); >>>> + return -1; >>> >>> This function appears to promise a reasonable errno on failure. > > Actually, looking at that function again, > > > +int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd) > +{ > + MonFdset *mon_fdset; > + MonFdsetFd *mon_fdset_fd_dup; > + > + QLIST_FOREACH(mon_fdset, &mon_fdsets, next) { > + if (mon_fdset->id != fdset_id) { > + continue; > + } > + QLIST_FOREACH(mon_fdset_fd_dup, &mon_fdset->dup_fds, next) { > + if (mon_fdset_fd_dup->fd == dup_fd) { > + return -1; > + } > + } > + mon_fdset_fd_dup = g_malloc0(sizeof(*mon_fdset_fd_dup)); > + mon_fdset_fd_dup->fd = dup_fd; > + QLIST_INSERT_HEAD(&mon_fdset->dup_fds, mon_fdset_fd_dup, next); > + return 0; > + } > + return -1; > +} > > The only way it could fail is if we are trying to add an fd that is > already in the set, or if we don't find mon_fdset; both of which would > indicate logic bugs earlier in our program. Would it be worth asserting > that these conditions are impossible, and making this function return > void (the addition is always successful if it returns, since g_malloc0 > aborts rather than failing with ENOMEM)? I think what I did in v10 should suffice. I didn't update monitor_fdset_dup_fd_add(), but I did update the calling code. If the call fails then I set errno to EINVAL since (unless there's a bug) the only possible error is that the fdset ID was non-existent. It makes sense to add the asserts, but at this point I'd like to stick with what we have in v10 if that's ok. > > And the more I think about it, the more I think that qemu_open MUST > provide a sane errno value on exit, so you need to make sure that all > exit paths out of qemu_open have a sensible errno (whether or not the > helper functions also have to leave errno sane is a matter of taste). > Yes, I agree. I went through the code and at this point (with the v10 patches) we're always setting errno, or calling a library API that should be setting it. -- Regards, Corey