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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Attachment: signature.asc
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux