Re: [Qemu-devel] Safely reopening image files by stashing fds

[Kevin Wolf]

Am 10.08.2011 19:20, schrieb Blue Swirl:
> On Wed, Aug 10, 2011 at 7:58 AM, Kevin Wolf <address@hidden> wrote:
>> Am 09.08.2011 21:39, schrieb Blue Swirl:
>>> On Tue, Aug 9, 2011 at 12:24 PM, Kevin Wolf <address@hidden> 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