Patchwork [2,of,7] rust-cpython: add wrapper around decapsule_make_dirstate_tuple()

login
register
mail settings
Submitter Yuya Nishihara
Date Oct. 13, 2019, 9:47 a.m.
Message ID <00cd8123f876e03a3af2.1570960025@mimosa>
Download mbox | patch
Permalink /patch/42288/
State New
Headers show

Comments

Yuya Nishihara - Oct. 13, 2019, 9:47 a.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1570953317 -32400
#      Sun Oct 13 16:55:17 2019 +0900
# Node ID 00cd8123f876e03a3af2660c6cf4ba1a77b2bee3
# Parent  6ecc771edbad738c634b50846de1943ea7d34d9e
rust-cpython: add wrapper around decapsule_make_dirstate_tuple()

There are a couple of safety issues. First, the returned function pointer
must be unsafe. Second, its return value must be a raw pointer (i.e.
python27_sys::PyObject), not a cpython::PyObject. The wrapper function will
address these issues.

Patch

diff --git a/rust/hg-cpython/src/dirstate.rs b/rust/hg-cpython/src/dirstate.rs
--- a/rust/hg-cpython/src/dirstate.rs
+++ b/rust/hg-cpython/src/dirstate.rs
@@ -45,9 +45,7 @@  type MakeDirstateTupleFn = extern "C" fn
 /// This is largely a copy/paste from cindex.rs, pending the merge of a
 /// `py_capsule_fn!` macro in the rust-cpython project:
 /// https://github.com/dgrunwald/rust-cpython/pull/169
