Skip to content

Commit

Permalink
Make output handling thread-local
Browse files Browse the repository at this point in the history 
This is needed because for transactions (which are always run in
a thread today) we want output to go to the transaction's DBus
progress.  But for other methods which are not transactions, we don't
have a channel for status reporting, so output needs to
continue to go to the journal.   If we mix these two things due to
concurrent method invocations, the client may get confused and crash.

Closes: coreos#4284
  • Loading branch information
@cgwalters
cgwalters committed May 11, 2023
1 parent 9f6656e commit 1046c52
Show file tree
Hide file tree
Showing 4 changed files with 127 additions and 114 deletions.
98 changes: 0 additions & 98 deletions src/daemon/rpmostreed-sysroot.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,100 +104,6 @@ static RpmostreedSysroot *_sysroot_instance;
/* ----------------------------------------------------------------------------------------------------
*/

static void
sysroot_output_cb (RpmOstreeOutputType type, void *data, void *opaque)
{
RpmostreedSysroot *self = RPMOSTREED_SYSROOT (opaque);
gboolean output_to_self = FALSE;

// The API previously passed these each time, but now we retain them as
// statics.
static char *progress_str;
static bool progress_state_percent;
static guint progress_state_n_items;

if (self->transaction)
g_object_get (self->transaction, "output-to-self", &output_to_self, NULL);

if (!self->transaction || output_to_self)
{
rpmostree_output_default_handler (type, data, opaque);
return;
}

RPMOSTreeTransaction *transaction = RPMOSTREE_TRANSACTION (self->transaction);
switch (type)
{
case RPMOSTREE_OUTPUT_MESSAGE:
rpmostree_transaction_emit_message (transaction, ((RpmOstreeOutputMessage *)data)->text);
break;
case RPMOSTREE_OUTPUT_PROGRESS_BEGIN:
{
auto begin = static_cast<RpmOstreeOutputProgressBegin *> (data);
g_clear_pointer (&progress_str, g_free);
progress_state_percent = false;
progress_state_n_items = 0;
if (begin->percent)
{
progress_str = g_strdup (begin->prefix);
rpmostree_transaction_emit_percent_progress (transaction, progress_str, 0);
progress_state_percent = true;
}
else if (begin->n > 0)
{
progress_str = g_strdup (begin->prefix);
progress_state_n_items = begin->n;
/* For backcompat, this is a percentage. See below */
rpmostree_transaction_emit_percent_progress (transaction, progress_str, 0);
}
else
{
rpmostree_transaction_emit_task_begin (transaction, begin->prefix);
}
}
break;
case RPMOSTREE_OUTPUT_PROGRESS_UPDATE:
{
auto update = static_cast<RpmOstreeOutputProgressUpdate *> (data);
if (progress_state_n_items)
{
/* We still emit PercentProgress for compatibility with older clients as
* well as Cockpit. It's not worth trying to deal with version skew just
* for this yet.
*/
int percentage = (update->c == progress_state_n_items)
? 100
: (((double)(update->c)) / (progress_state_n_items)*100);
g_autofree char *newtext
= g_strdup_printf ("%s (%u/%u)", progress_str, update->c, progress_state_n_items);
rpmostree_transaction_emit_percent_progress (transaction, newtext, percentage);
}
else
{
rpmostree_transaction_emit_percent_progress (transaction, progress_str, update->c);
}
}
break;
case RPMOSTREE_OUTPUT_PROGRESS_SUB_MESSAGE:
{
/* Not handled right now */
}
break;
case RPMOSTREE_OUTPUT_PROGRESS_END:
{
if (progress_state_percent || progress_state_n_items > 0)
{
rpmostree_transaction_emit_progress_end (transaction);
}
else
{
rpmostree_transaction_emit_task_end (transaction, "done");
}
}
break;
}
}

