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.
Christophe
> + } else {
> + if (!reference) {
> + g_hash_table_remove(loader->priv->entityRefs, id);
> + }
> }
> return datamap;
> }
>
> static OsinfoDevice *osinfo_loader_get_device(OsinfoLoader *loader,
> - const gchar *id)
> + const gchar *id,
> + gboolean reference)
> {
> OsinfoDevice *dev = osinfo_db_get_device(loader->priv->db, id);
> if (!dev) {
> dev = osinfo_device_new(id);
> osinfo_db_add_device(loader->priv->db, dev);
> g_object_unref(dev);
> + if (reference) {
> + g_hash_table_insert(loader->priv->entityRefs, g_strdup(id), dev);
> + }
> + } else {
> + if (!reference) {
> + g_hash_table_remove(loader->priv->entityRefs, id);
> + }
> }
> return dev;
> }
>
> static OsinfoOs *osinfo_loader_get_os(OsinfoLoader *loader,
> - const gchar *id)
> + const gchar *id,
> + gboolean reference)
> {
> OsinfoOs *os = osinfo_db_get_os(loader->priv->db, id);
> if (!os) {
> os = osinfo_os_new(id);
> osinfo_db_add_os(loader->priv->db, os);
> g_object_unref(os);
> + if (reference) {
> + g_hash_table_insert(loader->priv->entityRefs, g_strdup(id), os);
> + }
> + } else {
> + if (!reference) {
> + g_hash_table_remove(loader->priv->entityRefs, id);
> + }
> }
> return os;
> }
>
> static OsinfoPlatform *osinfo_loader_get_platform(OsinfoLoader *loader,
> - const gchar *id)
> + const gchar *id,
> + gboolean reference)
> {
> OsinfoPlatform *platform = osinfo_db_get_platform(loader->priv->db, id);
> if (!platform) {
> platform = osinfo_platform_new(id);
> osinfo_db_add_platform(loader->priv->db, platform);
> g_object_unref(platform);
> + if (reference) {
> + g_hash_table_insert(loader->priv->entityRefs, g_strdup(id), platform);
> + }
> + } else {
> + if (!reference) {
> + g_hash_table_remove(loader->priv->entityRefs, id);
> + }
> }
> return platform;
> }
>
> static OsinfoInstallScript *osinfo_loader_get_install_script(OsinfoLoader *loader,
> - const gchar *id)
> + const gchar *id,
> + gboolean reference)
> {
> OsinfoInstallScript *script = osinfo_db_get_install_script(loader->priv->db, id);
> if (!script) {
> script = osinfo_install_script_new(id);
> osinfo_db_add_install_script(loader->priv->db, script);
> g_object_unref(script);
> + if (reference) {
> + g_hash_table_insert(loader->priv->entityRefs, g_strdup(id), script);
> + }
> + } else {
> + if (!reference) {
> + g_hash_table_remove(loader->priv->entityRefs, id);
> + }
> }
> return script;
> }
> @@ -490,7 +537,7 @@ static void osinfo_loader_device(OsinfoLoader *loader,
> return;
> }
>
> - OsinfoDevice *device = osinfo_loader_get_device(loader, id);
> + OsinfoDevice *device = osinfo_loader_get_device(loader, id, FALSE);
> xmlFree(id);
>
> osinfo_loader_entity(loader, OSINFO_ENTITY(device), keys, ctxt, root, err);
> @@ -519,7 +566,7 @@ static void osinfo_loader_device_link(OsinfoLoader *loader,
> OSINFO_ERROR(err, _("Missing device link id property"));
> goto cleanup;
> }
> - OsinfoDevice *dev = osinfo_loader_get_device(loader, id);
> + OsinfoDevice *dev = osinfo_loader_get_device(loader, id, TRUE);
> xmlFree(id);
>
> OsinfoDeviceLink *devlink = NULL;
> @@ -571,9 +618,9 @@ static void osinfo_loader_product_relshp(OsinfoLoader *loader,
> }
> OsinfoProduct *relproduct;
> if (OSINFO_IS_PLATFORM(product))
> - relproduct = OSINFO_PRODUCT(osinfo_loader_get_platform(loader, id));
> + relproduct = OSINFO_PRODUCT(osinfo_loader_get_platform(loader, id, TRUE));
> else
> - relproduct = OSINFO_PRODUCT(osinfo_loader_get_os(loader, id));
> + relproduct = OSINFO_PRODUCT(osinfo_loader_get_os(loader, id, TRUE));
> xmlFree(id);
>
> osinfo_product_add_related(product, relshp, relproduct);
> @@ -642,7 +689,7 @@ static void osinfo_loader_platform(OsinfoLoader *loader,
> return;
> }
>
> - OsinfoPlatform *platform = osinfo_loader_get_platform(loader, id);
> + OsinfoPlatform *platform = osinfo_loader_get_platform(loader, id, FALSE);
> xmlFree(id);
>
> osinfo_loader_entity(loader, OSINFO_ENTITY(platform), NULL, ctxt, root, err);
> @@ -676,7 +723,7 @@ static void osinfo_loader_deployment(OsinfoLoader *loader,
> xmlFree(id);
> return;
> }
> - OsinfoOs *os = osinfo_loader_get_os(loader, osid);
> + OsinfoOs *os = osinfo_loader_get_os(loader, osid, TRUE);
> g_free(osid);
>
> gchar *platformid = osinfo_loader_string("string(./platform/@id)", loader,
> @@ -686,7 +733,7 @@ static void osinfo_loader_deployment(OsinfoLoader *loader,
> xmlFree(id);
> return;
> }
> - OsinfoPlatform *platform = osinfo_loader_get_platform(loader, platformid);
> + OsinfoPlatform *platform = osinfo_loader_get_platform(loader, platformid, TRUE);
> g_free(platformid);
>
> OsinfoDeployment *deployment = osinfo_deployment_new(id, os, platform);
> @@ -724,7 +771,7 @@ static void osinfo_loader_datamap(OsinfoLoader *loader,
> return;
> }
>
> - OsinfoDatamap *map = osinfo_loader_get_datamap(loader, id);
> + OsinfoDatamap *map = osinfo_loader_get_datamap(loader, id, FALSE);
>
> nnodes = osinfo_loader_nodeset("./entry", loader, ctxt, &nodes, err);
> if (error_is_set(err))
> @@ -773,7 +820,7 @@ static void osinfo_loader_install_config_params(OsinfoLoader *loader,
> param);
> if (mapid != NULL) {
> OsinfoDatamap *map;
> - map = osinfo_loader_get_datamap(loader, mapid);
> + map = osinfo_loader_get_datamap(loader, mapid, TRUE);
> if (map != NULL)
> osinfo_install_config_param_set_value_map(param, map);
> }
> @@ -843,7 +890,8 @@ static void osinfo_loader_install_script(OsinfoLoader *loader,
> }
>
> OsinfoInstallScript *installScript = osinfo_loader_get_install_script(loader,
> - id);
> + id,
> + FALSE);
> xmlFree(id);
>
> osinfo_loader_entity(loader, OSINFO_ENTITY(installScript), keys, ctxt, root, err);
> @@ -1254,7 +1302,8 @@ static OsinfoDeviceDriver *osinfo_loader_driver(OsinfoLoader *loader,
> OSINFO_DEVICE_DRIVER_PROP_DEVICE) == 0) {
> xmlChar *device_id = xmlGetProp(nodes[i], BAD_CAST "id");
> OsinfoDevice *device = osinfo_loader_get_device(loader,
> - (gchar *)device_id);
> + (gchar *)device_id,
> + TRUE);
> xmlFree(device_id);
>
> osinfo_device_driver_add_device(driver, device);
> @@ -1289,7 +1338,7 @@ static void osinfo_loader_os(OsinfoLoader *loader,
> return;
> }
>
> - OsinfoOs *os = osinfo_loader_get_os(loader, id);
> + OsinfoOs *os = osinfo_loader_get_os(loader, id, FALSE);
>
> osinfo_loader_entity(loader, OSINFO_ENTITY(os), keys, ctxt, root, err);
> if (error_is_set(err))
> @@ -1399,7 +1448,7 @@ static void osinfo_loader_os(OsinfoLoader *loader,
> goto cleanup;
> }
> OsinfoInstallScript *script;
> - script = osinfo_loader_get_install_script(loader, scriptid);
> + script = osinfo_loader_get_install_script(loader, scriptid, TRUE);
> xmlFree(scriptid);
>
> osinfo_os_add_install_script(os, script);
> @@ -1651,7 +1700,7 @@ osinfo_loader_process_file_reg_ids(OsinfoLoader *loader,
> gchar *id = g_strdup_printf("%s/%s/%s",
> baseURI, vendor_id, device_id);
>
> - OsinfoDevice *dev = osinfo_loader_get_device(loader, id);
> + OsinfoDevice *dev = osinfo_loader_get_device(loader, id, FALSE);
> OsinfoEntity *entity = OSINFO_ENTITY(dev);
> osinfo_entity_set_param(entity,
> OSINFO_DEVICE_PROP_VENDOR_ID,
> @@ -1908,6 +1957,13 @@ void osinfo_loader_process_default_path(OsinfoLoader *loader, GError **err)
> if (error)
> goto error;
>
> + GHashTableIter iter;
> + gpointer key, value;
> + g_hash_table_iter_init(&iter, loader->priv->entityRefs);
> + while (g_hash_table_iter_next(&iter, &key, &value)) {
> + g_warning("Entity %s referenced but not defined", (const char *)key);
> + }
> + g_hash_table_remove_all(loader->priv->entityRefs);
> return;
>
> error:
> --
> 2.4.3
>
> _______________________________________________
> Libosinfo mailing list
> Libosinfo@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libosinfo
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Libosinfo mailing list Libosinfo@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libosinfo
