Samba Access Wrapper

Question

I created a Python module to access Samba shares some time ago, using Mike Teos' SMB module as some kind of a base to it. It is mostly a wrapper, but as I said, I wrote it some time ago and would appreciate feedback on readability, usability and modularity of this module.

ServerAccess.py

#!/usr/bin/env python2
# -*- coding: utf-8 -*-
__DEBUG__=9
from smb.SMBConnection import SMBConnection
from smb.base import SharedDevice
from smb.base import SharedFile
import tempfile
from socket import getfqdn
from Networking import resolveNetworkTuple
import logging
import sys
import os # Path operations
class ServerException(Exception):
 pass
class ServerAccess:
 """
 Description: ServerAccess Class and File Handler
 """
 def __init__(self,username, password, client_name=None, server_name=None, server_ip=None, shared_folder=None, timeout=5):
 self.__logger=logging.getLogger("Notenverwaltung")
 # use_ntlm_v2 must be passed as named argument
 self.__logger.debug(" username = %s"%(username))
 self.__logger.debug(" password = xxxxxxxxxx")
 self.__logger.debug(" client_name = %s"%(client_name))
 self.__logger.debug(" server_name = %s"%(server_name))
 self.__logger.debug(" server_ip = %s"%(server_ip))
 self.__logger.debug(" shared_folder = %s"%(shared_folder))
 self.__logger.debug(" timeout = %s"%(timeout))
 self.__user=username
 self.__pass=password
 if client_name==None:
 client_name=getfqdn()
 self.__client=client_name
 if server_ip:
 server_ip, server_name=resolveNetworkTuple("%s"%server_ip)
 elif server_name:
 server_ip, server_name=resolveNetworkTuple("%s"%server_name)
 self.__server=server_name
 self.__serverIP=server_ip
 self.__port=445
 self.__timeout=timeout
 self.conn = SMBConnection(username, password, client_name, server_name, use_ntlm_v2 = False)
 self.shared = shared_folder
 self.__logger.debug("Constructor connects to: \n%s"%self)
 assert self.conn.connect("%s"%self.__serverIP, self.__port, timeout=self.__timeout)
 def __del__(self):
 try:
 self.conn.close()
 except:
 pass
 def __str__(self):
 result=("\n<Server Access Object>")
 result+=("\n <user>%s</user>"%self.__user)
 result+=("\n <pass>xxxxxx</pass>")
 result+=("\n <client>%s</client>"%self.__client)
 result+=("\n <server>%s</server>"%self.__server)
 result+=("\n <serverIP>%s</serverIP>"%self.__serverIP)
 result+=("\n <port>%s</port>"%self.__port)
 if self.shared:
 result+=("\n <share>%s</share>"%self.shared)
 result+=("\n <timeout>%s</timeout>"%self.__timeout)
 result+=("\n</Server Access Object>")
 return result
 def __getHost(self): return self.__server
 def __setHost(self, value): self.__server=value
 Host=property(__getHost, __setHost)
 def listFiles(self, share=None, path=""):
 if share!=None:
 return self.conn.listPath(share, path, 55)
 elif self.shared:
 return self.conn.listPath(self.shared, path, 55)
 def listDirectories(self, share=None, path=""):
 return ([ __dir for __dir in self.listFiles(share, path) if __dir.isDirectory])
 def listShares(self):
 self.__ServerShares=[]
 if self.conn:
 for dev in self.conn.listShares(self.__timeout):
 if dev.type == SharedDevice.DISK_TREE:
 self.__ServerShares.append(dev.name)
 return self.__ServerShares
 def getFile(self, filename, share=None):
 file_obj = tempfile.NamedTemporaryFile()
 if share==None: share=self.shared
 file_attributes, filesize = self.conn.retrieveFile(share,filename,file_obj)
 # Otherwise all we get is crap
 file_obj.seek(0,0)
 return file_obj
 def writeFile(self,file_obj,filename, share=None):
 if share!=None: self.shared=share
 filename=os.path.normpath(filename)
 filename=filename.replace('\\','/')
 __curr_dir=""
 dirnames=os.path.split(filename)
 for __dir_idx in range(len(dirnames[:-1])):
 __curr_dir="%s%s/"%(__curr_dir, dirnames[__dir_idx])
 self.writeDirectory("%s"%__curr_dir)
 try:
 self.conn.storeFile(self.shared,"%s"%filename,file_obj)
 except:
 raise ServerException("Creating File <%s> failed using %s"%(filename, self))
 def writeDirectory(self, dirpath, share=None):
 if share!=None: self.shared=share
 # Check if Path already exists
 if len(self.conn.listPath(self.shared, dirpath))!=0:
 return
 try:
 self.conn.createDirectory(self.shared, dirpath)
 except Exception as ex:
 raise ServerException("Creating Directory <%s> failed using %s"%(dirpath, self))
 def moveFile(self,filename1,filename2):
 file_obj = tempfile.NamedTemporaryFile()
 file_attributes, filesize = self.conn.retrieveFile(self.shared,filename,file_obj)
 self.conn.storeFile(self.shared,filename2,file_obj)
 file_obj.close()
 self.conn.deleteFiles(self.shared,filename1)
 def deleteFile(self, filename, share=None):
 if share!=None: self.shared=share
 self.conn.deleteFiles(self.shared,filename)
 def deleteDirectory(self, dirpath, share=None):
 if share!=None: self.shared=share
 # Check if Path already exists
 if len(self.conn.listPath(self.shared, dirpath))!=0:
 return
 try:
 self.conn.deleteDirectory(self.shared, dirpath)
 except:
 raise ServerException("Deletion of Directory <%s> failed using %s"%(dirpath, self))