static gboolean
handle_get_os (RPMOSTreeSysroot *object, GDBusMethodInvocation *invocation, const char *arg_name)
{
Expand Down Expand Up @@ -528,8 +434,6 @@ sysroot_finalize (GObject *object)

g_clear_object (&self->monitor);

rpmostree_output_set_callback (NULL, NULL);

G_OBJECT_CLASS (rpmostreed_sysroot_parent_class)->finalize (object);
}

Expand All @@ -545,8 +449,6 @@ rpmostreed_sysroot_init (RpmostreedSysroot *self)
= g_hash_table_new_full (g_str_hash, g_str_equal, g_free, (GDestroyNotify)g_object_unref);

self->monitor = NULL;

rpmostree_output_set_callback (sysroot_output_cb, self);
}

static gboolean
Expand Down
97 changes: 97 additions & 0 deletions src/daemon/rpmostreed-transaction.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,99 @@ transaction_gpg_verify_result_cb (OstreeRepo *repo, const char *checksum,
g_strdup (checksum));
}

static void
transaction_output_cb (RpmOstreeOutputType type, void *data, void *opaque)
{
RpmostreedTransaction *self = RPMOSTREED_TRANSACTION (opaque);
gboolean output_to_self = FALSE;

// The API previously passed these each time, but now we retain them as
// statics.
static char *progress_str;
static bool progress_state_percent;
static guint progress_state_n_items;

g_object_get (self, "output-to-self", &output_to_self, NULL);
if (output_to_self)
{
rpmostree_output_default_handler (type, data, NULL);
return;
}

RPMOSTreeTransaction *transaction = RPMOSTREE_TRANSACTION (self);

switch (type)
{
case RPMOSTREE_OUTPUT_MESSAGE:
rpmostree_transaction_emit_message (transaction, ((RpmOstreeOutputMessage *)data)->text);
break;
case RPMOSTREE_OUTPUT_PROGRESS_BEGIN:
{
auto begin = static_cast<RpmOstreeOutputProgressBegin *> (data);
g_clear_pointer (&progress_str, g_free);
progress_state_percent = false;
progress_state_n_items = 0;
if (begin->percent)
{
progress_str = g_strdup (begin->prefix);
rpmostree_transaction_emit_percent_progress (transaction, progress_str, 0);
progress_state_percent = true;
}
else if (begin->n > 0)
{
progress_str = g_strdup (begin->prefix);
progress_state_n_items = begin->n;
/* For backcompat, this is a percentage. See below */
rpmostree_transaction_emit_percent_progress (transaction, progress_str, 0);
}
else
{
rpmostree_transaction_emit_task_begin (transaction, begin->prefix);
}
}
break;
case RPMOSTREE_OUTPUT_PROGRESS_UPDATE:
{
auto update = static_cast<RpmOstreeOutputProgressUpdate *> (data);
if (progress_state_n_items)
{
/* We still emit PercentProgress for compatibility with older clients as
* well as Cockpit. It's not worth trying to deal with version skew just
* for this yet.
*/
int percentage = (update->c == progress_state_n_items)
? 100
: (((double)(update->c)) / (progress_state_n_items)*100);
g_autofree char *newtext
= g_strdup_printf ("%s (%u/%u)", progress_str, update->c, progress_state_n_items);
rpmostree_transaction_emit_percent_progress (transaction, newtext, percentage);
}
else
{
rpmostree_transaction_emit_percent_progress (transaction, progress_str, update->c);
}
}
break;
case RPMOSTREE_OUTPUT_PROGRESS_SUB_MESSAGE:
{
/* Not handled right now */
}
break;
case RPMOSTREE_OUTPUT_PROGRESS_END:
{
if (progress_state_percent || progress_state_n_items > 0)
{
rpmostree_transaction_emit_progress_end (transaction);
}
else
{
rpmostree_transaction_emit_task_end (transaction, "done");
}
}
break;
}
}

