refactor(profiling)!: create FFI Error type and remove *Result_drop methods

ddcommon-ffi/src/error.rs

@@ -0,0 +1,86 @@
+// Unless explicitly stated otherwise all files in this repository are licensed under the Apache License Version 2.0.
+// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2021-Present Datadog, Inc.
+
+use crate::slice::CharSlice;
+use crate::vec::Vec;
+use std::fmt::{Display, Formatter};
+
+/// Please treat this as opaque; do not reach into it, and especially don't
+/// write into it!
+#[derive(Debug)]
+#[repr(C)]
+pub struct Error {
+    /// This is a String stuffed into the vec.
+    message: Vec<u8>,
+}
+
+impl AsRef<str> for Error {
+    fn as_ref(&self) -> &str {
+        // Safety: .message is a String (just FFI safe).
+        unsafe { std::str::from_utf8_unchecked(self.message.as_slice().as_slice()) }
+    }
+}
+
+impl Display for Error {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        f.write_str(self.as_ref())
+    }
+}
+
+impl std::error::Error for Error {}
+
+impl From<String> for Error {
+    fn from(value: String) -> Self {
+        let message = Vec::from(value.into_bytes());
+        Self { message }
+    }
+}
+
+impl From<Error> for String {
+    fn from(value: Error) -> String {
+        // Safety: .message is a String (just FFI safe).
+        unsafe { String::from_utf8_unchecked(value.message.into()) }
+    }
+}
+
+impl From<&str> for Error {
+    fn from(value: &str) -> Self {
+        Self::from(value.to_string())
+    }
+}
+
+impl From<anyhow::Error> for Error {
+    fn from(value: anyhow::Error) -> Self {
+        Self::from(value.to_string())
+    }
+}
+
+impl From<Box<&dyn std::error::Error>> for Error {
+    fn from(value: Box<&dyn std::error::Error>) -> Self {
+        Self::from(value.to_string())
+    }
+}
+
+/// # Safety
+/// Only pass null or a valid reference to a `ddog_Error`.
+#[no_mangle]
+pub unsafe extern "C" fn ddog_Error_drop(error: Option<&mut Error>) {
+    if let Some(err) = error {
+        // Leave an empty String in place to help with double-free issues.
+        let mut tmp = Error::from(String::new());
+        std::mem::swap(&mut tmp, err);
+        drop(tmp)
+    }
+}
+
+/// Returns a CharSlice of the error's message that is valid until the error
+/// is dropped.
+/// # Safety
+/// Only pass null or a valid reference to a `ddog_Error`.
+#[no_mangle]
+pub unsafe extern "C" fn ddog_Error_message(error: Option<&Error>) -> CharSlice {
+    match error {
+        None => CharSlice::default(),
+        Some(err) => CharSlice::from(err.as_ref()),
+    }
+}

ddcommon-ffi/src/lib.rs

@@ -1,11 +1,15 @@
 // Unless explicitly stated otherwise all files in this repository are licensed under the Apache License Version 2.0.
 // This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2021-Present Datadog, Inc.
 
+mod error;
+
 pub mod option;
 pub mod slice;
 pub mod tags;
 pub mod vec;
 
+pub use error::*;
+
 pub use option::Option;
 pub use slice::{CharSlice, Slice};
 pub use vec::Vec;

ddcommon-ffi/src/tags.rs

@@ -1,7 +1,8 @@
 // Unless explicitly stated otherwise all files in this repository are licensed under the Apache License Version 2.0.
 // This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2022-Present Datadog, Inc.
 
-use crate::{slice::AsBytes, slice::CharSlice};
+use crate::slice::{AsBytes, CharSlice};
+use crate::Error;
 use ddcommon::tag::{parse_tags, Tag};
 
 #[must_use]
@@ -16,12 +17,9 @@ pub extern "C" fn ddog_Vec_Tag_drop(_: crate::Vec<Tag>) {}
 #[repr(C)]
 pub enum PushTagResult {
     Ok,
-    Err(crate::Vec<u8>),
+    Err(Error),
 }
 
