Re: [libvirt] [Qemu-devel] [PATCH v2 2/3] monitor: Enable adding an inherited fd to an fd set
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