Skip to content

[REFACTOR] FSAL typed error enum to replace substring-based NFS error mapping #28

Description

@amarok-60T

Background

NFS handlers in src/nfs/*.rs currently map FSAL errors to nfsstat3 codes by
substring-matching error.to_string(). The pattern looks like:

let error_status = if e.to_string().contains("not found")
    || e.to_string().contains("Invalid handle")
{
    nfsstat3::NFS3ERR_STALE
} else {
    nfsstat3::NFS3ERR_IO
};

This was introduced opportunistically during the multi-export work (#26)
and now appears in roughly 14 sites across the NFS handlers, with subtly
different substrings ("not found", "No such", "no such file",
"Invalid handle", "cross-device" etc.). Each new FSAL error type
risks a silent fall-through to NFS3ERR_IO.

Scope

Replace the substring matchers with a typed FSAL error so handlers
classify errors at the type level:

pub enum FsalError {
    NotFound,
    InvalidHandle,
    PermissionDenied,
    ReadOnly,
    CrossDevice,
    NotSupported,
    Other(String),
}
  • Convert LocalFilesystem / MultiExportFilesystem methods to return
    Result<T, FsalError> instead of anyhow::Result<T>.
  • Update each NFS handler to match the error variant directly:
    match filesystem.getattr(&handle).await {
        Ok(attr) => ...,
        Err(FsalError::NotFound | FsalError::InvalidHandle) =>
            return create_pathconf_error(xid, nfsstat3::NFS3ERR_STALE),
        Err(FsalError::PermissionDenied) =>
            return create_pathconf_error(xid, nfsstat3::NFS3ERR_ACCES),
        Err(_) => return create_pathconf_error(xid, nfsstat3::NFS3ERR_IO),
    }
  • Remove the ~14 substring-match sites and the temporary helper at
    src/nfs/error.rs::classify_handle_error introduced by [FEATURE] Support multiple NFS exports #26.

Acceptance criteria

  • FsalError enum exists with variants covering every wire-mappable
    error category currently surfaced (NFS3ERR_STALE, NFS3ERR_IO,
    NFS3ERR_ACCES, NFS3ERR_ROFS, NFS3ERR_XDEV, NFS3ERR_NOTSUPP,
    NFS3ERR_PERM, etc.).
  • All Filesystem and ExportRegistry trait methods return
    Result<T, FsalError>.
  • LocalFilesystem and MultiExportFilesystem impls map IO/syscall
    errors into the correct variants (e.g. io::ErrorKind::NotFound
    FsalError::NotFound).
  • Every e.to_string().contains(...) call site in src/nfs/ is
    replaced with a type-level match.
  • src/nfs/error.rs::classify_handle_error (the multi-export
    transitional helper) is removed.
  • make test and make nfstest pass.
  • No regression in wire-level NFS status codes returned to clients —
    add a Python integration test that triggers each error category if
    one doesn't already exist.

References

  • The multi-export PR introduced the helper that this issue is meant to
    retire: #27
  • Sites currently using substring matchers (non-exhaustive grep at config: introduce per-export configuration #27 head):
    src/nfs/access.rs, src/nfs/commit.rs, src/nfs/create.rs,
    src/nfs/fsstat.rs, src/nfs/fsinfo.rs, src/nfs/lookup.rs,
    src/nfs/pathconf.rs, src/nfs/read.rs, src/nfs/setattr.rs,
    src/nfs/write.rs, src/nfs/rename.rs, src/nfs/link.rs, etc.
  • Phase 3 review identified this as a known fragility:

    "NFS handler 用 substring match 抓 error 換 typed enum"

  • Phase 4 / Phase 5 deliberately kept substring matching to avoid
    scope creep on PR config: introduce per-export configuration #27.

Out of scope (do separately)

  • Restructuring the broader FSAL trait hierarchy (this issue assumes the
    trait shape from [FEATURE] Support multiple NFS exports #26 stays).
  • Adding new NFS error mappings beyond what is already returned today
    (e.g. richer ACL errors).

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions