5:45 p.m.
On 10/05/2011 11:31 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange(a)redhat.com>
There is now a standard QEMU guest agent that can be installed
and given a virtio serial channel
<channel type='unix'>
<source mode='bind'
path='/var/lib/libvirt/qemu/f16x86_64.agent'/>
<target type='virtio' name='org.qemu.guest_agent.0'/>
</channel>
And the next step would be to have virt-manager/virt-install
automatically set this up when creating new domains.
The protocol that runs over the guest agent is JSON based and
very similar to the JSON monitor. We can't use exaclty the same
s/exaclty/exactly/
code because there are some odd differences in the way messages
and errors are strucutured. The qemu_agent.c file is based on
s/strucutured/structured/
a combination and simplification of qemu_monitor.c and
qemu_monitor_json.c
* src/qemu/qemu_agent.c, src/qemu/qemu_agent.h: Support for
talking to the agent for shutdown
* src/qemu/qemu_domain.c, src/qemu/qemu_domain.h: Add thread
helpers for talking to the agent
* src/qemu/qemu_process.c: Connect to agent whenever starting
a guest
* src/qemu/qemu_monitor_json.c: Make variable static
---
src/Makefile.am | 1 +
src/qemu/qemu_agent.c | 1135 ++++++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_agent.h | 69 +++
src/qemu/qemu_domain.c | 97 ++++
src/qemu/qemu_domain.h | 22 +
src/qemu/qemu_monitor_json.c | 2 +-
src/qemu/qemu_process.c | 187 +++++++
7 files changed, 1512 insertions(+), 1 deletions(-)
create mode 100644 src/qemu/qemu_agent.c
create mode 100644 src/qemu/qemu_agent.h
We'll see how far I get in this review. At any rate, this patch should
be independently back-portable to Fedora 16 without requiring a rebase,
although the same cannot be said of patches 2-4.
+
+static struct {
+ const char *type;
+ void (*handler)(qemuAgentPtr mon, virJSONValuePtr data);
+} eventHandlers[] = {
+};
What good is a 0-element initializer? Should we nuke this struct until
we actually have an event that we need to handle?
+
+static void qemuAgentFree(qemuAgentPtr mon)
+{
+ VIR_DEBUG("mon=%p", mon);
+ if (mon->cb&& mon->cb->destroy)
+ (mon->cb->destroy)(mon, mon->vm);
+ if (virCondDestroy(&mon->notify)< 0)
+ {}
I'd write this as:
ignore_value(virCondDestroy(&mon->notify));
I know it was just copy-and-paste from old code that predated our use of
ignore_value.
+static int
+qemuAgentOpenUnix(const char *monitor, pid_t cpid, bool *inProgress)
+{
+ struct sockaddr_un addr;
+ int monfd;
+ int timeout = 3; /* In seconds */
+ int ret, i = 0;
+
+ *inProgress = false;
+
+ if ((monfd = socket(AF_UNIX, SOCK_STREAM, 0))< 0) {
+ virReportSystemError(errno,
+ "%s", _("failed to create socket"));
+ return -1;
+ }
+
+ if (virSetNonBlock(monfd)< 0) {
Someday, I'll get gnulib to support SOCK_NONBLOCK, so we can do this in
fewer kernel calls with newer kernels...
+
+static int
+qemuAgentIOProcessEvent(qemuAgentPtr mon,
+ virJSONValuePtr obj)
+{
+ const char *type;
+ int i;
+ VIR_DEBUG("mon=%p obj=%p", mon, obj);
+
+ type = virJSONValueObjectGetString(obj, "event");
+ if (!type) {
+ VIR_WARN("missing event type in message");
+ errno = EINVAL;
+ return -1;
+ }
+
+ for (i = 0 ; i< ARRAY_CARDINALITY(eventHandlers) ; i++) {
I guess you really are using a 0-element array. But does that compile
correctly on all C99 compilers, or should we also be commenting out this
function as long as we lack agent events?
+
+static int qemuAgentIOProcessData(qemuAgentPtr mon,
+ const char *data,
+ size_t len,
+ qemuAgentMessagePtr msg)
+{
+ int used = 0;
+#if DEBUG_IO
+# if DEBUG_RAW_IO
+ char *str1 = qemuAgentEscapeNonPrintable(data);
+ VIR_ERROR(_("[%s]"), str1);
Doesn't this trip up 'make syntax-check', since VIR_ERROR shouldn't need
to translate its string?
+/* This method processes data that has been received
+ * from the monitor. Looking for async events and
+ * replies/errors.
+ */
+static int
+qemuAgentIOProcess(qemuAgentPtr mon)
+{
+ int len;
+ qemuAgentMessagePtr msg = NULL;
+
+ /* See if there's a message& whether its ready for its reply
+ * ie whether its completed writing all its data */
Copy-and-paste, but the grammar here is awful. How about:
See if there's a message ready for reply; that is, one that has
completed writing all its data.
+ if (mon->msg&& mon->msg->txOffset ==
mon->msg->txLength)
+ msg = mon->msg;
+
+#if DEBUG_IO
+# if DEBUG_RAW_IO
+ char *str1 = qemuAgentEscapeNonPrintable(msg ? msg->txBuffer : "");
+ char *str2 = qemuAgentEscapeNonPrintable(mon->buffer);
+ VIR_ERROR(_("Process %d %p %p [[[[%s]]][[[%s]]]"),
(int)mon->bufferOffset, mon->msg, msg, str1, str2);
Unbalanced: four [[[[, but only three ]]]
+/*
+ * Called when the monitor is able to write data
+ * Call this function while holding the monitor lock.
+ */
+static int
+qemuAgentIOWrite(qemuAgentPtr mon)
+{
+ int done;
+
+ /* If no active message, or fully transmitted, the no-op */
s/the/then/
+
+ switch (config->type) {
+ case VIR_DOMAIN_CHR_TYPE_UNIX:
+ mon->fd = qemuAgentOpenUnix(config->data.nix.path, vm->pid,
+&mon->connectPending);
+ break;
+
+ case VIR_DOMAIN_CHR_TYPE_PTY:
+ mon->fd = qemuAgentOpenPty(config->data.file.path);
+ break;
+
+ default:
+ qemuReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unable to handle monitor type: %s"),
+ virDomainChrTypeToString(config->type));
+ goto cleanup;
+ }
+
+ if (mon->fd == -1) goto cleanup;
+
+ if (virSetCloseExec(mon->fd)< 0) {
Why do we set nonblock in qemuAgentOpen{Unix,Pty}, but delay
SetCloseExec to here? This seems like it might be worth moving closer
to where the fds are opened (that, and someday I want to teach gnulib to
guarantee that SOCK_CLOEXEC works for socket()...)
+
+static int qemuAgentSend(qemuAgentPtr mon,
+ qemuAgentMessagePtr msg)
+{
+ int ret = -1;
+
+ /* Check whether qemu quited unexpectedly */
s/quited/quit/
+
+
+#if 0
+static int
+qemuAgentHasError(virJSONValuePtr reply,
+ const char *klass)
Do we need to be adding #if 0 stuff here, or can you trim this
completely out?
+
+ /* This doesn't supports maps/arrays. This hasn't
s/supports/support/
* This is a callback registered with a qemuMonitorPtr instance,
* and to be invoked when the monitor console hits an end of file
* condition, or error, thus indicating VM shutdown should be
@@ -2602,6 +2759,14 @@ qemuProcessReconnect(void *opaque)
if (qemuConnectMonitor(driver, obj)< 0)
goto error;
+ /* Failure to connect to agent shouldn't be made to be fatal */
s/be made to be/be/
+ if (qemuConnectAgent(driver, obj)< 0) {
+ VIR_WARN("Cannot connect to QEMU guest agent for %s",
+ obj->def->name);
+ virResetLastError();
+ priv->agentError = true;
+ }
+
if (qemuUpdateActivePciHostdevs(driver, obj->def)< 0) {
goto error;
}
@@ -3152,6 +3317,14 @@ int qemuProcessStart(virConnectPtr conn,
if (qemuProcessWaitForMonitor(driver, vm, priv->qemuCaps, pos)< 0)
goto cleanup;
+ /* Failure to connect to agent shouldn't be made to be fatal */
again
@@ -3607,6 +3786,14 @@ int qemuProcessAttach(virConnectPtr conn
ATTRIBUTE_UNUSED,
if (qemuProcessWaitForMonitor(driver, vm, priv->qemuCaps, -1)< 0)
goto cleanup;
+ /* Failure to connect to agent shouldn't be made to be fatal */
and again
ACK with nits fixed - and looks like most of the nits are due to copy
and pasting of working code, rather than implementing new code from scratch.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org