From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54637) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SymuG-00044G-UL for qemu-devel@nongnu.org; Tue, 07 Aug 2012 12:45:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SymuF-0008Q5-7X for qemu-devel@nongnu.org; Tue, 07 Aug 2012 12:45:20 -0400 Received: from e31.co.us.ibm.com ([32.97.110.149]:43803) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SymuF-0008Pz-0h for qemu-devel@nongnu.org; Tue, 07 Aug 2012 12:45:19 -0400 Received: from /spool/local by e31.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 7 Aug 2012 10:45:16 -0600 Received: from d03relay04.boulder.ibm.com (d03relay04.boulder.ibm.com [9.17.195.106]) by d03dlp02.boulder.ibm.com (Postfix) with ESMTP id DEDE33E4006A for ; Tue, 7 Aug 2012 16:43:44 +0000 (WET) 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 q77GhI5Y167304 for ; Tue, 7 Aug 2012 10:43:19 -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 q77GhH0x013667 for ; Tue, 7 Aug 2012 10:43:17 -0600 Message-ID: <502145A3.3070402@linux.vnet.ibm.com> Date: Tue, 07 Aug 2012 12:43:15 -0400 From: Corey Bryant MIME-Version: 1.0 References: <1343048885-1701-1-git-send-email-coreyb@linux.vnet.ibm.com> <1343048885-1701-7-git-send-email-coreyb@linux.vnet.ibm.com> <500D4E42.8080709@linux.vnet.ibm.com> <501AFD68.8010009@linux.vnet.ibm.com> <501F8B27.2070508@redhat.com> <501FC775.1050800@linux.vnet.ibm.com> <501FCBE5.3050007@redhat.com> <501FD175.6010804@linux.vnet.ibm.com> In-Reply-To: <501FD175.6010804@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 6/6] block: Enable qemu_open/close to work with fd sets List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: aliguori@us.ibm.com, stefanha@linux.vnet.ibm.com, libvir-list@redhat.com, qemu-devel@nongnu.org, Luiz Capitulino , eblake@redhat.com On 08/06/2012 10:15 AM, Corey Bryant wrote: > > > On 08/06/2012 09:51 AM, Kevin Wolf wrote: >> Am 06.08.2012 15:32, schrieb Corey Bryant: >>> On 08/06/2012 05:15 AM, Kevin Wolf wrote: >>>> Am 03.08.2012 00:21, schrieb Corey Bryant: >>>>>>> @@ -84,6 +158,36 @@ int qemu_open(const char *name, int flags, ...) >>>>>>> int ret; >>>>>>> int mode = 0; >>>>>>> >>>>>>> +#ifndef _WIN32 >>>>>>> + const char *fdset_id_str; >>>>>>> + >>>>>>> + /* Attempt dup of fd from fd set */ >>>>>>> + if (strstart(name, "/dev/fdset/", &fdset_id_str)) { >>>>>>> + int64_t fdset_id; >>>>>>> + int fd, dupfd; >>>>>>> + >>>>>>> + fdset_id = qemu_parse_fdset(fdset_id_str); >>>>>>> + if (fdset_id == -1) { >>>>>>> + errno = EINVAL; >>>>>>> + return -1; >>>>>>> + } >>>>>>> + >>>>>>> + fd = monitor_fdset_get_fd(default_mon, fdset_id, flags); >>>>>> >>>>>> I know that use of default_mon in this patch is not correct, but I >>>>>> wanted to get these patches out for review. I used default_mon for >>>>>> testing because cur_mon wasn't pointing to the monitor I'd added >>>>>> fd sets >>>>>> to. I need to figure out why. >>>>>> >>>>> >>>>> Does it make sense to use default_mon here? After digging into this >>>>> some more, I'm thinking it makes sense, and I'll explain why. >>>>> >>>>> It looks like cur_mon can't be used. cur_mon will point to the >>>>> monitor >>>>> object for the duration of a command, and be reset to old_mon (NULL in >>>>> my case) after the command completes. >>>>> >>>>> qemu_open() and qemu_close() are frequently called long after a >>>>> monitor >>>>> command has come and gone, so cur_mon won't work. For example, >>>>> drive_add will cause qemu_open() to be called, but after the >>>>> command has >>>>> completed, the file will keep getting opened/closed during normal QEMU >>>>> operation. I'm not sure why, I've just noticed this behavior. >>>>> >>>>> Does anyone have any thoughts on this? It would require fd sets to be >>>>> added to the default monitor only. >>>> >>>> I think we have two design options that would make sense: >>>> >>>> 1. Make the file descriptors global instead of per-monitor. Is there a >>>> reason why each monitor has its own set of fds? (Also I'm >>>> wondering >>>> if they survive a monitor disconnect this way?) >>> >>> I'd prefer to have them associated with a monitor object so that we can >>> more easily keep track of whether or not they're in use by a monitor >>> connection. >> >> Hm, I see. >> >>>> 2. Save a monitor reference with the fdset information. >>>> >>> >>> Are you saying that each monitor would have the same copy of fdset >>> information? >> >> This one doesn't really make sense indeed... >> >>> >>>> Allowing to send file descriptors on every monitor, but making only >>>> those of the default monitor actually usable, sounds like a bad choice >>>> to me. >>> >>> What if we also allow them to be added only to the default monitor? >> >> Would get you some kind of consistency at least, even though I don't >> like that secondary monitors can't use the functionality. >> >> Can't we make the fdset information global, so that a qemu_open/close() >> searches all of them, but let it have a Monitor* owner for keeping track >> whether it's in use? > > I think global fdsets might work (sorry I didn't think it through enough > on my first reply). I think I'll need to drop the "in_use" flag and tie > monitor references into the refcount. (I know I know, you suggested > that a while back.. :). I'll give it a shot and see how it goes. I just submitted the v7 patch series which makes fd sets global, rather than each Monitor object having an fd set. This allows the list of fd sets to be shared among all monitor connections. -- Regards, Corey