-pub fn decapsule_make_dirstate_tuple(
-    py: Python,
-) -> PyResult<MakeDirstateTupleFn> {
+fn decapsule_make_dirstate_tuple(py: Python) -> PyResult<MakeDirstateTupleFn> {
     unsafe {
         let caps_name = CStr::from_bytes_with_nul_unchecked(
             b"mercurial.cext.parsers.make_dirstate_tuple_CAPI\0",
@@ -60,6 +58,25 @@  pub fn decapsule_make_dirstate_tuple(
     }
 }
 
+pub fn make_dirstate_tuple(
+    py: Python,
+    entry: &DirstateEntry,
+) -> PyResult<PyObject> {
+    let make = decapsule_make_dirstate_tuple(py)?;
+
+    let &DirstateEntry {
+        state,
+        mode,
+        size,
+        mtime,
+    } = entry;
+    // Explicitly go through u8 first, then cast to platform-specific `c_char`
+    // because Into<u8> has a specific implementation while `as c_char` would
+    // just do a naive enum cast.
+    let state_code: u8 = state.into();
+    Ok(make(state_code as c_char, mode, size, mtime))
+}
+
 pub fn extract_dirstate(py: Python, dmap: &PyDict) -> Result<StateMap, PyErr> {
     dmap.items(py)
         .iter()
diff --git a/rust/hg-cpython/src/dirstate/dirstate_map.rs b/rust/hg-cpython/src/dirstate/dirstate_map.rs
--- a/rust/hg-cpython/src/dirstate/dirstate_map.rs
+++ b/rust/hg-cpython/src/dirstate/dirstate_map.rs
@@ -16,11 +16,10 @@  use cpython::{
     exc, ObjectProtocol, PyBool, PyBytes, PyClone, PyDict, PyErr, PyObject,
     PyResult, PyTuple, Python, PythonObject, ToPyObject,
 };
-use libc::c_char;
 
 use crate::{
     dirstate::copymap::{CopyMap, CopyMapItemsIterator, CopyMapKeysIterator},
-    dirstate::{decapsule_make_dirstate_tuple, dirs_multiset::Dirs},
+    dirstate::{dirs_multiset::Dirs, make_dirstate_tuple},
     ref_sharing::{PyLeakedRef, PySharedRefCell},
 };
 use hg::{
@@ -66,15 +65,7 @@  py_class!(pub class DirstateMap |py| {
         let key = key.extract::<PyBytes>(py)?;
         match self.inner_shared(py).borrow().get(HgPath::new(key.data(py))) {
             Some(entry) => {
-                // Explicitly go through u8 first, then cast to
-                // platform-specific `c_char`.
-                let state: u8 = entry.state.into();
-                Ok(Some(decapsule_make_dirstate_tuple(py)?(
-                        state as c_char,
-                        entry.mode,
-                        entry.size,
-                        entry.mtime,
-                    )))
+                Ok(Some(make_dirstate_tuple(py, entry)?))
             },
             None => Ok(default)
         }
@@ -303,15 +294,7 @@  py_class!(pub class DirstateMap |py| {
         let key = HgPath::new(key.data(py));
         match self.inner_shared(py).borrow().get(key) {
             Some(entry) => {
-                // Explicitly go through u8 first, then cast to
-                // platform-specific `c_char`.
-                let state: u8 = entry.state.into();
-                Ok(decapsule_make_dirstate_tuple(py)?(
-                        state as c_char,
-                        entry.mode,
-                        entry.size,
-                        entry.mtime,
-                    ))
+                Ok(make_dirstate_tuple(py, entry)?)
             },
             None => Err(PyErr::new::<exc::KeyError, _>(
                 py,
@@ -483,18 +466,9 @@  impl DirstateMap {
         res: (&HgPathBuf, &DirstateEntry),
     ) -> PyResult<Option<(PyBytes, PyObject)>> {
         let (f, entry) = res;
-
-        // Explicitly go through u8 first, then cast to
-        // platform-specific `c_char`.
-        let state: u8 = entry.state.into();
         Ok(Some((
             PyBytes::new(py, f.as_ref()),
-            decapsule_make_dirstate_tuple(py)?(
-                state as c_char,
-                entry.mode,
-                entry.size,
-                entry.mtime,
-            ),
+            make_dirstate_tuple(py, entry)?,
         )))
     }
 }
diff --git a/rust/hg-cpython/src/parsers.rs b/rust/hg-cpython/src/parsers.rs
--- a/rust/hg-cpython/src/parsers.rs
+++ b/rust/hg-cpython/src/parsers.rs
@@ -15,15 +15,13 @@  use cpython::{
     PythonObject, ToPyObject,
 };
 use hg::{
-    pack_dirstate, parse_dirstate, utils::hg_path::HgPathBuf, DirstateEntry,
+    pack_dirstate, parse_dirstate, utils::hg_path::HgPathBuf,
     DirstatePackError, DirstateParents, DirstateParseError, PARENT_SIZE,
 };
 use std::collections::HashMap;
 use std::convert::TryInto;
 
-use libc::c_char;
-
-use crate::dirstate::{decapsule_make_dirstate_tuple, extract_dirstate};
+use crate::dirstate::{extract_dirstate, make_dirstate_tuple};
 use std::time::Duration;
 
 fn parse_dirstate_wrapper(
@@ -37,22 +35,11 @@  fn parse_dirstate_wrapper(
 
     match parse_dirstate(&mut dirstate_map, &mut copies, st.data(py)) {
         Ok(parents) => {
-            for (filename, entry) in dirstate_map {
-                // Explicitly go through u8 first, then cast to
-                // platform-specific `c_char` because Into<u8> has a specific
-                // implementation while `as c_char` would just do a naive enum
-                // cast.
-                let state: u8 = entry.state.into();
-
+            for (filename, entry) in &dirstate_map {
                 dmap.set_item(
                     py,
                     PyBytes::new(py, filename.as_ref()),
-                    decapsule_make_dirstate_tuple(py)?(
-                        state as c_char,
-                        entry.mode,
-                        entry.size,
-                        entry.mtime,
-                    ),
+                    make_dirstate_tuple(py, entry)?,
                 )?;
             }
             for (path, copy_path) in copies {
@@ -127,30 +114,11 @@  fn pack_dirstate_wrapper(
         Duration::from_secs(now.as_object().extract::<u64>(py)?),
     ) {
         Ok(packed) => {
-            for (
-                filename,
-                DirstateEntry {
-                    state,
-                    mode,
-                    size,
-                    mtime,
-                },
-            ) in dirstate_map
-            {
-                // Explicitly go through u8 first, then cast to
-                // platform-specific `c_char` because Into<u8> has a specific
-                // implementation while `as c_char` would just do a naive enum
-                // cast.
-                let state: u8 = state.into();
+            for (filename, entry) in &dirstate_map {
                 dmap.set_item(
                     py,
                     PyBytes::new(py, filename.as_ref()),
-                    decapsule_make_dirstate_tuple(py)?(
-                        state as c_char,
-                        mode,
-                        size,
-                        mtime,
-                    ),
+                    make_dirstate_tuple(py, entry)?,
                 )?;
             }
             Ok(PyBytes::new(py, &packed))