On Tue, Jun 30, 2015 at 11:52:35 +0200, Michal Privoznik wrote:
On 30.06.2015 10:57, Peter Krempa wrote:
> processSerialChangedEvent processes events for all channels. Commit
> 2af51483 broke all agent interaction if a channel other than the agent
> closes since it did not check that the event actually originated from
> the guest agent channel.
>
> Resolves:
https://bugzilla.redhat.com/show_bug.cgi?id=1236924
> Fixes up:
https://bugzilla.redhat.com/show_bug.cgi?id=890648
> ---
> src/qemu/qemu_driver.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 2b530c8..353be31 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4448,10 +4448,20 @@ processSerialChangedEvent(virQEMUDriverPtr driver,
>
> if (newstate == VIR_DOMAIN_CHR_DEVICE_STATE_DISCONNECTED &&
> virDomainObjIsActive(vm) && priv->agent) {
> - /* Close agent monitor early, so that other threads
> - * waiting for the agent to reply can finish and our
> - * job we acquire below can succeed. */
> - qemuAgentNotifyClose(priv->agent);
> + /* peek into the domain definition to find the channel */
> + if (virDomainDefFindDevice(vm->def, devAlias, &dev, true) == 0
&&
> + dev.type == VIR_DOMAIN_DEVICE_CHR &&
> + dev.data.chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL
&&
> + dev.data.chr->targetType ==
VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO &&
> + STREQ_NULLABLE(dev.data.chr->target.name,
"org.qemu.guest_agent.0"))
> + /* Close agent monitor early, so that other threads
> + * waiting for the agent to reply can finish and our
> + * job we acquire below can succeed. */
> + qemuAgentNotifyClose(priv->agent);
> +
> + /* now discard the data, since it may possibly change once we unlock
> + * while entering the job */
> + memset(&dev, 0, sizeof(dev));
Well, it's not used anywhere in between here and the other place where
the variable is re-initialized with another virDomainDefFindDevice()
call. But it doesn't hurt either.
I want be super sure in this case that somebody does not optimize that
out later in some way.
> }
>
> if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
>
ACK and safe for the freeze.
Pushed; Thanks.
Peter