[libvirt] [PATCH v2 3/3] virt-aa-helper: drop pointer checks in get_files
jamie at canonical.com
Wed Nov 20 17:25:41 UTC 2019
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).
Jamie Strandboge | http://www.canonical.com
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 833 bytes
Desc: not available
More information about the libvir-list