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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
