[libvirt] [Qemu-devel] [PATCH v2 2/3] monitor: Enable adding an inherited fd to an fd set
Corey Bryant
coreyb at linux.vnet.ibm.com
Thu Oct 11 15:04:21 UTC 2012
On 10/11/2012 07:25 AM, Kevin Wolf wrote:
> Am 10.10.2012 16:20, schrieb Corey Bryant:
>> qmp_add_fd() gets an fd that was received over a socket with
>> SCM_RIGHTS and adds it to an fd set. This patch adds support
>> that will enable adding an fd that was inherited on the
>> command line to an fd set.
>>
>> This patch also prevents removal of an fd from an fd set during
>> initialization. This allows the fd to remain in the fd set after
>> probing of the image file.
>
> "This patch also..." usually means that it should be split in two
> patches. Though in this case I'd vote for immediately dropping the
> second patch again: This makes the probing work with file descriptors
> using a hack for a certain situation (namely qemu startup) and leaves
> other cases (like hotplug) broken.
I don't think hotplug is broken. In that case the fd will only be
removed from the fd set if the following is true:
(mon_fdset_fd->removed || (QLIST_EMPTY(&mon_fdset->dup_fds) &&
mon_refcount == 0))
We can ignore the removed part for now. What's important here is that
if there are no dup_fd references and there is at least one monitor
connected, an fd will *not* be removed.
The problem with the command line -add-fd is that there is no equivalent
to mon_refcount. So I was using the check for runstate_is_running() as
a (somewhat) equivalent to mon_refcount. In other words, if an fd is
added to an fd set via the command line and it is not referenced by
another command line option (ie. -drive or -blockdev), then clean it up
after QEMU initialization.
>
> I think it's kind of acceptable to leave it broken for both cases in
> this patch series, you can work around it by passing the 'format'
> option. If we do fix it, however, we should fix it consistently for all
> use cases. The best approach for this is probably to avoid opening the
> image file twice for probing; instead, we should first open the
> BlockDriverState for raw-posix, use that for probing and then put a
> second qcow2 or whatever BlockDriverState on top of the very same BDS
> without reopening it.
>
>
> The rest is code motion, except for:
>
> - if (mon_fdset->id == 0) {
> + if (!mon_fdset_cur) {
>
> It's not nice to hide such changes in code motion patches, but the
> change is obviously not wrong.
Sorry about that. I didn't mean to hide it. This probably makes more
sense in patch 1.
>
> The commit message should be changed to tell that this is (mostly) code
> motion. This doesn't really change anything semantically (except for the
> probing part).
Ok
--
Regards,
Corey Bryant
More information about the libvir-list
mailing list