12:47 p.m.
https://bugzilla.redhat.com/show_bug.cgi?id=831159
Michal Privoznik (4):
Introduce virDomainFSTrim() public API
remote: Implement virDomainFSTrim
qemu: Implement virDomainFSTrim
virsh: Expose new virDomainFSTrim API
include/libvirt/libvirt.h.in | 4 ++
src/driver.h | 6 +++
src/libvirt.c | 50 +++++++++++++++++++++++++
src/libvirt_public.syms | 5 +++
src/qemu/qemu_agent.c | 25 +++++++++++++
src/qemu/qemu_agent.h | 2 +
src/qemu/qemu_driver.c | 83 ++++++++++++++++++++++++++++++++++++++++++
src/remote/remote_driver.c | 1 +
src/remote/remote_protocol.x | 10 +++++-
src/remote_protocol-structs | 7 ++++
src/rpc/gendispatch.pl | 1 +
tools/virsh-domain.c | 40 ++++++++++++++++++++
tools/virsh.pod | 12 ++++++
13 files changed, 245 insertions(+), 1 deletions(-)
--
1.7.8.6
12:47 p.m.
New subject: [libvirt] [PATCH 1/4] Introduce virDomainFSTrim() public API
This will call FITRIM within guest. The API has 4 arguments,
however, only 2 will be used for now (@dom and @minumum).
The rest two are there if in future qemu guest agent learns them.
---
include/libvirt/libvirt.h.in | 4 +++
src/driver.h | 6 +++++
src/libvirt.c | 50 ++++++++++++++++++++++++++++++++++++++++++
src/libvirt_public.syms | 5 ++++
4 files changed, 65 insertions(+), 0 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 49a361a..b161bce 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -4438,6 +4438,10 @@ int virDomainOpenGraphics(virDomainPtr dom,
int virDomainInjectNMI(virDomainPtr domain, unsigned int flags);
+int virDomainFSTrim(virDomainPtr dom,
+ const char *mountPoint,
+ unsigned long long minimum,
+ unsigned int flags);
/**
* virSchedParameterType:
diff --git a/src/driver.h b/src/driver.h
index 7ba66ad..5163840 100644
--- a/src/driver.h
+++ b/src/driver.h
@@ -903,6 +903,11 @@ typedef int
unsigned char **cpumap,
unsigned int *online,
unsigned int flags);
+typedef int
+ (*virDrvDomainFSTrim)(virDomainPtr dom,
+ const char *mountPoint,
+ unsigned long long minimum,
+ unsigned int flags);
/**
* _virDriver:
@@ -1094,6 +1099,7 @@ struct _virDriver {
virDrvNodeGetMemoryParameters nodeGetMemoryParameters;
virDrvNodeSetMemoryParameters nodeSetMemoryParameters;
virDrvNodeGetCPUMap nodeGetCPUMap;
+ virDrvDomainFSTrim domainFSTrim;
};
typedef int
diff --git a/src/libvirt.c b/src/libvirt.c
index bdb1dc6..1c6863f 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -20164,3 +20164,53 @@ error:
virDispatchError(conn);
return -1;
}
+
+/**
+ * virDomainFSTrim:
+ * @dom: a domain object
+ * @mountPoint: which mount point trim
+ * @minimum: Minimum contiguous free range to discard in bytes
+ * @flags: extra flags, not used yet, so callers should always pass 0
+ *
+ * Calls FITRIM within the guest (hence guest agent may be
+ * required depending on hypervisor used). Either call it on each
+ * mounted filesystem (@mountPoint is NULL) or just on specified
+ * @mountPoint. @minimum tell that free ranges smaller than this
+ * may be ignored (this is a hint and the guest may not respect
+ * it). By increasing this value, the fstrim operation will
+ * complete more quickly for filesystems with badly fragmented
+ * free space, although not all blocks will be discarded.
+ *
+ * Returns 0 on success, -1 otherwise.
+ */
+int
+virDomainFSTrim(virDomainPtr dom,
+ const char *mountPoint,
+ unsigned long long minimum,
+ unsigned int flags)
+{
+ VIR_DOMAIN_DEBUG(dom, "mountPoint=%s, minimum=%llu, flags=%x",
+ mountPoint, minimum, flags);
+
+ virResetLastError();
+
+ if (!VIR_IS_DOMAIN(dom)) {
+ virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
+ virDispatchError(NULL);
+ return -1;
+ }
+
+ if (dom->conn->driver->domainFSTrim) {
+ int ret = dom->conn->driver->domainFSTrim(dom, mountPoint,
+ minimum, flags);
+ if (ret < 0)
+ goto error;
+ return ret;
+ }
+
+ virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
+
+error:
+ virDispatchError(dom->conn);
+ return -1;
+}
diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
index f494821..f699e93 100644
--- a/src/libvirt_public.syms
+++ b/src/libvirt_public.syms
@@ -574,4 +574,9 @@ LIBVIRT_1.0.0 {
virNodeGetCPUMap;
} LIBVIRT_0.10.2;
+LIBVIRT_1.0.1 {
+ global:
+ virDomainFSTrim;
+} LIBVIRT_1.0.0;
+
# .... define new API here using predicted next version number ....
--
1.7.8.6
12:57 p.m.
New subject: [libvirt] [PATCH 1/4] Introduce virDomainFSTrim() public API
On Tue, Nov 20, 2012 at 07:47:35PM +0100, Michal Privoznik wrote:
This will call FITRIM within guest. The API has 4 arguments,
however, only 2 will be used for now (@dom and @minumum).
The rest two are there if in future qemu guest agent learns them.
---
include/libvirt/libvirt.h.in | 4 +++
src/driver.h | 6 +++++
src/libvirt.c | 50 ++++++++++++++++++++++++++++++++++++++++++
src/libvirt_public.syms | 5 ++++
4 files changed, 65 insertions(+), 0 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 49a361a..b161bce 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -4438,6 +4438,10 @@ int virDomainOpenGraphics(virDomainPtr dom,
int virDomainInjectNMI(virDomainPtr domain, unsigned int flags);
+int virDomainFSTrim(virDomainPtr dom,
+ const char *mountPoint,
+ unsigned long long minimum,
+ unsigned int flags);
/**
* virSchedParameterType:
diff --git a/src/driver.h b/src/driver.h
index 7ba66ad..5163840 100644
--- a/src/driver.h
+++ b/src/driver.h
@@ -903,6 +903,11 @@ typedef int
unsigned char **cpumap,
unsigned int *online,
unsigned int flags);
+typedef int
+ (*virDrvDomainFSTrim)(virDomainPtr dom,
+ const char *mountPoint,
+ unsigned long long minimum,
+ unsigned int flags);
/**
* _virDriver:
@@ -1094,6 +1099,7 @@ struct _virDriver {
virDrvNodeGetMemoryParameters nodeGetMemoryParameters;
virDrvNodeSetMemoryParameters nodeSetMemoryParameters;
virDrvNodeGetCPUMap nodeGetCPUMap;
+ virDrvDomainFSTrim domainFSTrim;
};
typedef int
diff --git a/src/libvirt.c b/src/libvirt.c
index bdb1dc6..1c6863f 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -20164,3 +20164,53 @@ error:
virDispatchError(conn);
return -1;
}
+
+/**
+ * virDomainFSTrim:
+ * @dom: a domain object
+ * @mountPoint: which mount point trim
+ * @minimum: Minimum contiguous free range to discard in bytes
+ * @flags: extra flags, not used yet, so callers should always pass 0
+ *
+ * Calls FITRIM within the guest (hence guest agent may be
+ * required depending on hypervisor used). Either call it on each
+ * mounted filesystem (@mountPoint is NULL) or just on specified
+ * @mountPoint. @minimum tell that free ranges smaller than this
+ * may be ignored (this is a hint and the guest may not respect
+ * it). By increasing this value, the fstrim operation will
+ * complete more quickly for filesystems with badly fragmented
+ * free space, although not all blocks will be discarded.
+ *
+ * Returns 0 on success, -1 otherwise.
+ */
+int
+virDomainFSTrim(virDomainPtr dom,
+ const char *mountPoint,
+ unsigned long long minimum,
+ unsigned int flags)
+{
+ VIR_DOMAIN_DEBUG(dom, "mountPoint=%s, minimum=%llu, flags=%x",
+ mountPoint, minimum, flags);
+
+ virResetLastError();
+
+ if (!VIR_IS_DOMAIN(dom)) {
+ virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
+ virDispatchError(NULL);
+ return -1;
+ }
You're missing the check for read-only connections
+
+ if (dom->conn->driver->domainFSTrim) {
+ int ret = dom->conn->driver->domainFSTrim(dom, mountPoint,
+ minimum, flags);
+ if (ret < 0)
+ goto error;
+ return ret;
+ }
+
+ virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
+
+error:
+ virDispatchError(dom->conn);
+ return -1;
+}
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 :|
5:03 a.m.
New subject: [libvirt] [PATCH 1/4] Introduce virDomainFSTrim() public API
On 11/20/12 19:47, Michal Privoznik wrote:
This will call FITRIM within guest. The API has 4 arguments,
however, only 2 will be used for now (@dom and @minumum).
The rest two are there if in future qemu guest agent learns them.
---
include/libvirt/libvirt.h.in | 4 +++
src/driver.h | 6 +++++
src/libvirt.c | 50 ++++++++++++++++++++++++++++++++++++++++++
src/libvirt_public.syms | 5 ++++
4 files changed, 65 insertions(+), 0 deletions(-)
...
+
+/**
+ * virDomainFSTrim:
+ * @dom: a domain object
+ * @mountPoint: which mount point trim
+ * @minimum: Minimum contiguous free range to discard in bytes
+ * @flags: extra flags, not used yet, so callers should always pass 0
+ *
+ * Calls FITRIM within the guest (hence guest agent may be
+ * required depending on hypervisor used). Either call it on each
+ * mounted filesystem (@mountPoint is NULL) or just on specified
+ * @mountPoint. @minimum tell that free ranges smaller than this
+ * may be ignored (this is a hint and the guest may not respect
+ * it). By increasing this value, the fstrim operation will
+ * complete more quickly for filesystems with badly fragmented
+ * free space, although not all blocks will be discarded.
+ *
+ * Returns 0 on success, -1 otherwise.
+ */
+int
+virDomainFSTrim(virDomainPtr dom,
+ const char *mountPoint,
+ unsigned long long minimum,
+ unsigned int flags)
+{
+ VIR_DOMAIN_DEBUG(dom, "mountPoint=%s, minimum=%llu, flags=%x",
+ mountPoint, minimum, flags);
+
+ virResetLastError();
+
+ if (!VIR_IS_DOMAIN(dom)) {
+ virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
+ virDispatchError(NULL);
+ return -1;
+ }
as Dan pointed out. The check for read-only connections is missing.
+
+ if (dom->conn->driver->domainFSTrim) {
+ int ret = dom->conn->driver->domainFSTrim(dom, mountPoint,
+ minimum, flags);
+ if (ret < 0)
+ goto error;
+ return ret;
+ }
+
+ virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
+
+error:
+ virDispatchError(dom->conn);
+ return -1;
otherwise ACK.
Peter
12:47 p.m.
New subject: [libvirt] [PATCH 2/4] remote: Implement virDomainFSTrim
A new rule to fixup_name() in gendispatch.pl needs to be added,
otherwise we are left with remoteDomainFstrim which is not wanted.
---
src/remote/remote_driver.c | 1 +
src/remote/remote_protocol.x | 10 +++++++++-
src/remote_protocol-structs | 7 +++++++
src/rpc/gendispatch.pl | 1 +
4 files changed, 18 insertions(+), 1 deletions(-)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 62b7729..a8c6993 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -6143,6 +6143,7 @@ static virDriver remote_driver = {
.nodeSetMemoryParameters = remoteNodeSetMemoryParameters, /* 0.10.2 */
.nodeGetMemoryParameters = remoteNodeGetMemoryParameters, /* 0.10.2 */
.nodeGetCPUMap = remoteNodeGetCPUMap, /* 1.0.0 */
+ .domainFSTrim = remoteDomainFSTrim, /* 1.0.1 */
};
static virNetworkDriver network_driver = {
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index d6ac3c1..31567e2 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -2682,6 +2682,13 @@ struct remote_node_get_cpu_map_ret {
int ret;
};
+struct remote_domain_fstrim_args {
+ remote_nonnull_domain dom;
+ remote_string mountPoint;
+ unsigned hyper minimum;
+ unsigned int flags;
+};
+
/*----- Protocol. -----*/
/* Define the program number, protocol version and procedure numbers here. */
@@ -3026,7 +3033,8 @@ enum remote_procedure {
REMOTE_PROC_NETWORK_UPDATE = 291, /* autogen autogen priority:high */
REMOTE_PROC_DOMAIN_EVENT_PMSUSPEND_DISK = 292, /* autogen autogen */
- REMOTE_PROC_NODE_GET_CPU_MAP = 293 /* skipgen skipgen */
+ REMOTE_PROC_NODE_GET_CPU_MAP = 293, /* skipgen skipgen */
+ REMOTE_PROC_DOMAIN_FSTRIM = 294 /* autogen autogen */
/*
* Notice how the entries are grouped in sets of 10 ?
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index 6fe7213..d0d4f53 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -2139,6 +2139,12 @@ struct remote_node_get_cpu_map_ret {
u_int online;
int ret;
};
+struct remote_domain_fstrim_args {
+ remote_nonnull_domain dom;
+ remote_string mountPoint;
+ uint64_t minimum;
+ u_int flags;
+};
enum remote_procedure {
REMOTE_PROC_OPEN = 1,
REMOTE_PROC_CLOSE = 2,
@@ -2433,4 +2439,5 @@ enum remote_procedure {
REMOTE_PROC_NETWORK_UPDATE = 291,
REMOTE_PROC_DOMAIN_EVENT_PMSUSPEND_DISK = 292,
REMOTE_PROC_NODE_GET_CPU_MAP = 293,
+ REMOTE_PROC_DOMAIN_FSTRIM = 294,
};
diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
index ae7ecba..899f4bc 100755
--- a/src/rpc/gendispatch.pl
+++ b/src/rpc/gendispatch.pl
@@ -44,6 +44,7 @@ sub fixup_name {
$name =~ s/Os$/OS/;
$name =~ s/Nmi$/NMI/;
$name =~ s/Pm/PM/;
+ $name =~ s/Fstrim$/FSTrim/;
return $name;
}
--
1.7.8.6
4:59 a.m.
New subject: [libvirt] [PATCH 2/4] remote: Implement virDomainFSTrim
On 11/20/12 19:47, Michal Privoznik wrote:
A new rule to fixup_name() in gendispatch.pl needs to be added,
otherwise we are left with remoteDomainFstrim which is not wanted.
---
src/remote/remote_driver.c | 1 +
src/remote/remote_protocol.x | 10 +++++++++-
src/remote_protocol-structs | 7 +++++++
src/rpc/gendispatch.pl | 1 +
4 files changed, 18 insertions(+), 1 deletions(-)
Straightforward. ACK
Peter
12:47 p.m.
New subject: [libvirt] [PATCH 3/4] qemu: Implement virDomainFSTrim
using qemu guest agent. As said in previous patch,
@mountPoint must be NULL and @flags zero because
qemu guest agent doesn't support these arguments
yet. If qemu learns them, we can start supporting
them as well.
---
src/qemu/qemu_agent.c | 25 ++++++++++++++
src/qemu/qemu_agent.h | 2 +
src/qemu/qemu_driver.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 110 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index ab6dc22..6f517c9 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1448,3 +1448,28 @@ qemuAgentArbitraryCommand(qemuAgentPtr mon,
virJSONValueFree(reply);
return ret;
}
+
+int
+qemuAgentFSTrim(qemuAgentPtr mon,
+ unsigned long long minimum)
+{
+ int ret = -1;
+ virJSONValuePtr cmd;
+ virJSONValuePtr reply = NULL;
+
+ cmd = qemuAgentMakeCommand("guest-fstrim",
+ "U:minimum", minimum,
+ NULL);
+ if (!cmd)
+ return ret;
+
+ ret = qemuAgentCommand(mon, cmd, &reply,
+ VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK);
+
+ if (reply && ret == 0)
+ ret = qemuAgentCheckError(cmd, reply);
+
+ virJSONValueFree(cmd);
+ virJSONValueFree(reply);
+ return ret;
+}
diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
index c62c438..dad068b 100644
--- a/src/qemu/qemu_agent.h
+++ b/src/qemu/qemu_agent.h
@@ -83,4 +83,6 @@ int qemuAgentArbitraryCommand(qemuAgentPtr mon,
const char *cmd,
char **result,
int timeout);
+int qemuAgentFSTrim(qemuAgentPtr mon,
+ unsigned long long minimum);
#endif /* __QEMU_AGENT_H__ */
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 595c452..3dc5bdd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14733,6 +14733,88 @@ cleanup:
return result;
}
+static int
+qemuDomainFSTrim(virDomainPtr dom,
+ const char *mountPoint,
+ unsigned long long minimum,
+ unsigned int flags)
+{
+ struct qemud_driver *driver = dom->conn->privateData;
+ virDomainObjPtr vm;
+ int ret = -1;
+ qemuDomainObjPrivatePtr priv;
+
+ virCheckFlags(0, -1);
+
+ if (mountPoint) {
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("Specifying mount point "
+ "is not supported for now"));
+ return -1;
+ }
+
+ qemuDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ qemuDriverUnlock(driver);
+
+ if (!vm) {
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
+ virUUIDFormat(dom->uuid, uuidstr);
+ virReportError(VIR_ERR_NO_DOMAIN,
+ _("no domain with matching uuid '%s'"),
uuidstr);
+ goto cleanup;
+ }
+
+ priv = vm->privateData;
+
+ if (!virDomainObjIsActive(vm)) {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("domain is not running"));
+ goto cleanup;
+ }
+
+ if (priv->agentError) {
+ virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s",
+ _("QEMU guest agent is not "
+ "available due to an error"));
+ goto cleanup;
+ }
+
+ if (!priv->agent) {
+ virReportError(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)) {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("domain is not running"));
+ goto endjob;
+ }
+
+ qemuDomainObjEnterAgent(driver, vm);
+ ret = qemuAgentFSTrim(priv->agent, minimum);
+ qemuDomainObjExitAgent(driver, vm);
+ if (ret < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Failed to execute agent command"));
+ goto endjob;
+ }
+
+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,
@@ -14906,6 +14988,7 @@ static virDriver qemuDriver = {
.nodeGetMemoryParameters = nodeGetMemoryParameters, /* 0.10.2 */
.nodeSetMemoryParameters = nodeSetMemoryParameters, /* 0.10.2 */
.nodeGetCPUMap = nodeGetCPUMap, /* 1.0.0 */
+ .domainFSTrim = qemuDomainFSTrim, /* 1.0.1 */
};
--
1.7.8.6
4:51 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: Implement virDomainFSTrim
On 11/20/12 19:47, Michal Privoznik wrote:
using qemu guest agent. As said in previous patch,
@mountPoint must be NULL and @flags zero because
qemu guest agent doesn't support these arguments
yet. If qemu learns them, we can start supporting
them as well.
---
src/qemu/qemu_agent.c | 25 ++++++++++++++
src/qemu/qemu_agent.h | 2 +
src/qemu/qemu_driver.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 110 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index ab6dc22..6f517c9 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1448,3 +1448,28 @@ qemuAgentArbitraryCommand(qemuAgentPtr mon,
virJSONValueFree(reply);
return ret;
}
+
+int
+qemuAgentFSTrim(qemuAgentPtr mon,
+ unsigned long long minimum)
+{
+ int ret = -1;
+ virJSONValuePtr cmd;
+ virJSONValuePtr reply = NULL;
+
+ cmd = qemuAgentMakeCommand("guest-fstrim",
+ "U:minimum", minimum,
+ NULL);
+ if (!cmd)
+ return ret;
+
+ ret = qemuAgentCommand(mon, cmd, &reply,
+ VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK);
I didn't check the full call graph of qemuAgentCommand, but most of the
paths already contain error reporting ... (relevance of this command
will be revealed later :))
+
+ if (reply && ret == 0)
+ ret = qemuAgentCheckError(cmd, reply);
+
+ virJSONValueFree(cmd);
+ virJSONValueFree(reply);
+ return ret;
+}
diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
index c62c438..dad068b 100644
--- a/src/qemu/qemu_agent.h
+++ b/src/qemu/qemu_agent.h
@@ -83,4 +83,6 @@ int qemuAgentArbitraryCommand(qemuAgentPtr mon,
const char *cmd,
char **result,
int timeout);
+int qemuAgentFSTrim(qemuAgentPtr mon,
+ unsigned long long minimum);
#endif /* __QEMU_AGENT_H__ */
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 595c452..3dc5bdd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14733,6 +14733,88 @@ cleanup:
return result;
}
+static int
+qemuDomainFSTrim(virDomainPtr dom,
+ const char *mountPoint,
+ unsigned long long minimum,
+ unsigned int flags)
+{
+ struct qemud_driver *driver = dom->conn->privateData;
+ virDomainObjPtr vm;
+ int ret = -1;
+ qemuDomainObjPrivatePtr priv;
+
+ virCheckFlags(0, -1);
+
+ if (mountPoint) {
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("Specifying mount point "
+ "is not supported for now"));
+ return -1;
+ }
+
+ qemuDriverLock(driver);
+ vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ qemuDriverUnlock(driver);
+
+ if (!vm) {
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
+ virUUIDFormat(dom->uuid, uuidstr);
+ virReportError(VIR_ERR_NO_DOMAIN,
+ _("no domain with matching uuid '%s'"),
uuidstr);
+ goto cleanup;
+ }
You can avoid the two blocks of code above using the
qemuDomObjFromDomain() helper that does that for you.
+
+ priv = vm->privateData;
+
+ if (!virDomainObjIsActive(vm)) {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("domain is not running"));
+ goto cleanup;
+ }
+
+ if (priv->agentError) {
+ virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s",
+ _("QEMU guest agent is not "
+ "available due to an error"));
+ goto cleanup;
+ }
+
+ if (!priv->agent) {
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("QEMU guest agent is not configured"));
+ goto cleanup;
+ }
I'd swap the two blocks above, it makes more sense to check if there's
an agent error after you check for the agent.
+
+ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+ goto cleanup;
+
+ if (!virDomainObjIsActive(vm)) {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ "%s", _("domain is not running"));
+ goto endjob;
+ }
+
+ qemuDomainObjEnterAgent(driver, vm);
+ ret = qemuAgentFSTrim(priv->agent, minimum);
+ qemuDomainObjExitAgent(driver, vm);
+ if (ret < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Failed to execute agent command"));
+ goto endjob;
(the relevant part from the comment up)
... this masks the error from the underlying helper function. The goto
statement is redundant here.
+ }
+
+endjob:
+ if (qemuDomainObjEndJob(driver, vm) == 0) {
block quotes are not needed here
+ vm = NULL;
+ }
+
+cleanup:
+ if (vm)
+ virDomainObjUnlock(vm);
+ return ret;
+}
+
static virDriver qemuDriver = {
.no = VIR_DRV_QEMU,
.name = QEMU_DRIVER_NAME,
@@ -14906,6 +14988,7 @@ static virDriver qemuDriver = {
.nodeGetMemoryParameters = nodeGetMemoryParameters, /* 0.10.2 */
.nodeSetMemoryParameters = nodeSetMemoryParameters, /* 0.10.2 */
.nodeGetCPUMap = nodeGetCPUMap, /* 1.0.0 */
+ .domainFSTrim = qemuDomainFSTrim, /* 1.0.1 */
};
I don't like masking errors from underlying layers. I'd rather have it
changed so that more specific errors are shown but it's not a hard
requirement if you think it's not needed.
12:47 p.m.
New subject: [libvirt] [PATCH 4/4] virsh: Expose new virDomainFSTrim API
under domfstrim command. Since API doesn't support all
parameters (some must be NULL or zero), don't expose
these in virsh neither.
---
tools/virsh-domain.c | 40 ++++++++++++++++++++++++++++++++++++++++
tools/virsh.pod | 12 ++++++++++++
2 files changed, 52 insertions(+), 0 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index cc47383..4ea08f4 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -8300,6 +8300,45 @@ cleanup:
return ret;
}
+static const vshCmdInfo info_domfstrim[] = {
+ {"help", N_("Invoke fstrim on domain's mounted
filesystems.")},
+ {"desc", N_("Invoke fstrim on domain's mounted
filesystems.")},
+ {NULL, NULL}
+};
+
+static const vshCmdOptDef opts_domfstrim[] = {
+ {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or
uuid")},
+ {"minimum", VSH_OT_INT, 0, N_("Just a hint to ignore contiguous
"
+ "free ranges smaller than this (Bytes)")},
+ {NULL, 0, 0, NULL}
+};
+static bool
+cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd)
+{
+ virDomainPtr dom = NULL;
+ bool ret = false;
+ unsigned long long minimum = 0;
+ unsigned int flags = 0;
+
+ if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
+ goto cleanup;
+
+ if (vshCommandOptULongLong(cmd, "minimum", &minimum) < 0) {
+ vshError(ctl, _("Unable to parse integer parameter"));
+ goto cleanup;
+ }
+
+ if (virDomainFSTrim(dom, NULL, minimum, flags) < 0) {
+ vshError(ctl, _("Unable to invoke fstrim"));
+ goto cleanup;
+ }
+
+ ret = true;
+
+cleanup:
+ return ret;
+}
+
const vshCmdDef domManagementCmds[] = {
{"attach-device", cmdAttachDevice, opts_attach_device,
info_attach_device, 0},
@@ -8332,6 +8371,7 @@ const vshCmdDef domManagementCmds[] = {
{"detach-interface", cmdDetachInterface, opts_detach_interface,
info_detach_interface, 0},
{"domdisplay", cmdDomDisplay, opts_domdisplay, info_domdisplay, 0},
+ {"domfstrim", cmdDomFSTrim, opts_domfstrim, info_domfstrim, 0},
{"domhostname", cmdDomHostname, opts_domhostname, info_domhostname, 0},
{"domid", cmdDomid, opts_domid, info_domid, 0},
{"domif-setlink", cmdDomIfSetLink, opts_domif_setlink, info_domif_setlink,
0},
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 29be39e..c3fdcb7 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -878,6 +878,18 @@ Output a URI which can be used to connect to the graphical display of
the
domain via VNC, SPICE or RDP. If I<--include-password> is specified, the
SPICE channel password will be included in the URI.
+=item B<domfstrim> I<domain> [I<--minimum> B<bytes>]
+
+Issue a fstrim command on all mounted filesystems within a running
+domain. It discards blocks which are not in use by the filesystem.
+If I<--minimum> B<bytes> is specified, it tells guest kernel length
+of contiguous free range. Smaller than this may be ignored (this is
+a hint and the guest may not respect it). By increasing this value,
+the fstrim operation will complete more quickly for filesystems
+with badly fragmented free space, although not all blocks will
+be discarded. The default value is zero, meaning "discard
+every free block".
+
=item B<domhostname> I<domain>
Returns the hostname of a domain, if the hypervisor makes it available.
--
1.7.8.6
4:56 a.m.
New subject: [libvirt] [PATCH 4/4] virsh: Expose new virDomainFSTrim API
On 11/20/12 19:47, Michal Privoznik wrote:
under domfstrim command. Since API doesn't support all
parameters (some must be NULL or zero), don't expose
these in virsh neither.
---
tools/virsh-domain.c | 40 ++++++++++++++++++++++++++++++++++++++++
tools/virsh.pod | 12 ++++++++++++
2 files changed, 52 insertions(+), 0 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index cc47383..4ea08f4 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -8300,6 +8300,45 @@ cleanup:
return ret;
}
+static const vshCmdInfo info_domfstrim[] = {
+ {"help", N_("Invoke fstrim on domain's mounted
filesystems.")},
+ {"desc", N_("Invoke fstrim on domain's mounted
filesystems.")},
+ {NULL, NULL}
+};
+
+static const vshCmdOptDef opts_domfstrim[] = {
+ {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or
uuid")},
+ {"minimum", VSH_OT_INT, 0, N_("Just a hint to ignore contiguous
"
+ "free ranges smaller than this (Bytes)")},
Wouldn't it make sense to expose the mount point argument right now? I
know it's not implemented yet, but when somebody does implement that we
will need to change this later.
+ {NULL, 0, 0, NULL}
+};
+static bool
+cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd)
+{
+ virDomainPtr dom = NULL;
+ bool ret = false;
+ unsigned long long minimum = 0;
+ unsigned int flags = 0;
+
+ if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
+ goto cleanup;
+
+ if (vshCommandOptULongLong(cmd, "minimum", &minimum) < 0) {
+ vshError(ctl, _("Unable to parse integer parameter"));
As you know what's the name of the parameter print it in the error
message so the user is not left guessing where the error is.
+ goto cleanup;
+ }
+
+ if (virDomainFSTrim(dom, NULL, minimum, flags) < 0) {
+ vshError(ctl, _("Unable to invoke fstrim"));
+ goto cleanup;
+ }
+
+ ret = true;
+
+cleanup:
+ return ret;
+}
+
const vshCmdDef domManagementCmds[] = {
{"attach-device", cmdAttachDevice, opts_attach_device,
info_attach_device, 0},
@@ -8332,6 +8371,7 @@ const vshCmdDef domManagementCmds[] = {
{"detach-interface", cmdDetachInterface, opts_detach_interface,
info_detach_interface, 0},
{"domdisplay", cmdDomDisplay, opts_domdisplay, info_domdisplay, 0},
+ {"domfstrim", cmdDomFSTrim, opts_domfstrim, info_domfstrim, 0},
{"domhostname", cmdDomHostname, opts_domhostname, info_domhostname, 0},
{"domid", cmdDomid, opts_domid, info_domid, 0},
{"domif-setlink", cmdDomIfSetLink, opts_domif_setlink,
info_domif_setlink, 0},
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 29be39e..c3fdcb7 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -878,6 +878,18 @@ Output a URI which can be used to connect to the graphical display
of the
domain via VNC, SPICE or RDP. If I<--include-password> is specified, the
SPICE channel password will be included in the URI.
+=item B<domfstrim> I<domain> [I<--minimum> B<bytes>]
+
+Issue a fstrim command on all mounted filesystems within a running
+domain. It discards blocks which are not in use by the filesystem.
+If I<--minimum> B<bytes> is specified, it tells guest kernel length
+of contiguous free range. Smaller than this may be ignored (this is
+a hint and the guest may not respect it). By increasing this value,
+the fstrim operation will complete more quickly for filesystems
+with badly fragmented free space, although not all blocks will
+be discarded. The default value is zero, meaning "discard
+every free block".
+
=item B<domhostname> I<domain>
Returns the hostname of a domain, if the hypervisor makes it available.
I'd rather see this with the ability to specify mountpoint to the api
even if it still isn't supported.
Peter
5 a.m.
New subject: [libvirt] [PATCH 4/4] virsh: Expose new virDomainFSTrim API
On Wed, Nov 28, 2012 at 11:56:23AM +0100, Peter Krempa wrote:
On 11/20/12 19:47, Michal Privoznik wrote:
>under domfstrim command. Since API doesn't support all
>parameters (some must be NULL or zero), don't expose
>these in virsh neither.
>---
> tools/virsh-domain.c | 40 ++++++++++++++++++++++++++++++++++++++++
> tools/virsh.pod | 12 ++++++++++++
> 2 files changed, 52 insertions(+), 0 deletions(-)
>
>diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
>index cc47383..4ea08f4 100644
>--- a/tools/virsh-domain.c
>+++ b/tools/virsh-domain.c
>@@ -8300,6 +8300,45 @@ cleanup:
> return ret;
> }
>
>+static const vshCmdInfo info_domfstrim[] = {
>+ {"help", N_("Invoke fstrim on domain's mounted
filesystems.")},
>+ {"desc", N_("Invoke fstrim on domain's mounted
filesystems.")},
>+ {NULL, NULL}
>+};
>+
>+static const vshCmdOptDef opts_domfstrim[] = {
>+ {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or
uuid")},
>+ {"minimum", VSH_OT_INT, 0, N_("Just a hint to ignore contiguous
"
>+ "free ranges smaller than this
(Bytes)")},
Wouldn't it make sense to expose the mount point argument right now?
I know it's not implemented yet, but when somebody does implement
that we will need to change this later.
>+ {NULL, 0, 0, NULL}
>+};
>+static bool
>+cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd)
>+{
>+ virDomainPtr dom = NULL;
>+ bool ret = false;
>+ unsigned long long minimum = 0;
>+ unsigned int flags = 0;
>+
>+ if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
>+ goto cleanup;
>+
>+ if (vshCommandOptULongLong(cmd, "minimum", &minimum) < 0) {
>+ vshError(ctl, _("Unable to parse integer parameter"));
As you know what's the name of the parameter print it in the error
message so the user is not left guessing where the error is.
>+ goto cleanup;
>+ }
>+
>+ if (virDomainFSTrim(dom, NULL, minimum, flags) < 0) {
>+ vshError(ctl, _("Unable to invoke fstrim"));
>+ goto cleanup;
>+ }
>+
>+ ret = true;
>+
>+cleanup:
>+ return ret;
>+}
>+
> const vshCmdDef domManagementCmds[] = {
> {"attach-device", cmdAttachDevice, opts_attach_device,
> info_attach_device, 0},
>@@ -8332,6 +8371,7 @@ const vshCmdDef domManagementCmds[] = {
> {"detach-interface", cmdDetachInterface, opts_detach_interface,
> info_detach_interface, 0},
> {"domdisplay", cmdDomDisplay, opts_domdisplay, info_domdisplay, 0},
>+ {"domfstrim", cmdDomFSTrim, opts_domfstrim, info_domfstrim, 0},
> {"domhostname", cmdDomHostname, opts_domhostname, info_domhostname,
0},
> {"domid", cmdDomid, opts_domid, info_domid, 0},
> {"domif-setlink", cmdDomIfSetLink, opts_domif_setlink,
info_domif_setlink, 0},
>diff --git a/tools/virsh.pod b/tools/virsh.pod
>index 29be39e..c3fdcb7 100644
>--- a/tools/virsh.pod
>+++ b/tools/virsh.pod
>@@ -878,6 +878,18 @@ Output a URI which can be used to connect to the graphical
display of the
> domain via VNC, SPICE or RDP. If I<--include-password> is specified, the
> SPICE channel password will be included in the URI.
>
>+=item B<domfstrim> I<domain> [I<--minimum> B<bytes>]
>+
>+Issue a fstrim command on all mounted filesystems within a running
>+domain. It discards blocks which are not in use by the filesystem.
>+If I<--minimum> B<bytes> is specified, it tells guest kernel length
>+of contiguous free range. Smaller than this may be ignored (this is
>+a hint and the guest may not respect it). By increasing this value,
>+the fstrim operation will complete more quickly for filesystems
>+with badly fragmented free space, although not all blocks will
>+be discarded. The default value is zero, meaning "discard
>+every free block".
>+
> =item B<domhostname> I<domain>
>
> Returns the hostname of a domain, if the hypervisor makes it available.
>
I'd rather see this with the ability to specify mountpoint to the
api even if it still isn't supported.
Agreed, we should not cripple virsh to suit a particular libvirtd
feature set, because you have to bear in mind that we have an RPC
system that allows an old virsh to talk to a new libvirtd.
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 :|
4378
days inactive
4386
days old
10 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Michal Privoznik -
Peter Krempa