Don't patch non-stub drives #240
Conversation
|
Honestly speaking, I'm unsure this is the right place to address the issue. We could argue that the drive handler shouldn't know about a root drive and the caller of the handler must pass only stub drives. |
runtime/drive_handler.go
Outdated
| @@ -125,10 +125,20 @@ func (h *stubDriveHandler) createStubDrive(driveID, path string) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func filterOnlyStubDrives(drives []models.Drive) []models.Drive { | |||
There was a problem hiding this comment.
May be a good idea to document why this occurs which is due to the root drive being always being the last element when Build is called on the drive builder from the SDK.
There was a problem hiding this comment.
You meant, this Build.
BTW, why do we want to place the root drive at the end of the drives?
There was a problem hiding this comment.
The location of root drive is arbitrary. End was chosen since you didn't need to create a []models.Drive to append to.
There was a problem hiding this comment.
Thanks. The new revision has some comments around drives initialization. Would it be good enough?
runtime/drive_handler.go
Outdated
| @@ -125,10 +125,20 @@ func (h *stubDriveHandler) createStubDrive(driveID, path string) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func filterOnlyStubDrives(drives []models.Drive) []models.Drive { | |||
| stubDrives := make([]models.Drive, 0) | |||
There was a problem hiding this comment.
nit: I'd rather prefer preallocating here as we know the size in advance: make([]models.Drive, 0, len(drives))
There was a problem hiding this comment.
Wouldn't it allocate more than what we need?
There was a problem hiding this comment.
Should it be just make([]models.Drive) then?
There was a problem hiding this comment.
We don't have the filter logic anymore, and all make calls explicitly specify the capacity.
|
I wonder if it makes sense to just have an array of stub drives that is passed into |
|
It does make sense. |
Before this change, a stub drive handler knew non-stub drives, including the root drive. However patching non-stub drives is not supported and must be prevented. Related: firecracker-microvm#230 Signed-off-by: Kazuyoshi Kato <[email protected]>
Signed-off-by: Kazuyoshi Kato <[email protected]>
Signed-off-by: Kazuyoshi Kato <[email protected]>
…ependabot/go_modules/github.com/stretchr/testify-1.6.1 Bump github.com/stretchr/testify from 1.6.0 to 1.6.1
Issue #, if available:
Related to, but may need more changes to actually fix #230
Description of changes:
Currently the drive handler knows non-stub drives (root drive). Because of that PatchStubDrive() replace non-stub drives, once all stub drives are patched.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.