-
Notifications
You must be signed in to change notification settings - Fork 166
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
qemu: support '4k' and 'serial' disk options #3242
Conversation
@@ -107,13 +107,16 @@ func ParseDiskSpec(spec string) (*Disk, error) { | |||
split := strings.Split(spec, ":") | |||
var size string | |||
multipathed := false | |||
sectorSize := 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about defaulting it at 512?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When SectorSize
is 0, we just let the default QEMU behaviour kick in, which is 512 logical/physical sector sizes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(In Rust this would be an Option<u32>
which would better express semantics)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see that makes sense! cool thank you.
any chance we could get some documentation for this? |
Support for e.g. `cosa run --add-disk 10G:4k`.
Support for e.g. `cosa run --add-disk 10G:serial=foobar`. This makes it easier to work with multiple disks when testing more complex storage setups.
7a4de24
to
cc1953d
Compare
Done! |
docs look great! will let @prestist review the code. |
@@ -107,13 +107,16 @@ func ParseDiskSpec(spec string) (*Disk, error) { | |||
split := strings.Split(spec, ":") | |||
var size string | |||
multipathed := false | |||
sectorSize := 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(In Rust this would be an Option<u32>
which would better express semantics)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM great job!
See individual commits.