[libvirt] [PATCH 0/5] qemu: Connect to guest agent after channel hotplug
Clean up some parts of the agent code, fix a recently added bug and add support for guest agent hotplug, all this while removing more lines than adding. Peter Krempa (5): qemu: agent: Reuse virJSONValueObjectCreateVArgs in qemuAgentMakeCommand qemu: Fix domain object leak in qemuDomainInterfaceAddresses qemu: Reuse qemuDomainAgentAvailable in qemuDomainInterfaceAddresses qemu: agent: Differentiate errors when the agent channel was hotplugged qemu: Connect to guest agent after channel hotplug src/qemu/qemu_agent.c | 73 ++----------------------------------------------- src/qemu/qemu_domain.c | 45 +++++++++++++++++++++++++++--- src/qemu/qemu_domain.h | 2 ++ src/qemu/qemu_driver.c | 56 +++++++++++++++++++++---------------- src/qemu/qemu_process.c | 22 +-------------- src/qemu/qemu_process.h | 2 ++ 6 files changed, 81 insertions(+), 119 deletions(-) -- 2.3.5
Since the code is now separated into the common helper, we can reuse it instead of maintaining two copies. --- src/qemu/qemu_agent.c | 73 ++------------------------------------------------- 1 file changed, 2 insertions(+), 71 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 548d580..3f6a9bf 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1136,7 +1136,6 @@ qemuAgentMakeCommand(const char *cmdname, virJSONValuePtr obj; virJSONValuePtr jargs = NULL; va_list args; - char *key; va_start(args, cmdname); @@ -1146,76 +1145,8 @@ qemuAgentMakeCommand(const char *cmdname, if (virJSONValueObjectAppendString(obj, "execute", cmdname) < 0) goto error; - while ((key = va_arg(args, char *)) != NULL) { - int ret; - char type; - - if (strlen(key) < 3) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("argument key '%s' is too short, missing type prefix"), - key); - goto error; - } - - /* Keys look like s:name the first letter is a type code */ - type = key[0]; - key += 2; - - if (!jargs && - !(jargs = virJSONValueNewObject())) - goto error; - - /* This doesn't support maps/arrays. This hasn't - * proved to be a problem..... yet :-) */ - switch (type) { - case 's': { - char *val = va_arg(args, char *); - ret = virJSONValueObjectAppendString(jargs, key, val); - } break; - case 'i': { - int val = va_arg(args, int); - ret = virJSONValueObjectAppendNumberInt(jargs, key, val); - } break; - case 'u': { - unsigned int val = va_arg(args, unsigned int); - ret = virJSONValueObjectAppendNumberUint(jargs, key, val); - } break; - case 'I': { - long long val = va_arg(args, long long); - ret = virJSONValueObjectAppendNumberLong(jargs, key, val); - } break; - case 'U': { - /* qemu silently truncates numbers larger than LLONG_MAX, - * so passing the full range of unsigned 64 bit integers - * is not safe here. Pass them as signed 64 bit integers - * instead. - */ - long long val = va_arg(args, long long); - ret = virJSONValueObjectAppendNumberLong(jargs, key, val); - } break; - case 'd': { - double val = va_arg(args, double); - ret = virJSONValueObjectAppendNumberDouble(jargs, key, val); - } break; - case 'b': { - int val = va_arg(args, int); - ret = virJSONValueObjectAppendBoolean(jargs, key, val); - } break; - case 'n': { - ret = virJSONValueObjectAppendNull(jargs, key); - } break; - case 'a': { - virJSONValuePtr val = va_arg(args, virJSONValuePtr); - ret = virJSONValueObjectAppend(jargs, key, val); - } break; - default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unsupported data type '%c' for arg '%s'"), type, key - 2); - goto error; - } - if (ret < 0) - goto error; - } + if (virJSONValueObjectCreateVArgs(&jargs, args) < 0) + goto error; if (jargs && virJSONValueObjectAppend(obj, "arguments", jargs) < 0) -- 2.3.5
The API didn't use virDomainObjEndAPI to release the domain object thus it leaked a reference to it. --- src/qemu/qemu_driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 989c20c..7d2c5ed 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19789,8 +19789,7 @@ qemuDomainInterfaceAddresses(virDomainPtr dom, } cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } -- 2.3.5
--- src/qemu/qemu_driver.c | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7d2c5ed..ce38e36 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19750,27 +19750,11 @@ qemuDomainInterfaceAddresses(virDomainPtr dom, break; case VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT: - if (priv->agentError) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("QEMU guest agent is not " - "available due to an error")); - goto cleanup; - } - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) goto cleanup; - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain is not running")); - goto endjob; - } - - if (!priv->agent) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("QEMU guest agent is not configured")); + if (qemuDomainAgentAvailable(vm, true) < 0) goto endjob; - } qemuDomainObjEnterAgent(vm); ret = qemuAgentGetInterfaces(priv->agent, ifaces); -- 2.3.5
When the guest agent channel gets hotplugged to a VM, libvirt would still report that "QEMU guest agent is not configured" rather than stating that the connection was not established yet. Currently the code won't be able to connect to the agent after hotplug but that will change in a later patch. As the qemuFindAgentConfig() helper is quite helpful in this case move it to a more usable place and export it. --- src/qemu/qemu_domain.c | 45 +++++++++++++++++++++++++++++++++++++++++---- src/qemu/qemu_domain.h | 2 ++ src/qemu/qemu_process.c | 20 -------------------- 3 files changed, 43 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 6c480c6..f57768c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2976,11 +2976,19 @@ qemuDomainAgentAvailable(virDomainObjPtr vm, return false; } if (!priv->agent) { - if (reportError) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("QEMU guest agent is not configured")); + if (qemuFindAgentConfig(vm->def)) { + if (reportError) { + virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s", + _("QEMU guest agent is not connected")); + } + return false; + } else { + if (reportError) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("QEMU guest agent is not configured")); + } + return false; } - return false; } if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_RUNNING) { if (reportError) { @@ -3078,3 +3086,32 @@ qemuDomainSupportsBlockJobs(virDomainObjPtr vm, return 0; } + + +/** + * qemuFindAgentConfig: + * @def: domain definition + * + * Returns the pointer to the channel definition that is used to access the + * guest agent if the agent is configured or NULL otherwise. + */ +virDomainChrSourceDefPtr +qemuFindAgentConfig(virDomainDefPtr def) +{ + virDomainChrSourceDefPtr config = NULL; + size_t i; + + for (i = 0; i < def->nchannels; i++) { + virDomainChrDefPtr channel = def->channels[i]; + + if (channel->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO) + continue; + + if (STREQ_NULLABLE(channel->target.name, "org.qemu.guest_agent.0")) { + config = &channel->source; + break; + } + } + + return config; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index d550ae3..76a4819 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -439,4 +439,6 @@ bool qemuDomainDiskBlockJobIsActive(virDomainDiskDefPtr disk); int qemuDomainAlignMemorySizes(virDomainDefPtr def); void qemuDomainMemoryDeviceAlignSize(virDomainMemoryDefPtr mem); +virDomainChrSourceDefPtr qemuFindAgentConfig(virDomainDefPtr def); + #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6707170..7ce8fa5 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -201,26 +201,6 @@ static qemuAgentCallbacks agentCallbacks = { .errorNotify = qemuProcessHandleAgentError, }; -static virDomainChrSourceDefPtr -qemuFindAgentConfig(virDomainDefPtr def) -{ - virDomainChrSourceDefPtr config = NULL; - size_t i; - - for (i = 0; i < def->nchannels; i++) { - virDomainChrDefPtr channel = def->channels[i]; - - if (channel->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO) - continue; - - if (STREQ_NULLABLE(channel->target.name, "org.qemu.guest_agent.0")) { - config = &channel->source; - break; - } - } - - return config; -} static int qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm) -- 2.3.5
If a user hot-attaches the guest agent channel libvirt would ignore it until the restart of libvirtd or shutdown/destroy and start of the VM itself. This patch adds code that opens or closes the guest agent connection according to the state of the guest agent channel according to connect/disconnect events. To allow opening the channel from the event handler qemuConnectAgent needed to be exported. --- src/qemu/qemu_driver.c | 35 +++++++++++++++++++++++++++++++---- src/qemu/qemu_process.c | 2 +- src/qemu/qemu_process.h | 2 ++ 3 files changed, 34 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ce38e36..28e27f2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4419,6 +4419,8 @@ processSerialChangedEvent(virQEMUDriverPtr driver, virDomainChrDeviceState newstate; virObjectEventPtr event = NULL; virDomainDeviceDef dev; + qemuDomainObjPrivatePtr priv = vm->privateData; + int rc; if (connected) newstate = VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED; @@ -4445,10 +4447,35 @@ processSerialChangedEvent(virQEMUDriverPtr driver, dev.data.chr->targetType != VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO) goto endjob; - if (STREQ_NULLABLE(dev.data.chr->target.name, "org.qemu.guest_agent.0") && - (event = virDomainEventAgentLifecycleNewFromObj(vm, newstate, - VIR_CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_REASON_CHANNEL))) - qemuDomainEventQueue(driver, event); + if (STREQ_NULLABLE(dev.data.chr->target.name, "org.qemu.guest_agent.0")) { + switch (newstate) { + case VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED: + if (!priv->agent) { + if ((rc = qemuConnectAgent(driver, vm)) == -2) + goto endjob; + + if (rc < 0) + priv->agentError = true; + } + break; + + case VIR_DOMAIN_CHR_DEVICE_STATE_DISCONNECTED: + if (priv->agent) { + qemuAgentClose(priv->agent); + priv->agent = NULL; + priv->agentError = false; + } + break; + + case VIR_DOMAIN_CHR_DEVICE_STATE_DEFAULT: + case VIR_DOMAIN_CHR_DEVICE_STATE_LAST: + break; + }; + + if ((event = virDomainEventAgentLifecycleNewFromObj(vm, newstate, + VIR_CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_REASON_CHANNEL))) + qemuDomainEventQueue(driver, event); + } dev.data.chr->state = newstate; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 7ce8fa5..3105101 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -202,7 +202,7 @@ static qemuAgentCallbacks agentCallbacks = { }; -static int +int qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index c67b0cb..d40f68d 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -113,4 +113,6 @@ int qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver, virDomainDiskDefPtr qemuProcessFindDomainDiskByAlias(virDomainObjPtr vm, const char *alias); +int qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm); + #endif /* __QEMU_PROCESS_H__ */ -- 2.3.5
On 24.04.2015 16:52, Peter Krempa wrote:
If a user hot-attaches the guest agent channel libvirt would ignore it until the restart of libvirtd or shutdown/destroy and start of the VM itself.
This patch adds code that opens or closes the guest agent connection according to the state of the guest agent channel according to connect/disconnect events.
To allow opening the channel from the event handler qemuConnectAgent needed to be exported. --- src/qemu/qemu_driver.c | 35 +++++++++++++++++++++++++++++++---- src/qemu/qemu_process.c | 2 +- src/qemu/qemu_process.h | 2 ++ 3 files changed, 34 insertions(+), 5 deletions(-)
Finally. I had this in my mind for a while, but always thought it's gonna be more complex so I've put it aside. But now that I see we have everything prepared, how silly my thinking was. Good job. Michal
On 24.04.2015 16:52, Peter Krempa wrote:
Clean up some parts of the agent code, fix a recently added bug and add support for guest agent hotplug, all this while removing more lines than adding.
Peter Krempa (5): qemu: agent: Reuse virJSONValueObjectCreateVArgs in qemuAgentMakeCommand qemu: Fix domain object leak in qemuDomainInterfaceAddresses qemu: Reuse qemuDomainAgentAvailable in qemuDomainInterfaceAddresses qemu: agent: Differentiate errors when the agent channel was hotplugged qemu: Connect to guest agent after channel hotplug
src/qemu/qemu_agent.c | 73 ++----------------------------------------------- src/qemu/qemu_domain.c | 45 +++++++++++++++++++++++++++--- src/qemu/qemu_domain.h | 2 ++ src/qemu/qemu_driver.c | 56 +++++++++++++++++++++---------------- src/qemu/qemu_process.c | 22 +-------------- src/qemu/qemu_process.h | 2 ++ 6 files changed, 81 insertions(+), 119 deletions(-)
ACK series. Michal
On Fri, Apr 24, 2015 at 17:09:13 +0200, Michal Privoznik wrote:
On 24.04.2015 16:52, Peter Krempa wrote:
Clean up some parts of the agent code, fix a recently added bug and add support for guest agent hotplug, all this while removing more lines than adding.
Peter Krempa (5): qemu: agent: Reuse virJSONValueObjectCreateVArgs in qemuAgentMakeCommand qemu: Fix domain object leak in qemuDomainInterfaceAddresses qemu: Reuse qemuDomainAgentAvailable in qemuDomainInterfaceAddresses qemu: agent: Differentiate errors when the agent channel was hotplugged qemu: Connect to guest agent after channel hotplug
src/qemu/qemu_agent.c | 73 ++----------------------------------------------- src/qemu/qemu_domain.c | 45 +++++++++++++++++++++++++++--- src/qemu/qemu_domain.h | 2 ++ src/qemu/qemu_driver.c | 56 +++++++++++++++++++++---------------- src/qemu/qemu_process.c | 22 +-------------- src/qemu/qemu_process.h | 2 ++ 6 files changed, 81 insertions(+), 119 deletions(-)
ACK series.
Thanks, I've pushed this series. Peter
participants (2)
-
Michal Privoznik -
Peter Krempa