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

[Laine Stump]

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.