[Twisted-Python] SMB server component for twisted
I have begun work on a SMB (Server Message Block; Windows filesharing) server protocol for twisted. Work so far is here: https://github.com/ihaywood3/twsmb I'm looking for any advice particularly around what I should be doing so this code is suitable for inclusion in twisted. Ian Haywood
Hi Ian On Tue, 5 May 2020 at 05:17, Ian Haywood <ian@haywood.id.au> wrote:
I have begun work on a SMB (Server Message Block; Windows filesharing) server protocol for twisted.
Work so far is here: https://github.com/ihaywood3/twsmb
I'm looking for any advice particularly around what I should be doing so this code is suitable for inclusion in twisted.
Ian Haywood
Not sure if you want the repo included in the Twisted GitHub organization or the twisted/twisted repo? I think is best to have the SMB protocol implementation as a separate repo to be able to release at any time, but it can be added to Twisted organization. This is similar to the LDAP ldaptor repo. I don't think there are any rules for adding repos to the GitHub organization. I would say, as a minimal requirement are: * end user documentation * dev documentation / CONTRIBUTORS file * automated tests / CI I have a long term goal for SMB3 protocol implementation (client and server side). I am currently considering reusing https://github.com/jborean93/smbprotocol The network communication is not ideal... but the design is not that bad. That repo already has the Python code for parsing SMB messages. Regards
On May 4, 2020, at 9:17 PM, Ian Haywood <ian@haywood.id.au> wrote:
I have begun work on a SMB (Server Message Block; Windows filesharing) server protocol for twisted.
Very exciting! Thanks for telling us about this. It would be great to have a memory-safe/Python implementation of SMB3!
Work so far is here: https://github.com/ihaywood3/twsmb
I'm looking for any advice particularly around what I should be doing so this code is suitable for inclusion in twisted.
If you want to include it in Twisted itself, your best bet is to actually develop it within twisted, as a series of small contributions, rather than as one gigantic one-shot one. Contributions over, say, 400 lines, take exponentially longer to review. Developing it within Twisted will make things go slower; you'll need to get everything code reviewed, you'll need to support multiple versions of Python (no py2 any more, but py3.5 is still pretty old), you'll have to have full test coverage from the get-go. But doing these things from the start is much easier than trying to retrofit them. I actually think that this would be a pretty good fit for Twisted, in the same way that it's been a benefit to have Conch maintained alongside the rest of Twisted. I can see you're developing things very much in line with Twisted's architecture (using cred for authentication, a realm interface, etc) and you've voiced this interest, so it would be great to have you along! -g
On 7/05/2020 5:48 pm, Glyph wrote:
If you want to include it in Twisted itself, your best bet is to actually develop it /within/ twisted, as a series of small contributions, rather than as one gigantic one-shot one. Contributions over, say, 400 lines, take exponentially longer to review.
Sounds great, I'll prepare a GitHub PR. Unfortunately the first one will be ~2000 lines just to login and connect to a share, but after that each new packet-type will be small. Regarding unit-tests, I've found the best way to test the server is to use reactor.spawnProcess to launch the Samba command-line client, but that requires smbclient be available to run tests. Ian
On May 8, 2020, at 12:39 AM, Ian Haywood <ian@haywood.id.au> wrote:
On 7/05/2020 5:48 pm, Glyph wrote:
If you want to include it in Twisted itself, your best bet is to actually develop it /within/ twisted, as a series of small contributions, rather than as one gigantic one-shot one. Contributions over, say, 400 lines, take exponentially longer to review.
Sounds great, I'll prepare a GitHub PR. Unfortunately the first one will be ~2000 lines just to login and connect to a share, but after that each new packet-type will be small.
Regarding unit-tests, I've found the best way to test the server is to use reactor.spawnProcess to launch the Samba command-line client, but that requires smbclient be available to run tests.
You can feel free to edit the CI configuration to ensure that smbclient is installed in some of the environments to be sure it's tested. (And these kinds of little details are good reasons to do this develop-in-Twisted strategy; easier to deal with the gnarly configuration issues early than to try to debug them once you have a big pile of code.) However, tests like this are whole-system integration tests. While they're great, they also tend to be slow and flaky, particularly if you don't have other forms of testing. You might want finer-grained unit tests, particularly to deal with error scenarios or boundary conditions which might be hard to provoke with a real client doing real I/O. Were you thinking of writing an SMB client as well? One way to write these finer-grained tests is to have the client and the server talk to each other. It's been a long while since we had a major new subsystem. I'm excited to see this stuff come in! One minor note: you may want to do the initial development in an underscore-prefixed package, like twisted._fileserver or somesuch, so that we can separate out the discussion of "public API design" from the issues of code quality / test coverage / etc. -glyph
On May 7, 2020, at 12:48 AM, Glyph <glyph@twistedmatrix.com> wrote:
If you want to include it in Twisted itself, your best bet is to actually develop it within twisted, as a series of small contributions, rather than as one gigantic one-shot one. Contributions over, say, 400 lines, take exponentially longer to review.
Developing it within Twisted will make things go slower; you'll need to get everything code reviewed, you'll need to support multiple versions of Python (no py2 any more, but py3.5 is still pretty old), you'll have to have full test coverage from the get-go. But doing these things from the start is much easier than trying to retrofit them.
I actually think that this would be a pretty good fit for Twisted, in the same way that it's been a benefit to have Conch maintained alongside the rest of Twisted. I can see you're developing things very much in line with Twisted's architecture (using cred for authentication, a realm interface, etc) and you've voiced this interest, so it would be great to have you along!
I think it's great to get an SMB implementation in the Twisted org, but why would we even consider adding something like this to the main Twisted project? The main repo is large, and including Twisted in your project brings along far more functionality than you are likely to use, and packages are a fine way to get the functionality you want, so what's the logic for including a new thing in Twisted proper? You cite Conch above, where I'd actually argue that Conch should be moved to a separate repo. Am I crazy? That said, a counter argument is that projects in the Twisted org suffer greatly from poor participation and strict development rules, resulting in glacial progress. It's not unusual for me to have a PR take 6 months to a year to get a review (I have two right now that have been waiting since November), and perhaps such PRs would get some attention in the main repo. But I think that's an unfortunate way to address that problem. -wsv
participants (4)
-
Adi Roiban -
Glyph -
Ian Haywood -
Wilfredo Sánchez Vega