if __name__=="__main__":
 import argparse, getpass
 import Networking, socket
 logger=logging.getLogger("ServerAccess")
 parser=argparse.ArgumentParser(description="Server Access Library")
 parser.add_argument('--host', type=str, required=True, dest='host')
 parser.add_argument('-u', '--user', type=str, required=True, dest='user')
 parser.add_argument('-p', '--password', dest='usePW', default=False, action='store_true')
 parser.add_argument('-s', '--share', type=str, dest='share', default="")
 args=parser.parse_args(sys.argv[1:])
 hostIP, hostName=Networking.resolveNetworkTuple(args.host)
 logger.debug("%s trying to connect to %s"%(socket.getfqdn(), hostName))
 if args.usePW:
 password=getpass.getpass("Please enter password: ")
 if args.share!="":
 app=ServerAccess(args.user, password, socket.getfqdn(), hostName, hostIP, args.share)
 print args.share
 for shareFile in app.listFiles(args.share):
 if not shareFile.filename in [".",".."]:
 logger.debug(" %s"%shareFile.filename)
 else:
 app=ServerAccess(args.user, password, server_name=hostName, server_ip=hostIP)
 for share in app.listShares():
 logger.debug(share)
 for shareFile in app.listFiles(share):
 if not shareFile.filename in [".",".."]:
 logger.debug(" %s"%shareFile.filename

Networking.py is a module I wrote myself as well:

Networking.py

#!/usr/bin/env python2
# -*- coding: utf-8 -*-
import socket, re, time
import logging
from errno import ECONNREFUSED
__DEBUG__=1
logger=logging.getLogger("Networking")
def resolveNetworkTuple(addr):
 '''
 Resolve addr to receive IP Address and Hostname
 '''
 IP=""
 Hostname=""
 logger.info("Trying to resolve %s ... "%addr)
 if re.compile("^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}$").match(addr):
 # We got an IP
 logger.info("Read address [%s] as IP"%addr)
 IP=addr
 Hostname=socket.gethostbyaddr(IP)[0]
 else:
 # We got a Hostname
 Hostname=addr
 logger.info("Read address [%s] as Hostname"%Hostname)
 #we resolve until we get info or max retries reached
 __tries=5
 while IP=="" and __tries>0:
 try:
 logger.info("Resolve %s: %s"%(Hostname, socket.getaddrinfo(Hostname, 80, socket.AF_INET)))
 IP=socket.getaddrinfo(Hostname, 80, socket.AF_INET)[0][4][0]
 except:
 IP=""
 finally:
 time.sleep(2)
 __tries=__tries-1
 logger.info("Result: %s | %s (IP, Hostname)"%(IP, Hostname))
 return IP, Hostname

Answer 1

Please stick to 4 spaces for indentation. It's the Python standard and makes things much easier to read. You can read about this and many other Python style notes in the official style guide, I highly recommend it for readability.

Instead of == None it's more Pythonic to use is None.

You're also using the old % string formatting, use the newer one instead as it makes things easier and more readable. The new way is str.format, here's an example:

self.__logger.debug("Constructor connects to: \n%s"%self)

turns into

self.__logger.debug("Constructor connects to: \n{}".format(self))

Those {} get replaced by the parameters passed to format. It might not seem much use in this case, but later when you have multiple parameters it'll be much better, so it's good to be used to it.

It's great to see a __str__, implementing them is good practice. You can make your life a whole lot easier though by using implicit string concatenation. If two string literals are placed side by side with nothing but space between them, Python just concatenates them observe:

>>> "concat" "enation"
'concatenation'
>>> "concat" "enation"
'concatenation'

This can work over multiple lines, if you put parentheses around the full expression:

>>> ("concat"
 "enation")
'concatenation'
>>> ("concat"
 "ena"
 "tio"
 "n")
'concatenation'

Of course this can make your result making much easier:

def __str__(self):
 result = ("\n"
 "\n %s"%self.__user
 "\n xxxxxx"
 "\n %s"%self.__client
 "\n %s"%self.__server
 "\n %s"%self.__serverIP
 "\n %s"%self.__port)
 if self.shared:
 result+=("\n %s"%self.shared)
 result += ("\n %s"%self.__timeout
 "\n")
 return result

Avoid using bare try excepts! You do it a few times. This will ignore any exceptions that arise. Even something like a SystemExit or a KeyboardInterrupt. You should at least use except Exception: in order to ignore those two cases. It would also be a good idea to log what exceptions occurred, as it could have been one you weren't anticipating. If you don't log unanticipated exceptions then you'll never know what they are, nor will you know how to fix the bug causing them.

You should always put import statements at the top of the file, so that people know where to look for them when they see you using them. Otherwise people will get confused when they scroll to the top and don't see them anywhere.

• R4PH43L: Thank you very much for your input, i will update the question as soon as i reworked the code for another review round.
• SuperBiasedMan: @R4PH43L Glad to help. When you want to get another review, make sure you ask a new question and then just link to this one explaining it's a follow up.
• R4PH43L: Is it better to use property or __getattr__ __setattr__?
• SuperBiasedMan: @R4PH43L They're for different purposes. __getattr__ and __setattr__ modify how your class deals with all attributes when using dot syntax like class.attribute, whereas property allows you to set behaviour for one specific attribute.
• R4PH43L: So there wouldn't be a problem in overloading getattr__/__setattr, or would it? (As long as the time when they are called are taken into account)