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.

Sign up for GitHub

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

Close Open disk file handles #147

Merged
merged 2 commits into from
Jun 3, 2021
Merged

Close Open disk file handles #147

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c277b04
Close Open disk file handles
wongma7 Jun 3, 2021
e9c0cbe
Check identifier page CodeSet
wongma7 Jun 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 25 additions & 62 deletions internal/os/disk/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,52 +194,6 @@ func (DiskAPI) GetDiskNumber(disk syscall.Handle) (uint32, error) {
return devNum.DeviceNumber, err
}

func (DiskAPI) DiskHasPage83ID(disk syscall.Handle, matchID string) (bool, error) {
query := StoragePropertyQuery{}

bufferSize := uint32(4 * 1024)
buffer := make([]byte, 4*1024)
var size uint32
var n uint32
var m uint16

query.QueryType = PropertyStandardQuery
query.PropertyID = StorageDeviceIDProperty

querySize := uint32(unsafe.Sizeof(query.PropertyID)) + uint32(unsafe.Sizeof(query.QueryType)) + uint32(unsafe.Sizeof(query.Byte))
querySize = uint32(unsafe.Sizeof(query))
err := syscall.DeviceIoControl(disk, IOCTL_STORAGE_QUERY_PROPERTY, (*byte)(unsafe.Pointer(&query)), querySize, (*byte)(unsafe.Pointer(&buffer[0])), bufferSize, &size, nil)
if err != nil {
return false, fmt.Errorf("IOCTL_STORAGE_QUERY_PROPERTY failed: %v", err)
}

devIDDesc := (*StorageDeviceIDDescriptor)(unsafe.Pointer(&buffer[0]))

pID := (*StorageIdentifier)(unsafe.Pointer(&devIDDesc.Identifiers[0]))

page83ID := []byte{}
byteSize := unsafe.Sizeof(byte(0))
for n = 0; n < devIDDesc.NumberOfIdentifiers; n++ {
if pID.Association == StorageIDAssocDevice && (pID.CodeSet == StorageIDCodeSetBinary || pID.CodeSet == StorageIDCodeSetASCII) {
Copy link
Member

Choose a reason for hiding this comment

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

I saw that this line is different in GetDiskPage83ID, @ddebroy might help us here

for m = 0; m < pID.IdentifierSize; m++ {
page83ID = append(page83ID, *(*byte)(unsafe.Pointer(uintptr(unsafe.Pointer(&pID.Identifier[0])) + byteSize*uintptr(m))))
Copy link
Member

@mauriciopoppe mauriciopoppe Jun 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

never mind, it looks like codeset can also be "UTF8" or "reserved"? probably better to have the condition.

Copy link
Contributor

@ddebroy ddebroy Jun 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

var page83IDString string
if pID.CodeSet == StorageIDCodeSetASCII {
page83IDString = string(page83ID)
} else if pID.CodeSet == StorageIDCodeSetBinary {
page83IDString = hex.EncodeToString(page83ID)
}
if strings.Contains(page83IDString, matchID) {
return true, nil
}
}
pID = (*StorageIdentifier)(unsafe.Pointer(uintptr(unsafe.Pointer(pID)) + byteSize*uintptr(pID.NextOffset)))
}
return false, nil
}

func (DiskAPI) GetDiskPage83ID(disk syscall.Handle) (string, error) {
query := StoragePropertyQuery{}

Expand All @@ -266,7 +220,7 @@ func (DiskAPI) GetDiskPage83ID(disk syscall.Handle) (string, error) {
page83ID := []byte{}
byteSize := unsafe.Sizeof(byte(0))
for n = 0; n < devIDDesc.NumberOfIdentifiers; n++ {
if pID.Association == StorageIDAssocDevice {
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))))
}
Expand Down Expand Up @@ -294,20 +248,39 @@ func (imp DiskAPI) GetDiskNumberWithID(page83ID string) (uint32, error) {
json.Unmarshal([]byte(outString), &disks)

for i := range disks {
h, err := syscall.Open(disks[i].Path, syscall.O_RDONLY, 0)
diskNumber, diskPage83ID, err := imp.GetDiskNumberAndPage83ID(disks[i].Path)
if err != nil {
return 0, err
}

found, err := imp.DiskHasPage83ID(h, page83ID)
if found {
return imp.GetDiskNumber(h)
if diskPage83ID == page83ID {
return diskNumber, nil
}
}

return 0, fmt.Errorf("Could not find disk with Page83 ID %s", page83ID)
}

func (imp DiskAPI) GetDiskNumberAndPage83ID(path string) (uint32, string, error) {
h, err := syscall.Open(path, syscall.O_RDONLY, 0)
defer syscall.Close(h)
if err != nil {
return 0, "", err
}

diskNumber, err := imp.GetDiskNumber(h)
if err != nil {
return 0, "", err
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like in other functions we return 0 in the error case too. So makes sense to check error first.

}

page83ID, err := imp.GetDiskPage83ID(h)
if err != nil {
return 0, "", err
}

return diskNumber, page83ID, nil
}

// ListDiskIDs - constructs a map with the disk number as the key and the DiskID structure
// as the value. The DiskID struct has a field for the page83 ID.
func (imp DiskAPI) ListDiskIDs() (map[uint32]shared.DiskIDs, error) {
Expand All @@ -334,21 +307,11 @@ func (imp DiskAPI) ListDiskIDs() (map[uint32]shared.DiskIDs, error) {
m := make(map[uint32]shared.DiskIDs)

for i := range disks {
h, err := syscall.Open(disks[i].Path, syscall.O_RDONLY, 0)
diskNumber, page83, err := imp.GetDiskNumberAndPage83ID(disks[i].Path)
if err != nil {
return nil, err
}

page83, err := imp.GetDiskPage83ID(h)
if err != nil {
return m, fmt.Errorf("Could not get page83 ID: %v", err)
}

diskNumber, err := imp.GetDiskNumber(h)
if err != nil {
return m, fmt.Errorf("Could not get disk number: %v", err)
}

m[diskNumber] = shared.DiskIDs{
Page83: page83,
SerialNumber: disks[i].SerialNumber,
Expand Down