lockfile: Switch to writing it from Rust

I wanted to modify the lockfile specification, but then remembered that
it currently lives in two places right now: once on the Rust side where
it's deserialized, and once more on the C side where it's serialized.

If we could write the lockfile from the Rust side, then we wouldn't have
to deal with the `GVariantBuild` and `json-glib` goop, and instead
we could consistently use serde against the same struct for both
serialization and deserialization.

But there isn't an easy way to do this given that the state to be
serialized is intrinsically linked to libdnf.

So this patch takes the next step in our oxidation process by adding a
minimal `libdnf_sys` module which allows us to call `libdnf` functions
from Rust. This is not the prettiest code I've written, and there's
definitely some polishing that could be done. But I think overall it's a
move in the right general direction: as we oxidize more things, we'll at
some point *have* to integrate more tightly with the C side in a
bidirectional way, instead of the "one-way" approach we've been using so
far.

For this patch specifically, in exchange we get a unique source of truth
for the lockfile spec, just like the treefile, and we drop a lot of C
code in the process.

Closes: #1867
Approved by: cgwalters

rust/cbindgen.toml

@@ -1,6 +1,6 @@
 autogen_warning = "/* Warning, this file is autogenerated by cbindgen. Don't modify this manually. */"
 language = "C"
-header = "#pragma once\n#include <gio/gio.h>\ntypedef GError RORGError;\ntypedef GHashTable RORGHashTable;"
+header = "#pragma once\n#include <gio/gio.h>\ntypedef GError RORGError;\ntypedef GHashTable RORGHashTable;\ntypedef GPtrArray RORGPtrArray;"
 trailer = """
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(RORTreefile, ror_treefile_free)
 """

rust/src/ffiutil.rs

@@ -54,6 +54,15 @@ use std::ptr;
  * outlive the function call.
  */
 
+/// Convert a C (UTF-8) string to a &str; will panic
+/// if it is NULL or isn't valid UTF-8.  Note the lifetime of
+/// the return value must be <= the pointer.
+pub(crate) fn ffi_view_str<'a>(s: *const libc::c_char) -> &'a str {
+    assert!(!s.is_null());
+    let s = unsafe { CStr::from_ptr(s) };
+    s.to_str().expect("ffi_view_str: valid utf-8")
+}
+
 /// Convert a C (UTF-8) string to a &str; will panic
 /// if it isn't valid UTF-8.  Note the lifetime of
 /// the return value must be <= the pointer.
@@ -61,8 +70,7 @@ pub(crate) fn ffi_view_nullable_str<'a>(s: *const libc::c_char) -> Option<&'a st
     if s.is_null() {
         None
     } else {
-        let s = unsafe { CStr::from_ptr(s) };
-        Some(s.to_str().expect("ffi_view_nullable_str: valid utf-8"))
+        Some(ffi_view_str(s))
     }
 }
 
@@ -86,6 +94,21 @@ pub(crate) fn ffi_view_os_str<'a>(s: *const libc::c_char) -> &'a OsStr {
     OsStr::from_bytes(ffi_view_bytestring(s))
 }
 
+/// Transform a GPtrArray to a Vec. There's no converter in glib yet to do this. See related
+/// discussions in: https://github.com/gtk-rs/glib/pull/482
+pub(crate) fn ffi_ptr_array_to_vec<T>(a: *mut glib_sys::GPtrArray) -> Vec<*mut T> {
+    assert!(!a.is_null());
+
+    let n = unsafe { (*a).len } as usize;
+    let mut v = Vec::with_capacity(n);
+    unsafe {
+        for i in 0..n {
+            v.push(ptr::read((*a).pdata.add(i as usize)) as *mut T);
+        }
+    }
+    v
+}
+
 // View `fd` as an `openat::Dir` instance.  Lifetime of return value
 // must be less than or equal to that of parameter.
 pub(crate) fn ffi_view_openat_dir(fd: libc::c_int) -> openat::Dir {

rust/src/lib.rs

@@ -18,6 +18,7 @@
 
 // pub(crate) utilities
 mod ffiutil;
+mod libdnf_sys;
 mod openat_utils;
 
 mod composepost;

rust/src/libdnf_sys.rs

@@ -0,0 +1,39 @@
+/*
+ * Copyright (C) 2019 Red Hat, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+use libc;
+use glib_sys;
+
+/* This is an experiment in calling libdnf functions from Rust. Really, it'd be more sustainable to
+ * generate a "libdnf-sys" from SWIG (though it doesn't look like that's supported yet
+ * https://github.com/swig/swig/issues/1468) or at least `bindgen` for the strict C parts of the
+ * API. For now, I just took the shortcut of manually defining a tiny subset we care about. */
+
+#[repr(C)]
+pub(crate) struct DnfPackage(libc::c_void);
+
+#[allow(dead_code)]
+extern {
+    pub(crate) fn dnf_package_get_nevra(package: *mut DnfPackage) -> *const libc::c_char;
+}
+
+/* And some helper rpm-ostree C functions to deal with libdnf stuff. These are prime candidates for
+ * oxidation since it makes e.g. interacting with strings less efficient. */
+extern {
+    pub(crate) fn rpmostree_get_repodata_chksum_repr(package: *mut DnfPackage, chksum: *mut *mut libc::c_char, gerror: *mut *mut glib_sys::GError) -> libc::c_int;
+}

rust/src/lockfile.rs

@@ -135,6 +135,7 @@ mod ffi {
     use std::ptr;
 
     use crate::ffiutil::*;
+    use crate::libdnf_sys::*;
 
     #[no_mangle]
     pub extern "C" fn ror_lockfile_read(
@@ -160,5 +161,38 @@ mod ffi {
             }
         }
     }
+
+    #[no_mangle]
+    pub extern "C" fn ror_lockfile_write(
+        filename: *const libc::c_char,
+        packages: *mut glib_sys::GPtrArray,
+        gerror: *mut *mut glib_sys::GError,
+    ) -> libc::c_int {
+        let filename = ffi_view_os_str(filename);
+        let packages: Vec<*mut DnfPackage> = ffi_ptr_array_to_vec(packages);
+
+        let mut lockfile = LockfileConfig {
+            packages: Vec::new(),
+        };
+
+        for pkg in packages {
+            let nevra = ffi_new_string(unsafe { dnf_package_get_nevra(pkg) });
+
+            let mut chksum: *mut libc::c_char = ptr::null_mut();
+            let r = unsafe { rpmostree_get_repodata_chksum_repr(pkg, &mut chksum, gerror) };
+            if r == 0 {
+                return r;
+            }
+
+            lockfile.packages.push((nevra, ffi_new_string(chksum)));
+
+            // forgive me for this sin... need to oxidize chksum_repr()
+            unsafe { glib_sys::g_free(chksum as *mut libc::c_void) };
+        }
+
+        int_glib_error(utils::write_file(filename, |w| {
+            serde_json::to_writer_pretty(w, &lockfile).map_err(failure::Error::from)
+        }), gerror)
+    }
 }
 pub use self::ffi::*;

