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@xxxxxxxxxx>
> > ---
> > 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 :|
_______________________________________________
Libosinfo mailing list
Libosinfo@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libosinfo
