By the way, I am trying to re-write to move some functions to libvirt-qemu as you adviced. But the result is not good not to print Guest Agent's command response... In addition, VIR_DEBUG() in src/libvirt-qemu.c also does not output to log file. Though I minded much before, I think that 'qemu-monitor-command' does not work correctly. Does anyone can get good result with its command ? Regards MATSUDA Daiki

On Fri, Jun 29, 2012 at 01:58:05PM +0900, MATSUDA, Daiki wrote:

...

diff -uNrp libvirt-0.9.13.orig/daemon/remote.c libvirt-0.9.13/daemon/remote.c --- libvirt-0.9.13.orig/daemon/remote.c 2012-06-25 16:06:18.000000000 +0900 +++ libvirt-0.9.13/daemon/remote.c 2012-06-29 12:50:03.752806682 +0900 @@ -3923,6 +3923,42 @@ cleanup: return rv; }

+ +static int +remoteDispatchDomainQemuAgentCommand(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_domain_qemu_agent_command_args *args, + remote_domain_qemu_agent_command_ret *ret) +{ + virDomainPtr dom = NULL; + int rv = -1; + struct daemonClientPrivate *priv = + virNetServerClientGetPrivateData(client); + + if (!priv->conn) { + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if (!(dom = get_nonnull_domain(priv->conn, args->dom))) + goto cleanup; + + if (virDomainQemuAgentCommand(dom, args->cmd, &ret->result, args->flags) <0) { + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("Guest Agent Error")); + goto cleanup; + } + + rv = 0; +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (dom) + virDomainFree(dom); + return rv; +} + /*----- Helpers. -----*/

/* get_nonnull_domain and get_nonnull_network turn an on-wire diff -uNrp libvirt-0.9.13.orig/daemon/remote_dispatch.h libvirt-0.9.13/daemon/remote_dispatch.h --- libvirt-0.9.13.orig/daemon/remote_dispatch.h 2012-06-25 19:48:08.000000000 +0900 +++ libvirt-0.9.13/daemon/remote_dispatch.h 2012-06-29 10:21:21.460454579 +0900 @@ -12889,6 +12889,28 @@ static int remoteDispatchSupportsFeature

+static int remoteDispatchDomainQemuAgentCommand( + virNetServerPtr server, + virNetServerClientPtr client, + virNetMessagePtr msg, + virNetMessageErrorPtr rerr, + remote_domain_qemu_agent_command_args *args, + remote_domain_qemu_agent_command_ret *ret); +static int remoteDispatchDomainQemuAgentCommandHelper( + virNetServerPtr server, + virNetServerClientPtr client, + virNetMessagePtr msg, + virNetMessageErrorPtr rerr, + void *args, + void *ret) +{ + VIR_DEBUG("server=%p client=%p msg=%p rerr=%p args=%p ret=%p", server, client, msg, rerr, args, ret); + return remoteDispatchDomainQemuAgentCommand(server, client, msg, rerr, args, ret); +} +/* remoteDispatchDomainQemuAgentCommand body has to be implemented manually */ + + + virNetServerProgramProc remoteProcs[] = { { /* Unused 0 */ NULL, @@ -15374,5 +15396,14 @@ virNetServerProgramProc remoteProcs[] = true, 1 }, +{ /* Method DomainQemuAgentCommand => 276 */ + remoteDispatchDomainQemuAgentCommandHelper, + sizeof(remote_domain_qemu_agent_command_args), + (xdrproc_t)xdr_remote_qemu_agent_command_args, + sizeof(remote_domain_qemu_agent_command_ret), + (xdrproc_t)remote_domain_qemu_agent_command_ret, + true, + 0 +}, }; size_t remoteNProcs = ARRAY_CARDINALITY(remoteProcs);

This is an auto-generated file. Instead of doing a diff against two unpacked tar.gz archives, developer against a git checkout, and use GIT to produce the patch without the auto-generated cruft.

...

diff -uNrp libvirt-0.9.13.orig/include/libvirt/libvirt.h.in libvirt-0.9.13/include/libvirt/libvirt.h.in --- libvirt-0.9.13.orig/include/libvirt/libvirt.h.in 2012-06-25 21:42:32.000000000 +0900 +++ libvirt-0.9.13/include/libvirt/libvirt.h.in 2012-06-29 11:22:38.113455058 +0900 @@ -4132,6 +4132,9 @@ typedef struct _virTypedParameter virMem */ typedef virMemoryParameter *virMemoryParameterPtr;