static void
transaction_execute_thread (GTask *task, gpointer source_object, gpointer task_data,
GCancellable *cancellable)
Expand All @@ -333,6 +426,8 @@ transaction_execute_thread (GTask *task, gpointer source_object, gpointer task_d
// Further, we join the main Tokio async runtime.
auto guard = rpmostreecxx::rpmostreed_daemon_tokio_enter (rpmostreed_daemon_get ());

rpmostree_output_set_callback (transaction_output_cb, self);

try
{
rpmostreecxx::failpoint ("transaction::execute");
Expand Down Expand Up @@ -377,6 +472,8 @@ transaction_execute_thread (GTask *task, gpointer source_object, gpointer task_d
g_task_return_boolean (task, success);
}

rpmostree_output_set_callback (NULL, NULL);

/* Clean up context */
g_main_context_pop_thread_default (mctx);
}
Expand Down
43 changes: 28 additions & 15 deletions src/libpriv/rpmostree-output.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,15 +71,28 @@ rpmostree_output_default_handler (RpmOstreeOutputType type, void *data, void *op
}
}

static void (*active_cb) (RpmOstreeOutputType, void *, void *) = rpmostree_output_default_handler;

static void *active_cb_opaque;
// Thread-local output functions. This is needed because for transactions (which are always run in
// a thread today) we want output to go to the transaction's DBus progress. But for other methods
// which are not transactions, we don't have a channel for status reporting, so output needs to
// continue to go to the journal. If we mix these two things due to concurrent method invocations,
// the client may get confused and crash. https://github.com/coreos/rpm-ostree/issues/4284
static GPrivate active_cb;
static GPrivate active_cb_opaque;

static void
invoke_output (RpmOstreeOutputType ty, void *task)
{
OutputCallback cb
= (OutputCallback)g_private_get (&active_cb) ?: rpmostree_output_default_handler;
void *data = g_private_get (&active_cb_opaque);
cb (ty, task, data);
}

void
rpmostree_output_set_callback (void (*cb) (RpmOstreeOutputType, void *, void *), void *opaque)
rpmostree_output_set_callback (OutputCallback cb, void *opaque)
{
active_cb = cb ?: rpmostree_output_default_handler;
active_cb_opaque = opaque;
g_private_replace (&active_cb, (void *)cb);
g_private_replace (&active_cb_opaque, opaque);
}

#define strdup_vprintf(format) \
Expand All @@ -99,7 +112,7 @@ rpmostree_output_message (const char *format, ...)
{
g_autofree char *final_msg = strdup_vprintf (format);
RpmOstreeOutputMessage task = { final_msg };
active_cb (RPMOSTREE_OUTPUT_MESSAGE, &task, active_cb_opaque);
invoke_output (RPMOSTREE_OUTPUT_MESSAGE, &task);
}

namespace rpmostreecxx
Expand All @@ -120,7 +133,7 @@ output_message (const rust::Str msg)
{
auto msg_c = std::string (msg);
RpmOstreeOutputMessage task = { msg_c.c_str () };
active_cb (RPMOSTREE_OUTPUT_MESSAGE, &task, active_cb_opaque);
invoke_output (RPMOSTREE_OUTPUT_MESSAGE, &task);
}

