Re: [Qemu-devel] [PATCH v2 2/3] monitor: Enable adding an inherited fd to an fd set

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

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