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

DFS support #21

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

DFS support #21

wants to merge 8 commits into from

Conversation

tooptoop4
Copy link

@tooptoop4
Copy link
Author

@arashpayan can u merge?

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link

@b-turchyn b-turchyn left a comment

Choose a reason for hiding this comment

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

Compilation error that needs to be resolved, otherwise the PR will break the library.

internal/smb2/request.go Show resolved Hide resolved
internal/smb2/request.go Outdated Show resolved Hide resolved
@b-turchyn
Copy link

Thanks for providing the missing file.

A few points:

  1. I've hit some defects in the code from my original testing, namely that Share doesn't have mapWriterLock and dfsTargetList populated when mounting a share. I've got this fixed locally and successfully traversed one DFS link. Which leads into...
  2. Something I've noticed while testing this is if the DFS link is mid-way through the network share, this implementation won't detect it. Rather, it's the responsibility of the user of this new functionality to iterate through each step of the path, see if there's a link present, and traverse it to go to the next server.
    i.e. if the user wants to mount \\example.com\foo\bar\baz and bar is the DFS link, this current implementation is clunky.
    I think there's opportunities to make this nicer. Some sort of helper to be able to access the "true" location across multiple directory traversals would be a welcome addition.

I don't want to downplay the value that this will provide. It's still better than no DFS support, and that's great. But usage is based around the assumptions that:

  1. The developer knows which specific shares are the DFS links
  2. The developer is willing to be responsible for handle the jumping between servers to get to the final target

Maybe the original developer anticipated some of this. Not sure! It's more likely that I'm stoopid and am missing something.

Others are more than welcome to get into this and kick the tires. I have no affiliation to this project; I'm just interested in this functionality for my own needs, and my Golang and SMB knowledge are weak enough that I wouldn't consider my approval to carry any weight. All I'll really be able to do is give an "it works for my use case" thumbs-up.

@arashpayan
Copy link
Collaborator

I intend to review it and bring it in. I'll be working on/with this package next week, so I'll have hours allocated to review this then. In the meantime, I'd appreciate any feedback from others similar to what @b-turchyn provided. I'm a complete novice when it comes to DFS so it'll take me some time to get up to speed, and there's a good chance I'll still overlook something.

@tooptoop4
Copy link
Author

👋

@arashpayan
Copy link
Collaborator

Don't worry. I haven't forgotten about this PR. I've recently been working on adding support for getting and setting security descriptors. Once I bring those changes into main I can focus on DFS.

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.

DFS support
3 participants