Skip to content

java.nio.FileSystem support#859

Open
dgodfrey wants to merge 37 commits intohierynomus:masterfrom
dgodfrey:feature/fs
Open

java.nio.FileSystem support#859
dgodfrey wants to merge 37 commits intohierynomus:masterfrom
dgodfrey:feature/fs

Conversation

@dgodfrey
Copy link
Copy Markdown

This mainly adds support for reading from SMB shares using a FileSystem implementation. It use the URI format from jcifs, to ease migrations.

It was developed on Windows, so hopefully is ok on Linux, and includes a few "fixes" to SmbStubServer for Windows and a change so it only rebuilds the docker image used for integration testing once, rather than every times it's used.

It just skips the build and uses the cache, and creates a new tag for
the same image.
This is to work around the server closing the connections.
This allows the FileSystem to be set-up, ahead of time using
newFilesystem, then to be used through Paths#get(URI) without a
reference to the Filesystem itself.
This is instead of a mock.
@dgodfrey dgodfrey requested a review from hierynomus as a code owner January 31, 2025 07:31
@laeubi
Copy link
Copy Markdown
Contributor

laeubi commented Mar 19, 2025

@dgodfrey I think it would help if you squash all commit into one here.

@hierynomus any chance to make progress here? I also using a private fork of this code since a while and like to see this in the main repo and contribute some additional fixes and enhancements to this.

@dgodfrey
Copy link
Copy Markdown
Author

I don't really like squash commits, as it doesn't tell the "story", although I suppose it won't make too much difference here as 95% of the files are new. I don't really know Github - is it possible to squash at merge time?

@laeubi
Copy link
Copy Markdown
Contributor

laeubi commented Mar 20, 2025

@dgodfrey yes you can do it in Github UI, while I agree that the history is often valuable for a PR it is often better to have a single commit (or maybe two) maybe done in a separate branch as it is easier to rebase if the repository code changes.

In any case @hierynomus has of course to tell what is the best here, it was just a thought as I see 30 commits are here for a single feature :-)

* @return the actual number of bytes that was written to the file
*/
public long write(ByteBuffer buffer, long fileOffset) {
public int write(ByteBuffer buffer, long fileOffset) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should not be truncated here, if the caller wants an int it is better cast there.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I changed them as the maximum size of a ByteBuffer is an int, so narrowed it here to match the read version and to do it once, rather than everybody who calls the method having to do it. But pretty easy either int or long.

* @return the actual number of bytes that were read; or -1 if the end of the file was reached
*/
public long read(ByteBuffer buffer, long fileOffset) {
public int read(ByteBuffer buffer, long fileOffset) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here better use a long as long as possible.

Copy link
Copy Markdown
Author

@dgodfrey dgodfrey Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was getting widened from an int to a long here, then any callers were needing to narrow it back to an int - this just removes that need. Pretty much the whole IO library uses int's for reading/writing bytes (I guess) because that's the maximum size of an array. But again, pretty easy on int or long.

@dgodfrey
Copy link
Copy Markdown
Author

Hi - @hierynomus do you have time to look at this - or would I be better putting it into a separate library?

@dkocher
Copy link
Copy Markdown

dkocher commented May 26, 2025

Hi - @hierynomus do you have time to look at this - or would I be better putting it into a separate library?

I am not a maintainer but I would prefer having this in a separate library keeping smbj lean.

@laeubi
Copy link
Copy Markdown
Contributor

laeubi commented May 26, 2025

Hi - @hierynomus do you have time to look at this - or would I be better putting it into a separate library?

I am not a maintainer but I would prefer having this in a separate library keeping smbj lean.

I don't see that this adds anything as it all is plain java API, having it directly in smbj (given its need for tight integration) makes sense as otherwise there is a need to maintain a stable API or expose internal accessors.

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.

3 participants