Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improves parsing of Unix format file names, updates comments #523

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

nbriggs
Copy link
Collaborator

@nbriggs nbriggs commented Dec 28, 2024

  • Adds a (static inline) helper, parse_file_version() to version parsing code.

    • Removes handling of "filename%" as version 0 (version 0 is not valid)
    • Adds handling of Alto/IFS version "filename!nnn"
    • Improves robustness of version parsing (no integer overflow errors)
  • Replaces private FNAMETOOLONG error code with ENAMETOOLONG standard error.

  • Replaces tests for "magic numbers" with standard ERRNO values (ENOENT, EXDEV) in error case for rename() operations.

  • Reduces storage requirement for versions from 16 bytes to 10 bytes to match the maximum acceptable version number, "999999999".

  • Updates various code comments to be correct and clearer.

* Adds a (static inline) helper, parse_file_version() to version parsing code.
  - Removes handling of "filename%" as version 0 (version 0 is not valid)
  - Adds handling of Alto/IFS version "filename!nnn"
  - Improves robustness of version parsing (no integer overflow errors)

* Replaces private FNAMETOOLONG error code with ENAMETOOLONG standard error.

* Replaces tests for "magic numbers" with standard ERRNO values
  (ENOENT, EXDEV) in error case for rename() operations.

* Reduces storage requirement for versions from 16 bytes to 10 bytes to
  match the maximum acceptable version number, "999999999".

* Updates various code comments to be correct and clearer.
@nbriggs
Copy link
Collaborator Author

nbriggs commented Dec 28, 2024

I think this is ready to merge -- I'd appreciate testing on non-macOS systems.

@pamoroso
Copy link

pamoroso commented Jan 2, 2025

Initial testing on amd64 Linux Mint 22 Cinnamon reveals no issues so far.

@nbriggs
Copy link
Collaborator Author

nbriggs commented Jan 2, 2025

Edge cases are an excellent candidate for testing. MAXVERSION is 999999999, so, for example:

(CLOSEF(OPENSTREAM "{dsk}/tmp/testversion.;999999998" 'OUTPUT 'NEW)) ; should succeed
(COPYFILE "{dsk}/tmp/testversion" "{dsk}/tmp/testversion") ; should succeed
(COPYFILE "{dsk}/tmp/testversion" "{dsk}/tmp/testversion") ; should fail because next version too large

(currently it'll take a SIGSEGV and fall into URaid - I have a fix underway.)

@nbriggs
Copy link
Collaborator Author

nbriggs commented Jan 2, 2025

Errors reported back from the DSK and UFS code return an "errno" value, which is processed by \UFSError to generate an error (ERROR ...) or an exception (CL:ERROR ...) but only for the very limited number of errno values that are hardcoded. While we can't produce particularly useful actions/messages for every errno, I think we should at least generate some (possibly generic) error for all non-zero codes. We should also consider whether there are Lisp-specific codes we should standardize on, outside the normal range of errno values, for Maiko to report certain error conditions that are due to its limitations, such as "version number too large", or "too many versions"

…ine procedures.

    These are used in many places so code bloat can be reduced by using procedures.
@pamoroso
Copy link

pamoroso commented Jan 3, 2025

Here are the results of your tests on amd64 Linux Mint 22 Cinnamon.

(CLOSEF(OPENSTREAM "{dsk}/tmp/testversion.;999999998" 'OUTPUT 'NEW)) yields:

*** buffer overflow detected ***: terminated
Aborted (core dumped)

The other tests report the file is not found.

The head of my local copy of the report from git log:

commit d6e2e12df589c10c8c541ab5221df7e471002b2e (HEAD -> nhb-rewrite-version-parser, origin/nhb-rewrite-version-parser)
Author: Nick Briggs <[email protected]>
Date:   Thu Jan 2 15:24:52 2025 -0800

    Adds error returns for case where new file version would exceed MAXVERSION

commit ca98cc2606609475d95bb7cc29d4e73d6846997e
Author: Nick Briggs <[email protected]>
Date:   Thu Jan 2 15:17:42 2025 -0800

    Replaces ConcDirAndName and ConcNameAndVersion macros with static inline procedures.
    
        These are used in many places so code bloat can be reduced by using procedures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants