[libvirt] [PATCH] qemu: fix the audit log is not correct after hot-plug memory success
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(-) 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, 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); - /* 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: 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; } @@ -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; 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; qemuDomainObjEnterMonitor(driver, vm); rc = qemuMonitorDelObject(priv->mon, backendAlias); + VIR_FREE(backendAlias); if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) - goto cleanup; + return -1; vm->def->mem.cur_balloon -= mem->size; + virDomainAuditMemory(vm, oldmem, newmem, "update", rc == 0); + 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; } -- 1.8.3.1
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 (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
}
On 07/24/2015 08:14 PM, John Ferlan wrote:
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
OKay, i will split them in two patches, thanks your advise.
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?
Actually i still not know the audit rules of "attach some device but get fail", and i notice the old function (attach disk,network....): these functions will audit the failure when there is a failure when qemu get fail, and won't audit the failure before qemuDomainObjEnterMonitor(). So in my opinion if there's a failure from virDomainMemoryInsert, no need audit failure, because we still not really try to attach such device :)
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...
Some of them is easy to fix but some for them is hard to fix (need refactor functions), we could fix them later ;)
/* 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.
Hmm...but then i need 3 virDomainAuditMemory() functions: 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; ... removedef: if (qemuDomainObjExitMonitor(driver, vm) < 0) { mem = NULL; virDomainAuditMemory(vm, oldmem, newmem, "update", false); <---two goto cleanup; } if ((id = virDomainMemoryFindByDef(vm->def, mem)) >= 0) mem = virDomainMemoryRemove(vm->def, id); else mem = NULL; virDomainAuditMemory(vm, oldmem, newmem, "update", false); <----three goto cleanup; It looks a little tedious for me, how about you ? :)
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
Okay
@@ -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;
Okay, i agree with your idea, and i think we can remove 'ret' and just return 'rc', so...
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; }
...these place will be: virDomainMemoryDefFree(mem); cleanup: VIR_FREE(backendAlias); return rc; And during fix this i have found another way which could remove ret and cleanup: / / qemuDomainObjEnterMonitor(driver, vm); rc = qemuMonitorDelObject(priv->mon, backendAlias); if (qemuDomainObjExitMonitor(driver, vm) < 0) rc = -1; VIR_FREE(backendAlias); virDomainAuditMemory(vm, oldmem, newmem, "update", rc == 0); if (rc < 0) return -1; vm->def->mem.cur_balloon -= mem->size; if ((idx = virDomainMemoryFindByDef(vm->def, mem)) >= 0) virDomainMemoryRemove(vm->def, idx); virDomainMemoryDefFree(mem); return 0; Thanks a lot for your review and help.
John
Luyao
participants (3)
-
John Ferlan -
lhuang -
Luyao Huang