lithammer commented on code in PR #2192:
URL: 
https://github.com/apache/incubator-opendal/pull/2192#discussion_r1221638905


##########
core/src/services/sftp/backend.rs:
##########
@@ -461,58 +442,102 @@ impl Accessor for SftpBackend {
     }
 
     async fn list(&self, path: &str, args: OpList) -> Result<(RpList, 
Self::Pager)> {
-        let client = self.sftp_connect().await?;
-        let mut fs = client.sftp.fs();
-        fs.set_cwd(self.root.clone());
+        let client = self.connect().await?;
+        let mut fs = client.fs();
+        fs.set_cwd(&self.root);
 
         let file_path = format!("./{}", path);
 
-        let mut dir = match fs.open_dir(file_path.clone()).await {
+        let dir = match fs.open_dir(&file_path).await {
             Ok(dir) => dir,
             Err(e) => {
                 if is_not_found(&e) {
-                    return Ok((RpList::default(), SftpPager::empty()));
+                    return Ok((RpList::default(), None));
                 } else {
                     return Err(e.into());
                 }
             }
-        };
-        let dir = dir.read_dir().await?;
+        }
+        .read_dir();
 
         Ok((
             RpList::default(),
-            SftpPager::new(dir.into_inner(), path.to_owned(), args.limit()),
+            Some(SftpPager::new(dir, path.to_owned(), args.limit())),
         ))
     }
 }
 
 impl SftpBackend {
-    async fn pool(&self) -> Result<&bb8::Pool<Manager>> {
-        let pool = self
-            .sftp
-            .get_or_try_init(|| async {
-                let manager = Manager {
-                    endpoint: self.endpoint.clone(),
-                    user: self.user.clone(),
-                    key: self.key.clone(),
-                };
-
-                bb8::Pool::builder().max_size(10).build(manager).await
+    async fn connect(&self) -> Result<&Sftp> {
+        let sftp = self
+            .client
+            .get_or_try_init(|| {
+                Box::pin(connect_sftp(
+                    self.endpoint.as_str(),
+                    self.root.clone(),
+                    self.user.clone(),
+                    self.key.clone(),
+                    self.known_hosts_strategy.clone(),
+                ))
             })
             .await?;
 
-        Ok(pool)
+        Ok(sftp)
+    }
+}
+
+async fn connect_sftp(
+    endpoint: &str,
+    root: String,
+    user: String,
+    key: Option<String>,
+    known_hosts_strategy: KnownHosts,
+) -> Result<Sftp> {
+    let mut session = SessionBuilder::default();
+
+    session.user(user);
+
+    if let Some(key) = &key {
+        session.keyfile(key);
     }
 
-    pub async fn sftp_connect(&self) -> Result<PooledConnection<'_, Manager>> {
-        let conn = self.pool().await?.get().await?;
+    // set control directory to avoid temp files in root directory when panic
+    if let Some(dir) = dirs::runtime_dir() {
+        session.control_directory(dir);
+    }
 
-        Ok(conn)
+    #[cfg(target_os = "macos")]
+    {
+        let _ = std::fs::create_dir("/private/tmp/.opendal/");
+        session.control_directory("/private/tmp/.opendal/");
     }
 
-    pub async fn sftp_connect_owned(&self) -> Result<PooledConnection<'static, 
Manager>> {
-        let conn = self.pool().await?.get_owned().await?;
+    session.server_alive_interval(Duration::from_secs(5));
+    session.known_hosts_check(known_hosts_strategy);
 
-        Ok(conn)
+    let session = session.connect(&endpoint).await?;
+
+    let sftp = Sftp::from_session(session, SftpOptions::default()).await?;
+
+    let mut fs = sftp.fs();
+    fs.set_cwd("/");
+
+    let paths = Path::new(&root).components();
+    let mut current = PathBuf::from("/");

Review Comment:
   Why not use the default directory set by the server instead? This would 
allow you to use relative paths which is a lot more convenient in an SFTP 
environment instead of trying to navigate to the user's home directory. On some 
servers you might not even have permission to change directory to `/`.
   
   How about something like this? It would work for both relative and absolute 
paths.
   
   ```diff
   diff --git a/core/src/services/sftp/backend.rs 
b/core/src/services/sftp/backend.rs
   index 8bed2bf0..15fefdb9 100644
   --- a/core/src/services/sftp/backend.rs
   +++ b/core/src/services/sftp/backend.rs
   @@ -535,12 +535,11 @@ async fn connect_sftp(
        let sftp = Sftp::from_session(session, SftpOptions::default()).await?;
    
        let mut fs = sftp.fs();
   -    fs.set_cwd("/");
    
        let paths = Path::new(&root).components();
   -    let mut current = PathBuf::from("/");
   +    let mut current = fs.cwd().to_path_buf();
        for p in paths {
   -        current = current.join(p);
   +        current.push(p);
            let res = fs.create_dir(p).await;
    
            if let Err(e) = res {
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to