[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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list