rust/src/utils.rs

@@ -44,10 +44,36 @@ fn download_url_to_tmpfile(url: &str) -> Fallible<fs::File> {
     Ok(tmpf)
 }
 
-/// Open file and provide context containing filename on failures.
+/// Open file for reading and provide context containing filename on failures.
 pub fn open_file<P: AsRef<Path>>(filename: P) -> Fallible<fs::File> {
     return Ok(fs::File::open(filename.as_ref())
-        .with_context(|e| format!("Can't open file {:?}: {}", filename.as_ref().display(), e))?);
+        .with_context(|e| format!("Can't open file {:?} for reading: {}", filename.as_ref().display(), e))?);
+}
+
+/// Open file for writing and provide context containing filename on failures.
+pub fn create_file<P: AsRef<Path>>(filename: P) -> Fallible<fs::File> {
+    return Ok(fs::File::create(filename.as_ref())
+        .with_context(|e| format!("Can't open file {:?} for writing: {}", filename.as_ref().display(), e))?);
+}
+
+/// Open file for writing, passes a Writer to a closure, and closes the file, with O_TMPFILE
+/// semantics.
+pub fn write_file<P, F>(
+    filename: P,
+    f: F
+) -> Fallible<()>
+where
+    P: AsRef<Path>,
+    F: Fn(&mut io::BufWriter<&mut fs::File>) -> Fallible<()>,
+{
+    // XXX: enhance with tempfile + linkat + rename dance
+    let mut file = create_file(filename)?;
+    {
+        let mut w = io::BufWriter::new(&mut file);
+        f(&mut w)?;
+    }
+    file.sync_all()?;
+    Ok(())
 }
 
 /// Given an input string `s`, replace variables of the form `${foo}` with

src/app/rpmostree-compose-builtin-tree.c

