From aa2e59d82f5449aae83a6c34e401af8b42a84b47 Mon Sep 17 00:00:00 2001 From: Alexandre Courbot Date: Thu, 26 May 2022 11:54:00 +0900 Subject: [PATCH] base: improve documentation of Descriptor and AsRawDescriptor Although it can be useful for some limited cases, the Descriptor wrapper class should ideally be eschewed and an alternative based on implementing AsRawDescriptor on the right type be preferred. Make sure its documentation reflects that. The reason for avoiding Descriptor is that it encourages storing raw descriptors that can possibly be closed before the user expects it. A reasonable alternative to doing this is to call as_raw_descriptor() at the exact moment the descriptor is needed, and never storing the result - this ensures that the providing object (and underlying descriptor) remain alive for long enough. Update the documentation of AsRawDescriptor to explain and hopefully encourage this practice. BUG=233968702 TEST=cargo build Change-Id: I91c2ef11152b3b6adcc25f40d6de0a0f131700dd Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3670101 Reviewed-by: Daniel Verkamp Commit-Queue: Alexandre Courbot Tested-by: kokoro Reviewed-by: Dennis Kempin --- base/src/descriptor.rs | 38 +++++++++++++++++++++++++++++++++++--- 1 file changed, 35 insertions(+), 3 deletions(-) diff --git a/base/src/descriptor.rs b/base/src/descriptor.rs index 61acbc6177..e2d57572a1 100644 --- a/base/src/descriptor.rs +++ b/base/src/descriptor.rs @@ -25,11 +25,26 @@ pub trait IntoRawDescriptor { /// Trait for returning the underlying raw descriptor, without giving up ownership of the /// descriptor. pub trait AsRawDescriptor { + /// Returns the underlying raw descriptor. + /// + /// Since the descriptor is still owned by the provider, callers should not assume that it will + /// remain open for longer than the immediate call of this method. In particular, it is a + /// dangerous practice to store the result of this method for future use: instead, it should be + /// used to e.g. obtain a raw descriptor that is immediately passed to a system call. + /// + /// If you need to use the descriptor for a longer time (and particularly if you cannot reliably + /// track the lifetime of the providing object), you should probably consider using + /// [`SafeDescriptor`] (possibly along with [`trait@IntoRawDescriptor`]) to get full ownership + /// over a descriptor pointing to the same resource. fn as_raw_descriptor(&self) -> RawDescriptor; } /// A trait similar to `AsRawDescriptor` but supports an arbitrary number of descriptors. pub trait AsRawDescriptors { + /// Returns the underlying raw descriptors. + /// + /// Please refer to the documentation of [`AsRawDescriptor::as_raw_descriptor`] for limitations + /// and recommended use. fn as_raw_descriptors(&self) -> Vec; } @@ -106,9 +121,26 @@ impl From for SafeDescriptor { } } -/// For use cases where a simple wrapper around a RawDescriptor is needed. -/// This is a simply a wrapper and does not manage the lifetime of the descriptor. -/// Most usages should prefer SafeDescriptor or using a RawDescriptor directly +/// For use cases where a simple wrapper around a [`RawDescriptor`] is needed, in order to e.g. +/// implement [`trait@AsRawDescriptor`]. +/// +/// This is a simply a wrapper and does not manage the lifetime of the descriptor. As such it is the +/// responsibility of the user to ensure that the wrapped descriptor will not be closed for as long +/// as the `Descriptor` is alive. +/// +/// Most use-cases should prefer [`SafeDescriptor`] or implementing and using +/// [`trait@AsRawDescriptor`] on the type providing the descriptor. Using this wrapper usually means +/// something can be improved in your code. +/// +/// Valid uses of this struct include: +/// * You only have a valid [`RawDescriptor`] and need to pass something that implements +/// [`trait@AsRawDescriptor`] to a function, +/// * You need to serialize a [`RawDescriptor`], +/// * You need [`trait@Send`] or [`trait@Sync`] for your descriptor and properly handle the case +/// where your descriptor gets closed. +/// +/// Note that with the exception of the last use-case (which requires proper error checking against +/// the descriptor being closed), the `Descriptor` instance would be very short-lived. #[derive(Copy, Clone, Hash, PartialEq, Eq, PartialOrd, Ord)] #[repr(transparent)] pub struct Descriptor(pub RawDescriptor);