Close Open disk file handles#147
Conversation
k8s-ci-robot
added
release-note-none
|
Hi @wongma7. Thanks for your PR. I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
needs-ok-to-test
|
/assign @jingxu97 |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| page83ID := []byte{} | ||
| byteSize := unsafe.Sizeof(byte(0)) | ||
| for n = 0; n < devIDDesc.NumberOfIdentifiers; n++ { | ||
| if pID.Association == StorageIDAssocDevice && (pID.CodeSet == StorageIDCodeSetBinary || pID.CodeSet == StorageIDCodeSetASCII) { |
There was a problem hiding this comment.
I saw that this line is different in GetDiskPage83ID, @ddebroy might help us here
| for n = 0; n < devIDDesc.NumberOfIdentifiers; n++ { | ||
| if pID.Association == StorageIDAssocDevice && (pID.CodeSet == StorageIDCodeSetBinary || pID.CodeSet == StorageIDCodeSetASCII) { | ||
| for m = 0; m < pID.IdentifierSize; m++ { | ||
| page83ID = append(page83ID, *(*byte)(unsafe.Pointer(uintptr(unsafe.Pointer(&pID.Identifier[0])) + byteSize*uintptr(m)))) |
There was a problem hiding this comment.
page83ID will only be updated if pID.CodeSet == StorageIDCodeSetBinary || pID.CodeSet == StorageIDCodeSetASCII, however in GetDiskPage83ID it's updated without this condition, I think that's the only difference between these two methods
There was a problem hiding this comment.
good catch, it looked like just an optimization to me to check CodeSet early because, if the CodeSet is neither, there is no way that the function will find the page83ID there. behavior should be the same with or without the condition
There was a problem hiding this comment.
never mind, it looks like codeset can also be "UTF8" or "reserved"? probably better to have the condition.
There was a problem hiding this comment.
I think we restricted with the extra conditions for Ascii and Binary because those are the ones that were handled in the conversions with string and hex.EncodeToString a few lines below. Extending it to utf8 would be nice. For Reserved, we have no idea how to handle the formatting.
There was a problem hiding this comment.
I added the conditions back just to be safe. https://github.com/kubernetes-csi/csi-proxy/pull/147/files#diff-8dec85397edab1ec05fe2a8d6f36c534b7cd19dd8d6db9010b75536f597a54a6R223
I guess Reserved means it is reserved by spec and should never ever be seen in practice. i suppose utf8 might in the future but, at least for ebs disks none have utf8, i'll leave it out of scope of this PR
ddebroy
left a comment
ddebroy
left a comment
There was a problem hiding this comment.
LGTM with one suggestion
|
|
||
| diskNumber, err := imp.GetDiskNumber(h) | ||
| if err != nil { | ||
| return 0, "", err |
There was a problem hiding this comment.
A DiskNumber of 0 is a valid ID for a disk enumerated as \.\PhysicalDrive0. So returning 0 for the error scenario is not recommended. Maybe return a pointer to uint32 or -1?
There was a problem hiding this comment.
hmm I would expect caller to always check the error first. -1 i can't do since it's unsigned (or caller needs to convert to unsigned), pointer maybe I could do but then the caller needs to always check if it's nil before dereference, and I would argue in that case they should just check the error
There was a problem hiding this comment.
Looks like in other functions we return 0 in the error case too. So makes sense to check error first.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ddebroy, wongma7 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
6 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
this is a continuation/refactor of #126
@jingxu97 PTAL. as you suggested I combined the two confusing functions into one, rebased on the v1beta3 PR, then retested everything and it works! i.e. EBS detach succeeds!
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: