Move varsubst code into Rust, use it in treefile parsing

External tools often want to parse the ref; for example coreos-assembler
currently does so.  Let's ensure `${basearch}` is expanded with
`--print-only` so they can parse that JSON to get the expanded version
reliably.

Implementation note: this is the first Rust code which exposes a
"GLib-like" C API, notably with GHashTable, so we're making more use
of the glib-rs bindings.

Closes: #1653

Closes: #1655
Approved by: jlebon

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;"
+header = "#pragma once\n#include <gio/gio.h>\ntypedef GError RORGError;\ntypedef GHashTable RORGHashTable;"
 trailer = """
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(RORTreefile, ror_treefile_free)
 """

rust/src/treefile.rs

@@ -24,9 +24,11 @@ use c_utf8::CUtf8Buf;
 use openat;
 use serde_json;
 use serde_yaml;
+use std::collections::HashMap;
 use std::io::prelude::*;
 use std::path::Path;
 use std::{collections, fs, io};
+use utils;
 
 const ARCH_X86_64: &'static str = "x86_64";
 const INCLUDE_MAXDEPTH: u32 = 50;
@@ -89,6 +91,16 @@ fn treefile_parse_stream<R: io::Read>(
         }
     };
 
+    // Substitute ${basearch}
+    treefile.treeref = match (arch, treefile.treeref.take()) {
+        (Some(arch), Some(treeref)) => {
+            let mut varsubsts = HashMap::new();
+            varsubsts.insert("basearch".to_string(), arch.to_string());
+            Some(utils::varsubst(&treeref, &varsubsts)?)
+        }
+        (_, v) => v,
+    };
+
     // Special handling for packages, since we allow whitespace within items.
     // We also canonicalize bootstrap_packages to packages here so it's
     // easier to append the arch packages after.
