From 885eb9ce014e86a0f66f4b99868483b6fcef5f48 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 29 Oct 2020 17:19:47 +0000 Subject: [PATCH] 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 | 27 --------------------------- rust/src/lockfile.rs | 10 +++++----- rust/src/progress.rs | 24 ++++++++++++------------ rust/src/treefile.rs | 8 ++++++-- rust/src/utils.rs | 12 ++++++------ 5 files changed, 29 insertions(+), 52 deletions(-) diff --git a/rust/src/ffiutil.rs b/rust/src/ffiutil.rs index c938feddf2..826c81c9c8 100644 --- a/rust/src/ffiutil.rs +++ b/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] { diff --git a/rust/src/lockfile.rs b/rust/src/lockfile.rs index 608ba82fb3..44a3a7dba2 100644 --- a/rust/src/lockfile.rs +++ b/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, diff --git a/rust/src/progress.rs b/rust/src/progress.rs index dac9ebbdeb..e08026b784 100644 --- a/rust/src/progress.rs +++ b/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>(&self, msg: Option) { 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>(&self, suffix: Option) { 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>) { if let Some(ref state) = **m { panic!("Overwriting task: \"{}\"", state.message) @@ -168,7 +167,7 @@ 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)); @@ -176,7 +175,7 @@ mod ffi { #[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))); @@ -184,7 +183,8 @@ mod ffi { #[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)); @@ -192,7 +192,7 @@ mod ffi { #[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); @@ -200,7 +200,7 @@ mod ffi { #[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 = 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 = unsafe { from_glib_none(suffix) }; let mut lock = PROGRESS.lock().unwrap(); let state = lock.take().expect("progress to end"); state.end(suffix); diff --git a/rust/src/treefile.rs b/rust/src/treefile.rs index 6abfe32373..4ca30eb6ad 100644 --- a/rust/src/treefile.rs +++ b/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 = 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, ) } diff --git a/rust/src/utils.rs b/rust/src/utils.rs index c23df86b0e..a22b81716b 100644 --- a/rust/src/utils.rs +++ b/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 = 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);