rust/neon
1
0
Fork 0
mirror of https://github.com/neondatabase/neon.git synced 2026-01-05 20:42:54 +00:00
Code Issues Packages Projects Releases Wiki Activity

[pageserver/virtual_file] impr: Improve OpenOptions API ergonomics (#11789)

Browse Source 
# Improve OpenOptions API ergonomics

Closes #11787

This PR improves the OpenOptions API ergonomics by:

1. Making OpenOptions methods take and return owned Self instead of &mut
self
2. Changing VirtualFile::open_with_options_v2 to take an owned
OpenOptions
3. Removing unnecessary .clone() and .to_owned() calls

These changes make the API more idiomatic Rust by leveraging the builder
pattern with owned values, which is cleaner and more ergonomic than the
previous approach.

Link to Devin run:
https://app.devin.ai/sessions/c2a4b24f7aca40a3b3777f4259bf8ee1
Requested by: christian@neon.tech

---------

Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-authored-by: christian@neon.tech <christian@neon.tech>
This commit is contained in:
devin-ai-integration[bot]
2025-05-05 15:06:37 +02:00
committed by GitHub
parent 0b243242df
commit baf425a2cd
2 changed files with 23 additions and 36 deletions
Download Patch File Download Diff File Expand all files Collapse all files

42
pageserver/src/virtual_file.rs
View File

@@ -14,8 +14,6 @@
use std::fs::File;
use std::io::{Error, ErrorKind};
use std::os::fd::{AsRawFd, FromRawFd, IntoRawFd, OwnedFd, RawFd};
#[cfg(target_os = "linux")]
use std::os::unix::fs::OpenOptionsExt;
use std::sync::LazyLock;
use std::sync::atomic::{AtomicBool, AtomicU8, AtomicUsize, Ordering};
@@ -99,7 +97,7 @@ impl VirtualFile {
pub async fn open_with_options_v2<P: AsRef<Utf8Path>>(
path: P,
open_options: &OpenOptions,
#[cfg_attr(not(target_os = "linux"), allow(unused_mut))] mut open_options: OpenOptions,
ctx: &RequestContext,
) -> Result<Self, std::io::Error> {
let mode = get_io_mode();
@@ -112,21 +110,16 @@ impl VirtualFile {
#[cfg(target_os = "linux")]
(IoMode::DirectRw, _) => true,
};
let open_options = open_options.clone();
let open_options = if set_o_direct {
if set_o_direct {
#[cfg(target_os = "linux")]
{
let mut open_options = open_options;
open_options.custom_flags(nix::libc::O_DIRECT);
open_options
open_options = open_options.custom_flags(nix::libc::O_DIRECT);
}
#[cfg(not(target_os = "linux"))]
unreachable!(
"O_DIRECT is not supported on this platform, IoMode's that result in set_o_direct=true shouldn't even be defined"
);
} else {
open_options
};
}
let inner = VirtualFileInner::open_with_options(path, open_options, ctx).await?;
Ok(VirtualFile { inner, _mode: mode })
}
@@ -530,7 +523,7 @@ impl VirtualFileInner {
path: P,
ctx: &RequestContext,
) -> Result<VirtualFileInner, std::io::Error> {
Self::open_with_options(path.as_ref(), OpenOptions::new().read(true).clone(), ctx).await
Self::open_with_options(path.as_ref(), OpenOptions::new().read(true), ctx).await
}
/// Open a file with given options.
@@ -558,10 +551,11 @@ impl VirtualFileInner {
// It would perhaps be nicer to check just for the read and write flags
// explicitly, but OpenOptions doesn't contain any functions to read flags,
// only to set them.
let mut reopen_options = open_options.clone();
reopen_options.create(false);
reopen_options.create_new(false);
reopen_options.truncate(false);
let reopen_options = open_options
.clone()
.create(false)
.create_new(false)
.truncate(false);
let vfile = VirtualFileInner {
handle: RwLock::new(handle),
@@ -1307,7 +1301,7 @@ mod tests {
opts: OpenOptions,
ctx: &RequestContext,
) -> Result<MaybeVirtualFile, anyhow::Error> {
let vf = VirtualFile::open_with_options_v2(&path, &opts, ctx).await?;
let vf = VirtualFile::open_with_options_v2(&path, opts, ctx).await?;
Ok(MaybeVirtualFile::VirtualFile(vf))
}
}
@@ -1374,7 +1368,7 @@ mod tests {
let _ = file_a.read_string_at(0, 1, &ctx).await.unwrap_err();
// Close the file and re-open for reading
let mut file_a = A::open(path_a, OpenOptions::new().read(true).to_owned(), &ctx).await?;
let mut file_a = A::open(path_a, OpenOptions::new().read(true), &ctx).await?;
// cannot write to a file opened in read-only mode
let _ = file_a
@@ -1393,8 +1387,7 @@ mod tests {
.read(true)
.write(true)
.create(true)
.truncate(true)
.to_owned(),
.truncate(true),
&ctx,
)
.await?;
@@ -1412,12 +1405,7 @@ mod tests {
let mut vfiles = Vec::new();
for _ in 0..100 {
let mut vfile = A::open(
path_b.clone(),
OpenOptions::new().read(true).to_owned(),
&ctx,
)
.await?;
let mut vfile = A::open(path_b.clone(), OpenOptions::new().read(true), &ctx).await?;
assert_eq!("FOOBAR", vfile.read_string_at(0, 6, &ctx).await?);
vfiles.push(vfile);
}
@@ -1466,7 +1454,7 @@ mod tests {
for _ in 0..VIRTUAL_FILES {
let f = VirtualFileInner::open_with_options(
&test_file_path,
OpenOptions::new().read(true).clone(),
OpenOptions::new().read(true),
&ctx,
)
.await?;

17
pageserver/src/virtual_file/open_options.rs
View File

@@ -1,6 +1,7 @@
//! Enum-dispatch to the `OpenOptions` type of the respective [`super::IoEngineKind`];
use std::os::fd::OwnedFd;
use std::os::unix::fs::OpenOptionsExt;
use std::path::Path;
use super::io_engine::IoEngine;
@@ -43,7 +44,7 @@ impl OpenOptions {
self.write
}
pub fn read(&mut self, read: bool) -> &mut OpenOptions {
pub fn read(mut self, read: bool) -> Self {
match &mut self.inner {
Inner::StdFs(x) => {
let _ = x.read(read);
@@ -56,7 +57,7 @@ impl OpenOptions {
self
}
pub fn write(&mut self, write: bool) -> &mut OpenOptions {
pub fn write(mut self, write: bool) -> Self {
self.write = write;
match &mut self.inner {
Inner::StdFs(x) => {
@@ -70,7 +71,7 @@ impl OpenOptions {
self
}
pub fn create(&mut self, create: bool) -> &mut OpenOptions {
pub fn create(mut self, create: bool) -> Self {
match &mut self.inner {
Inner::StdFs(x) => {
let _ = x.create(create);
@@ -83,7 +84,7 @@ impl OpenOptions {
self
}
pub fn create_new(&mut self, create_new: bool) -> &mut OpenOptions {
pub fn create_new(mut self, create_new: bool) -> Self {
match &mut self.inner {
Inner::StdFs(x) => {
let _ = x.create_new(create_new);
@@ -96,7 +97,7 @@ impl OpenOptions {
self
}
pub fn truncate(&mut self, truncate: bool) -> &mut OpenOptions {
pub fn truncate(mut self, truncate: bool) -> Self {
match &mut self.inner {
Inner::StdFs(x) => {
let _ = x.truncate(truncate);
@@ -124,10 +125,8 @@ impl OpenOptions {
}
}
}
}
impl std::os::unix::prelude::OpenOptionsExt for OpenOptions {
fn mode(&mut self, mode: u32) -> &mut OpenOptions {
pub fn mode(mut self, mode: u32) -> Self {
match &mut self.inner {
Inner::StdFs(x) => {
let _ = x.mode(mode);
@@ -140,7 +139,7 @@ impl std::os::unix::prelude::OpenOptionsExt for OpenOptions {
self
}
fn custom_flags(&mut self, flags: i32) -> &mut OpenOptions {
pub fn custom_flags(mut self, flags: i32) -> Self {
match &mut self.inner {
Inner::StdFs(x) => {
let _ = x.custom_flags(flags);