Re: [libvirt] [PATCH 1/2] qemu: add vfio devices to cgroup ACL when appropriate

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.