From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49031) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TMKNl-000453-VP for qemu-devel@nongnu.org; Thu, 11 Oct 2012 11:09:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TMKNh-0002xg-Lf for qemu-devel@nongnu.org; Thu, 11 Oct 2012 11:09:05 -0400 Received: from e34.co.us.ibm.com ([32.97.110.152]:54631) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TMKNh-0002xa-Eh for qemu-devel@nongnu.org; Thu, 11 Oct 2012 11:09:01 -0400 Received: from /spool/local by e34.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 11 Oct 2012 09:09:00 -0600 Received: from d03relay04.boulder.ibm.com (d03relay04.boulder.ibm.com [9.17.195.106]) by d03dlp03.boulder.ibm.com (Postfix) with ESMTP id 5699C19D80E5 for ; Thu, 11 Oct 2012 09:04:51 -0600 (MDT) Received: from d03av03.boulder.ibm.com (d03av03.boulder.ibm.com [9.17.195.169]) by d03relay04.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q9BF4oQo090968 for ; Thu, 11 Oct 2012 09:04:50 -0600 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 q9BF4P25016705 for ; Thu, 11 Oct 2012 09:04:26 -0600 Message-ID: <5076DFF5.90001@linux.vnet.ibm.com> Date: Thu, 11 Oct 2012 11:04:21 -0400 From: Corey Bryant MIME-Version: 1.0 References: <1349878805-16352-1-git-send-email-coreyb@linux.vnet.ibm.com> <1349878805-16352-3-git-send-email-coreyb@linux.vnet.ibm.com> <5076AC91.4000109@redhat.com> In-Reply-To: <5076AC91.4000109@redhat.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 2/3] monitor: Enable adding an inherited fd to an fd set List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: libvir-list@redhat.com, qemu-devel@nongnu.org On 10/11/2012 07:25 AM, Kevin Wolf wrote: > Am 10.10.2012 16:20, schrieb Corey Bryant: >> qmp_add_fd() gets an fd that was received over a socket with >> SCM_RIGHTS and adds it to an fd set. This patch adds support >> that will enable adding an fd that was inherited on the >> command line to an fd set. >> >> This patch also prevents removal of an fd from an fd set during >> initialization. This allows the fd to remain in the fd set after >> probing of the image file. > > "This patch also..." usually means that it should be split in two > patches. Though in this case I'd vote for immediately dropping the > second patch again: This makes the probing work with file descriptors > using a hack for a certain situation (namely qemu startup) and leaves > other cases (like hotplug) broken. I don't think hotplug is broken. In that case the fd will only be removed from the fd set if the following is true: (mon_fdset_fd->removed || (QLIST_EMPTY(&mon_fdset->dup_fds) && mon_refcount == 0)) We can ignore the removed part for now. What's important here is that if there are no dup_fd references and there is at least one monitor connected, an fd will *not* be removed. The problem with the command line -add-fd is that there is no equivalent to mon_refcount. So I was using the check for runstate_is_running() as a (somewhat) equivalent to mon_refcount. In other words, if an fd is added to an fd set via the command line and it is not referenced by another command line option (ie. -drive or -blockdev), then clean it up after QEMU initialization. > > I think it's kind of acceptable to leave it broken for both cases in > this patch series, you can work around it by passing the 'format' > option. If we do fix it, however, we should fix it consistently for all > use cases. The best approach for this is probably to avoid opening the > image file twice for probing; instead, we should first open the > BlockDriverState for raw-posix, use that for probing and then put a > second qcow2 or whatever BlockDriverState on top of the very same BDS > without reopening it. > > > The rest is code motion, except for: > > - if (mon_fdset->id == 0) { > + if (!mon_fdset_cur) { > > It's not nice to hide such changes in code motion patches, but the > change is obviously not wrong. Sorry about that. I didn't mean to hide it. This probably makes more sense in patch 1. > > The commit message should be changed to tell that this is (mostly) code > motion. This doesn't really change anything semantically (except for the > probing part). Ok -- Regards, Corey Bryant