On Fri, Nov 06, 2015 at 02:23:17PM +0100, Christophe Fergeau wrote: > Hi, > > On Thu, Nov 05, 2015 at 04:59:20PM +0000, Daniel P. Berrange wrote: > > > > This makes sense purely from the POV of correctness of the API, but > > I think there is another bug hiding here somewhere, because we AFAIR > > we should never get in a situation where we are replacing an existing > > entity in the list. When loading entities from the XML, we always > > check to see if the entity already exists, and if so re-use it rather > > than creating a new one. IOW, while this fix is valid, fixing it is > > masking what I think might be a more serious flaw elsewhere, that I > > would see to identify. > > I ran a few more tests (added an assert in osinfo_list_add() if the > entity is already present in the list), and it turns out this happens > precisely because we reuse OS entities from the XML. We reuse what I'd > call 'top-level' entities, but we do not attempt to reuse entities used > to represent <media> nodes, or <tree> nodes, ... when we are parsing an > OS entity. So when we encounter a duplicate OS entity, we try to reuse > the existing one, and this entity has duplicates for the media/tree/... > we are trying to add, which triggers this misbehaviour. > The full list of entities which were duplicate are: > - OsinfoMedia > - OsinfoDeviceDriver > - OsinfoTree > - OsinfoOsVariant > - OsinfoInstallScript Ahhh, ok, so it is non-top level entities that are the problem only. > Looking at the loading code for install scripts, there might be a bug > there as it may not be trying to reuse pre-existing entities. The loader calls osinfo_loader_get_install_script() so it is not creating duplicate OsinfoInstallSCript objects. However, we setup a relationship between OsinfoOs and the install script, and that's where the duplication comes. This is OK I think. ACK to your original patch, all these non-top level cases are harmless Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| _______________________________________________ Libosinfo mailing list Libosinfo@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libosinfo
