Skip to content

Commit

Permalink
daemon: Make "diff" methods safer
Browse files Browse the repository at this point in the history 
Various OS "diff" methods can run concurrently with whatever else is
going on since they don't have to obtain the system root lock.

Just to make sure there's no conflicts when writing deployments or
downloading RPM package details, use an internal reader/writer lock
to protect the critical sections of upgrade, rebase, rollback, etc.
  • Loading branch information
@mbarnes
mbarnes committed Sep 10, 2015
1 parent a22c592 commit 73f2a7f
Show file tree
Hide file tree
Showing 4 changed files with 148 additions and 16 deletions.
27 changes: 24 additions & 3 deletions src/daemon/rpmostreed-os.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ get_rebase_diff_variant_in_thread (GTask *task,
GCancellable *cancellable)
{
RPMOSTreeOS *self = RPMOSTREE_OS (object);
RpmostreedSysroot *global_sysroot;
const gchar *name;

glnx_unref_object OstreeSysroot *ot_sysroot = NULL;
Expand All @@ -188,7 +189,11 @@ get_rebase_diff_variant_in_thread (GTask *task,
GError *error = NULL; /* freed when invoked */
gchar *refspec = data_ptr; /* freed by task */

if (!rpmostreed_sysroot_load_state (rpmostreed_sysroot_get (),
global_sysroot = rpmostreed_sysroot_get ();

rpmostreed_sysroot_reader_lock (global_sysroot);

if (!rpmostreed_sysroot_load_state (global_sysroot,
cancellable,
&ot_sysroot,
&ot_repo,
Expand Down Expand Up @@ -218,6 +223,8 @@ get_rebase_diff_variant_in_thread (GTask *task,
&error);

out:
rpmostreed_sysroot_reader_unlock (global_sysroot);

set_diff_task_result (task, value, error);
}

Expand All @@ -228,6 +235,7 @@ get_upgrade_diff_variant_in_thread (GTask *task,
GCancellable *cancellable)
{
RPMOSTreeOS *self = RPMOSTREE_OS (object);
RpmostreedSysroot *global_sysroot;
const gchar *name;
gchar *compare_deployment = data_ptr; /* freed by task */

Expand All @@ -239,7 +247,11 @@ get_upgrade_diff_variant_in_thread (GTask *task,
GVariant *value = NULL; /* freed when invoked */
GError *error = NULL; /* freed when invoked */

if (!rpmostreed_sysroot_load_state (rpmostreed_sysroot_get (),
global_sysroot = rpmostreed_sysroot_get ();

rpmostreed_sysroot_reader_lock (global_sysroot);

if (!rpmostreed_sysroot_load_state (global_sysroot,
cancellable,
&ot_sysroot,
&ot_repo,
Expand Down Expand Up @@ -286,6 +298,8 @@ get_upgrade_diff_variant_in_thread (GTask *task,
&error);

out:
rpmostreed_sysroot_reader_unlock (global_sysroot);

set_diff_task_result (task, value, error);
}

Expand All @@ -295,6 +309,7 @@ get_deployments_diff_variant_in_thread (GTask *task,
gpointer data_ptr,
GCancellable *cancellable)
{
RpmostreedSysroot *global_sysroot;
const gchar *ref0;
const gchar *ref1;

Expand All @@ -309,7 +324,11 @@ get_deployments_diff_variant_in_thread (GTask *task,

g_return_if_fail (compare_refs->len == 2);

if (!rpmostreed_sysroot_load_state (rpmostreed_sysroot_get (),
global_sysroot = rpmostreed_sysroot_get ();

rpmostreed_sysroot_reader_lock (global_sysroot);

if (!rpmostreed_sysroot_load_state (global_sysroot,
cancellable,
&ot_sysroot,
&ot_repo,
Expand Down Expand Up @@ -349,6 +368,8 @@ get_deployments_diff_variant_in_thread (GTask *task,
&error);

out:
rpmostreed_sysroot_reader_unlock (global_sysroot);

set_diff_task_result (task, value, error);
}

Expand Down
39 changes: 39 additions & 0 deletions src/daemon/rpmostreed-sysroot.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,13 @@ struct _RpmostreedSysroot {
GHashTable *os_interfaces;
GRWLock children_lock;

/* The OS interface's various diff methods can run concurrently with
* transactions, which is safe except when the transaction is writing
* new deployments to disk or downloading RPM package details. The
* writer lock protects these critical sections so the diff methods
* (holding reader locks) can run safely. */
GRWLock method_rw_lock;

GFileMonitor *monitor;
guint sig_changed;
guint64 last_monitor_event;
Expand Down Expand Up @@ -580,6 +587,7 @@ sysroot_finalize (GObject *object)
g_hash_table_unref (self->os_interfaces);

g_rw_lock_clear (&self->children_lock);
g_rw_lock_clear (&self->method_rw_lock);

g_clear_object (&self->cancellable);
g_clear_object (&self->monitor);
Expand All @@ -602,6 +610,7 @@ rpmostreed_sysroot_init (RpmostreedSysroot *self)
(GDestroyNotify) g_object_unref);

g_rw_lock_init (&self->children_lock);
g_rw_lock_init (&self->method_rw_lock);

self->monitor = NULL;
self->sig_changed = 0;
Expand Down Expand Up @@ -1005,6 +1014,36 @@ rpmostreed_sysroot_get (void)
return _sysroot_instance;
}

void
rpmostreed_sysroot_reader_lock (RpmostreedSysroot *self)
{
g_return_if_fail (RPMOSTREED_IS_SYSROOT (self));

g_rw_lock_reader_lock (&self->method_rw_lock);
}

void
rpmostreed_sysroot_reader_unlock (RpmostreedSysroot *self)
{
g_return_if_fail (RPMOSTREED_IS_SYSROOT (self));

g_rw_lock_reader_unlock (&self->method_rw_lock);
}

void
rpmostreed_sysroot_writer_lock (RpmostreedSysroot *self)
{
g_return_if_fail (RPMOSTREED_IS_SYSROOT (self));

g_rw_lock_writer_lock (&self->method_rw_lock);
}

void
rpmostreed_sysroot_writer_unlock (RpmostreedSysroot *self)
{
g_return_if_fail (RPMOSTREED_IS_SYSROOT (self));

g_rw_lock_writer_unlock (&self->method_rw_lock);
}

/* ---------------------------------------------------------------------------------------------------- */
4 changes: 4 additions & 0 deletions src/daemon/rpmostreed-sysroot.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,7 @@ gboolean rpmostreed_sysroot_load_state (RpmostreedSysroot *self
GError **error);

void rpmostreed_sysroot_emit_update (RpmostreedSysroot *self);
void rpmostreed_sysroot_reader_lock (RpmostreedSysroot *self);
void rpmostreed_sysroot_reader_unlock (RpmostreedSysroot *self);
void rpmostreed_sysroot_writer_lock (RpmostreedSysroot *self);
void rpmostreed_sysroot_writer_unlock (RpmostreedSysroot *self);
94 changes: 81 additions & 13 deletions src/daemon/rpmostreed-transaction-types.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include "rpmostreed-transaction-types.h"
#include "rpmostreed-transaction.h"
#include "rpmostreed-deployment-utils.h"
#include "rpmostreed-sysroot.h"
#include "rpmostreed-utils.h"

static gboolean
Expand Down Expand Up @@ -77,6 +78,69 @@ change_upgrader_refspec (OstreeSysroot *sysroot,
return ret;
}

static gboolean
safe_sysroot_upgrader_deploy (OstreeSysrootUpgrader *upgrader,
GCancellable *cancellable,
GError **error)
{
RpmostreedSysroot *global_sysroot = rpmostreed_sysroot_get ();
gboolean success;

rpmostreed_sysroot_writer_lock (global_sysroot);

success = ostree_sysroot_upgrader_deploy (upgrader, cancellable, error);

rpmostreed_sysroot_writer_unlock (global_sysroot);

return success;
}

static gboolean
safe_sysroot_upgrader_pull_package_diff (OstreeSysrootUpgrader *upgrader,
OstreeAsyncProgress *progress,
gboolean *out_changed,
GCancellable *cancellable,
GError **error)
{
RpmostreedSysroot *global_sysroot = rpmostreed_sysroot_get ();
gboolean success;

rpmostreed_sysroot_writer_lock (global_sysroot);

success = ostree_sysroot_upgrader_pull_one_dir (upgrader,
"/usr/share/rpm",
0, 0,
progress,
out_changed,
cancellable,
error);

rpmostreed_sysroot_writer_unlock (global_sysroot);

return success;
}

static gboolean
safe_sysroot_write_deployments (OstreeSysroot *sysroot,
GPtrArray *deployments,
GCancellable *cancellable,
GError **error)
{
RpmostreedSysroot *global_sysroot = rpmostreed_sysroot_get ();
gboolean success;

rpmostreed_sysroot_writer_lock (global_sysroot);

success = ostree_sysroot_write_deployments (sysroot,
deployments,
cancellable,
error);

rpmostreed_sysroot_writer_unlock (global_sysroot);

return success;
}

/* ============================= Package Diff ============================= */

typedef struct {
Expand Down Expand Up @@ -153,9 +217,9 @@ package_diff_transaction_execute (RpmostreedTransaction *transaction,
progress = ostree_async_progress_new ();
rpmostreed_transaction_connect_download_progress (transaction, progress);
rpmostreed_transaction_connect_signature_progress (transaction, repo);
if (!ostree_sysroot_upgrader_pull_one_dir (upgrader, "/usr/share/rpm",
0, 0, progress, &changed,
cancellable, error))

if (!safe_sysroot_upgrader_pull_package_diff (upgrader, progress, &changed,
cancellable, error))
goto out;

rpmostree_transaction_emit_progress_end (RPMOSTREE_TRANSACTION (transaction));
Expand Down Expand Up @@ -291,10 +355,13 @@ rollback_transaction_execute (RpmostreedTransaction *transaction,

/* if default changed write it */
if (old_deployments->pdata[0] != new_deployments->pdata[0])
ostree_sysroot_write_deployments (sysroot,
new_deployments,
cancellable,
error);
{
if (!safe_sysroot_write_deployments (sysroot,
new_deployments,
cancellable,
error))
goto out;
}

ret = TRUE;

Expand Down Expand Up @@ -403,10 +470,11 @@ clear_rollback_transaction_execute (RpmostreedTransaction *transaction,

g_ptr_array_remove_index (deployments, rollback_index);

ostree_sysroot_write_deployments (sysroot,
deployments,
cancellable,
error);
if (!safe_sysroot_write_deployments (sysroot,
deployments,
cancellable,
error))
goto out;

ret = TRUE;

Expand Down Expand Up @@ -542,7 +610,7 @@ upgrade_transaction_execute (RpmostreedTransaction *transaction,

if (changed)
{
if (!ostree_sysroot_upgrader_deploy (upgrader, cancellable, error))
if (!safe_sysroot_upgrader_deploy (upgrader, cancellable, error))
goto out;
}
else
Expand Down Expand Up @@ -689,7 +757,7 @@ rebase_transaction_execute (RpmostreedTransaction *transaction,

rpmostree_transaction_emit_progress_end (RPMOSTREE_TRANSACTION (transaction));

if (!ostree_sysroot_upgrader_deploy (upgrader, cancellable, error))
if (!safe_sysroot_upgrader_deploy (upgrader, cancellable, error))
goto out;

if (!self->skip_purge)
Expand Down

0 comments on commit 73f2a7f

Please sign in to comment.