Re: [libvirt] [PATCH 1/4] QEMU guest agent support
On Thu, Jan 19, 2012 at 03:18:00PM -0700, Eric Blake wrote: 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? This is an artifact of the way we are doing the guest agent support today. If you look at the SPICE guest agent, there is a special channel type 'spicevmc' that is used, and QEMU deals with that internally. In the future, QEMU will deal with its own guest agent internally and thus we'll have a new channel type kinda like 'spicevmc' which does not expose any paths. For now though, we're using the externally managed agent for which we do want to expose the path IMHO. Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] QEMU guest agent support
On Fri, Jan 20, 2012 at 07:16:22PM +0100, Michal Privoznik wrote: On 19.01.2012 23:18, Eric Blake wrote: 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'/ [snip] I am not sure we want this. Although I am not fully convinced by the opposite. Thing is, I don't want to enable something by default because users might have not noticed and things may break for them. On the other hand, users will definitely benefit from this feature so making it as easy as possible to enable is the right step. My initial thought is to have simple one line element: guest_agent name=org.qemu.guest_agent.0/ with name defaulting to org.qemu.guest_agent.0 so simply inserting bare guest_agent/ will turn everything on? No, we don't want to go about inventing new syntax for that - the very purpose of any channel element is to support a guest agent, so it is pointless creating another guest_agent element. Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] QEMU guest agent support
On 19.01.2012 23:18, Eric Blake wrote:
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
+++ 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.
Yes. You won't receive EOF if you close guest agent. But you'll get EOF
when qemu closes the socket. Even during reboots this socket remains
opened. So it is like monitor.
+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?
I am not sure we want this. Although I am not fully convinced by the
opposite. Thing is, I don't want to enable something by default because
users might have not noticed and things may break for them.
On the other hand, users will definitely benefit from this feature so
making it as easy as possible to enable is the right step.
My initial thought is to have simple one line element:
guest_agent name=org.qemu.guest_agent.0/
with name defaulting to org.qemu.guest_agent.0 so simply inserting
bare guest_agent/ will turn everything on?
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).
Thanks for review and watch out for v2 :) (as soon as we settle down on
XML part)
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] QEMU guest agent support
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;
Re: [libvirt] [PATCH 1/4] QEMU guest agent support
On 10/05/2011 11:31 AM, Daniel P. Berrange wrote:
From: Daniel P. Berrangeberra...@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:
