daemon: More error handling cleanup for loading deployment metadata

I really don't like doing a `g_warning()` in the middle of a call
stack and stumbling onwards.  If we fail to load a commit, we should
pass the error back up to toplevel boundary - normally a DBus method
invocation.  As opposed to giving partial or incorrect data.

This is a preparatory (git) commit for adding more data from the
(ostree) commit to the deployment variant.

Closes: #295
Approved by: jlebon

src/daemon/rpmostreed-deployment-utils.c

@@ -169,47 +169,29 @@ rpmostreed_deployment_generate_blank_variant (void)
 
 static void
 variant_add_commit_details (GVariantDict *dict,
-                            OstreeRepo *repo,
+			    GVariant     *commit)
-                            const gchar *csum)
 {
-  g_autoptr(GVariant) commit = NULL;
   g_autoptr(GVariant) metadata = NULL;
-  GError *error = NULL;
-
   g_autofree gchar *version_commit = NULL;
   guint64 timestamp = 0;
 
-  if (ostree_repo_load_variant (repo,
+  timestamp = ostree_commit_get_timestamp (commit);
-                                OSTREE_OBJECT_TYPE_COMMIT,
+  metadata = g_variant_get_child_value (commit, 0);
-                                csum,
+  if (metadata != NULL)
-                                &commit,
+    g_variant_lookup (metadata, "version", "s", &version_commit);
-                                &error))
-    {
-
-      timestamp = ostree_commit_get_timestamp (commit);
-      metadata = g_variant_get_child_value (commit, 0);
-      if (metadata != NULL)
-          g_variant_lookup (metadata, "version", "s", &version_commit);
-    }
-  else
-    {
-      g_warning ("Error loading commit %s", error->message);
-    }
 
   if (version_commit != NULL)
     g_variant_dict_insert (dict, "version", "s", version_commit);
   if (timestamp > 0)
     g_variant_dict_insert (dict, "timestamp", "t", timestamp);
-
-  g_clear_error (&error);
 }
 
 GVariant *
 rpmostreed_deployment_generate_variant (OstreeDeployment *deployment,
-                                        OstreeRepo *repo)
+                                        OstreeRepo *repo,
+					GError **error)
 {
   g_autoptr(GVariant) commit = NULL;
-
   g_autofree gchar *origin_refspec = NULL;
   g_auto(GStrv) origin_packages = NULL;
   g_autofree gchar *id = NULL;
@@ -221,6 +203,14 @@ rpmostreed_deployment_generate_variant (OstreeDeployment *deployment,
   const gchar *osname = ostree_deployment_get_osname (deployment);
   const gchar *csum = ostree_deployment_get_csum (deployment);
   gint serial = ostree_deployment_get_deployserial (deployment);
+
+  if (!ostree_repo_load_variant (repo,
+				 OSTREE_OBJECT_TYPE_COMMIT,
+				 csum,
+				 &commit,
+				 error))
+    return NULL;
+  
   id = rpmostreed_deployment_generate_id (deployment);
 
   rpmostreed_deployment_get_refspec_packages (deployment, &origin_refspec, &origin_packages);
@@ -235,7 +225,7 @@ rpmostreed_deployment_generate_variant (OstreeDeployment *deployment,
   g_variant_dict_insert (&dict, "serial", "i", serial);
   g_variant_dict_insert (&dict, "checksum", "s", csum);
 
-  variant_add_commit_details (&dict, repo, csum);
+  variant_add_commit_details (&dict, commit);
   if (origin_refspec != NULL)
     g_variant_dict_insert (&dict, "origin", "s", origin_refspec);
   if (origin_packages != NULL)
@@ -252,16 +242,15 @@ rpmostreed_deployment_generate_variant (OstreeDeployment *deployment,
 GVariant *
 rpmostreed_commit_generate_cached_details_variant (OstreeDeployment *deployment,
                                                    OstreeRepo *repo,
-                                                   const gchar *refspec)
+                                                   const gchar *refspec,
+						   GError **error)
 {
+  g_autoptr(GVariant) commit = NULL;
   g_autofree gchar *origin_refspec = NULL;
   g_autofree gchar *head = NULL;
   const gchar *osname;
-
   GVariant *sigs = NULL; /* floating variant */
   GVariant *ret = NULL; /* floating variant */
-  GError *error = NULL;
-
   GVariantDict dict;
 
   osname = ostree_deployment_get_osname (deployment);
@@ -275,11 +264,14 @@ rpmostreed_commit_generate_cached_details_variant (OstreeDeployment *deployment,
     goto out;
 
   if (!ostree_repo_resolve_rev (repo, origin_refspec,
-                                FALSE, &head, &error))
+                                FALSE, &head, error))
-    {
+    goto out;
-      g_warning ("Error loading resolving revision: %s", error->message);
+  if (!ostree_repo_load_variant (repo,
-      goto out;
+				 OSTREE_OBJECT_TYPE_COMMIT,
-    }
+				 head,
+				 &commit,
+				 error))
+    return NULL;
 
   sigs = rpmostreed_deployment_gpg_results (repo, origin_refspec, head);
 
@@ -287,13 +279,12 @@ rpmostreed_commit_generate_cached_details_variant (OstreeDeployment *deployment,
   if (osname != NULL)
     g_variant_dict_insert (&dict, "osname", "s", osname);
   g_variant_dict_insert (&dict, "checksum", "s", head);
-  variant_add_commit_details (&dict, repo, head);
+  variant_add_commit_details (&dict, commit);
   g_variant_dict_insert (&dict, "origin", "s", origin_refspec);
   if (sigs != NULL)
     g_variant_dict_insert_value (&dict, "signatures", sigs);
   ret = g_variant_dict_end (&dict);
 out:
-  g_clear_error (&error);
   return ret;
 }
 

src/daemon/rpmostreed-deployment-utils.h

@@ -37,11 +37,13 @@ void            rpmostreed_deployment_get_refspec_packages (OstreeDeployment *de
 GVariant *      rpmostreed_deployment_generate_blank_variant (void);
 
 GVariant *      rpmostreed_deployment_generate_variant (OstreeDeployment *deployment,
-                                                        OstreeRepo *repo);
+                                                        OstreeRepo *repo,
+							GError **error);
 
 GVariant *      rpmostreed_commit_generate_cached_details_variant (OstreeDeployment *deployment,
                                                                    OstreeRepo *repo,
-                                                                   const gchar *refspec);
+                                                                   const gchar *refspec,
+								   GError **error);
 
 gint            rpmostreed_rollback_deployment_index (const gchar *name,
                                                       OstreeSysroot *ot_sysroot,

src/daemon/rpmostreed-os.c

@@ -48,7 +48,7 @@ struct _RpmostreedOSClass
 
 static void rpmostreed_os_iface_init (RPMOSTreeOSIface *iface);
 
-static void rpmostreed_os_load_internals (RpmostreedOS *self);
+static gboolean rpmostreed_os_load_internals (RpmostreedOS *self, GError **error);
 
 G_DEFINE_TYPE_WITH_CODE (RpmostreedOS,
                          rpmostreed_os,
@@ -63,8 +63,15 @@ sysroot_changed (RpmostreedSysroot *sysroot,
                  gpointer user_data)
 {
   RpmostreedOS *self = RPMOSTREED_OS (user_data);
+  g_autoptr(GError) local_error = NULL;
+  GError **error = &local_error;
 
-  rpmostreed_os_load_internals (self);
+  if (!rpmostreed_os_load_internals (self, error))
+      goto out;
+  
+ out:
+  if (local_error)
+    g_warning ("%s", local_error->message);
 }
 
 static void
@@ -257,7 +264,10 @@ os_handle_get_cached_update_rpm_diff (RPMOSTreeOS *interface,
 
   details = rpmostreed_commit_generate_cached_details_variant (base_deployment,
                                                                ot_repo,
-                                                               comp_ref);
+                                                               comp_ref,
+							       &local_error);
+  if (!details)
+    goto out;
 
 out:
   if (local_error != NULL)
@@ -910,7 +920,10 @@ os_handle_get_cached_rebase_rpm_diff (RPMOSTreeOS *interface,
 
   details = rpmostreed_commit_generate_cached_details_variant (base_deployment,
                                                                ot_repo,
-                                                               comp_ref);
+                                                               comp_ref,
+							       &local_error);
+  if (!details)
+    goto out;
 
 out:
   if (local_error == NULL)
@@ -1051,7 +1064,10 @@ os_handle_get_cached_deploy_rpm_diff (RPMOSTreeOS *interface,
 
   details = rpmostreed_commit_generate_cached_details_variant (base_deployment,
                                                                ot_repo,
-                                                               NULL);
+                                                               NULL,
+							       &local_error);
+  if (!details)
+    goto out;
 
 out:
   if (local_error != NULL)
@@ -1130,8 +1146,8 @@ out:
   return TRUE;
 }
 
-static void
+static gboolean
-rpmostreed_os_load_internals (RpmostreedOS *self)
+rpmostreed_os_load_internals (RpmostreedOS *self, GError **error)
 {
   const gchar *name;
 
@@ -1164,7 +1180,9 @@ rpmostreed_os_load_internals (RpmostreedOS *self)
       if (g_strcmp0 (ostree_deployment_get_osname (deployments->pdata[i]), name) == 0)
         {
           default_variant = rpmostreed_deployment_generate_variant (deployments->pdata[i],
-                                                                    ot_repo);
+                                                                    ot_repo, error);
+	  if (default_variant == NULL)
+	    return FALSE;
           break;
         }
     }
@@ -1173,9 +1191,9 @@ rpmostreed_os_load_internals (RpmostreedOS *self)
   if (booted && g_strcmp0 (ostree_deployment_get_osname (booted),
                            name) == 0)
     {
-      booted_variant = rpmostreed_deployment_generate_variant (booted,
+      booted_variant = rpmostreed_deployment_generate_variant (booted, ot_repo, error);
-                                                               ot_repo);
+      if (!booted_variant)
-
+	return FALSE;
     }
 
   if (deployments->len >= 2)
@@ -1184,7 +1202,9 @@ rpmostreed_os_load_internals (RpmostreedOS *self)
       if (rollback_index >= 0)
 	{
 	  rollback_variant = rpmostreed_deployment_generate_variant (deployments->pdata[rollback_index],
-								     ot_repo);
+								     ot_repo, error);
+	  if (!rollback_variant)
+	    return FALSE;
 	}
     }
 
