Remove ffi_view_str/ffi_new_string functions

Let's just use the GLib translation bits rather than rolling our own;
this applies primarily to `ffi_new_string()`.

However, I think in most cases performance here doesn't
matter enough to have an even more special case that avoids duplicating
the string.  Let's remove the `ffi_view_str()` optimization too
in favor of consistently using GLib translation.

In the future perhaps we should argue for adding a `from_glib_str_unchecked()`
to the upstream GLib bindings.

rust/src/ffiutil.rs

@@ -42,33 +42,6 @@ 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.
-pub(crate) fn ffi_view_nullable_str<'a>(s: *const libc::c_char) -> Option<&'a str> {
-    if s.is_null() {
-        None
-    } else {
-        Some(ffi_view_str(s))
-    }
-}
-
-/// Given a NUL-terminated C string, copy it to an owned
-/// String.  Will panic if the C string is not valid UTF-8.
-pub(crate) fn ffi_new_string(s: *const libc::c_char) -> String {
-    let buf = ffi_view_bytestring(s);
-    String::from_utf8(buf.into()).expect("ffi_new_string: valid utf-8")
-}
-
 /// View a C "bytestring" (NUL terminated) as a Rust byte array.
 /// Panics if `s` is `NULL`.
 pub(crate) fn ffi_view_bytestring<'a>(s: *const libc::c_char) -> &'a [u8] {

rust/src/lockfile.rs

@@ -250,9 +250,9 @@ mod ffi {
         };
 
         for pkg in packages {
-            let name = ffi_new_string(unsafe { dnf_package_get_name(pkg) });
-            let evr = ffi_view_str(unsafe { dnf_package_get_evr(pkg) });
-            let arch = ffi_view_str(unsafe { dnf_package_get_arch(pkg) });
+            let name: String = unsafe { from_glib_none(dnf_package_get_name(pkg)) };
+            let evr: String = unsafe { from_glib_none(dnf_package_get_evr(pkg)) };
+            let arch: String = unsafe { from_glib_none(dnf_package_get_arch(pkg)) };
 
             let mut chksum: *mut libc::c_char = ptr::null_mut();
             let r = unsafe { rpmostree_get_repodata_chksum_repr(pkg, &mut chksum, gerror) };
@@ -264,7 +264,7 @@ mod ffi {
                 name,
                 LockedPackage {
                     evra: format!("{}.{}", evr, arch),
-                    digest: Some(ffi_new_string(chksum)),
+                    digest: Some(unsafe { from_glib_none(chksum) }),
                 },
             );
 
@@ -282,7 +282,7 @@ mod ffi {
             .unwrap();
 
         for rpmmd_repo in rpmmd_repos {
-            let id = ffi_new_string(unsafe { dnf_repo_get_id(rpmmd_repo) });
+            let id: String = unsafe { from_glib_none(dnf_repo_get_id(rpmmd_repo)) };
             let generated = unsafe { dnf_repo_get_timestamp_generated(rpmmd_repo) };
             lockfile_repos.insert(
                 id,

rust/src/progress.rs

@@ -98,9 +98,9 @@ impl ProgressState {
     /// Change the "sub message" which is the indicatif `{message}`. This text
     /// appears after everything else - it's meant for text that changes width
     /// often (otherwise the progress bar would bounce around).
-    fn set_sub_message(&self, msg: Option<&str>) {
+    fn set_sub_message<T: AsRef<str>>(&self, msg: Option<T>) {
         if let Some(ref sub_message) = msg {
-            self.bar.set_message(sub_message)
+            self.bar.set_message(sub_message.as_ref())
         } else {
             self.bar.set_message("");
         }
@@ -113,9 +113,9 @@ impl ProgressState {
     }
 
     /// Clear the progress bar and print a completion message even on non-ttys.
-    fn end(&self, suffix: Option<&str>) {
+    fn end<T: AsRef<str>>(&self, suffix: Option<T>) {
         self.bar.finish_and_clear();
-        let suffix = suffix.unwrap_or("done");
+        let suffix = suffix.as_ref().map(|s| s.as_ref()).unwrap_or("done");
         if self.is_hidden {
             println!("{}", suffix);
         } else {
@@ -155,11 +155,10 @@ mod tests {
 
 mod ffi {
     use super::*;
+    use glib::translate::*;
     use libc;
     use std::sync::MutexGuard;
 
-    use crate::ffiutil::*;
-
     fn assert_empty(m: &MutexGuard<Option<ProgressState>>) {
         if let Some(ref state) = **m {
             panic!("Overwriting task: \"{}\"", state.message)
@@ -168,39 +167,40 @@ mod ffi {
 
     #[no_mangle]
     pub extern "C" fn ror_progress_begin_task(msg: *const libc::c_char) {
-        let msg = ffi_new_string(msg);
+        let msg: String = unsafe { from_glib_none(msg) };
         let mut lock = PROGRESS.lock().unwrap();
         assert_empty(&lock);
         *lock = Some(ProgressState::new(msg, ProgressType::Task));
     }
 
     #[no_mangle]
     pub extern "C" fn ror_progress_begin_n_items(msg: *const libc::c_char, n: libc::c_int) {
-        let msg = ffi_new_string(msg);
+        let msg: String = unsafe { from_glib_none(msg) };
         let mut lock = PROGRESS.lock().unwrap();
         assert_empty(&lock);
         *lock = Some(ProgressState::new(msg, ProgressType::NItems(n as u64)));
     }
 
     #[no_mangle]
     pub extern "C" fn ror_progress_begin_percent(msg: *const libc::c_char) {
-        let msg = ffi_new_string(msg);
+        let msg: String = unsafe { from_glib_none(msg) };
+
         let mut lock = PROGRESS.lock().unwrap();
         assert_empty(&lock);
         *lock = Some(ProgressState::new(msg, ProgressType::Percent));
     }
 
     #[no_mangle]
     pub extern "C" fn ror_progress_set_message(msg: *const libc::c_char) {
-        let msg = ffi_new_string(msg);
+        let msg: String = unsafe { from_glib_none(msg) };
         let mut lock = PROGRESS.lock().unwrap();
         let state = lock.as_mut().expect("progress to update");
         state.set_message(msg);
     }
 
     #[no_mangle]
     pub extern "C" fn ror_progress_set_sub_message(msg: *const libc::c_char) {
-        let msg = ffi_view_nullable_str(msg);
+        let msg: Option<String> = unsafe { from_glib_none(msg) };
         let mut lock = PROGRESS.lock().unwrap();
         let state = lock.as_mut().expect("progress to update");
         state.set_sub_message(msg);
@@ -215,7 +215,7 @@ mod ffi {
 
     #[no_mangle]
     pub extern "C" fn ror_progress_end(suffix: *const libc::c_char) {
-        let suffix = ffi_view_nullable_str(suffix);
+        let suffix: Option<String> = unsafe { from_glib_none(suffix) };
         let mut lock = PROGRESS.lock().unwrap();
         let state = lock.take().expect("progress to end");
         state.end(suffix);

rust/src/treefile.rs

@@ -1410,12 +1410,16 @@ mod ffi {
     ) -> *mut Treefile {
         // Convert arguments
         let filename = ffi_view_os_str(filename);
-        let basearch = ffi_view_nullable_str(basearch);
+        let basearch: Option<String> = unsafe { from_glib_none(basearch) };
         let workdir = ffi_view_openat_dir_option(workdir_dfd);
         // Run code, map error if any, otherwise extract raw pointer, passing
         // ownership back to C.
         ptr_glib_error(
-            Treefile::new_boxed(filename.as_ref(), basearch, workdir),
+            Treefile::new_boxed(
+                filename.as_ref(),
+                basearch.as_ref().map(|s| s.as_str()),
+                workdir,
+            ),
             gerror,
         )
     }

rust/src/utils.rs

@@ -197,22 +197,22 @@ mod tests {
 
 mod ffi {
     use super::*;
+    use crate::ffiutil::*;
     use glib;
+    use glib::translate::*;
     use glib_sys;
     use libc;
     use std::ffi::CString;
     use std::os::unix::io::IntoRawFd;
     use std::ptr;
 
-    use crate::ffiutil::*;
-
     #[no_mangle]
     pub extern "C" fn ror_download_to_fd(
         url: *const libc::c_char,
         gerror: *mut *mut glib_sys::GError,
     ) -> libc::c_int {
-        let url = ffi_view_nullable_str(url).unwrap();
-        match download_url_to_tmpfile(url) {
+        let url: String = unsafe { from_glib_none(url) };
+        match download_url_to_tmpfile(url.as_str()) {
             Ok(f) => f.into_raw_fd(),
             Err(e) => {
                 error_to_glib(&e, gerror);
@@ -227,10 +227,10 @@ mod ffi {
         h: *mut glib_sys::GHashTable,
         gerror: *mut *mut glib_sys::GError,
     ) -> *mut libc::c_char {
-        let s = ffi_view_nullable_str(s).unwrap();
+        let s: String = unsafe { from_glib_none(s) };
         let h_rs: HashMap<String, String> =
             unsafe { glib::translate::FromGlibPtrContainer::from_glib_none(h) };
-        match varsubst(s, &h_rs) {
+        match varsubst(s.as_str(), &h_rs) {
             Ok(s) => CString::new(s).unwrap().into_raw(),
             Err(e) => {
                 error_to_glib(&e, gerror);