Added high-level 'Client' concept, tests code refactoring

[kvv81]

Effort for code simplicity, redability and no duplications (when suitable). Code improvement for complex use-cases when several parallel clients are used.

-Added smbclient.Client class -Extracted per-instance logic of global ClientConfig into ClientConfigInstance -Added new RealClientConfig class as a descendant of ClientConfigInstance -Fixed 'smb_real' fixture to return RealClientConfig instead of tuple -Refactored the code of tests to use smbclient.Client when suitable and use 'smb_real.attribute' instead of 'smb_real[number]'

Signed-off-by: Volodymyr Khomenko khomenko.volodymyr@gmail.com

[kvv81]

The main idea of this change is to make the code compact and simple for happy path. In the most cases you need only one connection, session and tree so lower-level objects of Connection, Session and TreeConnect take a lot of efforts for init/destroy. We need higher-level concept of 'Client' to use this abstraction for basic flows. For sure, in some cases (when you need several sessions per the same connection or several trees for the same session) you still need to use lover-level objects - as before.

Old abstraction level of smbclient._os POSIX-like functions (like mkdir and stat) are NOT suitable in some cases - when we want to use native Windows-like client flow. Also the mechanism of cached objects in pool makes this way less reusable - for example when we need two separate connections to the same server (but caching logic forces us to reuse the same connection).

I can propose the particular example we have - testing SMB parallel leases break flow (in our SMB server this code is very tricky). In this use-case, three different clients have separate connections to the same server/share and they want to operate with the same file. The first client opened the file-handle for this file with RHW lease (ReadHandleWrite, i.e. all possible client-side caching with exclusive access to file). After that two other concurrent clients want to open the same file with maximal possible lease level and some access_mask that may conflict with caching (for example, if some client want to Write the file, Read caching by other clients is not possible anymore and lease bust be broken to 'None' level).

Internally, on the reception of each conflicting Create request the server must send Lease break notification to the 1st client who still holds a cached state for it. And also the race between 2nd and 3rd client may occur so lease break notification can be sent several times to reduce the level of lease: RHW -> RH -> R -> None.

Without using higher-level concept of Client this code will be too messy (you need 12 different objects here - 3 Connections, 3 Sessions, 3 TreeConnects and 3 Opens).

[jborean93]

Sorry for the delay in reviewing this, I've been thinking about the changes and the best way forward. I can understand your use case here but I'm just wondering if you can re-utilise what's already in place to achieve your desired outcome.

My main goal of smbclient.ClientConfig is to provide a global configuration setting to replicate a Windows environment where you have a username and environment settings like a domain controller. Having a custom set of options for each os-like call is doable using the kwargs defined, if omitted then it falls back to the global ClientConfig options defined.

The first problem of having separate connections to the same server can be worked around by doing the following:

import smbclient

# Uses ClientConfig credentials (if set)
client1 = {
    "connection_cache": {},
}

# Uses it's own credentials
client2 = {
    "username": "user",
    "password": "pass",
    "connection_cache": {},
}

path = r'\\server\share\file.txt'
with smbclient.open_file(path, mode='rb', **client1) as fd1:
    with smbclient.open_file(path, mode='wb', **client2) as fd:
        ...

smbclient.reset_connection_cache(connection_cache=client1["connection_cache"])
smbclient.reset_connection_cache(connection_cache=client2["connection_cache"])

In the above case, the connection/session/tree objects are stored in the connection_cache of each client dict. Each file open to the path specified will be on a separate connection/session/tree. If all you care about is the cache and not having separate credentials you can just pass in connection_cache=cachex to the smbclient calls. The cache itself will contain the connection/session/tree objects and it won't be shared with the global "client".

There are unfortuantely downsides to this approach that I can see

• The caller is now responsible for clearing out the connection cache with smbclient.reset_connection_cache
• When sharing a connection, each session is only unique per username provided
  • It will still be a unique session per connection
• When sharing a session, each tree is only unique per share name provided
  • It will still be a unique tree per session

The first is annoying but is definitely fixable. The other 2 are arguably use cases that I don't think fit into the high level concepts that smbclient is designed to expose.

One last thing to note about smbclient is that I want to have a barrier between it and smbprotocol. That is I want to avoid, where possible, exposing smbprotocol objects like a connection/session/tree/file in the smbclient calls. There are definitely cases where I've failed to achieve this but that damage has already been done.

With the changes you have implemented I have no problem with trying to simplify the tests as they definitely sit on the verbose side. I'm just not sure that smbclient.Client is something that should exist. It seems to just be a way to couple the connection/session/tree instances from smbprotocol which going by my view on getting smbclient and smbprotocol separate is not something I think should be done. It really seems like a class that users of smbprotocol would implement themselves as per their requirements. I.e there is nothing stopping you from defining that same Client in your own code that calls smbprotocol to abstract some of the tersness that comes from the low level API. In the context of simplifying the tests you can define something like that in conftest.py for the other tests to use.

I appreciate the time and effort you've put into this so I'm more than happy to work through simplifying the testing side if you are still interested but smbclient.Client setup is not something I wish to add.