@@ -1193,7 +1213,10 @@ rpmostreed_os_load_internals (RpmostreedOS *self)
     {
       cached_update = rpmostreed_commit_generate_cached_details_variant (merge_deployment,
                                                                          ot_repo,
-                                                                         NULL);
+                                                                         NULL,
+									 error);
+      if (!cached_update)
+	return FALSE;
       has_cached_updates = cached_update != NULL;
     }
 
@@ -1216,6 +1239,8 @@ rpmostreed_os_load_internals (RpmostreedOS *self)
   rpmostree_os_set_has_cached_update_rpm_diff (RPMOSTREE_OS (self),
                                                has_cached_updates);
   g_dbus_interface_skeleton_flush(G_DBUS_INTERFACE_SKELETON (self));
+
+  return TRUE;
 }
 
 static void
@@ -1259,7 +1284,12 @@ rpmostreed_os_new (OstreeSysroot *sysroot,
   /* FIXME Make this a construct-only property? */
   obj->transaction_monitor = g_object_ref (monitor);
 
-  rpmostreed_os_load_internals (obj);
+  /* FIXME - use GInitable */
+  { g_autoptr(GError) local_error = NULL;
+    if (!rpmostreed_os_load_internals (obj, &local_error))
+      g_warning ("%s", local_error->message);
+  }
+  
   rpmostreed_daemon_publish (rpmostreed_daemon_get (), path, FALSE, obj);
 
   return RPMOSTREE_OS (obj);

src/daemon/rpmostreed-sysroot.c

@@ -465,7 +465,9 @@ sysroot_populate_deployments_unlocked (RpmostreedSysroot *self,
       OstreeDeployment *deployment = deployments->pdata[i];
       const char *deployment_os;
       
-      variant = rpmostreed_deployment_generate_variant (deployment, self->repo);
+      variant = rpmostreed_deployment_generate_variant (deployment, self->repo, error);
+      if (!variant)
+	goto out;
       g_variant_builder_add_value (&builder, variant);
 
       deployment_os = ostree_deployment_get_osname (deployment);