@@ -345,9 +345,14 @@ install_packages (RpmOstreeTreeComposeContext  *self,
 
   rpmostree_print_transaction (dnfctx);
 
-  if (opt_write_lockfile &&
-      !rpmostree_composeutil_write_lockfilejson (self->corectx, opt_write_lockfile, error))
-      return FALSE;
+  if (opt_write_lockfile)
+    {
+      g_autoptr(GPtrArray) pkgs = rpmostree_context_get_packages (self->corectx);
+      g_assert (pkgs);
+
+      if (!ror_lockfile_write (opt_write_lockfile, pkgs, error))
+        return FALSE;
+    }
 
   /* FIXME - just do a depsolve here before we compute download requirements */
   g_autofree char *ret_new_inputhash = NULL;

src/app/rpmostree-composeutil.c

@@ -466,55 +466,3 @@ rpmostree_composeutil_write_composejson (OstreeRepo  *repo,
 
   return TRUE;
 }
-
-/* Implements --write-lockfile-to.
- * If `path` is NULL, this is a NO-OP.
- */
-gboolean
-rpmostree_composeutil_write_lockfilejson (RpmOstreeContext  *ctx,
-                                          const char        *path,
-                                          GError           **error)
-{
-  if (!path)
-    return TRUE;
-
-  g_autoptr(GPtrArray) pkgs = rpmostree_context_get_packages (ctx);
-  g_assert (pkgs);
-
-  g_auto(GVariantBuilder) builder;
-  g_variant_builder_init (&builder, G_VARIANT_TYPE ("a{sv}"));
-
-  g_autoptr(GVariant) pkglist_v = NULL;
-  if (!rpmostree_create_pkglist_variant (pkgs, &pkglist_v, NULL, error))
-    return FALSE;
-  g_variant_builder_add (&builder, "{sv}", "packages", pkglist_v);
-
-  g_autoptr(GVariant) lock_v = g_variant_builder_end (&builder);
-  g_assert (lock_v != NULL);
-  g_autoptr(JsonNode) lock_node = json_gvariant_serialize (lock_v);
-  g_assert (lock_node != NULL);
-  glnx_unref_object JsonGenerator *generator = json_generator_new ();
-  json_generator_set_root (generator, lock_node);
-  /* Let's make it somewhat introspectable by humans */
-  json_generator_set_pretty (generator, TRUE);
-
-  char *dnbuf = strdupa (path);
-  const char *dn = dirname (dnbuf);
-  g_auto(GLnxTmpfile) tmpf = { 0, };
-  if (!glnx_open_tmpfile_linkable_at (AT_FDCWD, dn, O_WRONLY | O_CLOEXEC, &tmpf, error))
-    return FALSE;
-  g_autoptr(GOutputStream) out = g_unix_output_stream_new (tmpf.fd, FALSE);
-  /* See also similar code in status.c */
-  if (json_generator_to_stream (generator, out, NULL, error) <= 0 ||
-      (error != NULL && *error != NULL))
-    return FALSE;
-
-  /* World readable to match --write-commitid-to which uses mask */
-  if (!glnx_fchmod (tmpf.fd, 0644, error))
-    return FALSE;
-
-  if (!glnx_link_tmpfile_at (&tmpf, GLNX_LINK_TMPFILE_REPLACE, AT_FDCWD, path, error))
-    return FALSE;
-
-  return TRUE;
-}

src/app/rpmostree-composeutil.h

@@ -73,10 +73,4 @@ rpmostree_composeutil_write_composejson (OstreeRepo  *repo,
                                          GVariantBuilder *builder,
                                          GError    **error);
 
-gboolean
-rpmostree_composeutil_write_lockfilejson (RpmOstreeContext  *ctx,
-                                          const char        *path,
-                                          GError           **error);
-
 G_END_DECLS
-

src/libpriv/rpmostree-rpm-util.c

@@ -1257,34 +1257,6 @@ rpmostree_create_rpmdb_pkglist_variant (int              dfd,
   return TRUE;
 }
 
-gboolean
-rpmostree_create_pkglist_variant (GPtrArray     *pkglist,
-                                  GVariant     **out_variant,
-                                  GCancellable  *cancellable,
-                                  GError       **error)
-{
-  /* we insert it sorted here so it can efficiently be searched on retrieval */
-  g_ptr_array_sort (pkglist, (GCompareFunc)rpmostree_pkg_array_compare);
-
-  GVariantBuilder pkglist_v_builder;
-  g_variant_builder_init (&pkglist_v_builder, G_VARIANT_TYPE ("a(ss)"));
-  for (guint i = 0; i < pkglist->len; i++)
-    {
-      DnfPackage *pkg = pkglist->pdata[i];
-      g_autofree gchar *repodata_chksum = NULL;
-
-      if (!rpmostree_get_repodata_chksum_repr (pkg, &repodata_chksum, error))
-        return FALSE;
-
-      g_variant_builder_add (&pkglist_v_builder, "(ss)",
-                             dnf_package_get_nevra (pkg),
-                             repodata_chksum);
-    }
-
-  *out_variant = g_variant_ref_sink (g_variant_builder_end (&pkglist_v_builder));
-  return TRUE;
-}
-
 /* Simple wrapper around hy_split_nevra() that adds allow-none and GError convention */
 gboolean
 rpmostree_decompose_nevra (const char  *nevra,

src/libpriv/rpmostree-rpm-util.h

@@ -178,6 +178,7 @@ rpmostree_custom_nevra_strdup (const char *name,
 char *
 rpmostree_header_custom_nevra_strdup (Header h, RpmOstreePkgNevraFlags flags);
 
+/* NB: this function is exposed to Rust */
 gboolean
 rpmostree_get_repodata_chksum_repr (DnfPackage *pkg,
                                     char      **out_chksum_repr,
@@ -210,12 +211,6 @@ rpmostree_create_rpmdb_pkglist_variant (int              dfd,
                                         GCancellable    *cancellable,
                                         GError         **error);
 
-gboolean
-rpmostree_create_pkglist_variant (GPtrArray     *pkglist,
-                                  GVariant     **out_variant,
-                                  GCancellable  *cancellable,
-                                  GError       **error);
-
 char * rpmostree_get_cache_branch_for_n_evr_a (const char *name, const char *evr, const char *arch);
 char *rpmostree_get_cache_branch_header (Header hdr);
 char *rpmostree_get_rojig_branch_header (Header hdr);