Re: [libvirt] [PATCH v2 3/3] virt-aa-helper: drop pointer checks in get_files

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).