5:54 a.m.
[I am pasting your patch here again so I can point out some nits]
On 06.07.2012 03:29, MATSUDA, Daiki wrote:
diff -uNrp libvirt-0.9.13.orig/docs/hvsupport.pl
libvirt-0.9.13/docs/hvsupport.pl
--- libvirt-0.9.13.orig/docs/hvsupport.pl 2011-06-10 15:50:11.000000000 +0900
+++ libvirt-0.9.13/docs/hvsupport.pl 2012-07-06 09:18:36.363454752 +0900
@@ -129,6 +129,7 @@ $apis{virDomainMigratePerform3} = "0.9.2
$apis{virDomainMigrateFinish3} = "0.9.2";
$apis{virDomainMigrateConfirm3} = "0.9.2";
+$apis{virDomainQemuSupportCommand} = "0.9.14";
This is a result of joining function prototype ...
# Now we want to get the mapping between public APIs
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-06 09:18:12.497454257 +0900
@@ -680,7 +680,7 @@ typedef int
unsigned int flags);
typedef int
- (*virDrvDomainQemuMonitorCommand)(virDomainPtr domain, const char *cmd,
+ (*virDrvDomainQemuSupportCommand)(virDomainPtr domain, const char *cmd,
char **result, unsigned int flags);
... here. Despite QemuMonitorCommand and QemuAgentCommand having the same prototype we
don't join them together.
/* Choice of unsigned int rather than pid_t is intentional. */
@@ -1015,7 +1015,7 @@ struct _virDriver {
virDrvDomainSnapshotHasMetadata domainSnapshotHasMetadata;
virDrvDomainRevertToSnapshot domainRevertToSnapshot;
virDrvDomainSnapshotDelete domainSnapshotDelete;
- virDrvDomainQemuMonitorCommand qemuDomainMonitorCommand;
+ virDrvDomainQemuSupportCommand qemuDomainMonitorCommand;
Hence this change should not be here.
virDrvDomainQemuAttach qemuDomainAttach;
virDrvDomainOpenConsole domainOpenConsole;
virDrvDomainOpenGraphics domainOpenGraphics;
@@ -1041,6 +1041,7 @@ struct _virDriver {
virDrvDomainGetDiskErrors domainGetDiskErrors;
virDrvDomainSetMetadata domainSetMetadata;
virDrvDomainGetMetadata domainGetMetadata;
+ virDrvDomainQemuSupportCommand qemuDomainQemuAgentCommand;
And this should be virDrvDomainQemuAgentCommand.
};
typedef int
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-06 09:18:12.503579712 +0900
@@ -13158,6 +13158,78 @@ qemuListAllDomains(virConnectPtr conn,
return ret;
}
+static int
+qemuDomainQemuAgentCommand(virDomainPtr domain,
+ const char *cmd,
+ char **result,
+ unsigned int flags)
+{
+ struct qemud_driver *driver = domain->conn->privateData;
+ virDomainObjPtr vm;
+ int ret = -1;
+ qemuDomainObjPrivatePtr priv;
+
+ virCheckFlags(0, -1);
+
+ qemuDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, domain->uuid);
+ qemuDriverUnlock(driver);
+
+ 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;
+ }
+
+ priv = vm->privateData;
+
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("domain is not running"));
+ goto cleanup;
+ }
+
+ 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;
+ }
+
+ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+ goto cleanup;
+
+ if (!virDomainObjIsActive(vm)) {
+ qemuReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("domain is not running"));
+ goto endjob;
+ }
+
+ qemuDomainObjEnterAgent(driver, vm);
+ ret = qemuAgentQemuAgentCommand(priv->agent, cmd, result);
+ qemuDomainObjExitAgent(driver, vm);
+
+ VIR_DEBUG ("qemu-agent-command ret: '%d' domain: '%s' string:
%s",
+ ret, vm->def->name, *result);
This is unnecessary. qemuAgentQemuAgentCommand() already logged result here.
+
+endjob:
+ if (qemuDomainObjEndJob(driver, vm) == 0) {
+ vm = NULL;
+ }
+
+cleanup:
+ if (vm)
+ virDomainObjUnlock(vm);
+ return ret;
+}
+
static virDriver qemuDriver = {
.no = VIR_DRV_QEMU,
.name = QEMU_DRIVER_NAME,
@@ -13323,6 +13395,7 @@ static virDriver qemuDriver = {
.domainPMSuspendForDuration = qemuDomainPMSuspendForDuration, /* 0.9.11 */
.domainPMWakeup = qemuDomainPMWakeup, /* 0.9.11 */
.domainGetCPUStats = qemuDomainGetCPUStats, /* 0.9.11 */
+ .qemuDomainQemuAgentCommand = qemuDomainQemuAgentCommand, /* 0.9.14 */
};
Otherwise looking good. ACK with this squashed in:
diff --git a/docs/hvsupport.pl b/docs/hvsupport.pl
index b2b6df7..b0d1f0f 100755
--- a/docs/hvsupport.pl
+++ b/docs/hvsupport.pl
@@ -129,7 +129,6 @@ $apis{virDomainMigratePerform3} = "0.9.2";
$apis{virDomainMigrateFinish3} = "0.9.2";
$apis{virDomainMigrateConfirm3} = "0.9.2";
-$apis{virDomainQemuSupportCommand} = "0.9.14";
# Now we want to get the mapping between public APIs
diff --git a/src/driver.h b/src/driver.h
index ab9e3b4..e905e42 100644
--- a/src/driver.h
+++ b/src/driver.h
@@ -680,8 +680,11 @@ typedef int
unsigned int flags);
typedef int
- (*virDrvDomainQemuSupportCommand)(virDomainPtr domain, const char *cmd,
+ (*virDrvDomainQemuMonitorCommand)(virDomainPtr domain, const char *cmd,
char **result, unsigned int flags);
+typedef int
+ (*virDrvDomainQemuAgentCommand)(virDomainPtr domain, const char *cmd,
+ char **result, unsigned int flags);
/* Choice of unsigned int rather than pid_t is intentional. */
typedef virDomainPtr
@@ -1015,7 +1018,7 @@ struct _virDriver {
virDrvDomainSnapshotHasMetadata domainSnapshotHasMetadata;
virDrvDomainRevertToSnapshot domainRevertToSnapshot;
virDrvDomainSnapshotDelete domainSnapshotDelete;
- virDrvDomainQemuSupportCommand qemuDomainMonitorCommand;
+ virDrvDomainQemuMonitorCommand qemuDomainMonitorCommand;
virDrvDomainQemuAttach qemuDomainAttach;
virDrvDomainOpenConsole domainOpenConsole;
virDrvDomainOpenGraphics domainOpenGraphics;
@@ -1041,7 +1044,7 @@ struct _virDriver {
virDrvDomainGetDiskErrors domainGetDiskErrors;
virDrvDomainSetMetadata domainSetMetadata;
virDrvDomainGetMetadata domainGetMetadata;
- virDrvDomainQemuSupportCommand qemuDomainQemuAgentCommand;
+ virDrvDomainQemuAgentCommand qemuDomainQemuAgentCommand;
};
typedef int
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 66bbce1..af20437 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -13217,9 +13217,6 @@ qemuDomainQemuAgentCommand(virDomainPtr domain,
ret = qemuAgentQemuAgentCommand(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;
However, I will hold the push for a while. There is one general problem with this patch.
Unlike qemu monitor, guest agent has several commands which don't return any
successful message (guest-suspend-* family). with current implementation libvirt takes an
event on command as confirmation of success. That is - we block until an event is
received. But with this patch, if user will issue such command the whole API would block
endlessly. I am not sure how we should fix this. Either we will take the current
implementation (what if qemu will ever come with new command which doesn't report
successful run?) or we introduce a flag DONT_WAIT_FOR_REPLY (tricky, we want to wait for
error response - but what timeout should we choose? moreover, timeouts are bad).
Therefore, if anybody has any suggestions I am more than happy to hear them.
Michal