Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

packetbeat: nfs: fix first-class operation code in compound #1821

Merged
merged 1 commit into from
Jun 24, 2016
Merged

packetbeat: nfs: fix first-class operation code in compound #1821

merged 1 commit into from
Jun 24, 2016

Conversation

kofemann
Copy link
Contributor

@kofemann kofemann commented Jun 8, 2016

NFS4 compound operations can be constructed from multiple
single requests. Nevertheless, each compound request as a
main operation.

current code failing to detect main operation if it was in
the middle of compound call.

Signed-off-by: Tigran Mkrtchyan [email protected]

@elasticsearch-release
Copy link

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

1 similar comment
@elasticsearch-release
Copy link

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

@ruflin
Copy link
Member

ruflin commented Jun 9, 2016

jenkins, test it

@@ -167,18 +167,20 @@ func (nfs *Nfs) eatData(op int, xdr *Xdr) {

func (nfs *Nfs) getV4Opcode(xdr *Xdr) string {

// did we found a main operation opcode?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't it say did we find ... ?

@kofemann
Copy link
Contributor Author

makes sense, updated.

@kofemann
Copy link
Contributor Author

@ruflin are there more changes required?

//
// GETATTR is the main operation.
//
// This function tries to guess main purpose of the request. If not found, then the last operation is used.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice explanation. Thanks!!!

some minor details:

  • I don't see the caller of getV4Opcode being changed. Does it compile?
  • spacing on line 170 and 172 seems to be a little off
  • godoc convention kinda mandates putting the function and short description first (before going into detail):
findV4MainOpcode finds the main operation in a compound call. If no main operation can be found, the last operation in compound call is returned.

Compount requests group ...

You can remove the last line This function tries..., as this information should be present at beginning of comment (see my sample).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Is there a chance of having multiple 'main' operations in one compound request?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

findV4MainOpcode is called in nfs.go:29, it compiles :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, one can build a compound with READDIR and WRITE in it. In practice, nfs clients don't do that, as all nfs requests triggered by user activities, and there are no posix call that will trigger readdir+write. Well, synthetic clients can....but they used only for server functional testing.

NFS4 compound operations can be constructed from multiple
single requests. Nevertheless, each compound request as a
main operation.

current code failing to detect main operation if it was in
the middle of compound call.

Signed-off-by: Tigran Mkrtchyan <[email protected]>
@urso
Copy link

urso commented Jun 24, 2016

Thanks for the fix and all the patience!

@urso urso merged commit b88eb05 into elastic:master Jun 24, 2016
@kofemann kofemann deleted the fix-compound-opcode branch June 24, 2016 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants