On Tue, Apr 29, 2014 at 08:03:57PM -0400, Tomoki Sekiyama wrote: We shouldn't be ignoring the mountpoints argument. I'd reword this as "Not all drivers will support freezing individual mount points, and may return VIR_ERR_ARGUMENT_UNSUPPORTED if @mountpoints is non-NULL" Again, clarify the text in the same way. Should include mountpoints/nmountpoints in debug line. Those nit-picks aside, this API looks good. Regards, 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 :|

New rules are added in fixup_name in gendispatch.pl to keep the name FSFreeze and FSThaw. This adds a new ACL permission 'fs_freeze', which is also applied to VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE flag. Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama(a)hds.com&gt; --- src/access/viraccessperm.c | 2 +- src/access/viraccessperm.h | 6 ++++++ src/remote/remote_driver.c | 2 ++ src/remote/remote_protocol.x | 38 ++++++++++++++++++++++++++++++++++++-- src/remote_protocol-structs | 18 ++++++++++++++++++ src/rpc/gendispatch.pl | 2 ++ 6 files changed, 65 insertions(+), 3 deletions(-) diff --git a/src/access/viraccessperm.c b/src/access/viraccessperm.c index d517c66..462f46c 100644 --- a/src/access/viraccessperm.c +++ b/src/access/viraccessperm.c @@ -39,7 +39,7 @@ VIR_ENUM_IMPL(virAccessPermDomain, "start", "stop", "reset", "save", "delete", "migrate", "snapshot", "suspend", "hibernate", "core_dump", "pm_control", - "init_control", "inject_nmi", "send_input", "send_signal", "fs_trim", + "init_control", "inject_nmi", "send_input", "send_signal", "fs_trim", "fs_freeze", "block_read", "block_write", "mem_read", "open_graphics", "open_device", "screenshot", "open_namespace"); diff --git a/src/access/viraccessperm.h b/src/access/viraccessperm.h index 6d14f05..ac48d70 100644 --- a/src/access/viraccessperm.h +++ b/src/access/viraccessperm.h @@ -242,6 +242,12 @@ typedef enum { */ VIR_ACCESS_PERM_DOMAIN_FS_TRIM, /* Issue TRIM to guest filesystems */ + /** + * @desc: Freeze and thaw domain filesystems + * @message: Freezing and thawing domain filesystems require authorization + */ + VIR_ACCESS_PERM_DOMAIN_FS_FREEZE, /* Freeze and thaw guest filesystems */ + /* Peeking at guest */ /** diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index d9be756..4c3b38a 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -7768,6 +7768,8 @@ static virDriver remote_driver = { .domainMigrateFinish3Params = remoteDomainMigrateFinish3Params, /* 1.1.0 */ .domainMigrateConfirm3Params = remoteDomainMigrateConfirm3Params, /* 1.1.0 */ .connectGetCPUModelNames = remoteConnectGetCPUModelNames, /* 1.1.3 */ + .domainFSFreeze = remoteDomainFSFreeze, /* 1.2.5 */ + .domainFSThaw = remoteDomainFSThaw, /* 1.2.5 */ }; static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 6c445cc..608490c 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -235,6 +235,9 @@ const REMOTE_DOMAIN_JOB_STATS_MAX = 64; /* Upper limit on number of CPU models */ const REMOTE_CONNECT_CPU_MODELS_MAX = 8192; +/* Upper limit on number of mountpoints to frozen */ +const REMOTE_DOMAIN_FSFREEZE_MOUNTPOINTS_MAX = 256; + /* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */ typedef opaque remote_uuid[VIR_UUID_BUFLEN]; @@ -2959,6 +2962,25 @@ struct remote_network_event_lifecycle_msg { int detail; }; +struct remote_domain_fsfreeze_args { + remote_nonnull_domain dom; + remote_nonnull_string mountpoints<REMOTE_DOMAIN_FSFREEZE_MOUNTPOINTS_MAX>; /* (const char **) */ + unsigned int flags; +}; + +struct remote_domain_fsfreeze_ret { + int filesystems; +}; + +struct remote_domain_fsthaw_args { + remote_nonnull_domain dom; + remote_nonnull_string mountpoints<REMOTE_DOMAIN_FSFREEZE_MOUNTPOINTS_MAX>; /* (const char **) */ + unsigned int flags; +}; + +struct remote_domain_fsthaw_ret { + int filesystems; +}; /*----- Protocol. -----*/ @@ -4289,7 +4311,7 @@ enum remote_procedure { /** * @generate: both * @acl: domain:snapshot - * @acl: domain:write:VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE + * @acl: domain:fs_freeze:VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE */ REMOTE_PROC_DOMAIN_SNAPSHOT_CREATE_XML = 185, @@ -5275,5 +5297,17 @@ enum remote_procedure { * @generate: both * @acl: domain:core_dump */ - REMOTE_PROC_DOMAIN_CORE_DUMP_WITH_FORMAT = 334 + REMOTE_PROC_DOMAIN_CORE_DUMP_WITH_FORMAT = 334, + + /** + * @generate: both + * @acl: domain:fs_freeze + */ + REMOTE_PROC_DOMAIN_FSFREEZE = 335, + + /** + * @generate: both + * @acl: domain:fs_freeze + */ + REMOTE_PROC_DOMAIN_FSTHAW = 336 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 456d0da..15c95ca 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -2427,6 +2427,22 @@ struct remote_network_event_lifecycle_msg { int event; int detail; }; +struct remote_domain_fsfreeze_args { + remote_nonnull_domain dom; + struct { + u_int mountpoints_len; + remote_nonnull_string * mountpoints_val; + } mountpoints; + u_int flags; +}; +struct remote_domain_fsthaw_args { + remote_nonnull_domain dom; + struct { + u_int mountpoints_len; + remote_nonnull_string * mountpoints_val; + } mountpoints; + u_int flags; +}; enum remote_procedure { REMOTE_PROC_CONNECT_OPEN = 1, REMOTE_PROC_CONNECT_CLOSE = 2, @@ -2762,4 +2778,6 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_EVENT_CALLBACK_PMSUSPEND_DISK = 332, REMOTE_PROC_DOMAIN_EVENT_CALLBACK_DEVICE_REMOVED = 333, REMOTE_PROC_DOMAIN_CORE_DUMP_WITH_FORMAT = 334, + REMOTE_PROC_DOMAIN_FSFREEZE = 335, + REMOTE_PROC_DOMAIN_FSTHAW = 336, }; diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index b76bbac..9cd620e 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -64,6 +64,8 @@ sub fixup_name { $name =~ s/Nmi$/NMI/; $name =~ s/Pm/PM/; $name =~ s/Fstrim$/FSTrim/; + $name =~ s/Fsfreeze$/FSFreeze/; + $name =~ s/Fsthaw$/FSThaw/; $name =~ s/Scsi/SCSI/; $name =~ s/Wwn$/WWN/;

Adds 'quiesced' status into qemuDomainObjPrivate that tracks whether FSFreeze is requested in the domain. It modifies error code from qemuDomainSnapshotFSFreeze and qemuDomainSnapshotFSThaw, so that a caller can know whether the command is actually sent to the guest agent. If the error is caused before sending a freeze command, a counterpart thaw command shouldn't be sent either, not to confuse fsfreeze status tracking. Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama(a)hds.com&gt; --- src/qemu/qemu_domain.c | 5 +++ src/qemu/qemu_domain.h | 2 + src/qemu/qemu_driver.c | 70 +++++++++++++++++++++++++++++++++++++++--------- 3 files changed, 64 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8fa58f3..1dd507f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -357,6 +357,9 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf, void *data) virBufferAddLit(buf, "</devices>\n"); } + if (priv->quiesced) + virBufferAddLit(buf, "<quiesced/>\n"); + return 0; } @@ -518,6 +521,8 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, void *data) } VIR_FREE(nodes); + priv->quiesced = virXPathBoolean("boolean(./quiesced)", ctxt) == 1; + return 0; error: diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 31611b5..ab27f15 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -176,6 +176,8 @@ struct _qemuDomainObjPrivate { char **qemuDevices; /* NULL-terminated list of devices aliases known to QEMU */ bool hookRun; /* true if there was a hook run over this domain */ + + bool quiesced; /* true if filesystems are quiesced */ }; typedef enum { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 75cf8cb..c3e0682 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12060,32 +12060,62 @@ qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, } +/* Return -1 if request is not sent to agent due to misconfig, -2 if request + * is sent but failed, and number of frozen filesystems on success. If -2 is + * returned, FSThaw should be called revert the quiesced status. */ static int -qemuDomainSnapshotFSFreeze(virDomainObjPtr vm) +qemuDomainSnapshotFSFreeze(virQEMUDriverPtr driver, + virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; + virQEMUDriverConfigPtr cfg; int freezed; + if (priv->quiesced) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is already quiesced")); + return -1; + } + if (!qemuDomainAgentAvailable(priv, true)) return -1; + priv->quiesced = true; + + cfg = virQEMUDriverGetConfig(driver); + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) { + priv->quiesced = false; + virObjectUnref(cfg); + return -1; + } + virObjectUnref(cfg); + qemuDomainObjEnterAgent(vm); freezed = qemuAgentFSFreeze(priv->agent); qemuDomainObjExitAgent(vm); - - return freezed; + return freezed < 0 ? -2 : freezed; } +/* Return -1 on error, otherwise number of thawed filesystems. */ static int -qemuDomainSnapshotFSThaw(virDomainObjPtr vm, bool report) +qemuDomainSnapshotFSThaw(virQEMUDriverPtr driver, + virDomainObjPtr vm, + bool report) { qemuDomainObjPrivatePtr priv = vm->privateData; + virQEMUDriverConfigPtr cfg; int thawed; virErrorPtr err = NULL; if (!qemuDomainAgentAvailable(priv, report)) return -1; + if (!priv->quiesced && report) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not quiesced")); + return -1; + } + qemuDomainObjEnterAgent(vm); if (!report) err = virSaveLastError(); @@ -12095,6 +12125,19 @@ qemuDomainSnapshotFSThaw(virDomainObjPtr vm, bool report) qemuDomainObjExitAgent(vm); virFreeError(err); + + if (!report || thawed >= 0) { + priv->quiesced = false; + + cfg = virQEMUDriverGetConfig(driver); + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) { + /* Revert the statuses when we failed to save them. */ + priv->quiesced = true; + thawed = -1; + } + virObjectUnref(cfg); + } + return thawed; } @@ -13092,17 +13135,18 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, goto cleanup; /* If quiesce was requested, then issue a freeze command, and a - * counterpart thaw command, no matter what. The command will - * fail if the guest is paused or the guest agent is not - * running. */ + * counterpart thaw command when it is actually sent to agent. + * The command will fail if the guest is paused or the guest agent + * is not running, or is already quiesced. */ if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE) { - if (qemuDomainSnapshotFSFreeze(vm) < 0) { - /* helper reported the error */ - thaw = -1; + int freeze = qemuDomainSnapshotFSFreeze(driver, vm); + if (freeze < 0) { + /* the helper reported the error */ + if (freeze == -2) + thaw = -1; /* the command is sent but agent failed */ goto endjob; - } else { - thaw = 1; } + thaw = 1; } /* We need to track what state the guest is in, since taking the @@ -13243,7 +13287,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, goto cleanup; } if (vm && thaw != 0 && - qemuDomainSnapshotFSThaw(vm, thaw > 0) < 0) { + qemuDomainSnapshotFSThaw(driver, vm, thaw > 0) < 0) { /* helper reported the error, if it was needed */ if (thaw > 0) ret = -1;

On Tue, Apr 29, 2014 at 08:04:18PM -0400, Tomoki Sekiyama wrote: We should report VIR_ERR_ARGUMENT_UNSUPPORTED and return -1 to the caller Same comment about VIR_ERR_ARGUMENT_UNSUPPORTED for nmountpoints being non-NULL. Regards, 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 :|

With this patch, virDomainFSFreeze will pass the mountpoints argument to qemu guest agent. For example, virDomainFSFreeze(dom, {"/mnt/vol1", "/mnt/vol2"}, 2, 0) will issue qemu guest agent command: {"execute":"guest-fsfreeze-freeze", "arguments":{"mountpoints": ["/mnt/vol1","/mnt/vol2"]}} Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama(a)hds.com&gt; --- src/qemu/qemu_agent.c | 47 +++++++++++++++++++++++++++++++++++++++++++---- src/qemu/qemu_agent.h | 3 ++- src/qemu/qemu_driver.c | 13 ++++++------- tests/qemuagenttest.c | 8 +++++--- 4 files changed, 56 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 4082331..57c7cc5 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1235,6 +1235,32 @@ qemuAgentMakeCommand(const char *cmdname, return NULL; } +static virJSONValuePtr +qemuAgentMakeStringsArray(const char **strings, unsigned int len) +{ + size_t i; + virJSONValuePtr ret = virJSONValueNewArray(), str; + + if (!ret) + return NULL; + + for (i = 0; i < len; i++) { + str = virJSONValueNewString(strings[i]); + if (!str) + goto error; + + if (virJSONValueArrayAppend(ret, str) < 0) { + virJSONValueFree(str); + goto error; + } + } + return ret; + + error: + virJSONValueFree(ret); + return NULL; +} + void qemuAgentNotifyEvent(qemuAgentPtr mon, qemuAgentEvent event) { @@ -1287,21 +1313,34 @@ int qemuAgentShutdown(qemuAgentPtr mon, /* * qemuAgentFSFreeze: * @mon: Agent + * @mountpoints: Array of mountpoint paths to be frozen, or NULL for all + * @nmountpoints: Number of mountpoints to be frozen, or 0 for all * * Issue guest-fsfreeze-freeze command to guest agent, - * which freezes all mounted file systems and returns + * which freezes file systems mounted on specified mountpoints + * (or all file systems when @mountpoints is NULL), and returns * number of frozen file systems on success. * * Returns: number of file system frozen on success, * -1 on error. */ -int qemuAgentFSFreeze(qemuAgentPtr mon) +int qemuAgentFSFreeze(qemuAgentPtr mon, const char **mountpoints, + unsigned int nmountpoints) { int ret = -1; - virJSONValuePtr cmd; + virJSONValuePtr cmd, arg; virJSONValuePtr reply = NULL; - cmd = qemuAgentMakeCommand("guest-fsfreeze-freeze", NULL); + if (mountpoints && nmountpoints) { + arg = qemuAgentMakeStringsArray(mountpoints, nmountpoints); + if (!arg) + return -1; + + cmd = qemuAgentMakeCommand("guest-fsfreeze-freeze", + "a:mountpoints", arg, NULL); + } else { + cmd = qemuAgentMakeCommand("guest-fsfreeze-freeze", NULL); + } if (!cmd) return -1; diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h index 5fbacdb..58531d5 100644 --- a/src/qemu/qemu_agent.h +++ b/src/qemu/qemu_agent.h @@ -70,7 +70,8 @@ typedef enum { int qemuAgentShutdown(qemuAgentPtr mon, qemuAgentShutdownMode mode); -int qemuAgentFSFreeze(qemuAgentPtr mon); +int qemuAgentFSFreeze(qemuAgentPtr mon, + const char **mountpoints, unsigned int nmountpoints); int qemuAgentFSThaw(qemuAgentPtr mon); int qemuAgentSuspend(qemuAgentPtr mon, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6bfd508..ae2d561 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12065,7 +12065,9 @@ qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, * returned, FSThaw should be called revert the quiesced status. */ static int qemuDomainSnapshotFSFreeze(virQEMUDriverPtr driver, - virDomainObjPtr vm) + virDomainObjPtr vm, + const char **mountpoints, + unsigned int nmountpoints) { qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverConfigPtr cfg; @@ -12091,7 +12093,7 @@ qemuDomainSnapshotFSFreeze(virQEMUDriverPtr driver, virObjectUnref(cfg); qemuDomainObjEnterAgent(vm); - freezed = qemuAgentFSFreeze(priv->agent); + freezed = qemuAgentFSFreeze(priv->agent, mountpoints, nmountpoints); qemuDomainObjExitAgent(vm); return freezed < 0 ? -2 : freezed; } @@ -13139,7 +13141,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, * The command will fail if the guest is paused or the guest agent * is not running, or is already quiesced. */ if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE) { - int freeze = qemuDomainSnapshotFSFreeze(driver, vm); + int freeze = qemuDomainSnapshotFSFreeze(driver, vm, NULL, 0); if (freeze < 0) { /* the helper reported the error */ if (freeze == -2) @@ -16540,9 +16542,6 @@ qemuDomainFSFreeze(virDomainPtr dom, virCheckFlags(0, -1); - if (mountpoints || nmountpoints) - VIR_INFO("mountpoints option is not supported and ignored for now"); - if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; @@ -16558,7 +16557,7 @@ qemuDomainFSFreeze(virDomainPtr dom, goto endjob; } - ret = qemuDomainSnapshotFSFreeze(driver, vm); + ret = qemuDomainSnapshotFSFreeze(driver, vm, mountpoints, nmountpoints); if (ret == -2) { qemuDomainSnapshotFSThaw(driver, vm, false); ret = -1; diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c index 1131d98..be207e8 100644 --- a/tests/qemuagenttest.c +++ b/tests/qemuagenttest.c @@ -36,6 +36,7 @@ testQemuAgentFSFreeze(const void *data) { virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt); + const char *mountpoints[] = {"/fs1", "/fs2", "/fs3", "/fs4", "/fs5"}; int ret = -1; if (!test) @@ -55,7 +56,8 @@ testQemuAgentFSFreeze(const void *data) "{ \"return\" : 7 }") < 0) goto cleanup; - if ((ret = qemuAgentFSFreeze(qemuMonitorTestGetAgent(test))) < 0) + if ((ret = qemuAgentFSFreeze(qemuMonitorTestGetAgent(test), + mountpoints, 5)) < 0) goto cleanup; if (ret != 5) { @@ -64,7 +66,7 @@ testQemuAgentFSFreeze(const void *data) goto cleanup; } - if ((ret = qemuAgentFSFreeze(qemuMonitorTestGetAgent(test))) < 0) + if ((ret = qemuAgentFSFreeze(qemuMonitorTestGetAgent(test), NULL, 0)) < 0) goto cleanup; if (ret != 7) { @@ -547,7 +549,7 @@ testQemuAgentTimeout(const void *data) NULL, NULL) < 0) goto cleanup; - if (qemuAgentFSFreeze(qemuMonitorTestGetAgent(test)) != -1) { + if (qemuAgentFSFreeze(qemuMonitorTestGetAgent(test), NULL, 0) != -1) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", "agent command should have failed"); goto cleanup;