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