From b640892f0401d359b55e674ad0ba1102ada70e26 Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Tue, 9 Feb 2021 18:07:37 -0500 Subject: [PATCH] libdnf-sys: Drop C API, replace with cxx.rs bridge Right now, we're using libdnf APIs from Rust via hand-crafted `extern C` interfaces, which is extra dangerous because there is no signature checking that happens at compile-time. Until either we can automate libdnf bindings or use its C++ API directly via cxx.rs, let's do some basic wrapping in C++ ourselves and use libdnf through that API only instead. That gives us a lot more confidence and makes the libdnf API feel more natural to use in Rust. --- Cargo.lock | 52 +++++++++++++++++++++++++++++++++- rust/libdnf-sys/Cargo.toml | 4 ++- rust/libdnf-sys/build.rs | 19 +++++++++++-- rust/libdnf-sys/cxx/libdnf.cxx | 44 ++++++++++++++++++++++++++++ rust/libdnf-sys/cxx/libdnf.hxx | 37 ++++++++++++++++++++++++ rust/libdnf-sys/lib.rs | 41 ++++++++++++++++++++------- rust/src/lockfile.rs | 14 +++++---- 7 files changed, 189 insertions(+), 22 deletions(-) create mode 100644 rust/libdnf-sys/cxx/libdnf.cxx create mode 100644 rust/libdnf-sys/cxx/libdnf.hxx diff --git a/Cargo.lock b/Cargo.lock index c2433586..37aa4c77 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -140,6 +140,16 @@ dependencies = [ "cc", ] +[[package]] +name = "codespan-reporting" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c6ce42b8998a383572e0a802d859b1f00c79b7b7474e62fff88ee5c2845d9c13" +dependencies = [ + "termcolor", + "unicode-width", +] + [[package]] name = "console" version = "0.14.0" @@ -259,6 +269,21 @@ dependencies = [ "link-cplusplus", ] +[[package]] +name = "cxx-build" +version = "1.0.31" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "69c0ba9a6c36216f1cd3cb6aedc605ca22feb9fc64d9a1b76099a9e368fa553d" +dependencies = [ + "cc", + "codespan-reporting", + "lazy_static", + "proc-macro2", + "quote", + "scratch", + "syn", +] + [[package]] name = "cxxbridge-flags" version = "1.0.30" @@ -640,7 +665,8 @@ version = "0.1.0" dependencies = [ "anyhow", "cmake", - "libc", + "cxx", + "cxx-build", "system-deps 2.0.3", ] @@ -1246,6 +1272,12 @@ version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d29ab0c6d3fc0ee92fe66e2d99f700eab17a8d57d1c1d3b748380fb20baa78cd" +[[package]] +name = "scratch" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7e114536316b51a5aa7a0e59fc49661fd263c5507dd08bd28de052e57626ce69" + [[package]] name = "serde" version = "1.0.123" @@ -1458,6 +1490,15 @@ dependencies = [ "winapi", ] +[[package]] +name = "termcolor" +version = "1.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2dfed899f0eb03f32ee8c6a0aabdb8a7949659e3466561fc0adf54e26d88c5f4" +dependencies = [ + "winapi-util", +] + [[package]] name = "terminal_size" version = "0.1.16" @@ -1605,6 +1646,15 @@ version = "0.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ac3b87c63620426dd9b991e5ce0329eff545bccbbb34f3be09ff6fb6ab51b7b6" +[[package]] +name = "winapi-util" +version = "0.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "70ec6ce85bb158151cae5e5c87f95a8e97d2c0c4b001223f33a334e3ce5de178" +dependencies = [ + "winapi", +] + [[package]] name = "winapi-x86_64-pc-windows-gnu" version = "0.4.0" diff --git a/rust/libdnf-sys/Cargo.toml b/rust/libdnf-sys/Cargo.toml index 9db2b440..67641e4c 100644 --- a/rust/libdnf-sys/Cargo.toml +++ b/rust/libdnf-sys/Cargo.toml @@ -6,7 +6,7 @@ edition = "2018" links = "dnf" [dependencies] -libc = "0.2" +cxx = "1.0.30" [lib] name = "libdnf_sys" @@ -16,6 +16,7 @@ path = "lib.rs" cmake = "0.1.45" system-deps = "2.0" anyhow = "1.0" +cxx-build = "1.0.30" # This currently needs to duplicate the libraries from libdnf [package.metadata.system-deps] @@ -30,3 +31,4 @@ libcurl = "7" sqlite3 = "3" modulemd = { name = "modulemd-2.0", version = "2" } jsonc = { name = "json-c", version = "0" } +glib = { name = "glib-2.0", version = "2" } diff --git a/rust/libdnf-sys/build.rs b/rust/libdnf-sys/build.rs index 7b281f5d..3cc46121 100644 --- a/rust/libdnf-sys/build.rs +++ b/rust/libdnf-sys/build.rs @@ -1,9 +1,10 @@ use anyhow::Result; fn main() -> Result<()> { - system_deps::Config::new().probe()?; - use cmake::Config; - let libdnf = Config::new("../../libdnf") + let libs = system_deps::Config::new().probe()?; + + // first, the submodule proper + let libdnf = cmake::Config::new("../../libdnf") // Needed for hardened builds .cxxflag("-fPIC") // I picked /usr/libexec/rpm-ostree just because we need an @@ -34,5 +35,17 @@ fn main() -> Result<()> { libdnf.display() ); println!("cargo:rustc-link-lib=static=dnf"); + + // now, our thin cxx.rs bridge wrapper + let mut libdnfcxx = cxx_build::bridge("lib.rs"); + libdnfcxx + .file("cxx/libdnf.cxx") + .flag("-std=c++17") + .include("cxx") // this is needed for cxx.rs' `include!("libdnf.hxx")` to work + .include("../../libdnf"); + // until https://github.com/gdesmott/system-deps/pull/32 + libdnfcxx.includes(libs.iter().flat_map(|lib| lib.1.include_paths.iter())); + libdnfcxx.compile("libdnfcxx.a"); + Ok(()) } diff --git a/rust/libdnf-sys/cxx/libdnf.cxx b/rust/libdnf-sys/cxx/libdnf.cxx new file mode 100644 index 00000000..4a138bdb --- /dev/null +++ b/rust/libdnf-sys/cxx/libdnf.cxx @@ -0,0 +1,44 @@ +/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- + * + * Copyright (C) 2021 Red Hat, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License as published + * by the Free Software Foundation; either version 2 of the licence or (at + * your option) any later version. + * + * This library 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 + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General + * Public License along with this library; if not, write to the + * Free Software Foundation, Inc., 59 Temple Place, Suite 330, + * Boston, MA 02111-1307, USA. + */ + +#include "libdnf.hxx" + +namespace dnfcxx { + // XXX: use macro to dedupe + rust::String dnf_package_get_nevra(DnfPackage &pkg) { + return rust::String(::dnf_package_get_nevra(&pkg)); + } + rust::String dnf_package_get_name(DnfPackage &pkg) { + return rust::String(::dnf_package_get_name(&pkg)); + } + rust::String dnf_package_get_evr(DnfPackage &pkg) { + return rust::String(::dnf_package_get_evr(&pkg)); + } + rust::String dnf_package_get_arch(DnfPackage &pkg) { + return rust::String(::dnf_package_get_arch(&pkg)); + } + + rust::String dnf_repo_get_id(DnfRepo &repo) { + return rust::String(::dnf_repo_get_id(&repo)); + } + guint64 dnf_repo_get_timestamp_generated(DnfRepo &repo) { + return ::dnf_repo_get_timestamp_generated(&repo); + } +} diff --git a/rust/libdnf-sys/cxx/libdnf.hxx b/rust/libdnf-sys/cxx/libdnf.hxx new file mode 100644 index 00000000..5d9f7f73 --- /dev/null +++ b/rust/libdnf-sys/cxx/libdnf.hxx @@ -0,0 +1,37 @@ +/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- + * + * Copyright (C) 2021 Red Hat, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License as published + * by the Free Software Foundation; either version 2 of the licence or (at + * your option) any later version. + * + * This library 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 + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General + * Public License along with this library; if not, write to the + * Free Software Foundation, Inc., 59 Temple Place, Suite 330, + * Boston, MA 02111-1307, USA. + */ + +#pragma once + +#include + +#include "rust/cxx.h" + +namespace dnfcxx { + typedef ::DnfPackage DnfPackage; + rust::String dnf_package_get_nevra(DnfPackage &pkg); + rust::String dnf_package_get_name(DnfPackage &pkg); + rust::String dnf_package_get_evr(DnfPackage &pkg); + rust::String dnf_package_get_arch(DnfPackage &pkg); + + typedef ::DnfRepo DnfRepo; + rust::String dnf_repo_get_id(DnfRepo &repo); + guint64 dnf_repo_get_timestamp_generated(DnfRepo &repo); +} diff --git a/rust/libdnf-sys/lib.rs b/rust/libdnf-sys/lib.rs index b807ff79..07de1bfb 100644 --- a/rust/libdnf-sys/lib.rs +++ b/rust/libdnf-sys/lib.rs @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 OR MIT */ -use libc; +use cxx::{type_id, ExternType}; /* 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 @@ -13,15 +13,34 @@ use libc; // This technique for an uninstantiable/opaque type is used by libgit2-sys at least: // https://github.com/rust-lang/git2-rs/blob/master/libgit2-sys/lib.rs#L51 +// XXX: dedupe with macro pub enum DnfPackage {} -pub enum DnfRepo {} - -extern "C" { - pub fn dnf_package_get_nevra(package: *mut DnfPackage) -> *const libc::c_char; - pub fn dnf_package_get_name(package: *mut DnfPackage) -> *const libc::c_char; - pub fn dnf_package_get_evr(package: *mut DnfPackage) -> *const libc::c_char; - pub fn dnf_package_get_arch(package: *mut DnfPackage) -> *const libc::c_char; - - pub fn dnf_repo_get_id(repo: *mut DnfRepo) -> *const libc::c_char; - pub fn dnf_repo_get_timestamp_generated(repo: *mut DnfRepo) -> u64; +unsafe impl ExternType for DnfPackage { + type Id = type_id!(dnfcxx::DnfPackage); + type Kind = cxx::kind::Trivial; } + +pub enum DnfRepo {} +unsafe impl ExternType for DnfRepo { + type Id = type_id!(dnfcxx::DnfRepo); + type Kind = cxx::kind::Trivial; +} + +#[cxx::bridge(namespace = "dnfcxx")] +pub mod ffi { + unsafe extern "C++" { + include!("libdnf.hxx"); + + type DnfPackage = crate::DnfPackage; + fn dnf_package_get_nevra(pkg: &mut DnfPackage) -> Result; + fn dnf_package_get_name(pkg: &mut DnfPackage) -> Result; + fn dnf_package_get_evr(pkg: &mut DnfPackage) -> Result; + fn dnf_package_get_arch(pkg: &mut DnfPackage) -> Result; + + type DnfRepo = crate::DnfRepo; + fn dnf_repo_get_id(repo: &mut DnfRepo) -> Result; + fn dnf_repo_get_timestamp_generated(repo: &mut DnfRepo) -> Result; + } +} + +pub use ffi::*; diff --git a/rust/src/lockfile.rs b/rust/src/lockfile.rs index 308d7fbf..e4b1fc0a 100644 --- a/rust/src/lockfile.rs +++ b/rust/src/lockfile.rs @@ -193,7 +193,6 @@ mod ffi { use crate::ffiutil::*; use crate::includes::*; use glib::translate::*; - use glib::GString; use libdnf_sys::*; use std::ptr; @@ -251,9 +250,11 @@ mod ffi { }; for pkg in packages { - let name: Borrowed = unsafe { from_glib_borrow(dnf_package_get_name(pkg)) }; - let evr: Borrowed = unsafe { from_glib_borrow(dnf_package_get_evr(pkg)) }; - let arch: Borrowed = unsafe { from_glib_borrow(dnf_package_get_arch(pkg)) }; + let pkg_ref = unsafe { &mut *pkg }; + // XXX: remove unwraps when we move to cxx.rs + let name = dnf_package_get_name(pkg_ref).unwrap(); + let evr = dnf_package_get_evr(pkg_ref).unwrap(); + let arch = dnf_package_get_arch(pkg_ref).unwrap(); let mut chksum: *mut libc::c_char = ptr::null_mut(); let r = unsafe { rpmostree_get_repodata_chksum_repr(pkg, &mut chksum, gerror) }; @@ -281,8 +282,9 @@ mod ffi { .unwrap(); for rpmmd_repo in rpmmd_repos { - let id: String = unsafe { from_glib_none(dnf_repo_get_id(rpmmd_repo)) }; - let generated = unsafe { dnf_repo_get_timestamp_generated(rpmmd_repo) }; + let repo_ref = unsafe { &mut *rpmmd_repo }; + let id = dnf_repo_get_id(repo_ref).unwrap(); + let generated = dnf_repo_get_timestamp_generated(repo_ref).unwrap(); lockfile_repos.insert( id, LockfileRepoMetadata {