worry about TODO's

TODO.md

@@ -2,13 +2,13 @@
 
 # Code Quality
 
-- [ ] redo logging to use the debug module
-- [ ] there are a bunch of TODO's in the code still.
+- [ ] #now redo logging to use the debug module
 - [ ] support node v20
 
 # DONE
 
 
+- [x] there are a bunch of TODO's in the code still -- read them; delete ones that aren't relevant or address, or leave ones that may matter later.  Result: most of the worrisome ones are in copy functions that FUSE doesn't use.
 - [x] Remove all auth (was: support auth, i.e., an optional symmetric key that clients must present to be allowed to mount the filesystem. This is useful for "defense in depth".). It's better to do the auth at a different level.
 - [x] delete the "WEB" comments in code...
 - [x] require require's to use static import syntax instead

lib/sftp/charsets.ts

@@ -38,7 +38,6 @@ export class StringEncoder {
   private _value: string;
   private _done: boolean;
 
-  //TODO: add write():bool, change finish() to end():void, then expect read()
   finished(): boolean {
     return this._done;
   }

lib/sftp/fs-local.ts

@@ -1,12 +1,14 @@
 import fs from "fs";
 import { IFilesystem, IItem, IStats, RenameFlags } from "./fs-api";
 import { FileUtil, Path } from "./fs-misc";
-
 // note that this is in node.js v 18.15 and later
 // as (fs/promises).statvfs (they are both wrapping
 // the same uv_fs_statfs).
 import { statvfs } from "@wwa/statvfs";
 import type { StatFs } from "./fs-api";
+import debug from "debug";
+
+const log = debug("websocketfs:fs-local");
 
 export class LocalFilesystem implements IFilesystem {
   private isWindows: boolean;
@@ -336,7 +338,7 @@ export class LocalFilesystem implements IFilesystem {
 
       fs.lstat(itemPath, (err, stats) => {
         if (typeof err !== "undefined" && err != null) {
-          //TODO: log unsuccessful stat?
+          log("readdir -- Failed to compute lstat of ", itemPath, err);
         } else {
           //
           items.push({
@@ -456,9 +458,6 @@ export class LocalFilesystem implements IFilesystem {
     this.checkCallback(callback);
     oldPath = this.checkPath(oldPath, "oldPath");
     newPath = this.checkPath(newPath, "newPath");
-
-    //TODO: make sure the order is correct (beware - other SFTP client and server vendors are confused as well)
-    //TODO: make sure this work on Windows
     fs.symlink(newPath, oldPath, "file", callback);
   }
 

lib/sftp/fs-misc.ts

@@ -530,7 +530,7 @@ export class FileUtil {
           return;
         }
 
-        return callback(new Error("Path is not a directory"), false); //TODO: better error
+        return callback(new Error("Path is not a directory"), false);
       }
 
       if ((<any>err).code != "ENOENT") {

lib/sftp/fs-safe.ts

@@ -11,6 +11,9 @@ import { IFilesystem, IItem, IStats, RenameFlags } from "./fs-api";
 import { FileUtil } from "./fs-misc";
 import crypto from "crypto";
 import { MAX_WRITE_BLOCK_LENGTH } from "./sftp-client";
+import debug from "debug";
+
+const log = debug("websocketfs:fs-fsafe");
 
 class HandleInfo {
   safe: number;
@@ -39,7 +42,8 @@ export class SafeFilesystem implements IFilesystem {
 
   private _handles: HandleToHandleInfoMap;
   private _nextHandle: number;
-  private static MAX_HANDLE_COUNT = 1024; // 1024 = standard linux default...
+  // 1024 = standard linux default...
+  private static MAX_HANDLE_COUNT = 1024;
 
   constructor(
     fs: IFilesystem,
@@ -108,9 +112,13 @@ export class SafeFilesystem implements IFilesystem {
       i++;
     }
 
-    if (this.isWindows) path = path.replace(/\\/g, "/");
+    if (this.isWindows) {
+      path = path.replace(/\\/g, "/");
+    }
 
-    if (path.length == 0) path = "/";
+    if (path.length == 0) {
+      path = "/";
+    }
 
     return path;
   }
@@ -174,19 +182,19 @@ export class SafeFilesystem implements IFilesystem {
   }
 
   end() {
-    if (!this.fs) return;
-
-    //TODO: make sure all pending operations either complete or fail gracefully
+    if (!this.fs) {
+      return;
+    }
 
     for (let handle = 1; handle <= SafeFilesystem.MAX_HANDLE_COUNT; handle++) {
       const handleInfo = this.toHandleInfo(handle);
       if (handleInfo && handleInfo.real !== null) {
         try {
-          this.fs.close(handleInfo.real, (_err) => {
-            //TODO: report this
+          this.fs.close(handleInfo.real, (err) => {
+            log("end: error closing one file handle", err);
           });
-        } catch (_err) {
-          //TODO: report this
+        } catch (err) {
+          log("end: error closing one file handle", err);
         }
       }
       delete this._handles[handle];
@@ -239,7 +247,11 @@ export class SafeFilesystem implements IFilesystem {
 
     function done(err: Error) {
       if (finished) {
-        //TODO: callback called more than once - this is a fatal error and should not be ignored
+        // callback called more than once - must be an internal bug
+        console.trace();
+        log(
+          "BUG in done -- a callback was called more than once - this indicates a bug in SFTP",
+        );
         return;
       }
       finished = true;
@@ -800,7 +812,8 @@ export class SafeFilesystem implements IFilesystem {
                 return callback(err, null, alg);
               }
 
-              //TODO: when we got incomplete data, read again (the functionality is already in fs-local and should be moved to fs-safe)
+              //TODO: when we got incomplete data, read again (the functionality is already 
+              // in fs-local and should be moved to fs-safe)
 
               // make sure we got the requested data
               if (bytesRead != bytesToRead) {

lib/sftp/sftp-client.ts

@@ -336,7 +336,8 @@ class SftpClientCore implements IFilesystem {
       }
     }
 
-    // @ts-ignore -- TODO: id is sometimes null in unit tests
+    // @ts-ignore -- id is sometimes null in the unit tests during init, 
+    // so this needs to be supported
     const request = this._requests[response.id];
     if (request == null) {
       throw Error("Unknown response ID");
@@ -760,8 +761,6 @@ class SftpClientCore implements IFilesystem {
     const h = this.toHandle(handle);
     this.checkPosition(position);
 
-    //TODO: reject requests with oversize response
-
     const request = this.getRequest(SftpExtensions.CHECK_FILE_HANDLE);
 
     request.writeData(h);

lib/sftp/sftp-misc.ts

@@ -482,8 +482,8 @@ export class SftpAttributes implements IStats {
         typeof stats.mtime !== "undefined"
       ) {
         flags |= SftpAttributeFlags.ACMODTIME;
-        this.atime = stats.atime ?? new Date(0); //TODO: make sure its Date
-        this.mtime = stats.mtime ?? new Date(0); //TODO: make sure its Date
+        this.atime = new Date(stats.atime ?? 0);
+        this.mtime = new Date(stats.mtime ?? 0);
       }
 
       if (typeof (<any>stats).nlink !== "undefined") {

lib/sftp/sftp-server.ts

@@ -111,7 +111,6 @@ class SftpException {
         break;
       case -2: // ENOENT on Linux with Node >=0x12 (or node-webkit - see http://stackoverflow.com/questions/23158277/why-does-the-errno-in-node-webkit-differ-from-node-js)
       case -4058: // ENOENT on Windows with Node >=0.12
-      //TODO: need to look into those weird error codes (but err.code seems to consistently be set to "ENOENT"
       case 34: // ENOENT
         message = "ENOENT"; // No such file or directory";
         code = SftpStatusCode.NO_SUCH_FILE;

lib/sftp/sftp.ts

@@ -171,7 +171,6 @@ module SFTP {
       }
 
       if (typeof virtualRoot === "undefined") {
-        // TODO: serve a dummy filesystem in this case to prevent revealing any files accidently
         virtualRoot = process.cwd();
       } else {
         virtualRoot = path.resolve(virtualRoot);
@@ -188,8 +187,6 @@ module SFTP {
         hideUidGid: true && options.hideUidGid,
       };
 
-      // TODO: when no _fs and no _virtualRoot is specified, serve a dummy filesystem as well
-
       if (!noServer) {
         log("Creating WebSocketServer");
         this._wss = new WebSocketServer(serverOptions);