On 07/17/2015 04:52 AM, Luyao Huang wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1226234#c3

After hot-plug a memory device success, the audit log show
that memory update failed:

type=VIRT_RESOURCE ... old-mem=1024000 new-mem=1548288 \
exe="/usr/sbin/libvirtd" hostname=? addr=? terminal=pts/2 res=failed

This is because the ret is still -1 when we call audit function to help

Also we need audit when hot-plug/hot-unplug get failed in qemu side.
And i notice we use virDomainDefGetMemoryActual to get the newmem
, but when we failed to attach the memory device we the virDomainDefGetMemoryActual
will still output the oldmem size, so the audit log will not right
in that case.

Signed-off-by: Luyao Huang <lhuang@redhat.com>
---
 src/qemu/qemu_hotplug.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

Since it's two issues there should be two patches

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 1ea397f..cf7ffa9 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1745,6 +1745,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
     qemuDomainObjPrivatePtr priv = vm->privateData;
     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
     unsigned long long oldmem = virDomainDefGetMemoryActual(vm->def);
+    unsigned long long newmem = oldmem + mem->size;
     char *devstr = NULL;
     char *objalias = NULL;
     const char *backendType;
@@ -1800,7 +1801,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,

Above this line, if there's a failure from virDomainMemoryInsert, the
code goes to cleanup.  Should that audit failure too?

     if (qemuDomainObjExitMonitor(driver, vm) < 0) {
         /* we shouldn't touch mem now, as the def might be freed */
         mem = NULL;
-        goto cleanup;
+        goto audit;
     }
 
     event = virDomainEventDeviceAddedNewFromObj(vm, objalias);
@@ -1811,9 +1812,6 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
     if (fix_balloon)
         vm->def->mem.cur_balloon += mem->size;
 
-    virDomainAuditMemory(vm, oldmem, virDomainDefGetMemoryActual(vm->def),
-                         "update", ret == 0);
-

An alternative method would be to :


    if ((ret = qemuMonitorAddDevice(priv->mon, devstr)) < 0) {
...
    }

    if (qemuDomainObjExitMonitor(driver, vm) < 0) {
         /* we shouldn't touch mem now, as the def might be freed */
         mem = NULL;
         ret = -1;
    }

    virDomainAuditMemory(vm, oldmem, newmem, "update", ret == 0);
    if (ret < 0)
        goto cleanup;

FWIW:
It seems "many" paths in the code will fail to audit if ExitMonitor
fails. There's no "consensus" as to the best mechanism. Although the
AuditNet's and most AuditChardev do at least try. The calls around
AuditHostdev are close, but the set ret = -1 and jump to cleanup before
audit which has a jump to cleanup if ret == -1.

Cleanup of those is something for another day it seems though...

     /* mem is consumed by vm->def */
     mem = NULL;
 
@@ -1823,6 +1821,8 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
 
     ret = 0;
 
+ audit:
+    virDomainAuditMemory(vm, oldmem, newmem, "update", ret == 0);
  cleanup:
     virObjectUnref(cfg);
     VIR_FREE(devstr);
@@ -1833,7 +1833,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
  removedef:

Rather than mess with the goto audit changes here, why not just :

    virDomainAuditMemory(vm, oldmem, newmem, "update", false);


See qemuDomainChangeEjectableMedia for precedent.

    if ((ret = qemuMonitorAddDevice(priv->mon, devstr)) < 0) {
...
    }

    if (qemuDomainObjExitMonitor(driver, vm) < 0) {
         /* we shouldn't touch mem now, as the def might be freed */
         mem = NULL;
         ret = -1;
    }

    virDomainAuditMemory(vm, oldmem, newmem, "update", ret == 0);   <---- one
    if (ret < 0)
        goto cleanup;

     if (qemuDomainObjExitMonitor(driver, vm) < 0) {
         mem = NULL;
-        goto cleanup;
+        goto audit;
     }
 
     if ((id = virDomainMemoryFindByDef(vm->def, mem)) >= 0)
@@ -1841,7 +1841,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
     else
         mem = NULL;
 
-    goto cleanup;
+    goto audit;
 }
 
 

The following should be a separate patch... So patch#1 fixes attach and
#2 fixes remove

@@ -2904,11 +2904,11 @@ qemuDomainRemoveMemoryDevice(virQEMUDriverPtr driver,
 {
     qemuDomainObjPrivatePtr priv = vm->privateData;
     unsigned long long oldmem = virDomainDefGetMemoryActual(vm->def);
+    unsigned long long newmem = oldmem - mem->size;
     virObjectEventPtr event;
     char *backendAlias = NULL;
     int rc;
     int idx;
-    int ret = -1;

Don't delete this

 
     VIR_DEBUG("Removing memory device %s from domain %p %s",
               mem->info.alias, vm, vm->def->name);
@@ -2917,27 +2917,23 @@ qemuDomainRemoveMemoryDevice(virQEMUDriverPtr driver,
         qemuDomainEventQueue(driver, event);
 
     if (virAsprintf(&backendAlias, "mem%s", mem->info.alias) < 0)
-        goto cleanup;
+        return -1;

Drop this change

 
     qemuDomainObjEnterMonitor(driver, vm);
     rc = qemuMonitorDelObject(priv->mon, backendAlias);
+    VIR_FREE(backendAlias);

drop this change

     if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0)
-        goto cleanup;
+        return -1;

Modify the above to:
     rc = qemuMonitorDelObject(priv->mon, backendAlias);
     if (qemuDomainObjExitMonitor(driver, vm) < 0)
         rc = -1;

     virDomainAuditMemory(vm, oldmem, newmem, "update", rc == 0);
     if (rc < 0)
        goto cleanup;

 
     vm->def->mem.cur_balloon -= mem->size;
 
+    virDomainAuditMemory(vm, oldmem, newmem, "update", rc == 0);
+

This moved up...

     if ((idx = virDomainMemoryFindByDef(vm->def, mem)) >= 0)
         virDomainMemoryRemove(vm->def, idx);
 
     virDomainMemoryDefFree(mem);
-    ret = 0;
-
- cleanup:
-    virDomainAuditMemory(vm, oldmem, virDomainDefGetMemoryActual(vm->def),
-                         "update", ret == 0);
-
-    VIR_FREE(backendAlias);
-    return ret;
+    return 0;

Leaving the above hunk as:

    ret = 0;

 cleanup:
    VIR_FREE(backendAlias);
    return ret;
}

John