[libvirt] [PATCH v2 3/3] virt-aa-helper: drop pointer checks in get_files
crobinso at redhat.com
Wed Nov 20 17:53:34 UTC 2019
On 11/20/19 12:25 PM, Jamie Strandboge wrote:
> On Wed, 20 Nov 2019, Cole Robinson wrote:
>> On 11/19/19 4:31 PM, Jamie Strandboge wrote:
>>> On Thu, 14 Nov 2019, Christian Ehrhardt wrote:
>>>> It was mentioned that the pointers in loops like:
>>>> for (i = 0; i < ctl->def->nserials; i++)
>>>> if (ctl->def->serials[i] ...
>>>> will always be !NULL or we would have a much more serious problem.
>>>> Simplify the if chains in get_files by dropping these checks.
>>> I don't really see why a NULL check is a problem so this feels a bit
>>> like busy work and it seems short-circuiting and not doing is exactly
>>> what you would want to do in the event of a 'much more serious problem'.
>>> Is this really required? +0 to apply on principle (I've not reviewed the
>>> changes closely).
>> If for example def->nserials and def->serials[i] disagree, then there is
>> a serious bug somewhere. The internal API contract is that it should
>> never happen, so code shouldn't check for the condition. I'm pretty sure
>> if the XML parser is creating that situation, the qemu driver would
>> crash in a dozen different places.
>> So the patch isn't strictly required, but it is an improvement IMO: it
>> reduces code, improves readability, and helps simplify review for other
>> libvirt devs who may be confused by this uncommon idiom (I was). But I
>> will leave it up to you guys to decide whether to push or not
> To be clear, I am intentionally not blocking on this. If, as upstream,
> you'd prefer this to be a certain way for reasons that outweigh my POV,
> by all means feel free to do that. The changes are mechanical and IMHO
> don't need an apparmor-focused review (though if you'd prefer I go
> through the full patch, I can).
This patch moves the code to be more consistent with the rest of the
libvirt code base, so I think it's good to push. I thought Christian was
waiting on you for review before pushing apparmor patches though, I
didn't want to step on any toes :)
More information about the libvir-list