-#[no_mangle]
-pub extern "C" fn ddog_Vec_Tag_PushResult_drop(_: PushTagResult) {}
-
 /// Creates a new Tag from the provided `key` and `value` by doing a utf8
 /// lossy conversion, and pushes into the `vec`. The strings `key` and `value`
 /// are cloned to avoid FFI lifetime issues.
@@ -44,14 +42,14 @@ pub unsafe extern "C" fn ddog_Vec_Tag_push(
             vec.push(tag);
             PushTagResult::Ok
         }
-        Err(err) => PushTagResult::Err(err.as_bytes().to_vec().into()),
+        Err(err) => PushTagResult::Err(Error::from(err.as_ref())),
     }
 }
 
 #[repr(C)]
 pub struct ParseTagsResult {
     tags: crate::Vec<Tag>,
-    error_message: Option<Box<crate::Vec<u8>>>,
+    error_message: Option<Box<Error>>,
 }
 
 /// # Safety
@@ -64,7 +62,7 @@ pub unsafe extern "C" fn ddog_Vec_Tag_parse(string: CharSlice) -> ParseTagsResul
     let (tags, error) = parse_tags(string.as_ref());
     ParseTagsResult {
         tags: tags.into(),
-        error_message: error.map(|message| Box::new(crate::Vec::from(message.into_bytes()))),
+        error_message: error.map(Error::from).map(Box::new),
     }
 }
 
@@ -126,10 +124,11 @@ mod tests {
 
         // 'tags:' cannot end in a semi-colon, so expect an error.
         assert!(result.error_message.is_some());
-        let error_message: Vec<u8> = (*result.error_message.unwrap()).into();
+        let error = *result.error_message.unwrap();
+        let error_message = error.as_ref();
         assert!(!error_message.is_empty());
 
-        let expected_error_message = b"Errors while parsing tags: tag 'tags:' ends with a colon";
-        assert_eq!(expected_error_message, error_message.as_slice())
+        let expected_error_message = "Errors while parsing tags: tag 'tags:' ends with a colon";
+        assert_eq!(expected_error_message, error_message)
     }
 }

examples/ffi/exporter.cpp

@@ -15,8 +15,9 @@ struct Deleter {
   void operator()(ddog_prof_Profile *object) { ddog_prof_Profile_drop(object); }
 };
 
