From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:51462) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QrPna-0007M2-VX for qemu-devel@nongnu.org; Thu, 11 Aug 2011 03:35:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QrPnW-0004pN-2R for qemu-devel@nongnu.org; Thu, 11 Aug 2011 03:35:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:16467) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QrPnV-0004p0-RZ for qemu-devel@nongnu.org; Thu, 11 Aug 2011 03:35:22 -0400 Message-ID: <4E4386D3.9070203@redhat.com> Date: Thu, 11 Aug 2011 09:37:55 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <4E3BB2C3.4020607@redhat.com> <4E3BBC64.9020005@redhat.com> <4E3FFDE5.1020802@redhat.com> <4E410D68.1060701@redhat.com> <4E411C61.6050000@redhat.com> <20110809120014.GA11165@stefanha-thinkpad.localdomain> <4E4126F7.2090609@redhat.com> <4E423A17.2050707@redhat.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] Safely reopening image files by stashing fds List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: Stefan Hajnoczi , Anthony Liguori , qemu-devel , Supriya Kannery Am 10.08.2011 19:20, schrieb Blue Swirl: > On Wed, Aug 10, 2011 at 7:58 AM, Kevin Wolf wrote: >> Am 09.08.2011 21:39, schrieb Blue Swirl: >>> On Tue, Aug 9, 2011 at 12:24 PM, Kevin Wolf wrote: >>>> Am 09.08.2011 14:00, schrieb Stefan Hajnoczi: >>>>> I liked the idea of doing a generic FDStash type that the monitor and >>>>> bdrv_reopen() can use. Blue's idea to hook at the qemu_open() level >>>>> takes that further. >>>> >>>> Well, having something that works for the raw-posix, the monitor and >>>> maybe some more things is nice. Having something that works for >>>> raw-posix, Sheepdog and rbd is an absolute requirement and I can't see >>>> how FDStash solves that. >>> >>> For Sheepdog also network access functions would need to be hooked. >>> RBD seems to use librados functions for low level I/O, so that needs >>> some RBD specific wrappers. >>> >>>> Even raw-win32 doesn't have an int fd, but a >>>> HANDLE hfile for its backend. >>> >>> Just replace "int fd" with "FDStash fd" everywhere? >> >> And how is FDStash defined? The current proposed is this one: >> >> /* A stashed file descriptor */ >> typedef FDEntry { >> const char *name; >> int fd; >> QLIST_ENTRY(FDEntry) next; >> } FDEntry; >> >> /* A container for stashing file descriptors */ >> typedef struct FDStash { >> QLIST_HEAD(, FDEntry) fds; >> } FDStash; >> >> So there we have the int fd again. If you want something generic, you > > What's the problem: > typedef struct FDEntry { > const char *name; > #ifdef _WIN32 > HANDLE fd; > #else > int fd; > #endif > QLIST_ENTRY(FDEntry) next; > } FDEntry; > > Renaming 'fd' to something that does not immediately look like 'int' > may be useful. This isn't any more generic, it merely covers two special cases instead of only one. You have used the fact that raw-posix and raw-win32 are never compiled in at the same time, so that you can use an #ifdef to conditionally pull out parts of their internal data structures into a generic struct (which in itself is a layering violation) It doesn't help with the other backend drivers like curl, nbd, rbd, sheepdog and last but not least our special friend vvfat. >> would have to start letting block drivers extend FDEntry with their own >> fields (and how would the first bdrv_open work then?) and basically >> arrive at something like a BDRVReopenState. > > I'd not handle FDEntries at block level at all (or only at raw level), > then there would not be first mover problems. Well, but how does it solve the bdrv_reopen problem if it's not visible on the block level? > Extending the uses of FDEntries to for example network stuff > (Sheepdog) could need some kind of unified API for both file and net > operations which is not what we have now (bdrv_read/write etc. vs. > direct recv/send). Then this new API would use FDEntries instead of > fds, sockets or HANDLEs. The 'name' field could be either network > address or file path. Though maybe we are only interested in > open/connect part so unification would not be needed for other > operations. Now you have handled three special cases, but still haven't got a generic solution. But considering that you don't want to touch the block layer API and therefore I don't even have an idea of what you would use FDEntries for... Let's go back one step: What problem are you trying to solve, and what does the API look like that you're thinking of? It doesn't seem to be the same as Stefan suggested. Kevin