// Begin a task (that can't easily be "nitems" or percentage).
Expand All @@ -130,7 +143,7 @@ progress_begin_task (const rust::Str msg) noexcept
{
auto msg_c = std::string (msg);
RpmOstreeOutputProgressBegin begin = { msg_c.c_str (), false, 0 };
active_cb (RPMOSTREE_OUTPUT_PROGRESS_BEGIN, &begin, active_cb_opaque);
invoke_output (RPMOSTREE_OUTPUT_PROGRESS_BEGIN, &begin);
auto v = std::make_unique<Progress> (ProgressType::TASK);
g_debug ("init progress task serial=%" G_GUINT64_FORMAT " text=%s", v->serial, msg_c.c_str ());
return v;
Expand All @@ -142,7 +155,7 @@ void
Progress::set_sub_message (const rust::Str msg)
{
g_autofree char *msg_c = util::ruststr_dup_c_optempty (msg);
active_cb (RPMOSTREE_OUTPUT_PROGRESS_SUB_MESSAGE, (void *)msg_c, active_cb_opaque);
invoke_output (RPMOSTREE_OUTPUT_PROGRESS_SUB_MESSAGE, (void *)msg_c);
}

// Start working on a 0-n task.
Expand All @@ -151,7 +164,7 @@ progress_nitems_begin (guint n, const rust::Str msg) noexcept
{
auto msg_c = std::string (msg);
RpmOstreeOutputProgressBegin begin = { msg_c.c_str (), false, n };
active_cb (RPMOSTREE_OUTPUT_PROGRESS_BEGIN, &begin, active_cb_opaque);
invoke_output (RPMOSTREE_OUTPUT_PROGRESS_BEGIN, &begin);
auto v = std::make_unique<Progress> (ProgressType::N_ITEMS);
g_debug ("init progress nitems serial=%" G_GUINT64_FORMAT " text=%s", v->serial, msg_c.c_str ());
return v;
Expand All @@ -163,7 +176,7 @@ Progress::nitems_update (guint n)
{
RpmOstreeOutputProgressUpdate progress = { n };
g_debug ("progress nitems update serial=%" G_GUINT64_FORMAT, this->serial);
active_cb (RPMOSTREE_OUTPUT_PROGRESS_UPDATE, &progress, active_cb_opaque);
invoke_output (RPMOSTREE_OUTPUT_PROGRESS_UPDATE, &progress);
}

// Start a percentage task.
Expand All @@ -172,7 +185,7 @@ progress_percent_begin (const rust::Str msg) noexcept
{
auto msg_c = std::string (msg);
RpmOstreeOutputProgressBegin begin = { msg_c.c_str (), true, 0 };
active_cb (RPMOSTREE_OUTPUT_PROGRESS_BEGIN, &begin, active_cb_opaque);
invoke_output (RPMOSTREE_OUTPUT_PROGRESS_BEGIN, &begin);
auto v = std::make_unique<Progress> (ProgressType::PERCENT);
g_debug ("init progress percent serial=%" G_GUINT64_FORMAT " text=%s", v->serial, msg_c.c_str ());
return v;
Expand All @@ -184,7 +197,7 @@ Progress::percent_update (guint n)
{
RpmOstreeOutputProgressUpdate progress = { (guint)n };
g_debug ("progress percent update serial=%" G_GUINT64_FORMAT, this->serial);
active_cb (RPMOSTREE_OUTPUT_PROGRESS_UPDATE, &progress, active_cb_opaque);
invoke_output (RPMOSTREE_OUTPUT_PROGRESS_UPDATE, &progress);
}

// End the current task.
Expand All @@ -195,7 +208,7 @@ Progress::end (const rust::Str msg)
g_autofree char *final_msg = util::ruststr_dup_c_optempty (msg);
RpmOstreeOutputProgressEnd done = { final_msg };
g_debug ("progress end serial=%" G_GUINT64_FORMAT, this->serial);
active_cb (RPMOSTREE_OUTPUT_PROGRESS_END, &done, active_cb_opaque);
invoke_output (RPMOSTREE_OUTPUT_PROGRESS_END, &done);
this->ended = true;
}

Expand Down
3 changes: 2 additions & 1 deletion src/libpriv/rpmostree-output.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,8 @@ typedef enum

void rpmostree_output_default_handler (RpmOstreeOutputType type, void *data, void *opaque);

void rpmostree_output_set_callback (void (*cb) (RpmOstreeOutputType, void *, void *), void *);
typedef void (*OutputCallback) (RpmOstreeOutputType, void *, void *);
void rpmostree_output_set_callback (OutputCallback cb, void *);

typedef struct
{
Expand Down

0 comments on commit 1046c52

Please sign in to comment.