On 01/17/2012 04:44 AM, Michal Privoznik wrote:
> 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'/>
Do we really want to be documenting a path in the libvirt-controlled
hierarchy?
Or is this something that we should be automatically creating on the
user's behalf if it is apparent that qemu supports it, rather than
requiring the user to modify their XML to add it? At which point, it
would _not_ be part of 'virsh dumpxml' output, but would only be visible
in the /var/run/libvirt/qemu/dom.xml runtime state (similar to how
<domstatus>/<monitor> is the chardev element automatically created for
the monitor)?
> <target type='virtio' name='org.qemu.guest_agent.0'/>
> </channel>
>
> The protocol that runs over the guest agent is JSON based and
> very similar to the JSON monitor. We can't use exactly the same
> code because there are some odd differences in the way messages
> and errors are structured. The qemu_agent.c file is based on
> 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
> +struct _qemuAgentMessage {
> + char *txBuffer;
> + int txOffset;
> + int txLength;
> +
> + /* Used by the text monitor reply / error */
> + char *rxBuffer;
> + int rxLength;
This comment is misleading, since we are a JSON monitor and not a text
monitor.
> + /* Used by the JSON monitor to hold reply / error */
> + void *rxObject;
> +
> + /* True if rxBuffer / rxObject are ready, or a
> + * fatal error occurred on the monitor channel
> + */
> + bool finished;
> +};
> +
> +
> +#if DEBUG_RAW_IO
> +# include <c-ctype.h>
> +static char * qemuAgentEscapeNonPrintable(const char *text)
Formatting - I'd do this in two lines:
static char *
qemuAgentEscapeNonPrintable(const char *text)
> +{
> + int i;
> + virBuffer buf = VIR_BUFFER_INITIALIZER;
> + for (i = 0 ; text[i] != '\0' ; i++) {
> + if (c_isprint(text[i]) ||
> + text[i] == '\n' ||
> + (text[i] == '\r' && text[i+1] == '\n'))
> + virBufferAsprintf(&buf,"%c", text[i]);
> + else
> + virBufferAsprintf(&buf, "0x%02x", text[i]);
That gives ambiguous output, and isn't very efficient. I'd do it more like:
if (text[i] == '\\')
virBufferAddLit(&buf, "\\\\");
else if (c_isprint(text[i]) || text[i] == '\n' ||
(text[i] == '\r' && text[i+1] == '\n'))
virBufferAddChar(&buf, text[i]);
else
virBufferAsprintf(&buf, "\\x%02x", text[i]);
> +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) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR,
> + "%s", _("Unable to put monitor into non-blocking mode"));
> + goto error;
> + }
> +
> + if (virSetCloseExec(monfd) < 0) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR,
> + "%s", _("Unable to set monitor close-on-exec flag"));
> + goto error;
> + }
You know, if I ever got around to fixing gnulib to guarantee things for
older platforms, we could use socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC
| SOCK_NONBLOCK, 0) and shave off a few syscalls. But for now, your
code is the best we can do.
> +static int
> +qemuAgentOpenPty(const char *monitor)
> +{
> + int monfd;
> +
> + if ((monfd = open(monitor, O_RDWR)) < 0) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to open monitor path %s"), monitor);
> + return -1;
> + }
> +
> + if (virSetNonBlock(monfd) < 0) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR,
> + "%s", _("Unable to put monitor into non-blocking mode"));
> + goto error;
> + }
Why not open(monitor, O_RDWR | O_NONBLOCK), and shave off a syscall here?
> +
> + if (virSetCloseExec(monfd) < 0) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR,
> + "%s", _("Unable to set monitor close-on-exec flag"));
> + goto error;
> + }
Alas, I also need to find time to make gnulib guarantee open(O_CLOEXEC).
> +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);
> + VIR_FREE(str1);
> +# else
> + VIR_DEBUG("Data %zu bytes [%s]", len, data);
> +# endif
> +#endif
> +
> + while (used < len) {
> + char *nl = strstr(data + used, LINE_ENDING);
Would strchr() be more efficient, since LINE_ENDING is a single character?
> +
> + if (nl) {
> + int got = nl - (data + used);
> + char *line = strndup(data + used, got);
> + if (!line) {
> + virReportOOMError();
> + return -1;
> + }
> + used += got + strlen(LINE_ENDING);
> + line[got] = '\0'; /* kill \n */
The '\n' was already killed by virtue of the strndup(). This line is
redundant.
> + if (qemuAgentIOProcessLine(mon, line, msg) < 0) {
> + VIR_FREE(line);
> + return -1;
> + }
> +
> + VIR_FREE(line);
Then again, do we really need to strndup(), or should we just modify
data[] in place, before calling qemuAgentIOProcessLine(mon, data + used,
got), and before incrementing used?
> +#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);
Why are we using %d and a cast to int, instead of %zu and
mon->bufferOffset as-is?
> + VIR_FREE(str1);
> + VIR_FREE(str2);
> +# else
> + VIR_DEBUG("Process %d", (int)mon->bufferOffset);
And again.
> +
> +static int
> +qemuAgentCheckError(virJSONValuePtr cmd,
> + virJSONValuePtr reply)
Indentation.
> +
> +#if 0
> +static int
> +qemuAgentHasError(virJSONValuePtr reply,
> + const char *klass)
> +{
Do we still need this block of code?
> +
> +static virJSONValuePtr ATTRIBUTE_SENTINEL
> +qemuAgentMakeCommand(const char *cmdname,
> + ...)
Indentation (and probably more instances, so I'll quit pointing them out).
> +++ b/src/qemu/qemu_process.c
> @@ -107,6 +107,164 @@ qemuProcessRemoveDomainStatus(struct qemud_driver *driver,
> extern struct qemud_driver *qemu_driver;
>
> /*
> + * This is a callback registered with a qemuAgentPtr instance,
> + * and to be invoked when the agent console hits an end of file
> + * condition, or error, thus indicating VM shutdown should be
> + * performed
Does agent shutdown really imply VM shutdown? I hope not - that should
be reserved for just monitor EOF.
> +static virDomainChrSourceDefPtr
> +qemuFindAgentConfig(virDomainDefPtr def)
> +{
> + virDomainChrSourceDefPtr config = NULL;
> + int 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(channel->target.name, "org.qemu.guest_agent.0")) {
> + config = &channel->source;
> + break;
> + }
> + }
This implementation requires users to add the agent in their XML; is
there any way we can automate the use of an agent without doing this?
Then again, it's not just that qemu has to be new enough to have agent
support, but it's also that the guest has to have an agent installed.
Maybe we should consider (as a followup, not for this patch) an XML
shorthand that lets the user request use of an agent without having to
specify full details (that is, without having to choose a patch for the
socket, and without having to know the name "org.qemu.guest_agent.0" for
the protocol of a channel).
Overall, I'm looking forward to using the agent (I want to support a new
flag to snapshot creation that uses the agent's ability to request file
system queiscing from the guest).
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list