From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37024) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SyO7W-00082l-5H for qemu-devel@nongnu.org; Mon, 06 Aug 2012 10:17:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SyO7U-0003MK-TJ for qemu-devel@nongnu.org; Mon, 06 Aug 2012 10:17:22 -0400 Received: from e37.co.us.ibm.com ([32.97.110.158]:51962) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SyO7U-0003MA-Mw for qemu-devel@nongnu.org; Mon, 06 Aug 2012 10:17:20 -0400 Received: from /spool/local by e37.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 6 Aug 2012 08:17:18 -0600 Received: from d03relay05.boulder.ibm.com (d03relay05.boulder.ibm.com [9.17.195.107]) by d03dlp03.boulder.ibm.com (Postfix) with ESMTP id 417A719D8043 for ; Mon, 6 Aug 2012 14:16:06 +0000 (WET) Received: from d03av03.boulder.ibm.com (d03av03.boulder.ibm.com [9.17.195.169]) by d03relay05.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q76EFb4E123010 for ; Mon, 6 Aug 2012 08:15:53 -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 q76EFLMZ015369 for ; Mon, 6 Aug 2012 08:15:21 -0600 Message-ID: <501FD175.6010804@linux.vnet.ibm.com> Date: Mon, 06 Aug 2012 10:15:17 -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> In-Reply-To: <501FCBE5.3050007@redhat.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 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. -- Regards, Corey