[libvirt] [PATCH 1/4] QEMU guest agent support
Eric Blake
eblake at redhat.com
Wed Oct 5 22:45:22 UTC 2011
On 10/05/2011 11:31 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange"<berrange at 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 at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
More information about the libvir-list
mailing list