+int virDomainQemuAgentCommand(virDomainPtr domain, const char *cmd, + char **result, unsigned int flags); + #ifdef __cplusplus } #endif

This should be in libvirt-qemu.h

...

diff -uNrp libvirt-0.9.13.orig/src/libvirt.c libvirt-0.9.13/src/libvirt.c --- libvirt-0.9.13.orig/src/libvirt.c 2012-06-28 12:05:04.000000000 +0900 +++ libvirt-0.9.13/src/libvirt.c 2012-06-29 12:51:51.336454508 +0900 @@ -18973,3 +18973,44 @@ error: virDispatchError(dom->conn); return -1; } + +/** + * virDomainQemuAgentCommand: + * @domain: a domain object + * @cmd: execution command on domain's guest agent + * @result: returning strings + * @flags: execution flags + * + * Provide a list of Guest Agent's support command. + * Returns 0 if succeeded, -1 in failing. + */ +int +virDomainQemuAgentCommand(virDomainPtr domain, + const char *cmd, + char **result, + unsigned int flags) +{ + virConnectPtr conn; + int ret = -1; + + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { + virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + return ret; + } + + conn = domain->conn; + + if (conn->driver->domainQemuAgentCommand) { + ret = conn->driver->domainQemuAgentCommand(domain, cmd, result, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibDomainError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + /* Copy to connection error object for back compatability */ + virDispatchError(domain->conn); + return ret; +}

Should be in libvirt-qemu.c

...

diff -uNrp libvirt-0.9.13.orig/src/libvirt_public.syms libvirt-0.9.13/src/libvirt_public.syms --- libvirt-0.9.13.orig/src/libvirt_public.syms 2012-06-25 16:06:18.000000000 +0900 +++ libvirt-0.9.13/src/libvirt_public.syms 2012-06-29 09:07:00.357580129 +0900 @@ -542,6 +542,7 @@ LIBVIRT_0.9.13 { virDomainSnapshotIsCurrent; virDomainSnapshotListAllChildren; virDomainSnapshotRef; + virDomainQemuAgentCommand; } LIBVIRT_0.9.11;

Should be in libvirt_qemu.syms

...

static virNetworkDriver network_driver = { diff -uNrp libvirt-0.9.13.orig/src/remote/remote_protocol.x libvirt-0.9.13/src/remote/remote_protocol.x --- libvirt-0.9.13.orig/src/remote/remote_protocol.x 2012-06-25 16:06:18.000000000 +0900 +++ libvirt-0.9.13/src/remote/remote_protocol.x 2012-06-29 12:55:43.752580212 +0900 @@ -2513,6 +2513,16 @@ struct remote_connect_list_all_domains_r unsigned int ret; };

+struct remote_domain_qemu_agent_command_args { + remote_nonnull_domain dom; + remote_nonnull_string cmd; + u_int flags; +}; + +struct remote_domain_qemu_agent_command_ret { + remote_nonnull_string result; +}; +

/*----- Protocol. -----*/

@@ -2838,7 +2848,8 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_SNAPSHOT_HAS_METADATA = 272, /* autogen autogen */ REMOTE_PROC_CONNECT_LIST_ALL_DOMAINS = 273, /* skipgen skipgen priority:high */ REMOTE_PROC_DOMAIN_LIST_ALL_SNAPSHOTS = 274, /* skipgen skipgen priority:high */ - REMOTE_PROC_DOMAIN_SNAPSHOT_LIST_ALL_CHILDREN = 275 /* skipgen skipgen priority:high */ + REMOTE_PROC_DOMAIN_SNAPSHOT_LIST_ALL_CHILDREN = 275, /* skipgen skipgen priority:high */ + REMOTE_PROC_DOMAIN_QEMU_AGENT_COMMAND = 276 /* skipgen skipgen */

/* * Notice how the entries are grouped in sets of 10 ?

This should bein qemu_protocol.x

Regards, Daniel

[I am pasting your patch here so I can point out some nits] On 03.07.2012 02:56, MATSUDA, Daiki wrote:

diff -uNrp libvirt-0.9.13.orig/daemon/remote.c libvirt-0.9.13/daemon/remote.c --- libvirt-0.9.13.orig/daemon/remote.c 2012-06-25 16:06:18.000000000 +0900 +++ libvirt-0.9.13/daemon/remote.c 2012-07-02 10:02:54.400454470 +0900 @@ -3923,6 +3923,42 @@ cleanup: return rv; }

+ +static int +qemuDispatchGuestAgentCommand(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + qemu_guest_agent_command_args *args, + qemu_guest_agent_command_ret *ret) +{ + virDomainPtr dom = NULL; + int rv = -1; + struct daemonClientPrivate *priv = + virNetServerClientGetPrivateData(client); + + if (!priv->conn) { + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if (!(dom = get_nonnull_domain(priv->conn, args->dom))) + goto cleanup; + + if (virDomainGuestAgentCommand(dom, args->cmd, &ret->result, args->flags) <0) { + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("Guest Agent Error"));

This is wrong as it overwrites any error reported by virDomainGuestAgentCommand. Moreover, there should be space between '<' and '0' in if statement which is btw longer than 80 characters.

+ goto cleanup; + } + + rv = 0; +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (dom) + virDomainFree(dom); + return rv; +} + /*----- Helpers. -----*/

/* get_nonnull_domain and get_nonnull_network turn an on-wire diff -uNrp libvirt-0.9.13.orig/include/libvirt/libvirt-qemu.h libvirt-0.9.13/include/libvirt/libvirt-qemu.h --- libvirt-0.9.13.orig/include/libvirt/libvirt-qemu.h 2012-03-06 22:59:21.000000000 +0900 +++ libvirt-0.9.13/include/libvirt/libvirt-qemu.h 2012-07-02 10:02:54.401454277 +0900 @@ -32,6 +32,9 @@ virDomainPtr virDomainQemuAttach(virConn unsigned int pid_value, unsigned int flags);

+int virDomainGuestAgentCommand(virDomainPtr domain, const char *cmd, + char **result, unsigned int flags); +

Do we really want this name? I mean in case of monitor we are qemu specific. So I think this should be either virDomainQemuAgentCommand or virDomainQemuGuestAgentCommand. We expose this as qemu-agent-command after all!

# ifdef __cplusplus } # endif diff -uNrp libvirt-0.9.13.orig/python/generator.py libvirt-0.9.13/python/generator.py --- libvirt-0.9.13.orig/python/generator.py 2012-06-25 16:06:18.000000000 +0900 +++ libvirt-0.9.13/python/generator.py 2012-07-02 10:03:01.616454719 +0900 @@ -429,6 +429,7 @@ skip_impl = (

qemu_skip_impl = ( 'virDomainQemuMonitorCommand', + 'virDomainGuestAgentCommand', )

diff -uNrp libvirt-0.9.13.orig/python/libvirt-qemu-override-api.xml libvirt-0.9.13/python/libvirt-qemu-override-api.xml --- libvirt-0.9.13.orig/python/libvirt-qemu-override-api.xml 2011-09-14 15:24:44.000000000 +0900 +++ libvirt-0.9.13/python/libvirt-qemu-override-api.xml 2012-07-02 10:02:54.403454795 +0900 @@ -8,5 +8,12 @@ <arg name='cmd' type='const char *' info='the command which will be passed to QEMU monitor'/> <arg name='flags' type='unsigned int' info='an OR'ed set of virDomainQemuMonitorCommandFlags'/> </function> + <function name='virDomainGuestAgentCommand' file='python-qemu'> + <info>Send a Guest Agent command to domain</info> + <return type='str *' info='the command output or None in case of error'/> + <arg name='domain' type='virDomainPtr' info='pointer to the domain'/> + <arg name='cmd' type='const char *' info='guest agent command on domain'/> + <arg name='flags' type='unsigned int' info='execution flags'/> + </function> </symbols> </api> diff -uNrp libvirt-0.9.13.orig/python/libvirt-qemu-override.c libvirt-0.9.13/python/libvirt-qemu-override.c --- libvirt-0.9.13.orig/python/libvirt-qemu-override.c 2012-03-30 11:45:28.000000000 +0900 +++ libvirt-0.9.13/python/libvirt-qemu-override.c 2012-07-02 10:02:54.404454019 +0900 @@ -82,6 +82,35 @@ libvirt_qemu_virDomainQemuMonitorCommand return py_retval; }

+static PyObject * +libvirt_qemu_virDomainGuestAgentCommand(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) +{ + PyObject *py_retval; + char *result = NULL; + virDomainPtr domain; + PyObject *pyobj_domain; + unsigned int flags; + char *cmd; + int c_retval; + + if (!PyArg_ParseTuple(args, (char *)"Ozi:virDomainGuestAgentCommand", + &pyobj_domain, &cmd, &flags)) + return NULL; + domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain); + + if (domain == NULL) + return VIR_PY_NONE; + LIBVIRT_BEGIN_ALLOW_THREADS; + c_retval = virDomainGuestAgentCommand(domain, cmd, &result, flags); + LIBVIRT_END_ALLOW_THREADS; + + if (c_retval < 0) + return VIR_PY_NONE; + + py_retval = PyString_FromString(result); + return py_retval; +} + /************************************************************************ * * * The registration stuff * @@ -90,6 +119,7 @@ libvirt_qemu_virDomainQemuMonitorCommand static PyMethodDef libvirtQemuMethods[] = { #include "libvirt-qemu-export.c" {(char *) "virDomainQemuMonitorCommand", libvirt_qemu_virDomainQemuMonitorCommand, METH_VARARGS, NULL}, + {(char *) "virDomainGuestAgentCommand", libvirt_qemu_virDomainGuestAgentCommand, METH_VARARGS, NULL}, {NULL, NULL, 0, NULL} };

diff -uNrp libvirt-0.9.13.orig/src/driver.h libvirt-0.9.13/src/driver.h --- libvirt-0.9.13.orig/src/driver.h 2012-06-25 16:06:18.000000000 +0900 +++ libvirt-0.9.13/src/driver.h 2012-07-02 10:02:54.405454285 +0900 @@ -860,6 +860,10 @@ typedef char * const char *uri, unsigned int flags);

+typedef int + (*virDrvDomainGuestAgentCommand)(virDomainPtr domain, const char *cmd, + char **result, unsigned int flags); + /** * _virDriver: * @@ -1041,6 +1045,7 @@ struct _virDriver { virDrvDomainGetDiskErrors domainGetDiskErrors; virDrvDomainSetMetadata domainSetMetadata; virDrvDomainGetMetadata domainGetMetadata; + virDrvDomainGuestAgentCommand qemuDomainGuestAgentCommand; };

I'd move these two as close as possible to virDrvDomainQemuMonitorCommand so we have similar commands grouped.

typedef int diff -uNrp libvirt-0.9.13.orig/src/libvirt-qemu.c libvirt-0.9.13/src/libvirt-qemu.c --- libvirt-0.9.13.orig/src/libvirt-qemu.c 2012-05-31 23:23:22.000000000 +0900 +++ libvirt-0.9.13/src/libvirt-qemu.c 2012-07-02 10:02:54.407454078 +0900 @@ -185,3 +185,48 @@ error: virDispatchError(conn); return NULL; } + +/** + * virDomainGuestAgentCommand: + * @domain: a domain object + * @cmd: the guest agent command string + * @result: a string returned by @cmd + * @flags: execution flags + * + * Execution Guest Agent Command + * + * Returns 0 if succeeded, -1 in failing. + */ +int +virDomainGuestAgentCommand(virDomainPtr domain, + const char *cmd, + char **result, + unsigned int flags) +{ + virConnectPtr conn; +

VIR_DEBUG() or equivalent is missing here. Each API must log arguments it was called with.

+ if (!VIR_IS_CONNECTED_DOMAIN(domain)) { + virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + conn = domain->conn; + + virCheckNonNullArgGoto(result, error); + + if (conn->driver->qemuDomainGuestAgentCommand) { + int ret; + ret = conn->driver->qemuDomainGuestAgentCommand(domain, cmd, result, flags);

Long line.

+ if (ret < 0) + goto error; + return ret; + } + + virLibConnError(conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + /* Copy to connection error object for back compatability */ + virDispatchError(domain->conn); + return -1; +} diff -uNrp libvirt-0.9.13.orig/src/libvirt_qemu.syms libvirt-0.9.13/src/libvirt_qemu.syms --- libvirt-0.9.13.orig/src/libvirt_qemu.syms 2011-07-14 12:00:39.000000000 +0900 +++ libvirt-0.9.13/src/libvirt_qemu.syms 2012-07-02 10:02:54.407454078 +0900 @@ -19,3 +19,8 @@ LIBVIRT_QEMU_0.9.4 { global: virDomainQemuAttach; } LIBVIRT_QEMU_0.8.3; + +LIBVIRT_QEMU_0.9.14 { + global: + virDomainGuestAgentCommand; +} LIBVIRT_QEMU_0.9.4; diff -uNrp libvirt-0.9.13.orig/src/qemu/qemu_agent.c libvirt-0.9.13/src/qemu/qemu_agent.c --- libvirt-0.9.13.orig/src/qemu/qemu_agent.c 2012-06-25 16:06:18.000000000 +0900 +++ libvirt-0.9.13/src/qemu/qemu_agent.c 2012-07-02 10:02:54.408488163 +0900 @@ -1410,3 +1410,29 @@ qemuAgentSuspend(qemuAgentPtr mon, virJSONValueFree(reply); return ret; } + +int qemuAgentGuestAgentCommand(qemuAgentPtr mon, + const char *cmd, + char **result) +{ + int ret = -1; + virJSONValuePtr jcmd; + virJSONValuePtr reply = NULL; + + jcmd = qemuAgentMakeCommand(cmd, NULL); + if (!jcmd) + return ret;

This is confusing. In case of qemu-monitor-command we require user to pass whole command string: {'execute':'some-dummy-command'} which is correct as we don't lock them in {'execute':'%s'} scheme. Remember, we want them to use commands/features not yet exposed by libvirt. So if qemu will ever come up with different command scheme this API will remain broken. In addition - and this is even more serious - there is no way of specifying arguments to a command, e.g. guest-sync take an integer: {'execute':'guest-sync', 'arguments':{'id':123}} Therefore I think we need to drop qemuAgentMakeCommand() here and require users to do the very same here as they do in qemu-monitor-command.

+ + ret = qemuAgentCommand(mon, jcmd, &reply); + + if (ret == 0) { + ret = qemuAgentCheckError(jcmd, reply); + *result = virJSONValueToString(reply); + } else { + *result = NULL; + } + + virJSONValueFree(jcmd); + virJSONValueFree(reply); + return ret; +} diff -uNrp libvirt-0.9.13.orig/src/qemu/qemu_agent.h libvirt-0.9.13/src/qemu/qemu_agent.h --- libvirt-0.9.13.orig/src/qemu/qemu_agent.h 2012-06-25 16:06:18.000000000 +0900 +++ libvirt-0.9.13/src/qemu/qemu_agent.h 2012-07-02 10:02:54.409454807 +0900 @@ -80,4 +80,8 @@ int qemuAgentFSThaw(qemuAgentPtr mon);

int qemuAgentSuspend(qemuAgentPtr mon, unsigned int target); + +int qemuAgentGuestAgentCommand(qemuAgentPtr mon, + const char *cmd, + char **result); #endif /* __QEMU_AGENT_H__ */ diff -uNrp libvirt-0.9.13.orig/src/qemu/qemu_driver.c libvirt-0.9.13/src/qemu/qemu_driver.c --- libvirt-0.9.13.orig/src/qemu/qemu_driver.c 2012-06-27 10:44:39.000000000 +0900 +++ libvirt-0.9.13/src/qemu/qemu_driver.c 2012-07-02 10:02:54.419455949 +0900 @@ -13158,6 +13158,78 @@ qemuListAllDomains(virConnectPtr conn, return ret; }

+static int +qemuDomainGuestAgentCommand(virDomainPtr domain, + const char *cmd, + char **result, + unsigned int flags ATTRIBUTE_UNUSED) +{ + struct qemud_driver *driver = domain->conn->privateData; + virDomainObjPtr vm; + int ret = -1; + qemuDomainObjPrivatePtr priv;

virCheckFlags(0, -1); what means flags can't be ATTRIBUTE_UNUSED.

+ + qemuDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, domain->uuid); + + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(domain->uuid, uuidstr); + qemuReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + }

Do we really want to hold driver locked through whole API?

+ + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto cleanup; + } + + if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto endjob; + } + + priv = vm->privateData; + + qemuDomainObjTaint(driver, vm, VIR_DOMAIN_TAINT_CUSTOM_MONITOR, -1); + + if (priv->agentError) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("QEMU guest agent is not " + "available due to an error")); + goto cleanup; + } + + if (!priv->agent) { + qemuReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("QEMU guest agent is not configured")); + goto cleanup; + } + + qemuDomainObjEnterAgent(driver, vm); + ret = qemuAgentGuestAgentCommand(priv->agent, cmd, result); + qemuDomainObjExitAgent(driver, vm); + + VIR_DEBUG ("qemu-agent-command ret: '%d' domain: '%s' string: %s", ret, vm->def->name, *result); + +endjob: + if (qemuDomainObjEndJob(driver, vm) == 0) { + vm = NULL; + } + +cleanup: + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return ret; +} + static virDriver qemuDriver = { .no = VIR_DRV_QEMU, .name = QEMU_DRIVER_NAME, @@ -13323,6 +13380,7 @@ static virDriver qemuDriver = { .domainPMSuspendForDuration = qemuDomainPMSuspendForDuration, /* 0.9.11 */ .domainPMWakeup = qemuDomainPMWakeup, /* 0.9.11 */ .domainGetCPUStats = qemuDomainGetCPUStats, /* 0.9.11 */ + .qemuDomainGuestAgentCommand = qemuDomainGuestAgentCommand, /* 0.9.14 */ };

diff -uNrp libvirt-0.9.13.orig/src/remote/qemu_protocol.x libvirt-0.9.13/src/remote/qemu_protocol.x --- libvirt-0.9.13.orig/src/remote/qemu_protocol.x 2012-03-06 22:59:21.000000000 +0900 +++ libvirt-0.9.13/src/remote/qemu_protocol.x 2012-07-02 10:02:54.421454763 +0900 @@ -47,6 +47,16 @@ struct qemu_domain_attach_ret { remote_nonnull_domain dom; };

+struct qemu_guest_agent_command_args { + remote_nonnull_domain dom; + remote_nonnull_string cmd; + u_int flags; +}; + +struct qemu_guest_agent_command_ret { + remote_nonnull_string result; +}; + /* Define the program number, protocol version and procedure numbers here. */ const QEMU_PROGRAM = 0x20008087; const QEMU_PROTOCOL_VERSION = 1; @@ -61,5 +71,6 @@ enum qemu_procedure { * are some exceptions to this rule, e.g. domainDestroy. Other APIs MAY * be marked as high priority. If in doubt, it's safe to choose low. */ QEMU_PROC_MONITOR_COMMAND = 1, /* skipgen skipgen priority:low */ - QEMU_PROC_DOMAIN_ATTACH = 2 /* autogen autogen priority:low */ + QEMU_PROC_DOMAIN_ATTACH = 2, /* autogen autogen priority:low */ + QEMU_PROC_GUEST_AGENT_COMMAND = 3 /* skipgen skipgen priority:low */ }; diff -uNrp libvirt-0.9.13.orig/src/remote/remote_driver.c libvirt-0.9.13/src/remote/remote_driver.c --- libvirt-0.9.13.orig/src/remote/remote_driver.c 2012-06-25 16:06:18.000000000 +0900 +++ libvirt-0.9.13/src/remote/remote_driver.c 2012-07-02 10:02:54.425455211 +0900 @@ -5127,6 +5127,44 @@ make_nonnull_domain_snapshot (remote_non make_nonnull_domain(&snapshot_dst->dom, snapshot_src->domain); }

+static int +remoteQemuDomainGuestAgentCommand (virDomainPtr domain, const char *cmd, + char **result, unsigned int flags) +{ + int rv = -1; + qemu_guest_agent_command_args args; + qemu_guest_agent_command_ret ret; + struct private_data *priv = domain->conn->privateData; + + remoteDriverLock(priv); + + make_nonnull_domain(&args.dom, domain); + args.cmd = (char *)cmd; + args.flags = flags; + + memset (&ret, 0, sizeof ret);

We require sizeof to be used as function: memset(&ret, 0, sizeof(ret));

+ + if (call (domain->conn, priv, REMOTE_CALL_QEMU, QEMU_PROC_GUEST_AGENT_COMMAND, + (xdrproc_t) xdr_qemu_guest_agent_command_args, (char *) &args, + (xdrproc_t) xdr_qemu_guest_agent_command_ret, (char *) &ret) == -1) + goto done;

Again long line, but it's among whole file so I think I can close one eye..

+ + *result = strdup(ret.result); + if (*result == NULL) { + virReportOOMError(); + goto cleanup; + } + + rv = 0; + +cleanup: + xdr_free ((xdrproc_t) xdr_qemu_guest_agent_command_ret, (char *) &ret); + +done: + remoteDriverUnlock(priv); + return rv; +} + /*----------------------------------------------------------------------*/

unsigned long remoteVersion(void) @@ -5303,6 +5339,7 @@ static virDriver remote_driver = { .domainGetDiskErrors = remoteDomainGetDiskErrors, /* 0.9.10 */ .domainSetMetadata = remoteDomainSetMetadata, /* 0.9.10 */ .domainGetMetadata = remoteDomainGetMetadata, /* 0.9.10 */ + .qemuDomainGuestAgentCommand = remoteQemuDomainGuestAgentCommand, /* 0.9.14 */ };

static virNetworkDriver network_driver = { diff -uNrp libvirt-0.9.13.orig/tools/virsh.c libvirt-0.9.13/tools/virsh.c --- libvirt-0.9.13.orig/tools/virsh.c 2012-06-28 12:05:04.000000000 +0900 +++ libvirt-0.9.13/tools/virsh.c 2012-07-02 10:02:54.441454424 +0900 @@ -160,6 +160,7 @@ typedef enum { #define VSH_CMD_GRP_SNAPSHOT "Snapshot" #define VSH_CMD_GRP_HOST_AND_HV "Host and Hypervisor" #define VSH_CMD_GRP_VIRSH "Virsh itself" +#define VSH_CMD_GRP_GUESTAGENT "QEMU Guest Agent"

Why this? I mean, qemu-monitor-command is under VSH_CMD_GRP_HOST_AND_HV so I think we should keep things consistent.

/* * Command Option Flags @@ -18138,6 +18139,68 @@ cleanup: return ret; }

+/* + * "qemu-agent-command" command + */ +static const vshCmdInfo info_qemu_agent_command[] = { + {"help", N_("Qemu Guest Agent Command")}, + {"desc", N_("Qemu Guest Agent Command")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_qemu_agent_command[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"cmd", VSH_OT_ARGV, VSH_OFLAG_REQ, N_("command")}, + {NULL, 0, 0, NULL} +}; + +static bool +cmdQemuAgentCommand(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + bool ret = false; + char *guest_agent_cmd = NULL; + char *result = NULL; + unsigned int flags = 0; + const vshCmdOpt *opt = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + bool pad = false; + + if (!vshConnectionUsability(ctl, ctl->conn)) + goto cleanup; + + dom = vshCommandOptDomain(ctl, cmd, NULL); + if (dom == NULL) + goto cleanup; + + while ((opt = vshCommandOptArgv(cmd, opt))) { + if (pad) + virBufferAddChar(&buf, ' '); + pad = true; + virBufferAdd(&buf, opt->data, -1); + } + if (virBufferError(&buf)) { + vshPrint(ctl, "%s", _("Failed to collect command")); + goto cleanup; + } + guest_agent_cmd = virBufferContentAndReset(&buf); + + if (virDomainGuestAgentCommand(dom, guest_agent_cmd, &result, flags) < 0) + goto cleanup; + + printf("%s\n", result); + + ret = true; + +cleanup: + VIR_FREE(result); + VIR_FREE(guest_agent_cmd); + if (dom) + virDomainFree(dom); + + return ret; +} + static const vshCmdDef domManagementCmds[] = { {"attach-device", cmdAttachDevice, opts_attach_device, info_attach_device, 0}, @@ -18453,6 +18516,11 @@ static const vshCmdDef hostAndHypervisor {NULL, NULL, NULL, NULL, 0} };

+static const vshCmdDef guestagentCmds[] = { + {"qemu-agent-command", cmdQemuAgentCommand, opts_qemu_agent_command, info_qemu_agent_command, 0}, + {NULL, NULL, NULL, NULL, 0} +}; + static const vshCmdGrp cmdGroups[] = { {VSH_CMD_GRP_DOM_MANAGEMENT, "domain", domManagementCmds}, {VSH_CMD_GRP_DOM_MONITORING, "monitor", domMonitoringCmds}, @@ -18465,6 +18533,7 @@ static const vshCmdGrp cmdGroups[] = { {VSH_CMD_GRP_SNAPSHOT, "snapshot", snapshotCmds}, {VSH_CMD_GRP_STORAGE_POOL, "pool", storagePoolCmds}, {VSH_CMD_GRP_STORAGE_VOL, "volume", storageVolCmds}, + {VSH_CMD_GRP_GUESTAGENT, "guestagent", guestagentCmds}, {VSH_CMD_GRP_VIRSH, "virsh", virshCmds}, {NULL, NULL, NULL}

Plus, you need to update src/qemu_protocol-structs as well For future version, can you please utilize git and git-send-email? It makes review simpler. Thanks. Michal

Hi, All. Thank you for many comments. And I extremely apologize not to use git... My company's network closes the almost port and only http via proxy and simple mail are available. The attached patch is reflected all comments. Especially,

...

diff -uNrp libvirt-0.9.13.orig/src/driver.h libvirt-0.9.13/src/driver.h --- libvirt-0.9.13.orig/src/driver.h 2012-06-25 16:06:18.000000000 +0900 +++ libvirt-0.9.13/src/driver.h 2012-07-02 10:02:54.405454285 +0900 @@ -860,6 +860,10 @@ typedef char * const char *uri, unsigned int flags);

+typedef int + (*virDrvDomainGuestAgentCommand)(virDomainPtr domain, const char *cmd, + char **result, unsigned int flags); + /** * _virDriver: * @@ -1041,6 +1045,7 @@ struct _virDriver { virDrvDomainGetDiskErrors domainGetDiskErrors; virDrvDomainSetMetadata domainSetMetadata; virDrvDomainGetMetadata domainGetMetadata; + virDrvDomainGuestAgentCommand qemuDomainGuestAgentCommand; };

I'd move these two as close as possible to virDrvDomainQemuMonitorCommand so we have similar commands grouped.

...

diff -uNrp libvirt-0.9.13.orig/src/qemu/qemu_agent.c

...

--- libvirt-0.9.13.orig/src/qemu/qemu_agent.c 2012-06-25 16:06:18.000000000 +0900 +++ libvirt-0.9.13/src/qemu/qemu_agent.c 2012-07-02 10:02:54.408488163 +0900 @@ -1410,3 +1410,29 @@ qemuAgentSuspend(qemuAgentPtr mon, virJSONValueFree(reply); return ret; } + +int qemuAgentGuestAgentCommand(qemuAgentPtr mon, + const char *cmd, + char **result) +{ + int ret = -1; + virJSONValuePtr jcmd; + virJSONValuePtr reply = NULL; + + jcmd = qemuAgentMakeCommand(cmd, NULL); + if (!jcmd) + return ret;

This is confusing. In case of qemu-monitor-command we require user to

I moved virDrvDomainQemuMonitorCommand to virDrvDomainQemuSupportCommand to close two functions. In addition, libvirt-0.9.13/src/qemu/qemu_agent.c pass whole command string:

{'execute':'some-dummy-command'} which is correct as we don't lock them in {'execute':'%s'} scheme.

Remember, we want them to

use commands/features not yet exposed by libvirt. So if qemu will ever come up with different command scheme this API will remain broken. In addition - and this is even more serious - there is no way of specifying arguments to a command, e.g. guest-sync take an integer: {'execute':'guest-sync', 'arguments':{'id':123}} Therefore I think we need to drop qemuAgentMakeCommand() here and require users to do the very same here as they do in qemu-monitor-command.

I prefer to the type of 'virsh qemu-agent-command guest-info', bacause it is user friendly. But I understood the problem and fixed. Regards MATSUDA Daiki $ diffstat libvirt-0.9.13_qemu_agent_command3.patch daemon/remote.c | 36 +++++++++++++++++ docs/hvsupport.pl | 1 include/libvirt/libvirt-qemu.h | 3 + python/generator.py | 1 python/libvirt-qemu-override-api.xml | 7 +++ python/libvirt-qemu-override.c | 31 ++++++++++++++ src/driver.h | 5 +- src/libvirt-qemu.c | 49 +++++++++++++++++++++++ src/libvirt_qemu.syms | 5 ++ src/qemu/qemu_agent.c | 26 ++++++++++++ src/qemu/qemu_agent.h | 4 + src/qemu/qemu_driver.c | 73 +++++++++++++++++++++++++++++++++++ src/qemu_protocol-structs | 9 ++++ src/remote/qemu_protocol.x | 13 +++++- src/remote/remote_driver.c | 39 ++++++++++++++++++ tools/virsh.c | 64 ++++++++++++++++++++++++++++++ 16 files changed, 363 insertions(+), 3 deletions(-) virsh # qemu-agent-command RHEL58_64 '{"execute":"guest-info"}' {"return":{"version":"1.1.50","supported_commands":[{"enabled":true,"name":"guest-network-get-interfaces"},{"enabled":true,"name":"guest-suspend-hybrid"},{"enabled":true,"name":"guest-suspend-ram"},{"enabled":true,"name":"guest-suspend-disk"},{"enabled":true,"name":"guest-fsfreeze-thaw"},{"enabled":true,"name":"guest-fsfreeze-freeze"},{"enabled":true,"name":"guest-fsfreeze-status"},{"enabled":true,"name":"guest-file-flush"},{"enabled":true,"name":"guest-file-seek"},{"enabled":true,"name":"guest-file-write"},{"enabled":true,"name":"guest-file-read"},{"enabled":true,"name":"guest-file-close"},{"enabled":true,"name":"guest-file-open"},{"enabled":true,"name":"guest-shutdown"},{"enabled":true,"name":"guest-info"},{"enabled":true,"name":"guest-ping"},{"enabled":true,"name":"guest-sync"},{"enabled":true,"name":"guest-sync-delimited"}]}} virsh # qemu-agent-command RHEL58_64 '{"execute":"guest-sync", "arguments":{"id":123}}' {"return":123}