-template <typename T> void print_error(const char *s, const T &err) {
-  printf("%s (%.*s)\n", s, static_cast<int>(err.len), err.ptr);
+void print_error(const char *s, const ddog_Error &err) {
+  auto charslice = ddog_Error_message(&err);
+  printf("%s (%.*s)\n", s, static_cast<int>(charslice.len), charslice.ptr);
 }
 
 int main(int argc, char *argv[]) {
@@ -69,14 +70,14 @@ int main(int argc, char *argv[]) {
   auto add_result = ddog_prof_Profile_add(profile.get(), sample);
   if (add_result.tag != DDOG_PROF_PROFILE_ADD_RESULT_OK) {
     print_error("Failed to add sample to profile: ", add_result.err);
-    ddog_prof_Profile_AddResult_drop(add_result);
+    ddog_Error_drop(&add_result.err);
     return 1;
   }
 
   ddog_prof_Profile_SerializeResult serialize_result = ddog_prof_Profile_serialize(profile.get(), nullptr, nullptr);
   if (serialize_result.tag == DDOG_PROF_PROFILE_SERIALIZE_RESULT_ERR) {
     print_error("Failed to serialize profile: ", serialize_result.err);
-    ddog_prof_Profile_SerializeResult_drop(serialize_result);
+    ddog_Error_drop(&serialize_result.err);
     return 1;
   }
 
@@ -90,12 +91,10 @@ int main(int argc, char *argv[]) {
       ddog_Vec_Tag_push(&tags, DDOG_CHARSLICE_C("service"), to_slice_c_char(service));
   if (tag_result.tag == DDOG_VEC_TAG_PUSH_RESULT_ERR) {
     print_error("Failed to push tag: ", tag_result.err);
-    ddog_Vec_Tag_PushResult_drop(tag_result);
+    ddog_Error_drop(&tag_result.err);
     return 1;
   }
 
-  ddog_Vec_Tag_PushResult_drop(tag_result);
-
   ddog_prof_Exporter_NewResult exporter_new_result = ddog_prof_Exporter_new(
       DDOG_CHARSLICE_C("exporter-example"),
       DDOG_CHARSLICE_C("1.2.3"),
@@ -107,7 +106,7 @@ int main(int argc, char *argv[]) {
 
   if (exporter_new_result.tag == DDOG_PROF_EXPORTER_NEW_RESULT_ERR) {
     print_error("Failed to create exporter: ", exporter_new_result.err);
-    ddog_prof_Exporter_NewResult_drop(exporter_new_result);
+    ddog_Error_drop(&exporter_new_result.err);
     return 1;
   }
 
@@ -132,7 +131,7 @@ int main(int argc, char *argv[]) {
 
   if (build_result.tag == DDOG_PROF_EXPORTER_REQUEST_BUILD_RESULT_ERR) {
     print_error("Failed to build request: ", build_result.err);
-    ddog_prof_Exporter_Request_BuildResult_drop(build_result);
+    ddog_Error_drop(&build_result.err);
     return 1;
   }
 
@@ -164,12 +163,12 @@ int main(int argc, char *argv[]) {
   if (send_result.tag == DDOG_PROF_EXPORTER_SEND_RESULT_ERR) {
     print_error("Failed to send profile: ", send_result.err);
     exit_code = 1;
+    ddog_Error_drop(&send_result.err);
   } else {
     printf("Response code: %d\n", send_result.http_response.code);
   }
 
-  ddog_prof_Exporter_NewResult_drop(exporter_new_result);
-  ddog_prof_Exporter_SendResult_drop(send_result);
+  ddog_prof_Exporter_drop(exporter);
   ddog_CancellationToken_drop(cancel);
   return exit_code;
 }

examples/ffi/profiles.c

@@ -48,8 +48,9 @@ int main(void) {
 
   ddog_prof_Profile_AddResult add_result = ddog_prof_Profile_add(profile, sample);
   if (add_result.tag != DDOG_PROF_PROFILE_ADD_RESULT_OK) {
-    fprintf(stderr, "%*s", (int)add_result.err.len, add_result.err.ptr);
-    ddog_prof_Profile_AddResult_drop(add_result);
+    ddog_CharSlice message = ddog_Error_message(&add_result.err);
+    fprintf(stderr, "%*s", (int)message.len, message.ptr);
+    ddog_Error_drop(&add_result.err);
   }
 
 

profiling-ffi/cbindgen.toml

@@ -22,6 +22,7 @@ renaming_overrides_prefixing = true
 "CancellationToken" = "ddog_CancellationToken"
 "CharSlice" = "ddog_CharSlice"
 "Endpoint" = "ddog_Endpoint"
+"Error" = "ddog_Error"
 "HttpStatus" = "ddog_HttpStatus"
 "Slice_CChar" = "ddog_Slice_CChar"
 "Slice_I64" = "ddog_Slice_I64"

profiling-ffi/src/exporter.rs

@@ -10,31 +10,28 @@ use datadog_profiling::exporter::{ProfileExporter, Request};
 use datadog_profiling::profile::profiled_endpoints;
 use ddcommon::tag::Tag;
 use ddcommon_ffi::slice::{AsBytes, ByteSlice, CharSlice, Slice};
+use ddcommon_ffi::Error;
 use std::borrow::Cow;
 use std::ptr::NonNull;
 use std::str::FromStr;
 
 #[repr(C)]
-pub enum SendResult {
-    HttpResponse(HttpStatus),
-    Err(ddcommon_ffi::Vec<u8>),
+pub enum ExporterNewResult {
+    Ok(*mut ProfileExporter),
+    Err(Error),
 }
 
 #[repr(C)]
-pub enum ExporterNewResult {
-    Ok(*mut ProfileExporter),
-    Err(ddcommon_ffi::Vec<u8>),
+pub enum RequestBuildResult {
+    // This is a Box::into_raw pointer.
+    Ok(*mut Request),
+    Err(Error),
 }
 
-#[no_mangle]
-pub unsafe extern "C" fn ddog_prof_Exporter_NewResult_drop(result: ExporterNewResult) {
-    match result {
-        ExporterNewResult::Ok(ptr) => {
-            let exporter = Box::from_raw(ptr);
-            drop(exporter)
-        }
-        ExporterNewResult::Err(message) => drop(message),
-    }
+#[repr(C)]
+pub enum SendResult {
+    HttpResponse(HttpStatus),
+    Err(Error),
 }
 
 #[repr(C)]
@@ -163,40 +160,23 @@ unsafe fn into_vec_files<'a>(slice: Slice<'a, File>) -> Vec<exporter::File<'a>>
         .collect()
 }
 
-#[repr(C)]
-pub enum RequestBuildResult {
-    // This is a Box::into_raw pointer.
-    Ok(*mut Request),
-    Err(ddcommon_ffi::Vec<u8>),
-}
-
 #[cfg(test)]
 impl From<RequestBuildResult> for Result<Box<Request>, String> {
     fn from(result: RequestBuildResult) -> Self {
         match result {
             // Safety: Request is opaque, can only be built from Rust.
             RequestBuildResult::Ok(ok) => Ok(unsafe { Box::from_raw(ok) }),
-            RequestBuildResult::Err(err) => {
-                // Safety: not generally safe but this is for test code.
-                Err(unsafe { String::from_utf8_unchecked(err.into()) })
-            }
+            RequestBuildResult::Err(err) => Err(String::from(err)),
         }
     }
 }
 
-/// Drops the result. Since `ddog_prof_Exporter_send` will take ownership of
-/// the Request object, do not call this method if you call
-/// `ddog_prof_Exporter_send` on the `ok` value!
-#[no_mangle]
-pub extern "C" fn ddog_prof_Exporter_Request_BuildResult_drop(_result: RequestBuildResult) {}
-
 /// If successful, builds a `ddog_prof_Exporter_Request` object based on the profile data supplied.
 /// If unsuccessful, it returns an error message.
 ///
 /// The main use of the `ddog_prof_Exporter_Request` object is to be used in
 /// `ddog_prof_Exporter_send`, which will take ownership of the object. Do not pass it to
-/// `ddog_prof_Exporter_Request_drop` in such cases, nor call
-/// `ddog_prof_Exporter_Request_BuildResult_drop` on the result!
+/// `ddog_prof_Exporter_Request_drop` after that!
 ///
 /// # Safety
 /// The `exporter` and the files inside of the `files` slice need to have been
@@ -238,9 +218,8 @@ pub unsafe extern "C" fn ddog_prof_Exporter_Request_build(
 ///
 /// # Arguments
 /// * `exporter` - Borrows the exporter for sending the request.
-/// * `request` - Takes ownership of the request. Do not call `ddog_prof_Request_drop` nor
-///               `ddog_prof_Exporter_SendResult_drop` on `request` after calling
-///               `ddog_prof_Exporter_send` on it.
+/// * `request` - Takes ownership of the request. Do not call `ddog_prof_Request_drop` on
+///               `request` after calling `ddog_prof_Exporter_send` on it.
 /// * `cancel` - Borrows the cancel, if any.
 ///
 /// # Safety
@@ -254,16 +233,14 @@ pub unsafe extern "C" fn ddog_prof_Exporter_send(
 ) -> SendResult {
     // Re-box the request first, to avoid leaks on other errors.
     let request = if request.is_null() {
-        let buf: &[u8] = b"Failed to export: request was null";
-        return SendResult::Err(ddcommon_ffi::Vec::from(Vec::from(buf)));
+        return SendResult::Err(Error::from("failed to export: request was null"));
     } else {
         Box::from_raw(request)
     };
 
     let exp_ptr = match exporter {
         None => {
-            let buf: &[u8] = b"Failed to export: exporter was null";
-            return SendResult::Err(ddcommon_ffi::Vec::from(Vec::from(buf)));
+            return SendResult::Err(Error::from("failed to export: exporter was null"));
         }
         Some(e) => e,
     };
@@ -357,11 +334,6 @@ pub extern "C" fn ddog_CancellationToken_drop(_cancel: Option<Box<CancellationTo
     // _cancel implicitly dropped because we've turned it into a Box
 }
 
-#[no_mangle]
-pub unsafe extern "C" fn ddog_prof_Exporter_SendResult_drop(result: SendResult) {
-    std::mem::drop(result)
-}
-
 #[cfg(test)]
 mod test {
     use crate::exporter::*;