auto-updates: Redirect output to self if timer

As mentioned during code review, it feels odd to have our automatic
service collect output under its name rather than the main daemon. It
shouldn't be thought of as a daemon with its own logs, it's really just
a trigger.

We change this here so that for automatic updates, output is redirected
to the daemon stdout, which of course goes to the journal. This should
make logs in case of errors more discoverable as well since
`rpm-ostreed` is the well-known name.

I drilled down the notion directly into `RpmostreedTransaction` since I
think it's a useful property that might be handy at a more general
level.

Closes: #1278
Approved by: cgwalters

src/app/rpmostree-builtin-upgrade.c

@@ -120,6 +120,10 @@ rpmostree_builtin_upgrade (int             argc,
       GVariantDict dict;
       g_variant_dict_init (&dict, NULL);
       g_variant_dict_insert (&dict, "mode", "s", check_or_preview ? "check" : "auto");
+      /* override default of TRUE if we're handling --check/--preview for backcompat,
+       * or we're *are* handling --trigger-automatic-update-policy, but on a tty */
+      if (check_or_preview || glnx_stdout_is_tty ())
+        g_variant_dict_insert (&dict, "output-to-self", "b", FALSE);
       g_autoptr(GVariant) options = g_variant_ref_sink (g_variant_dict_end (&dict));
 
       gboolean auto_updates_enabled;

src/daemon/org.projectatomic.rpmostree1.xml

@@ -96,6 +96,9 @@
          "mode" (type 's')
             One of auto, none, check. Defaults to auto, which follows configured
             policy (available in AutomaticUpdatePolicy property).
+         "output-to-self" (type 'b')
+            Whether output should go to the daemon itself rather than the
+            transaction. Defaults to TRUE.
 
          If automatic updates are not enabled, @enabled will be FALSE and
          @transaction_address will be the empty string.

src/daemon/rpm-ostreed-automatic.service.in

@@ -6,3 +6,4 @@ ConditionPathExists=/run/ostree-booted
 [Service]
 Type=simple
 ExecStart=@bindir@/rpm-ostree upgrade --trigger-automatic-update-policy
+StandardOutput=null

src/daemon/rpmostreed-os.c

@@ -692,9 +692,12 @@ start_deployment_txn (GDBusMethodInvocation  *invocation,
         return FALSE;
     }
 
+  const gboolean redirect_output =
+    vardict_lookup_bool (&options_dict, "output-to-self", FALSE);
   default_flags = deploy_flags_from_options (options, default_flags);
   return rpmostreed_transaction_new_deploy (invocation, ot_sysroot,
                                             default_flags,
+                                            redirect_output,
                                             osname,
                                             canon_refspec,
                                             revision,
@@ -968,11 +971,19 @@ os_handle_automatic_update_trigger (RPMOSTreeOS *interface,
       g_assert_not_reached ();
     }
 
+  /* if output-to-self is not explicitly set, default to TRUE */
+  g_autoptr(GVariant) new_dict = NULL;
+  if (!g_variant_dict_contains (&dict, "output-to-self"))
+    {
+      g_variant_dict_insert (&dict, "output-to-self", "b", TRUE);
+      arg_options = new_dict = g_variant_ref_sink (g_variant_dict_end (&dict));
+    }
+
   return os_merge_or_start_deployment_txn (
       interface,
       invocation,
       dfault,
-      NULL,
+      arg_options,
       NULL,
       NULL,
       automatic_update_trigger_completer);

src/daemon/rpmostreed-sysroot.c

@@ -106,11 +106,14 @@ sysroot_output_cb (RpmOstreeOutputType type, void *data, void *opaque)
 {
   RpmostreedSysroot *self = RPMOSTREED_SYSROOT (opaque);
   glnx_unref_object RpmostreedTransaction *transaction = NULL;
+  gboolean redirect = FALSE;
 
   transaction =
     rpmostreed_transaction_monitor_ref_active_transaction (self->transaction_monitor);
+  if (transaction)
+    g_object_get (transaction, "output-to-self", &redirect, NULL);
 
-  if (!transaction)
+  if (!transaction || redirect)
     {
       rpmostree_output_default_handler (type, data, opaque);
       return;

src/daemon/rpmostreed-transaction-types.c

@@ -1133,6 +1133,7 @@ RpmostreedTransaction *
 rpmostreed_transaction_new_deploy (GDBusMethodInvocation *invocation,
                                    OstreeSysroot *sysroot,
                                    RpmOstreeTransactionDeployFlags flags,
+                                   gboolean    redirect_output,
                                    const char *osname,
                                    const char *refspec,
                                    const char *revision,
@@ -1155,6 +1156,7 @@ rpmostreed_transaction_new_deploy (GDBusMethodInvocation *invocation,
                     cancellable, error,
                     "invocation", invocation,
                     "sysroot-path", gs_file_get_path_cached (ostree_sysroot_get_path (sysroot)),
+                    "output-to-self", redirect_output,
                     NULL);
 
   if (self != NULL)

src/daemon/rpmostreed-transaction-types.h

@@ -65,6 +65,7 @@ RpmostreedTransaction *
 rpmostreed_transaction_new_deploy (GDBusMethodInvocation *invocation,
                                    OstreeSysroot *sysroot,
                                    RpmOstreeTransactionDeployFlags flags,
+                                   gboolean    redirect_output,
                                    const char *osname,
                                    const char *refspec,
                                    const char *revision,

src/daemon/rpmostreed-transaction.c

@@ -37,6 +37,8 @@ struct _RpmostreedTransactionPrivate {
   char *sysroot_path;
   OstreeSysroot *sysroot;
 
+  gboolean redirect_output;
+
   GDBusServer *server;
   GHashTable *peer_connections;
 
@@ -50,7 +52,8 @@ enum {
   PROP_0,
   PROP_ACTIVE,
   PROP_INVOCATION,
-  PROP_SYSROOT_PATH
+  PROP_SYSROOT_PATH,
+  PROP_REDIRECT_OUTPUT
 };
 
 enum {
@@ -376,6 +379,9 @@ transaction_set_property (GObject *object,
       case PROP_SYSROOT_PATH:
         priv->sysroot_path = g_value_dup_string (value);
         break;
+      case PROP_REDIRECT_OUTPUT:
+        priv->redirect_output = g_value_get_boolean (value);
+        break;
       default:
         G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec);
         break;
@@ -399,6 +405,9 @@ transaction_get_property (GObject *object,
       case PROP_SYSROOT_PATH:
         g_value_set_string (value, priv->sysroot_path);
         break;
+      case PROP_REDIRECT_OUTPUT:
+        g_value_set_boolean (value, priv->redirect_output);
+        break;
       default:
         G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec);
         break;
@@ -674,6 +683,16 @@ rpmostreed_transaction_class_init (RpmostreedTransactionClass *class)
                                                         G_PARAM_CONSTRUCT_ONLY |
                                                         G_PARAM_STATIC_STRINGS));
 
+  g_object_class_install_property (object_class,
+                                   PROP_REDIRECT_OUTPUT,
+                                   g_param_spec_boolean ("output-to-self",
+                                                         "Output to self",
+                                                         "Whether to redirect output to daemon itself",
+                                                         FALSE,
+                                                         G_PARAM_READWRITE |
+                                                         G_PARAM_CONSTRUCT_ONLY |
+                                                         G_PARAM_STATIC_STRINGS));
+
   signals[CLOSED] = g_signal_new ("closed",
                                   RPMOSTREED_TYPE_TRANSACTION,
                                   G_SIGNAL_RUN_LAST,