From ccae8ca977a1916dfb60093d841e03b0351a5484 Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Tue, 27 Feb 2018 17:36:15 +0000 Subject: [PATCH] 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 | 4 ++++ src/daemon/org.projectatomic.rpmostree1.xml | 3 +++ src/daemon/rpm-ostreed-automatic.service.in | 1 + src/daemon/rpmostreed-os.c | 13 ++++++++++++- src/daemon/rpmostreed-sysroot.c | 5 ++++- src/daemon/rpmostreed-transaction-types.c | 2 ++ src/daemon/rpmostreed-transaction-types.h | 1 + src/daemon/rpmostreed-transaction.c | 21 ++++++++++++++++++++- 8 files changed, 47 insertions(+), 3 deletions(-) diff --git a/src/app/rpmostree-builtin-upgrade.c b/src/app/rpmostree-builtin-upgrade.c index a532cbef..0d122df7 100644 --- a/src/app/rpmostree-builtin-upgrade.c +++ b/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; diff --git a/src/daemon/org.projectatomic.rpmostree1.xml b/src/daemon/org.projectatomic.rpmostree1.xml index 5e961170..05fc1576 100644 --- a/src/daemon/org.projectatomic.rpmostree1.xml +++ b/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. diff --git a/src/daemon/rpm-ostreed-automatic.service.in b/src/daemon/rpm-ostreed-automatic.service.in index 6b3e634c..729748c2 100644 --- a/src/daemon/rpm-ostreed-automatic.service.in +++ b/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 diff --git a/src/daemon/rpmostreed-os.c b/src/daemon/rpmostreed-os.c index cb988558..93a1e440 100644 --- a/src/daemon/rpmostreed-os.c +++ b/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); diff --git a/src/daemon/rpmostreed-sysroot.c b/src/daemon/rpmostreed-sysroot.c index 4de637a1..3d7f7895 100644 --- a/src/daemon/rpmostreed-sysroot.c +++ b/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; diff --git a/src/daemon/rpmostreed-transaction-types.c b/src/daemon/rpmostreed-transaction-types.c index fb9c24e6..38972780 100644 --- a/src/daemon/rpmostreed-transaction-types.c +++ b/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) diff --git a/src/daemon/rpmostreed-transaction-types.h b/src/daemon/rpmostreed-transaction-types.h index eacc7de7..949a66a1 100644 --- a/src/daemon/rpmostreed-transaction-types.h +++ b/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, diff --git a/src/daemon/rpmostreed-transaction.c b/src/daemon/rpmostreed-transaction.c index 2ad807be..ee8b41ef 100644 --- a/src/daemon/rpmostreed-transaction.c +++ b/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,