On 04/30/2013 10:49 AM, Eric Blake wrote:
On 04/29/2013 07:46 PM, Laine Stump wrote:
+int qemuSetupHostdevCGroup(virDomainObjPtr vm, + virDomainHostdevDefPtr dev) ATTRIBUTE_RETURN_CHECK; +int qemuTeardownHostdevCgroup(virDomainObjPtr vm, + virDomainHostdevDefPtr dev); A bit odd that setup is ATTRIBUTE_RETURN_CHECK but teardown is not. I'd rather see both require a use...
I didn't include it for teardown because I wasn't checking the return (see below), and didn't want the extra line length caused by ignore_value(). (also, I notice that none of the other functions in qemu_cgroup.h have any sort of ATTRIBUTE_* usage at all). Then maybe the simplest fix is to drop the RETURN_CHECK on the setup function?
@@ -1257,6 +1260,9 @@ error: vm->def, hostdev, NULL) < 0) VIR_WARN("Unable to restore host device labelling on hotplug fail");
+teardown_cgroup: + qemuTeardownHostdevCgroup(vm, hostdev); ...and here, on cleanup paths after an earlier error, stick a VIR_WARN() that logs any failure trying to clean up (as we already did on the line before).
But the teardown function itself is already logging an error message (as is the security manager function preceding it), so I don't really see the point of the additional VIR_WARN (I had seen the VIR_WARN on failure of the security manager, and concluded that it was redundant, so didn't replicate it).
At any rate, I want to get this in before RC2, so I'll add a VIR_WARN and you can convince me (or I can convince you) later. I just wanted to make sure we have something in the log if cleanup goes wrong - it's unlikely, but that's exactly the case where having more information instead of less in the logs can help in deciphering what happened when things do go wrong, and how badly the system might have been compromised as a result of failure to clean up. It's fine with me if you can show that there was a warning emitted earlier in the call chain, so that a VIR_WARN on the cleanup path is redundant.
I'm not certain I agree, but what you're requesting won't hurt anything, so in the interest of time I'll modify it that way and push. What you pushed looks fine to me.
And something you said up above may have convinced me that putting in the VIR_WARN is useful - many times people will report a problem where they prominently display an error log message that turns out to be a problem inherent in attempting to clean up after the *real* failure occurred, and time is wasted speculating on the cause of this incidental error; with the VIR_WARN in there it would hopefully be immediately obvious that any errors logged were of this immaterial type, and didn't necessarily need further investigation.