Skip to content

Commit

Permalink
compose: Use GVariantDict, not manual hash table
Browse files Browse the repository at this point in the history 
There's a dedicated type for a mapping from "string to variant" in
`GVariantDict`.  Use it instead of rolling our own with `GHashTable`.
The advantage here is improved ergonomics from the C side; e.g.
we can directly pass C strings for values and have the variant
allocated internally.

But more importantly, this is prep for oxidizing things here because
we can pass `GVariantDict` to/from Rust easily, not so with
`GHashTable`.
  • Loading branch information
@cgwalters
cgwalters committed Jun 10, 2022
1 parent d44d81d commit dbc756e
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 55 deletions.
64 changes: 30 additions & 34 deletions src/app/rpmostree-compose-builtin-tree.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,8 @@ typedef struct
{
RpmOstreeContext *corectx;
GFile *treefile_path;
GHashTable *metadata;
GHashTable *detached_metadata;
GVariantDict *metadata;
GVariantDict *detached_metadata;
gboolean failed;
GLnxTmpDir workdir_tmp;
int workdir_dfd;
Expand All @@ -181,7 +181,8 @@ rpm_ostree_tree_compose_context_free (RpmOstreeTreeComposeContext *ctx)
{
g_clear_object (&ctx->corectx);
g_clear_object (&ctx->treefile_path);
g_clear_pointer (&ctx->metadata, g_hash_table_unref);
g_clear_pointer (&ctx->metadata, g_variant_dict_unref);
g_clear_pointer (&ctx->detached_metadata, g_variant_dict_unref);
ctx->treefile_rs.~optional ();
/* Only close workdir_dfd if it's not owned by the tmpdir */
if (!ctx->workdir_tmp.initialized)
Expand Down Expand Up @@ -515,17 +516,17 @@ install_packages (RpmOstreeTreeComposeContext *self, gboolean *out_unmodified,
}

static gboolean
parse_metadata_keyvalue_strings (char **strings, GHashTable *metadata_hash, GError **error)
parse_metadata_keyvalue_strings (char **strings, GVariantDict *metadata_hash, GError **error)
{
for (char **iter = strings; *iter; iter++)
{
const char *s = *iter;
const char *eq = strchr (s, '=');
if (!eq)
return glnx_throw (error, "Missing '=' in KEY=VALUE metadata '%s'", s);
g_autofree char *k = g_strndup (s, eq - s);

g_hash_table_insert (metadata_hash, g_strndup (s, eq - s),
g_variant_ref_sink (g_variant_new_string (eq + 1)));
g_variant_dict_insert (metadata_hash, k, "s", eq + 1);
}

return TRUE;
Expand Down Expand Up @@ -762,10 +763,8 @@ rpm_ostree_compose_context_new (const char *treefile_pathstr, const char *basear

self->treefile = json_node_get_object (self->treefile_rootval);

self->metadata
= g_hash_table_new_full (g_str_hash, g_str_equal, g_free, (GDestroyNotify)g_variant_unref);
self->detached_metadata
= g_hash_table_new_full (g_str_hash, g_str_equal, g_free, (GDestroyNotify)g_variant_unref);
self->metadata = g_variant_dict_new (NULL);
self->detached_metadata = g_variant_dict_new (NULL);

/* metadata from the treefile itself has the lowest priority */
JsonNode *add_commit_metadata_node
Expand Down Expand Up @@ -805,8 +804,8 @@ rpm_ostree_compose_context_new (const char *treefile_pathstr, const char *basear
{
g_variant_builder_add (&builder, "s", s.c_str ());
}
g_hash_table_insert (self->metadata, g_strdup ("ostree.container-cmd"),
g_variant_ref_sink (g_variant_builder_end (&builder)));
g_variant_dict_insert_value (self->metadata, "ostree.container-cmd",
g_variant_builder_end (&builder));
}

auto layers = (*self->treefile_rs)->get_all_ostree_layers ();
Expand Down Expand Up @@ -836,8 +835,7 @@ inject_advisories (RpmOstreeTreeComposeContext *self, GCancellable *cancellable,
g_autoptr (GVariant) advisories = rpmostree_advisories_variant (yum_sack, pkgs);

if (advisories && g_variant_n_children (advisories) > 0)
g_hash_table_insert (self->metadata, g_strdup ("rpmostree.advisories"),
g_steal_pointer (&advisories));
g_variant_dict_insert_value (self->metadata, "rpmostree.advisories", advisories);

return TRUE;
}
Expand Down Expand Up @@ -918,7 +916,7 @@ impl_install_tree (RpmOstreeTreeComposeContext *self, gboolean *out_changed,
rust::String next_version;
if (json_object_has_member (self->treefile, "automatic-version-prefix") &&
/* let --add-metadata-string=version=... take precedence */
!g_hash_table_contains (self->metadata, OSTREE_COMMIT_META_KEY_VERSION))
!g_variant_dict_contains (self->metadata, OSTREE_COMMIT_META_KEY_VERSION))
{
CXX_TRY_VAR (ver_prefix, (*self->treefile_rs)->require_automatic_version_prefix (), error);
auto ver_suffix = (*self->treefile_rs)->get_automatic_version_suffix ();
Expand All @@ -941,17 +939,15 @@ impl_install_tree (RpmOstreeTreeComposeContext *self, gboolean *out_changed,
last_version ?: ""),
error);
next_version = std::move (next_versionv);
g_hash_table_insert (self->metadata, g_strdup (OSTREE_COMMIT_META_KEY_VERSION),
g_variant_ref_sink (g_variant_new_string (next_version.c_str ())));
g_variant_dict_insert (self->metadata, OSTREE_COMMIT_META_KEY_VERSION, "s",
next_version.c_str ());
}
else
{
gpointer vp = g_hash_table_lookup (self->metadata, OSTREE_COMMIT_META_KEY_VERSION);
auto v = static_cast<GVariant *> (vp);
if (v)
const char *version = NULL;
if (g_variant_dict_lookup (self->metadata, "&s", OSTREE_COMMIT_META_KEY_VERSION, &version))
{
g_assert (g_variant_is_of_type (v, G_VARIANT_TYPE_STRING));
next_version = rust::String (static_cast<char *> (g_variant_dup_string (v, NULL)));
next_version = rust::String (version);
}
}

Expand Down Expand Up @@ -991,24 +987,26 @@ impl_install_tree (RpmOstreeTreeComposeContext *self, gboolean *out_changed,
= (*self->treefile_rs)->get_repo_metadata_target ();
if (repomd_target == rpmostreecxx::RepoMetadataTarget::Inline)
{
if (!g_hash_table_contains (self->metadata, "rpmostree.rpmmd-repos"))
if (!g_variant_dict_contains (self->metadata, "rpmostree.rpmmd-repos"))
{
g_hash_table_insert (self->metadata, g_strdup ("rpmostree.rpmmd-repos"),
rpmostree_context_get_rpmmd_repo_commit_metadata (self->corectx));
g_variant_dict_insert_value (
self->metadata, "rpmostree.rpmmd-repos",
rpmostree_context_get_rpmmd_repo_commit_metadata (self->corectx));
}
}
else if (repomd_target == rpmostreecxx::RepoMetadataTarget::Detached)
{
if (!g_hash_table_contains (self->detached_metadata, "rpmostree.rpmmd-repos"))
if (!g_variant_dict_contains (self->detached_metadata, "rpmostree.rpmmd-repos"))
{
g_hash_table_insert (self->detached_metadata, g_strdup ("rpmostree.rpmmd-repos"),
rpmostree_context_get_rpmmd_repo_commit_metadata (self->corectx));
g_variant_dict_insert_value (
self->detached_metadata, "rpmostree.rpmmd-repos",
rpmostree_context_get_rpmmd_repo_commit_metadata (self->corectx));
}
}
else
{
g_hash_table_remove (self->metadata, "rpmostree.rpmmd-repos");
g_hash_table_remove (self->detached_metadata, "rpmostree.rpmmd-repos");
g_variant_dict_remove (self->metadata, "rpmostree.rpmmd-repos");
g_variant_dict_remove (self->detached_metadata, "rpmostree.rpmmd-repos");
}

if (!inject_advisories (self, cancellable, error))
Expand Down Expand Up @@ -1057,8 +1055,7 @@ impl_install_tree (RpmOstreeTreeComposeContext *self, gboolean *out_changed,
}

/* Insert our input hash */
g_hash_table_replace (self->metadata, g_strdup ("rpmostree.inputhash"),
g_variant_ref_sink (g_variant_new_string (new_inputhash)));
g_variant_dict_insert (self->metadata, "rpmostree.inputhash", "s", new_inputhash);

*out_changed = TRUE;
return TRUE;
Expand Down Expand Up @@ -1120,8 +1117,7 @@ impl_commit_tree (RpmOstreeTreeComposeContext *self, GCancellable *cancellable,
GVariant *v = json_gvariant_deserialize (initramfs_args, "as", error);
if (!v)
return FALSE;
g_hash_table_insert (self->metadata, g_strdup ("rpmostree.initramfs-args"),
g_variant_ref_sink (v));
g_variant_dict_insert_value (self->metadata, "rpmostree.initramfs-args", v);
}

/* Convert metadata hash tables to GVariants */
Expand Down
44 changes: 27 additions & 17 deletions src/app/rpmostree-composeutil.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ rpmostree_composeutil_checksum (HyGoal goal, OstreeRepo *repo, const rpmostreecx
}

gboolean
rpmostree_composeutil_read_json_metadata (JsonNode *root, GHashTable *metadata, GError **error)
rpmostree_composeutil_read_json_metadata (JsonNode *root, GVariantDict *metadata, GError **error)
{
g_autoptr (GVariant) jsonmetav = json_gvariant_deserialize (root, "a{sv}", error);
if (!jsonmetav)
Expand All @@ -76,7 +76,7 @@ rpmostree_composeutil_read_json_metadata (JsonNode *root, GHashTable *metadata,
char *key;
GVariant *value;
while (g_variant_iter_loop (&viter, "{sv}", &key, &value))
g_hash_table_replace (metadata, g_strdup (key), g_variant_ref (value));
g_variant_dict_insert_value (metadata, key, value);
}

return TRUE;
Expand All @@ -86,7 +86,7 @@ rpmostree_composeutil_read_json_metadata (JsonNode *root, GHashTable *metadata,
* to a hash table of a{sv}; suitable for further extension.
*/
gboolean
rpmostree_composeutil_read_json_metadata_from_file (const char *path, GHashTable *metadata,
rpmostree_composeutil_read_json_metadata_from_file (const char *path, GVariantDict *metadata,
GError **error)
{
const char *errprefix = glnx_strjoina ("While parsing JSON file ", path);
Expand All @@ -101,19 +101,31 @@ rpmostree_composeutil_read_json_metadata_from_file (const char *path, GHashTable
}

static GVariantBuilder *
metadata_conversion_start (GHashTable *metadata)
variant_dict_to_builder (GVariantDict *metadata)
{
GVariantBuilder *builder = g_variant_builder_new (G_VARIANT_TYPE ("a{sv}"));

GLNX_HASH_TABLE_FOREACH_KV (metadata, const char *, strkey, GVariant *, v)
g_variant_builder_add (builder, "{sv}", strkey, v);
bool found_value = false;
g_autoptr (GVariant) final_metadata = g_variant_ref_sink (g_variant_dict_end (metadata));
GVariantIter viter;
g_variant_iter_init (&viter, final_metadata);
char *key;
GVariant *value;
g_autoptr (GVariantBuilder) builder = g_variant_builder_new (G_VARIANT_TYPE ("a{sv}"));
while (g_variant_iter_loop (&viter, "{sv}", &key, &value))
{
found_value = true;
g_variant_builder_add (builder, "{sv}", key, value);
}

return builder;
if (found_value)
return util::move_nullify (builder);
return NULL;
}

static GVariant *
metadata_conversion_end (GVariantBuilder *builder)
{
if (!builder)
return NULL;
g_autoptr (GVariant) ret = g_variant_ref_sink (g_variant_builder_end (builder));
/* Canonicalize to big endian, like OSTree does. Without this, any numbers
* we place in the metadata will be unreadable since clients won't know
Expand All @@ -126,9 +138,11 @@ metadata_conversion_end (GVariantBuilder *builder)

/* Convert hash table of metadata into finalized GVariant */
GVariant *
rpmostree_composeutil_finalize_metadata (GHashTable *metadata, int rootfs_dfd, GError **error)
rpmostree_composeutil_finalize_metadata (GVariantDict *metadata, int rootfs_dfd, GError **error)
{
g_autoptr (GVariantBuilder) builder = metadata_conversion_start (metadata);
g_autoptr (GVariantBuilder) builder = variant_dict_to_builder (metadata);
if (!builder)
builder = g_variant_builder_new (G_VARIANT_TYPE ("a{sv}"));

/* include list of packages in rpmdb; this is used client-side for easily previewing
* pending updates. once we only support unified core composes, this can easily be much
Expand All @@ -143,13 +157,9 @@ rpmostree_composeutil_finalize_metadata (GHashTable *metadata, int rootfs_dfd, G

/* Convert hash table of metadata into finalized GVariant */
GVariant *
rpmostree_composeutil_finalize_detached_metadata (GHashTable *detached_metadata)
rpmostree_composeutil_finalize_detached_metadata (GVariantDict *detached_metadata)
{
/* canonicalize empty detached metadata to NULL */
if (g_hash_table_size (detached_metadata) == 0)
return NULL;

g_autoptr (GVariantBuilder) builder = metadata_conversion_start (detached_metadata);
g_autoptr (GVariantBuilder) builder = variant_dict_to_builder (detached_metadata);
return metadata_conversion_end (builder);
}

Expand Down
9 changes: 5 additions & 4 deletions src/app/rpmostree-composeutil.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,16 @@ gboolean rpmostree_composeutil_checksum (HyGoal goal, OstreeRepo *repo,
const rpmostreecxx::Treefile &tf, JsonObject *treefile,
char **out_checksum, GError **error);

gboolean rpmostree_composeutil_read_json_metadata (JsonNode *root, GHashTable *metadata,
gboolean rpmostree_composeutil_read_json_metadata (JsonNode *root, GVariantDict *metadata,
GError **error);
gboolean rpmostree_composeutil_read_json_metadata_from_file (const char *path, GHashTable *metadata,
gboolean rpmostree_composeutil_read_json_metadata_from_file (const char *path,
GVariantDict *metadata,
GError **error);

GVariant *rpmostree_composeutil_finalize_metadata (GHashTable *metadata, int rootfs_dfd,
GVariant *rpmostree_composeutil_finalize_metadata (GVariantDict *metadata, int rootfs_dfd,
GError **error);

GVariant *rpmostree_composeutil_finalize_detached_metadata (GHashTable *detached_metadata);
GVariant *rpmostree_composeutil_finalize_detached_metadata (GVariantDict *detached_metadata);

gboolean rpmostree_composeutil_write_composejson (OstreeRepo *repo, const char *path,
const OstreeRepoTransactionStats *stats,
Expand Down

0 comments on commit dbc756e

Please sign in to comment.