@@ -625,7 +637,7 @@ packages-s390x:
     // This one has "comments" (hence unknown keys)
     static VALID_PRELUDE_JS: &str = r###"
 {
- "ref": "exampleos/x86_64/blah",
+ "ref": "exampleos/${basearch}/blah",
  "comment-packages": "We want baz to enable frobnication",
  "packages": ["foo", "bar", "baz"],
  "packages-x86_64": ["grub2", "grub2-tools"],

rust/src/utils.rs

@@ -16,6 +16,7 @@
  * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
+use std::collections::HashMap;
 use std::io::prelude::*;
 use std::{fs, io};
 use tempfile;
@@ -40,18 +41,99 @@ fn download_url_to_tmpfile(url: &str) -> io::Result<fs::File> {
     Ok(tmpf)
 }
 
+/// Given an input string `s`, replace variables of the form `${foo}` with
+/// values provided in `vars`.  No quoting syntax is available, so it is
+/// not possible to have a literal `${` in the string.
+#[allow(dead_code)]
+pub fn varsubst(instr: &str, vars: &HashMap<String, String>) -> io::Result<String> {
+    let mut buf = instr;
+    let mut s = "".to_string();
+    while buf.len() > 0 {
+        if let Some(start) = buf.find("${") {
+            let (prefix, rest) = buf.split_at(start);
+            let rest = &rest[2..];
+            s.push_str(prefix);
+            if let Some(end) = rest.find("}") {
+                let (varname, remainder) = rest.split_at(end);
+                let remainder = &remainder[1..];
+                if let Some(val) = vars.get(varname) {
+                    s.push_str(val);
+                } else {
+                    return Err(io::Error::new(
+                        io::ErrorKind::InvalidInput,
+                        format!("Unknown variable reference ${{{}}}", varname),
+                    ));
+                }
+                buf = remainder;
+            } else {
+                return Err(io::Error::new(
+                    io::ErrorKind::InvalidInput,
+                    format!("Unclosed variable"),
+                ));
+            }
+        } else {
+            s.push_str(buf);
+            break;
+        }
+    }
+    return Ok(s);
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn subs() -> HashMap<String, String> {
+        let mut h = HashMap::new();
+        h.insert("basearch".to_string(), "ppc64le".to_string());
+        h.insert("osvendor".to_string(), "fedora".to_string());
+        h
+    }
+
+    fn test_noop(s: &str, subs: &HashMap<String, String>) {
+        let r = varsubst(s, subs).unwrap();
+        assert_eq!(r, s);
+    }
+
+    #[test]
+    fn varsubst_noop() {
+        let subs = subs();
+        test_noop("", &subs);
+        test_noop("foo bar baz", &subs);
+        test_noop("}", &subs);
+        test_noop("$} blah$ }}", &subs);
+    }
+
+    #[test]
+    fn varsubsts() {
+        let subs = subs();
+        let r = varsubst(
+            "ostree/${osvendor}/${basearch}/blah/${basearch}/whee",
+            &subs,
+        ).unwrap();
+        assert_eq!(r, "ostree/fedora/ppc64le/blah/ppc64le/whee");
+        let r = varsubst("${osvendor}${basearch}", &subs).unwrap();
+        assert_eq!(r, "fedorappc64le");
+        let r = varsubst("${osvendor}", &subs).unwrap();
+        assert_eq!(r, "fedora");
+    }
+}
+
 mod ffi {
     use super::*;
+    use ffiutil::*;
+    use glib;
     use glib_sys;
     use libc;
+    use std::ffi::CString;
     use std::os::unix::io::IntoRawFd;
+    use std::ptr;
 
     #[no_mangle]
     pub extern "C" fn ror_download_to_fd(
         url: *const libc::c_char,
         gerror: *mut *mut glib_sys::GError,
     ) -> libc::c_int {
-        use ffiutil::*;
         let url = str_from_nullable(url).unwrap();
         match download_url_to_tmpfile(url) {
             Ok(f) => f.into_raw_fd(),
@@ -61,5 +143,23 @@ mod ffi {
             }
         }
     }
+
+    #[no_mangle]
+    pub extern "C" fn ror_util_varsubst(
+        s: *const libc::c_char,
+        h: *mut glib_sys::GHashTable,
+        gerror: *mut *mut glib_sys::GError,
+    ) -> *mut libc::c_char {
+        let s = str_from_nullable(s).unwrap();
+        let h_rs: HashMap<String, String> =
+            unsafe { glib::translate::FromGlibPtrContainer::from_glib_none(h) };
+        match varsubst(s, &h_rs) {
+            Ok(s) => CString::new(s).unwrap().into_raw(),
+            Err(e) => {
+                error_to_glib(&e, gerror);
+                ptr::null_mut()
+            }
+        }
+    }
 }
 pub use self::ffi::*;

src/libpriv/rpmostree-util.c

@@ -28,6 +28,7 @@
 #include <systemd/sd-journal.h>
 
 #include "rpmostree-util.h"
+#include "rpmostree-rust.h"
 #include "rpmostree-origin.h"
 #include "rpmostree-output.h"
 #include "libsd-locale-util.h"
@@ -69,49 +70,7 @@ _rpmostree_varsubst_string (const char *instr,
                             GHashTable *substitutions,
                             GError **error)
 {
-  const char *s;
-  const char *p;
-  /* Acts as a reusable buffer space */
-  g_autoptr(GString) varnamebuf = g_string_new ("");
-  g_autoptr(GString) result = g_string_new ("");
-
-  s = instr;
-  while ((p = strstr (s, "${")) != NULL)
-    {
-      const char *varstart = p + 2;
-      const char *varend = strchr (varstart, '}');
-      const char *value;
-      if (!varend)
-        return glnx_null_throw (error, "Unclosed variable reference in %s starting at %u bytes",
-                                instr, (guint)(p - instr));
-
-      /* Append leading bytes */
-      g_string_append_len (result, s, p - s);
-
-      /* Get a NUL-terminated copy of the variable name */
-      g_string_truncate (varnamebuf, 0);
-      g_string_append_len (varnamebuf, varstart, varend - varstart);
-
-      value = g_hash_table_lookup (substitutions, varnamebuf->str);
-      if (!value)
-        return glnx_null_throw (error, "Unknown variable reference ${%s} in %s",
-                                varnamebuf->str, instr);
-      /* Append the replaced value */
-      g_string_append (result, value);
-
-      /* On to the next */
-      s = varend+1;
-    }
-
-  /* Append trailing bytes */
-  if (s != instr)
-    {
-      g_string_append (result, s);
-      /* Steal the C string, NULL out the GString since we freed it */
-      return g_string_free (g_steal_pointer (&result), FALSE);
-    }
-  else
-    return g_strdup (instr);
+  return ror_util_varsubst (instr, substitutions, error);
 }
 
 gboolean

tests/compose-tests/test-basic-unified.sh

@@ -6,10 +6,13 @@ dn=$(cd $(dirname $0) && pwd)
 . ${dn}/libcomposetest.sh
 
 prepare_compose_test "basic-unified"
-# Test --print-only, currently requires --repo
+# Test --print-only, currently requires --repo.  We also
+# just in this test (for now) use ${basearch} to test substitution.
+pysetjsonmember "ref" '"fedora/stable/${basearch}/basic-unified"'
 rpm-ostree compose tree --repo=${repobuild} --print-only ${treefile} > treefile.json
 # Verify it's valid JSON
 jq -r .ref < treefile.json > ref.txt
+# Test substitution of ${basearch}
 assert_file_has_content_literal ref.txt "${treeref}"
 # Test metadata json with objects, arrays, numbers
 cat > metadata.json <<EOF