[Libosinfo] [PATCH 2/5] loader: identify referenced but not defined entities

[Daniel P. Berrange]

On Wed, Oct 07, 2015 at 10:26:04AM +0200, Christophe Fergeau wrote:
> On Tue, Oct 06, 2015 at 05:41:00PM +0100, Daniel P. Berrange wrote:
> > When loading the database we often have to instantiate
> > entities to represent relationships (upgrades, derives,
> > etc) before the entity is actually defined. This makes
> > the code liable to bugs as we might instantiate entities
> > which are never defined.
> > 
> > This extends the loader so that it tracks all entities
> > that are created via references, and once loading is
> > complete it prints out a warning if any referenced
> > entities were not defined fully.
> > 
> > This identifies a number of mistakes in our current
> > database that the following patches will fix.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> > ---
> >  osinfo/osinfo_loader.c | 94 ++++++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 75 insertions(+), 19 deletions(-)
> > 
> > diff --git a/osinfo/osinfo_loader.c b/osinfo/osinfo_loader.c
> > index 0c7ddfb..4251a90 100644
> > --- a/osinfo/osinfo_loader.c
> > +++ b/osinfo/osinfo_loader.c
> > @@ -56,6 +56,7 @@ struct _OsinfoLoaderPrivate
> >  {
> >      OsinfoDb *db;
> >      GHashTable *xpath_cache;
> > +    GHashTable *entityRefs;
> 
> "entity_refs"
> 
> >  };
> >  
> >  struct _OsinfoEntityKey
> > @@ -73,6 +74,8 @@ osinfo_loader_finalize(GObject *object)
> >      g_object_unref(loader->priv->db);
> >      g_hash_table_destroy(loader->priv->xpath_cache);
> >  
> > +    g_hash_table_destroy(loader->priv->entityRefs);
> > +
> >      /* Chain up to the parent class */
> >      G_OBJECT_CLASS(osinfo_loader_parent_class)->finalize(object);
> >  }
> > @@ -106,6 +109,10 @@ osinfo_loader_init(OsinfoLoader *loader)
> >                                                        g_str_equal,
> >                                                        g_free,
> >                                                        xpath_cache_value_free);
> > +    loader->priv->entityRefs = g_hash_table_new_full(g_str_hash,
> > +                                                     g_str_equal,
> > +                                                     g_free,
> > +                                                     NULL);
> >  }
> >  
> >  /** PUBLIC METHODS */
> > @@ -409,61 +416,101 @@ static void osinfo_loader_entity(OsinfoLoader *loader,
> >  }
> >  
> >  static OsinfoDatamap *osinfo_loader_get_datamap(OsinfoLoader *loader,
> > -                                                 const gchar *id)
> > +                                                const gchar *id,
> > +                                                gboolean reference)
> >  {
> >      OsinfoDatamap *datamap = osinfo_db_get_datamap(loader->priv->db, id);
> >      if (!datamap) {
> >          datamap = osinfo_datamap_new(id);
> >          osinfo_db_add_datamap(loader->priv->db, datamap);
> >          g_object_unref(datamap);
> > +        if (reference) {
> > +            g_hash_table_insert(loader->priv->entityRefs, g_strdup(id), datamap);
> > +        }
> 
> Given the way you use the hash table, you could use
> g_hash_table_add/g_hash_table_contains, but this was only added in glib
> 2.32.
> For what it's worth, I think the code would be a bit simpler without the
> 'reference' argument, ie you unconditionnally call g_hash_table_insert()
> in the if (!datamap) block, and right after the (only)
> osinfo_loader_get_datamap(loader, id, FALSE); call
> you g_hash_table_remove(entity_refs, id);
> 
> Same for the other osinfo_loader_get_xxx changes.
> 
> ACK from me without or with changes.

I guess the compelling reason for doing it your way is that the passing
of TRUE/FALSE is a bit opaque unless you lookup what that parameter
means. I'll change it before pushing. Would make the diff smaller too

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