Re: [libvirt] [PATCH 10/14] Introduce virDomainMigrate*CompressionCache APIs
On 02/19/13 13:35, Jiri Denemark wrote:
Introduce virDomainMigrateGetCompressionCache and
virDomainMigrateSetCompressionCache APIs.
---
include/libvirt/libvirt.h.in | 7 +++
python/generator.py | 1 +
src/driver.h | 11 +++++
src/libvirt.c | 100 +++++++++++++++++++++++++++++++++++++++++++
src/libvirt_public.syms | 2 +
5 files changed, 121 insertions(+)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 9d1c6ea..7e89e2e 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -1215,6 +1215,13 @@ int virDomainMigrateSetMaxDowntime (virDomainPtr domain,
unsigned long long downtime,
unsigned int flags);
+int virDomainMigrateGetCompressionCache(virDomainPtr domain,
+ unsigned long long *cacheSize,
+ unsigned int flags);
+int virDomainMigrateSetCompressionCache(virDomainPtr domain,
+ unsigned long long cacheSize,
+ unsigned int flags);
Hm, again those seem to me a bit too qemucentric. As we now have APIs to
set migration speed and this would introduce another set for cache size,
I'm wondering if we couldn't go for something more universal. For
example the flag would control which parameter of migration gets set.
As this would introduce increased complexity, I'm not insisting on this.
A second opinion is welcome.
+
int virDomainMigrateSetMaxSpeed(virDomainPtr domain,
unsigned long bandwidth,
unsigned int flags);
...
diff --git a/src/libvirt.c b/src/libvirt.c
index 3611839..d1dca40 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -17550,6 +17550,106 @@ error:
}
/**
+ * virDomainMigrateGetCompressionCache:
+ * @domain: a domain object
+ * @cacheSize: return value of current size of the cache (in bytes)
+ * @flags: extra flags; not used yet, so callers should always pass 0
+ *
+ * Gets current size of the cache (in bytes) used for compressing repeatedly
+ * transferred memory pages during live migration.
+ *
+ * Returns 0 in case of success, -1 otherwise.
+ */
+int
+virDomainMigrateGetCompressionCache(virDomainPtr domain,
+ unsigned long long *cacheSize,
+ unsigned int flags)
+{
+ virConnectPtr conn;
+
+ VIR_DOMAIN_DEBUG(domain, "cacheSize=%p, flags=%x", cacheSize, flags);
+
+ virResetLastError();
+
+ if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
+ virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
+ virDispatchError(NULL);
+ return -1;
+ }
+
+ conn = domain->conn;
+
+ virCheckNonNullArgGoto(cacheSize, error);
+
+ if (conn->flags & VIR_CONNECT_RO) {
+ virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
IMHO the *get* function could be allowed to work on RO connections.
+ goto error;
+ }
+
+ if (conn->driver->domainMigrateGetCompressionCache) {
+ if (conn->driver->domainMigrateGetCompressionCache(domain, cacheSize,
+ flags) < 0)
+ goto error;
+ return 0;
+ }
+
+ virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
+error:
+ virDispatchError(conn);
+ return -1;
+}
+
The code looks fine, so ACK with the check removed. We might want to
wait for a second opinion on the design though.
Peter