Re: [libvirt] [Qemu-devel] [PATCH v5 6/6] block: Enable qemu_open/close to work with fd sets

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?