CIFS client support for Linux

[shibumi]

Supported are any client statistics in SMB1 and SMB2. This should fix #95

[mdlayher]

Thanks for your contribution. I think this needs some work in the readability department, and I can't say I'm comfortable with the heavy use of regex for string parsing.

Is there any better way? Is there a more machine-readable CIFS interface?

I believe some improvements can be made here that will make this code more maintainable in the long run.

[shibumi]

@mdlayher Sure. I am full with you about the part about readability. About the regex part: I don't think there is an easier way for this. You can have a look at the example files in the issue: #95 Any other idea is more than welcome :)

[shibumi]

I will overwork this and open a new PR, when it's ready.

[mdlayher]

No need to close it, just following up with more commits is totally fine!

[shibumi]

@mdlayher I have finally some time to code further on it. So I have reopen the pull Request and did further research. I've found another solution for getting CIFS statistics. The cifsiostat command provides the following output:

Linux 3.10.0-862.3.3.el7.x86_64 (SERVER) 	14.09.2018 	_x86_64_	(16 CPU)

Filesystem:                    rB/s         wB/s    rops/s    wops/s         fo/s         fc/s         fd/s
\\SERVER\SHARE         0,00         0,00      0,00      0,00         0,00         0,00         0,00 

This is much less output as we get via /proc/fs/cifs/Stats:

Resources in use
CIFS Session: 1
Share (unique mount targets): 1
SMB Request/Response Buffer: 1 Pool size: 5
SMB Small Req/Resp Buffer: 1 Pool size: 30
Operations (MIDs): 0

0 session 0 share reconnects
Total vfs operations: 85 maximum at one time: 2

1) \\SERVER\SHARE
SMBs: 81 Oplocks breaks: 0
Reads:  0 Bytes: 0
Writes: 0 Bytes: 0
Flushes: 0
Locks: 0 HardLinks: 0 Symlinks: 0
Opens: 0 Closes: 0 Deletes: 0
Posix Opens: 0 Posix Mkdirs: 0
Mkdirs: 0 Rmdirs: 0
Renames: 0 T2 Renames 0
FindFirst: 0 FNext 0 FClose 0

So I would prefer for writing a parser for /proc/fs/cifs/Stats (although it's far more complicated with a lot of context switches and other nasty things. I can't understand that there is no easy machine readable output in the kernel at the moment)

[shibumi]

@mdlayher @SuperQ Can one of you have a look at the newest commit? If this commit is okay I would rebase, squash and commit with sign-off message.

I am unhappy with a few things at the moment:

• I need to move the flush regex line for SMB2 very early in the Regex Array otherwise we will have a mismatch and we will pollute our SMB2 statistics with a SMB1 flush statistic
• I need to check for a null pointer before calling parseSMBStats
• I am using the currentSMBSessionIDs variable just as check for a nullpointer.. I am pretty sure there is a more convenient way to do this.

EDIT: I have changed the datastructure. I am using the same hashmap for SMB1 and SMB2 now. This way the code is a lot easier to read and it should be easier to access the data as well.

[shibumi]

@mdlayher @SuperQ Could you review again? It builds fine now, but it still uses the regex approach to get the statistics. A better solution would be to write an own parser for cifs statistics (maybe in a seperate project?). However this approach works fine, feedback to my code is welcome. I am still a golang newbie and need to learn a lot.

[discordianfish]

Can we use a single multline regex instead of this array? I think it would make the code be more readable.

[discordianfish]

Can you use different numbers in the second fixture? I think there might be an issue with the parsing.

[discordianfish]

Sorry for not responding earlier! So this looks really hard to parse.. :-/

I don't think the loop over the file is doing the right thing. After it finds the 'new smb block', it always run the parse block function. Either way, I'd suggest you first try to split the file into the 'global' and per mount blocks and parse these with big multiline regex. I think that would make the code much more readable.

[shibumi]

Hi @discordianfish,
Thanks for the cool review :) I am totally with you. A multiline regex makes sense here.
Let me see if I got your points corrrectly. You want to split the file into blocks + split the regex into three multiline regex blocks. And then I should just parse all blocks? What about building one giant multiline regex for the whole file and firing it on the whole file? Never done multiline regex in golang before.

I don't think the loop over the file is doing the right thing. After it finds the 'new smb block', it always run the parse block function.

The tests that I have attached are working fine.

[discordianfish]

Try using different numbers in the different fixtures, that should expose the issue. Or I'm wrong, quite possible :)
One giant regexp for the whole file probably doesn't work because the sections are repeating but dunno, maybe there is a way.

[shibumi]

Ok I tried using different numbers in the second fixture and you are correct. The tests will fail, but I have no idea why. When I introspect the two stats want and have they are the same. I compared them. I guess you are talking about the stats hashmap for the second fixture, correct? They have the same values. I will upload a new version with new values in the second fixtures.

About parsing it as one file:
Yes, it's correct. The sections can repeat. So you can either have only SMB1 blocks or only SMB2 blocks or a mixed file with SMB1 and SMB2 blocks in no particular order. It's also possible to have 20 SMB2 blocks and just 2 SMB1 blocks after the 5th SMB2 block..

this makes parsing of the cifsstats really difficult. It's strange that no kernel developer tried changing this already. I mean human readable output is nice, but it would be nice for us to have more machine readable output (the NFS stats are awesome for example)

[discordianfish]

I'm going to close this for now. Let's re-open when the changes are addressed.