<html> <head> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> <div class="moz-cite-prefix">On 07/24/2015 08:14 PM, John Ferlan wrote: </div> <blockquote cite="mid:55B22C30.2070509@redhat.com" type="cite"> <pre wrap=""> On 07/17/2015 04:52 AM, Luyao Huang wrote: </pre> <blockquote type="cite"> <pre wrap=""><a class="moz-txt-link-freetext" href="https://bugzilla.redhat.com/show_bug.cgi?id=1226234#c3">https://bugzilla.redhat.com/show_bug.cgi?id=1226234#c3</a> 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 <a class="moz-txt-link-rfc2396E" href="mailto:lhuang@redhat.com"><lhuang@redhat.com></a> --- src/qemu/qemu_hotplug.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) </pre> </blockquote> <pre wrap=""> Since it's two issues there should be two patches </pre> </blockquote> OKay, i will split them in two patches, thanks your advise. <blockquote cite="mid:55B22C30.2070509@redhat.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">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, </pre> </blockquote> <pre wrap=""> Above this line, if there's a failure from virDomainMemoryInsert, the code goes to cleanup. Should that audit failure too? </pre> </blockquote> 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 :) <blockquote cite="mid:55B22C30.2070509@redhat.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap=""> 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); - </pre> </blockquote> <pre wrap=""> 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... </pre> </blockquote> Some of them is easy to fix but some for them is hard to fix (need refactor functions), we could fix them later ;) <blockquote cite="mid:55B22C30.2070509@redhat.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap=""> /* 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: </pre> </blockquote> <pre wrap=""> Rather than mess with the goto audit changes here, why not just : virDomainAuditMemory(vm, oldmem, newmem, "update", false); See qemuDomainChangeEjectableMedia for precedent. </pre> </blockquote> Hmm...but then i need 3 virDomainAuditMemory() functions: <pre wrap=""> 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;</pre> ... 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 ? :) <blockquote cite="mid:55B22C30.2070509@redhat.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap=""> 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; } </pre> </blockquote> <pre wrap=""> The following should be a separate patch... So patch#1 fixes attach and #2 fixes remove </pre> </blockquote> Okay <blockquote cite="mid:55B22C30.2070509@redhat.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">@@ -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; </pre> </blockquote> <pre wrap=""> Don't delete this </pre> <blockquote type="cite"> <pre wrap=""> 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; </pre> </blockquote> <pre wrap=""> Drop this change </pre> <blockquote type="cite"> <pre wrap=""> qemuDomainObjEnterMonitor(driver, vm); rc = qemuMonitorDelObject(priv->mon, backendAlias); + VIR_FREE(backendAlias); </pre> </blockquote> <pre wrap=""> drop this change </pre> <blockquote type="cite"> <pre wrap=""> if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) - goto cleanup; + return -1; </pre> </blockquote> <pre wrap=""> 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; </pre> </blockquote> Okay, i agree with your idea, and i think we can remove 'ret' and just return 'rc', so... <blockquote cite="mid:55B22C30.2070509@redhat.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap=""> vm->def->mem.cur_balloon -= mem->size; + virDomainAuditMemory(vm, oldmem, newmem, "update", rc == 0); + </pre> </blockquote> <pre wrap=""> This moved up... </pre> <blockquote type="cite"> <pre wrap=""> 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; </pre> </blockquote> <pre wrap=""> Leaving the above hunk as: ret = 0; cleanup: VIR_FREE(backendAlias); return ret; } </pre> </blockquote> ...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. <blockquote cite="mid:55B22C30.2070509@redhat.com" type="cite"> <pre wrap=""> John</pre> </blockquote> Luyao </body> </html>