(Question) About Oplock implementation pull request

[yin19941005]

Hi @hierynomus,

I had implemented Oplock Support for Smbj in my fork repository : https://github.com/yin19941005/smbj/tree/implement-oplock. (Only oplock is implemented, lease is not implemented.)

However, I had tracked the implement-oplock branch as kind of my private master. If you check the commits, you can the see the SMB2_LOCK command, some hot fix is also included in this branch.

I am wondering should I directly make a pull request for the whole implement-oplock branch to master or should I perform something like git cherry pick to create new branch for each features/hot fix and make pull request separately (Just like #350 and #351)? Which approach do you prefer?

[hierynomus]

I just did a quick diff between your implement-oplock branch and the current master. Given the size of the changeset, I would prefer smaller PRs.

Also a few tests here and there would be welcome with the PRs 😉

[hierynomus]

Thanks for the work by the way!

[yin19941005]

OK, I will create new branches and separate the feature then. I will also try to add some test for each PR. However, the async create with oplock implement itself is still a huge change. Because the oplock break notification is a Server initiated event.

By the way, would you mind me to add dependency on Google Guava Library? I am looking for something like taskQueue to ensure the execute sequence and I find SequentialExecutor in Guava.

[hierynomus]

Yes, was guessing that that was somewhat larger in scope.

As for Guava, I do mind indeed. Guava is not nice to depend on from a library point of view. They have a bad reputation for backwards compatibility with different versions. A lot of times application code will depend on a specific guava version, which might (or might not) break the version the library depends on, or vice versa. If we need to ensure FIFO ordering, use a Deque for maintaining the queue.

[yin19941005]

Hi @hierynomus,

The async create and oplock implement branch is on https://github.com/yin19941005/smbj/tree/implement-async-create-oplock.

The size of changeset of this commit should very similar with the final one. Maybe you can have a look first and give some feedback/opinion about it. There are only two remaining items I had planned to do before making the pull request :

1. Implement the task queue with one multi-threaded pool and replace the Single Thread Executor
2. Add some Test for it

[hierynomus]

Why the 2 classes for the OplockBreak(Notification|AcknowledgementResponse). They're exactly the same. Why don't we merge them, and give the class an isNotification method that checks the messageId with the 0xFF * 16 array. This also cleans up the SMB2MessageConverter class which now needs to do some magic.

[yin19941005]

To explain this in short, because of Lease. You may understand Lease as kind of Oplock v2 and Lease Break Notification is quite different from Lease Break Response.

In [MS-SMB2] document, we have :

2.2.23 SMB2 OPLOCK_BREAK Notification 2.2.23.1 Oplock Break Notification 2.2.23.2 Lease Break Notification 2.2.24 SMB2 OPLOCK_BREAK Acknowledgment 2.2.24.1 Oplock Break Acknowledgment 2.2.24.2 Lease Break Acknowledgment

And both Oplock Break Notification and Lease Break Notification header's command MUST be set to SMB2 OPLOCK_BREAK.

In my original plan, I was planned to implement the Lease as well. Although now is not implemented yet, I assume we will implement the Lease in future (probably not recently). So, I think we should merge the Break Notification and Break Acknowledgment. I didn't drill deep into Lease yet, so I simply follow the way of how Microsoft document separate them. But indeed, from current implementation they looks almost the same.

@hierynomus, what is your opinion about this?

By the way, if you need for quick intro to Lease, you may check this article : Client caching features: Oplock vs. Lease https://blogs.msdn.microsoft.com/openspecification/2009/05/22/client-caching-features-oplock-vs-lease/

Another enhancement of lease over oplock is that SMB2.1 in Windows 7 allows full caching when multiple handles are opened by the same client. This is also a significant improvement over oplock semantics when any additional handle opened by even the same client will break the existing batch oplock and thus disable the client caching.

[hierynomus]

Just read through the spec for this part. It seems that all of the Oplock and Lease commands share the same message command code (SMB2_OPLOCK_BREAK). They're only distinguishable by looking at the StructureSize of the message.

I would model is using a single SMB2OplockBreakFactory which will construct the different kind of SMB2OplockBreak messages. and keep the internals of how that message is constructed internal to it. Also then create a message hierarchy under SMB2OplockBreak (i.e. SMB2OplockBreakNotification extends SMB2OplockBreak, etc).

[hierynomus]

Also that factory should then of course construct the Lease variants of the message when appropriate...

[yin19941005]

Just read through the spec for this part. It seems that all of the Oplock and Lease commands share the same message command code (SMB2_OPLOCK_BREAK). They're only distinguishable by looking at the StructureSize of the message.

Read the document as well. I agree with you, this seems to be the case.

Also then create a message hierarchy under SMB2OplockBreak (i.e. SMB2OplockBreakNotification extends SMB2OplockBreak, etc).

Yes, I think this should be done as well after reading the document. And of course Lease in future should also extends SMB2OplockBreak.

I would model is using a single SMB2OplockBreakFactory which will construct the different kind of SMB2OplockBreak messages. and keep the internals of how that message is constructed internal to it.

I think I got what you mean. On SMB2MessageConverter we should have something like :

case SMB2_OPLOCK_BREAK:
    return SMB2OplockBreakFactory.read(buffer);

And leave how to determine Oplock/Lease and Notification/Response within the SMB2OplockBreakFactory. Am I get your meaning correctly?

[hierynomus]

Yes correct!

Op vr 6 jul. 2018 06:00 schreef yin19941005 notifications@github.com:

Just read through the spec for this part. It seems that all of the Oplock and Lease commands share the same message command code (SMB2_OPLOCK_BREAK). They're only distinguishable by looking at the StructureSize of the message.

Read the document as well. I agree with you, this seems to be the case.

Also then create a message hierarchy under SMB2OplockBreak (i.e. SMB2OplockBreakNotification extends SMB2OplockBreak, etc).

Yes, I think this should be done as well after reading the document. And of course Lease in future should also extends SMB2OplockBreak.

I would model is using a single SMB2OplockBreakFactory which will construct the different kind of SMB2OplockBreak messages. and keep the internals of how that message is constructed internal to it.

I think I got what you mean. On SMB2MessageConverter we should have something like :

case SMB2_OPLOCK_BREAK: return SMB2OplockBreakFactory.read(buffer);

And leave how to determine Oplock/Lease and Notification/Response within the SMB2OplockBreakFactory. Am I get your meaning correctly?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/hierynomus/smbj/issues/352#issuecomment-402919051, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHLo7RysjQjAwRw3Kb8SnlGHZOwCsR2ks5uDuD4gaJpZM4VATzJ .