From 50b255a8a9d6d875178b889bf72638f8cdacd036 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 1 Nov 2018 19:13:30 +0000 Subject: [PATCH] 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 | 2 +- rust/src/treefile.rs | 14 ++- rust/src/utils.rs | 102 +++++++++++++++++++++- src/libpriv/rpmostree-util.c | 45 +--------- tests/compose-tests/test-basic-unified.sh | 5 +- 5 files changed, 121 insertions(+), 47 deletions(-) diff --git a/rust/cbindgen.toml b/rust/cbindgen.toml index b9ccc328..643d6b5d 100644 --- a/rust/cbindgen.toml +++ b/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 \ntypedef GError RORGError;" +header = "#pragma once\n#include \ntypedef GError RORGError;\ntypedef GHashTable RORGHashTable;" trailer = """ G_DEFINE_AUTOPTR_CLEANUP_FUNC(RORTreefile, ror_treefile_free) """ diff --git a/rust/src/treefile.rs b/rust/src/treefile.rs index d9f3c344..3c042494 100644 --- a/rust/src/treefile.rs +++ b/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( } }; + // 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"], diff --git a/rust/src/utils.rs b/rust/src/utils.rs index c055dd25..15638a0f 100644 --- a/rust/src/utils.rs +++ b/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 { 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) -> io::Result { + 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 { + 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) { + 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 = + 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::*; diff --git a/src/libpriv/rpmostree-util.c b/src/libpriv/rpmostree-util.c index e88cb495..8476585a 100644 --- a/src/libpriv/rpmostree-util.c +++ b/src/libpriv/rpmostree-util.c @@ -28,6 +28,7 @@ #include #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 diff --git a/tests/compose-tests/test-basic-unified.sh b/tests/compose-tests/test-basic-unified.sh index 3e9024f1..c5559ce8 100755 --- a/tests/compose-tests/test-basic-unified.sh +++ b/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 <