[Libosinfo] [PATCH 6/8] list: Fix replacement in osinfo_list_add

[Daniel P. Berrange]

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 :|