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

Closed
wants to merge 8 commits into from
Closed

DFS support #21

wants to merge 8 commits into from

Conversation

tooptoop4
Copy link

Fixes #20

see hirochachacha/go-smb2#79

credit to @ashutosh-bhole-druva hirochachacha/go-smb2#79

@tooptoop4
Copy link
Author

@arashpayan can u merge?

README.md Outdated Show resolved Hide resolved
@tooptoop4 tooptoop4 requested a review from b-turchyn February 13, 2024 17:28
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
@tooptoop4 tooptoop4 requested a review from b-turchyn February 13, 2024 19:49
@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.

@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.

@cyberb
Copy link

cyberb commented Jul 19, 2024

sorry is it possible to merge this?

}

// this is for handling multiple calls
fs.mapWriterLock.Lock()
Copy link

Choose a reason for hiding this comment

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

panics with nil pointer as mapWriterLock is never set


//check if its present in the map
//TODO: Add the TTL for this target and check here. Otherwise invalidate this cache entry
cachedTargetList, ok := fs.dfsTargetList[getFirstChild(dfsname, dirname, isLink)]
Copy link

Choose a reason for hiding this comment

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

dfsTargetList is not initialized either

ctx context.Context
ctx context.Context
dfsTargetList map[string][]*DFSTarget //For caching the DFS targets for a path
mapWriterLock *sync.RWMutex
Copy link

Choose a reason for hiding this comment

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

if you make it non pointer then you do not need to initialize it

@tooptoop4
Copy link
Author

stale

@tooptoop4 tooptoop4 closed this Oct 17, 2024
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
4 participants