A Consistent and Principled Approach to GDAL C Pointers

.cargo/config.toml

@@ -1,5 +1,5 @@
 [alias]
-# Run doctests, displaying compiler output
+# doc-test-output: Run doctests, displaying compiler output
 dto = "test --doc -- --show-output"
 # Run clippy, raising warnings to errors
 nowarn = "clippy --all-targets -- -D warnings"

Cargo.toml

@@ -26,6 +26,7 @@ ndarray = { version = "0.15", optional = true }
 chrono = { version = "0.4.26", default-features = false }
 bitflags = "2.4"
 once_cell = "1.18"
+foreign-types = "0.5.0"
 
 [build-dependencies]
 semver = "1.0"

examples/rasterband.rs

@@ -1,4 +1,3 @@
-use gdal::raster::RasterBand;
 use gdal::{Dataset, Metadata};
 use std::path::Path;
 
@@ -7,7 +6,7 @@ fn main() {
     let dataset = Dataset::open(path).unwrap();
     println!("dataset description: {:?}", dataset.description());
 
-    let rasterband: RasterBand = dataset.rasterband(1).unwrap();
+    let rasterband = dataset.rasterband(1).unwrap();
     println!("rasterband description: {:?}", rasterband.description());
     println!("rasterband no_data_value: {:?}", rasterband.no_data_value());
     println!("rasterband type: {:?}", rasterband.band_type());

examples/read_write_ogr_datetime.rs

@@ -32,7 +32,7 @@ fn run() -> gdal::errors::Result<()> {
     for feature_a in layer_a.features() {
         let mut ft = Feature::new(&defn)?;
         if let Some(geom) = feature_a.geometry() {
-            ft.set_geometry(geom.clone())?;
+            ft.set_geometry(geom.to_owned())?;
         }
         // copy each field value of the feature:
         for field in defn.fields() {

fixtures/tinymarble.tif.aux.xml

@@ -1,3 +1,12 @@
 <PAMDataset>
   <SRS dataAxisToSRSAxisMapping="2,1">GEOGCS["WGS 84",DATUM["WGS_1984",SPHEROID["WGS 84",6378137,298.257223563,AUTHORITY["EPSG","7030"]],AUTHORITY["EPSG","6326"]],PRIMEM["Greenwich",0,AUTHORITY["EPSG","8901"]],UNIT["degree",0.0174532925199433,AUTHORITY["EPSG","9122"]],AXIS["Latitude",NORTH],AXIS["Longitude",EAST],AUTHORITY["EPSG","4326"]]</SRS>
+  <PAMRasterBand band="1">
+    <Metadata>
+      <MDI key="STATISTICS_MAXIMUM">255</MDI>
+      <MDI key="STATISTICS_MEAN">68.4716</MDI>
+      <MDI key="STATISTICS_MINIMUM">0</MDI>
+      <MDI key="STATISTICS_STDDEV">83.68444773935</MDI>
+      <MDI key="STATISTICS_VALID_PERCENT">100</MDI>
+    </Metadata>
+  </PAMRasterBand>
 </PAMDataset>

src/dataset.rs

@@ -1,29 +1,34 @@
+use foreign_types::{foreign_type, ForeignType, ForeignTypeRef};
+use std::fmt::{Debug, Formatter};
+use std::mem::ManuallyDrop;
 use std::{ffi::CString, ffi::NulError, path::Path, ptr};
 
-use gdal_sys::{self, CPLErr, GDALDatasetH, GDALMajorObjectH};
+use gdal_sys::{self, CPLErr, GDALMajorObjectH};
 
 use crate::cpl::CslStringList;
 use crate::errors::*;
 use crate::options::DatasetOptions;
 use crate::raster::RasterCreationOption;
+use crate::spatial_ref::SpatialRefRef;
 use crate::utils::{_last_cpl_err, _last_null_pointer_err, _path_to_c_string, _string};
 use crate::{
     gdal_major_object::MajorObject, spatial_ref::SpatialRef, Driver, GeoTransform, Metadata,
 };
 
-/// Wrapper around a [`GDALDataset`][GDALDataset] object.
-///
-/// Represents both a [vector dataset][vector-data-model]
-/// containing a collection of layers; and a
-/// [raster dataset][raster-data-model] containing a collection of raster-bands.
-///
-/// [vector-data-model]: https://gdal.org/user/vector_data_model.html
-/// [raster-data-model]: https://gdal.org/user/raster_data_model.html
-/// [GDALDataset]: https://gdal.org/api/gdaldataset_cpp.html#_CPPv411GDALDataset
-#[derive(Debug)]
-pub struct Dataset {
-    c_dataset: GDALDatasetH,
-    closed: bool,
+foreign_type! {
+    /// Wrapper around a [`GDALDataset`][GDALDataset] object.
+    ///
+    /// Represents both a [vector dataset][vector-data-model]
+    /// containing a collection of layers; and a
+    /// [raster dataset][raster-data-model] containing a collection of raster-bands.
+    ///
+    /// [vector-data-model]: https://gdal.org/user/vector_data_model.html
+    /// [raster-data-model]: https://gdal.org/user/raster_data_model.html
+    /// [GDALDataset]: https://gdal.org/api/gdaldataset_cpp.html#_CPPv411GDALDataset
+    pub unsafe type Dataset {
+        type CType = libc::c_void;
+        fn drop = gdal_sys::GDALClose;
+    }
 }
 
 // GDAL Docs state: The returned dataset should only be accessed by one thread at a time.
@@ -35,26 +40,6 @@ unsafe impl Send for Dataset {}
 
 /// Core dataset methods
 impl Dataset {
-    /// Returns the wrapped C pointer
-    ///
-    /// # Safety
-    /// This method returns a raw C pointer
-    pub fn c_dataset(&self) -> GDALDatasetH {
-        self.c_dataset
-    }
-
-    /// Creates a new Dataset by wrapping a C pointer
-    ///
-    /// # Safety
-    /// This method operates on a raw C pointer
-    /// The dataset must not have been closed (using [`GDALClose`]) before.
-    pub unsafe fn from_c_dataset(c_dataset: GDALDatasetH) -> Dataset {
-        Dataset {
-            c_dataset,
-            closed: false,
-        }
-    }
-
     /// Open a dataset at the given `path` with default
     /// options.
     pub fn open<P: AsRef<Path>>(path: P) -> Result<Dataset> {
@@ -156,10 +141,7 @@ impl Dataset {
         if c_dataset.is_null() {
             return Err(_last_null_pointer_err("GDALOpenEx"));
         }
-        Ok(Dataset {
-            c_dataset,
-            closed: false,
-        })
+        Ok(unsafe { Dataset::from_ptr(c_dataset) })
     }
 
     /// Flush all write cached data to disk.
@@ -170,67 +152,76 @@ impl Dataset {
     pub fn flush_cache(&mut self) -> Result<()> {
         #[cfg(any(all(major_ge_3, minor_ge_7), major_ge_4))]
         {
-            let rv = unsafe { gdal_sys::GDALFlushCache(self.c_dataset) };
+            let rv = unsafe { gdal_sys::GDALFlushCache(self.as_ptr()) };
             if rv != CPLErr::CE_None {
                 return Err(_last_cpl_err(rv));
             }
         }
         #[cfg(not(any(all(major_is_3, minor_ge_7), major_ge_4)))]
         {
             unsafe {
-                gdal_sys::GDALFlushCache(self.c_dataset);
+                gdal_sys::GDALFlushCache(self.as_ptr());
             }
         }
         Ok(())
     }
 
-    /// Close the dataset.
+    /// Close the dataset, consuming it.
     ///
-    /// See [`GDALClose`].
+    /// See [`GDALClose`](https://gdal.org/api/raster_c_api.html#_CPPv49GDALClose12GDALDatasetH).
     ///
-    /// Note: on GDAL versions older than 3.7.0, this function always succeeds.
-    pub fn close(mut self) -> Result<()> {
-        self.closed = true;
-
+    /// # Notes
+    /// * `Drop`ing automatically closes the Dataset, so this method is usually not needed.
+    /// * On GDAL versions older than 3.7.0, this function always succeeds.
+    pub fn close(self) -> Result<()> {
+        // According to the C documentation, GDALClose will release all resources
+        // using the C++ `delete` operator.
+        // According to https://doc.rust-lang.org/std/mem/fn.forget.html#relationship-with-manuallydrop
+        // `ManuallyDrop` is the suggested means of transferring memory management while
+        // robustly preventing a double-free.
+        let ds = ManuallyDrop::new(self);
         #[cfg(any(all(major_ge_3, minor_ge_7), major_ge_4))]
         {
-            let rv = unsafe { gdal_sys::GDALClose(self.c_dataset) };
+            let rv = unsafe { gdal_sys::GDALClose(ds.as_ptr()) };
             if rv != CPLErr::CE_None {
                 return Err(_last_cpl_err(rv));
             }
         }
         #[cfg(not(any(all(major_is_3, minor_ge_7), major_ge_4)))]
         {
             unsafe {
-                gdal_sys::GDALClose(self.c_dataset);
+                gdal_sys::GDALClose(ds.as_ptr());
             }
         }
         Ok(())
     }
 
     /// Fetch the projection definition string for this dataset.
     pub fn projection(&self) -> String {
-        let rv = unsafe { gdal_sys::GDALGetProjectionRef(self.c_dataset) };
+        let rv = unsafe { gdal_sys::GDALGetProjectionRef(self.as_ptr()) };
         _string(rv)
     }
 
     /// Set the projection reference string for this dataset.
     pub fn set_projection(&mut self, projection: &str) -> Result<()> {
         let c_projection = CString::new(projection)?;
-        unsafe { gdal_sys::GDALSetProjection(self.c_dataset, c_projection.as_ptr()) };
+        unsafe { gdal_sys::GDALSetProjection(self.as_ptr(), c_projection.as_ptr()) };
         Ok(())
     }
 
     #[cfg(major_ge_3)]
     /// Get the spatial reference system for this dataset.
     pub fn spatial_ref(&self) -> Result<SpatialRef> {
-        unsafe { SpatialRef::from_c_obj(gdal_sys::GDALGetSpatialRef(self.c_dataset)) }
+        Ok(
+            unsafe { SpatialRefRef::from_ptr(gdal_sys::GDALGetSpatialRef(self.as_ptr())) }
+                .to_owned(),
+        )
     }
 
     #[cfg(major_ge_3)]
     /// Set the spatial reference system for this dataset.
     pub fn set_spatial_ref(&mut self, spatial_ref: &SpatialRef) -> Result<()> {
-        let rv = unsafe { gdal_sys::GDALSetSpatialRef(self.c_dataset, spatial_ref.to_c_hsrs()) };
+        let rv = unsafe { gdal_sys::GDALSetSpatialRef(self.as_ptr(), spatial_ref.as_ptr()) };
         if rv != CPLErr::CE_None {
             return Err(_last_cpl_err(rv));
         }
@@ -260,7 +251,7 @@ impl Dataset {
                 gdal_sys::GDALCreateCopy(
                     driver.c_driver(),
                     c_filename.as_ptr(),
-                    ds.c_dataset,
+                    ds.as_ptr(),
                     0,
                     c_options.as_ptr(),
                     None,
@@ -270,15 +261,15 @@ impl Dataset {
             if c_dataset.is_null() {
                 return Err(_last_null_pointer_err("GDALCreateCopy"));
             }
-            Ok(unsafe { Dataset::from_c_dataset(c_dataset) })
+            Ok(unsafe { Dataset::from_ptr(c_dataset) })
         }
         _create_copy(self, driver, filename.as_ref(), options)
     }
 
     /// Fetch the driver to which this dataset relates.
     pub fn driver(&self) -> Driver {
         unsafe {
-            let c_driver = gdal_sys::GDALGetDatasetDriver(self.c_dataset);
+            let c_driver = gdal_sys::GDALGetDatasetDriver(self.as_ptr());
             Driver::from_c_driver(c_driver)
         }
     }
@@ -299,7 +290,7 @@ impl Dataset {
     pub fn set_geo_transform(&mut self, transformation: &GeoTransform) -> Result<()> {
         assert_eq!(transformation.len(), 6);
         let rv = unsafe {
-            gdal_sys::GDALSetGeoTransform(self.c_dataset, transformation.as_ptr() as *mut f64)
+            gdal_sys::GDALSetGeoTransform(self.as_ptr(), transformation.as_ptr() as *mut f64)
         };
         if rv != CPLErr::CE_None {
             return Err(_last_cpl_err(rv));
@@ -319,7 +310,7 @@ impl Dataset {
     pub fn geo_transform(&self) -> Result<GeoTransform> {
         let mut transformation = GeoTransform::default();
         let rv =
-            unsafe { gdal_sys::GDALGetGeoTransform(self.c_dataset, transformation.as_mut_ptr()) };
+            unsafe { gdal_sys::GDALGetGeoTransform(self.as_ptr(), transformation.as_mut_ptr()) };
 
         // check if the dataset has a GeoTransform
         if rv != CPLErr::CE_None {
@@ -329,24 +320,25 @@ impl Dataset {
     }
 }
 
+impl Debug for Dataset {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("Dataset")
+            .field(
+                "description",
+                &self.description().unwrap_or_else(|_| Default::default()),
+            )
+            .finish()
+    }
+}
+
 impl MajorObject for Dataset {
     fn gdal_object_ptr(&self) -> GDALMajorObjectH {
-        self.c_dataset
+        self.as_ptr()
     }
 }
 
 impl Metadata for Dataset {}
 
-impl Drop for Dataset {
-    fn drop(&mut self) {
-        if !self.closed {
-            unsafe {
-                gdal_sys::GDALClose(self.c_dataset);
-            }
-        }
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use gdal_sys::GDALAccess;

src/driver.rs

@@ -1,3 +1,4 @@
+use foreign_types::ForeignType;
 use std::ffi::CString;
 use std::path::Path;
 use std::sync::Once;
@@ -223,7 +224,7 @@ impl Driver {
             return Err(_last_null_pointer_err("GDALCreate"));
         };
 
-        Ok(unsafe { Dataset::from_c_dataset(c_dataset) })
+        Ok(unsafe { Dataset::from_ptr(c_dataset) })
     }
 
     /// Convenience for creating a vector-only dataset from a compatible driver.

src/gcp.rs

@@ -6,9 +6,10 @@ use std::marker::PhantomData;
 use gdal_sys::CPLErr;
 
 use crate::errors::Result;
-use crate::spatial_ref::SpatialRef;
+use crate::spatial_ref::{SpatialRef, SpatialRefRef};
 use crate::utils::{_last_cpl_err, _string};
 use crate::Dataset;
+use foreign_types::{ForeignType, ForeignTypeRef};
 
 /// An owned Ground Control Point.
 #[derive(Clone, Debug, PartialEq, PartialOrd)]
@@ -115,13 +116,14 @@ impl Dataset {
     ///
     /// See: [`GDALGetGCPSpatialRef`](https://gdal.org/api/raster_c_api.html#_CPPv420GDALGetGCPSpatialRef12GDALDatasetH)
     pub fn gcp_spatial_ref(&self) -> Option<SpatialRef> {
-        let c_ptr = unsafe { gdal_sys::GDALGetGCPSpatialRef(self.c_dataset()) };
+        let c_ptr = unsafe { gdal_sys::GDALGetGCPSpatialRef(self.as_ptr()) };
 
         if c_ptr.is_null() {
             return None;
         }
-
-        unsafe { SpatialRef::from_c_obj(c_ptr) }.ok()
+        // NB: Creating a `SpatialRefRef` from a pointer acknowledges that GDAL is giving us
+        // a shared reference. We then call `to_owned` to appropriately get a clone.
+        Some(unsafe { SpatialRefRef::from_ptr(c_ptr) }.to_owned())
     }
 
     /// Get the projection definition string for the GCPs in this dataset.
@@ -132,7 +134,7 @@ impl Dataset {
     ///
     ///  See: [`GDALGetGCPProjection`](https://gdal.org/api/raster_c_api.html#gdal_8h_1a85ffa184d3ecb7c0a59a66096b22b2ec)
     pub fn gcp_projection(&self) -> Option<String> {
-        let cc_ptr = unsafe { gdal_sys::GDALGetGCPProjection(self.c_dataset()) };
+        let cc_ptr = unsafe { gdal_sys::GDALGetGCPProjection(self.as_ptr()) };
         if cc_ptr.is_null() {
             return None;
         }
@@ -143,8 +145,8 @@ impl Dataset {
     ///
     /// See: [`GDALDataset::GetGCPs`](https://gdal.org/api/gdaldataset_cpp.html#_CPPv4N11GDALDataset7GetGCPsEv)
     pub fn gcps(&self) -> &[GcpRef] {
-        let len = unsafe { gdal_sys::GDALGetGCPCount(self.c_dataset()) };
-        let data = unsafe { gdal_sys::GDALGetGCPs(self.c_dataset()) };
+        let len = unsafe { gdal_sys::GDALGetGCPCount(self.as_ptr()) };
+        let data = unsafe { gdal_sys::GDALGetGCPs(self.as_ptr()) };
         unsafe { std::slice::from_raw_parts(data as *const GcpRef, len as usize) }
     }
 
@@ -204,10 +206,10 @@ impl Dataset {
 
         let rv = unsafe {
             gdal_sys::GDALSetGCPs2(
-                self.c_dataset(),
+                self.as_ptr(),
                 len,
                 gdal_gcps.as_ptr(),
-                spatial_ref.to_c_hsrs() as *mut _,
+                spatial_ref.as_ptr() as *mut _,
             )
         };
 

src/metadata.rs

@@ -382,7 +382,7 @@ mod tests {
     fn test_set_description() {
         let driver = DriverManager::get_driver_by_name("MEM").unwrap();
         let dataset = driver.create("", 1, 1, 1).unwrap();
-        let mut band = dataset.rasterband(1).unwrap();
+        let band = dataset.rasterband(1).unwrap();
 
         let description = "A merry and cheerful band description";
         assert_eq!(band.description().unwrap(), "");