Re: [Libvir] [PATCH] Implement memory operations for qemu driver

[Cole Robinson <crobinso redhat com>]

Cole Robinson wrote:
> The attached patch implements the following operations for the qemu driver:
> 
> virDomainGetMaxMemory
> virDomainSetMaxMemory
> virDomainSetMemory
> 
> A few questions/comments:
> 
> 1) I changed maxmem and memory in the qemu_vm_def struct to unsigned long
>    to match the public api for memory values. Seems to work, but not sure
>    if there are any undesirable side effects to this.
> 
> 2) Should SetMaxMem be able to be called on a running guest? This code
>    allows it, since maxmem is basically a metavalue that doesn't directly
>    affect a guest.
> 
> 3) Should maxmem be able to be set lower than the currently allocated mem?
>    This code does not allow this. If this changed, would also need to take
>    into account how we would handle this if we can change the maxmem while
>    the guest is running. After rethinking, we probably should be able to
>    do this, but I haven't changed the code.
> 
> Thanks,
> Cole
> 

In testing this with virt-manager I found a bug: I was using comparing maxmem
against the wrong memory value. Fixed this and updated the parameter names
to be a bit more clear. Seems to work as excepted now.

Thanks,
Cole

diff --git a/src/qemu_conf.c b/src/qemu_conf.c
index a196bb8..f6ae06b 100644
--- a/src/qemu_conf.c
+++ b/src/qemu_conf.c
@@ -1650,7 +1650,7 @@ int qemudBuildCommandLine(virConnectPtr conn,
          (vm->def->graphicsType == QEMUD_GRAPHICS_SDL ? 0 : 1)) + /* graphics */
         (vm->migrateFrom[0] ? 3 : 0); /* migrateFrom */
 
-    snprintf(memory, sizeof(memory), "%d", vm->def->memory/1024);
+    snprintf(memory, sizeof(memory), "%lu", vm->def->memory/1024);
     snprintf(vcpus, sizeof(vcpus), "%d", vm->def->vcpus);
 
     if (!(*argv = malloc(sizeof(**argv) * (len+1))))
@@ -2820,9 +2820,9 @@ char *qemudGenerateXML(virConnectPtr conn,
     virUUIDFormat(uuid, uuidstr);
     if (virBufferVSprintf(buf, "  <uuid>%s</uuid>\n", uuidstr) < 0)
         goto no_memory;
-    if (virBufferVSprintf(buf, "  <memory>%d</memory>\n", def->maxmem) < 0)
+    if (virBufferVSprintf(buf, "  <memory>%lu</memory>\n", def->maxmem) < 0)
         goto no_memory;
-    if (virBufferVSprintf(buf, "  <currentMemory>%d</currentMemory>\n", def->memory) < 0)
+    if (virBufferVSprintf(buf, "  <currentMemory>%lu</currentMemory>\n", def->memory) < 0)
         goto no_memory;
     if (virBufferVSprintf(buf, "  <vcpu>%d</vcpu>\n", def->vcpus) < 0)
         goto no_memory;
diff --git a/src/qemu_conf.h b/src/qemu_conf.h
index 12aa6ae..c1aae75 100644
--- a/src/qemu_conf.h
+++ b/src/qemu_conf.h
@@ -193,8 +193,8 @@ struct qemud_vm_def {
     unsigned char uuid[VIR_UUID_BUFLEN];
     char name[QEMUD_MAX_NAME_LEN];
 
-    int memory;
-    int maxmem;
+    unsigned long memory;
+    unsigned long maxmem;
     int vcpus;
 
     int noReboot;
diff --git a/src/qemu_driver.c b/src/qemu_driver.c
index 2b4c2a6..af5fc40 100644
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -1765,6 +1765,66 @@ static char *qemudDomainGetOSType(virDomainPtr dom) {
     return type;
 }
 
+/* Returns max memory in kb, 0 if error */
+static unsigned long qemudDomainGetMaxMemory(virDomainPtr dom) {
+    struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
+    struct qemud_vm *vm = qemudFindVMByUUID(driver, dom->uuid);
+
+    if (!vm) {
+        qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
+                         _("no domain with matching uuid '%s'"), dom->uuid);
+        return 0;
+    }
+
+    return vm->def->maxmem;
+}
+
+static int qemudDomainSetMaxMemory(virDomainPtr dom, unsigned long newmax) {
+    struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
+    struct qemud_vm *vm = qemudFindVMByUUID(driver, dom->uuid);
+
+    if (!vm) {
+        qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
+                         _("no domain with matching uuid '%s'"), dom->uuid);
+        return -1;
+    }
+
+    if (newmax < vm->def->memory) {
+        qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
+                         _("cannot set max memory lower than current memory"));
+        return -1;
+    }
+
+    vm->def->maxmem = newmax;
+    return 0;
+}
+
+static int qemudDomainSetMemory(virDomainPtr dom, unsigned long newmem) {
+    struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
+    struct qemud_vm *vm = qemudFindVMByUUID(driver, dom->uuid);
+
+    if (!vm) {
+        qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
+                         _("no domain with matching uuid '%s'"), dom->uuid);
+        return -1;
+    }
+
+    if (qemudIsActiveVM(vm)) {
+        qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
+                         _("cannot set memory of an active domain"));
+        return -1;
+    }
+
+    if (newmem > vm->def->maxmem) {
+        qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_ARG,
+                         _("cannot set memory higher than max memory"));
+        return -1;
+    }
+
+    vm->def->memory = newmem;
+    return 0;
+}
+
 static int qemudDomainGetInfo(virDomainPtr dom,
                        virDomainInfoPtr info) {
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
@@ -2886,9 +2946,9 @@ static virDriver qemuDriver = {
     NULL, /* domainReboot */
     qemudDomainDestroy, /* domainDestroy */
     qemudDomainGetOSType, /* domainGetOSType */
-    NULL, /* domainGetMaxMemory */
-    NULL, /* domainSetMaxMemory */
-    NULL, /* domainSetMemory */
+    qemudDomainGetMaxMemory, /* domainGetMaxMemory */
+    qemudDomainSetMaxMemory, /* domainSetMaxMemory */
+    qemudDomainSetMemory, /* domainSetMemory */
     qemudDomainGetInfo, /* domainGetInfo */
     qemudDomainSave, /* domainSave */
     qemudDomainRestore, /* domainRestore */