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

Avoid using NetBIOS when direct_tcp is set to True #25

Merged
merged 3 commits into from
Apr 12, 2021

Conversation

frafra
Copy link
Contributor

@frafra frafra commented Mar 4, 2021

Fix #24.

@frafra frafra force-pushed the if-direct_tcp-then-no-netbios branch from d2163fd to 88b417c Compare March 4, 2021 12:52
@codecov
Copy link

codecov bot commented Mar 4, 2021

Codecov Report

Merging #25 (5c5047d) into master (4882466) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #25   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          444       447    +3     
=========================================
+ Hits           444       447    +3     
Impacted Files Coverage Δ
fs/smbfs/smbfs.py 100.00% <ø> (ø)
fs/smbfs/utils.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4882466...5c5047d. Read the comment docs.

@frafra
Copy link
Contributor Author

frafra commented Mar 4, 2021

An alternative solution would be to set NETBIOS to None in case direct_tcp is True and then alter get_hostname_and_ip to avoid using NetBIOS.

@frafra frafra mentioned this pull request Mar 23, 2021
@althonos
Copy link
Owner

althonos commented Apr 9, 2021

Hi @frafra, Sorry for the delay!

I'd be in favor of the following solution:

  • make get_hostname_and_ip accept None for the netbios argument, and only run lines 78-90 if netbios is not None.
  • make SMBFS.__init__ pass its NETBIOS instance only if direct_tcp is False:
     self._server_name, self._server_ip = utils.get_hostname_and_ip(
         host, 
         None if direct_tcp else self.NETBIOS,
         timeout=timeout,
         name_port=name_port
     )

This way we can keep the code duplication minimal, while still supporting swapping the IP and host in the input tuple, and properly report if one of the two is missing like before.

@frafra frafra force-pushed the if-direct_tcp-then-no-netbios branch from e4246d5 to 250adc3 Compare April 9, 2021 14:57
@frafra
Copy link
Contributor Author

frafra commented Apr 9, 2021

@althonos no worries :)

It seems OK to me now.

@althonos
Copy link
Owner

Thanks! I added some extra tests just to make sure it works as expected.

@althonos althonos merged commit eabb9b2 into althonos:master Apr 12, 2021
@frafra frafra deleted the if-direct_tcp-then-no-netbios branch September 2, 2022 09:13
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.

direct_tcp=True tries to use NetBIOS
2 participants