Skip to content

Commit 8df513f

Browse files
lstocchilstocchi
committed
Refactor StorageConfig
crc-org#212 introduced a new property to the StorageConfig struct (URI) and, by doing so, now we have to check if we are dealing with a disk storage or a remote disk device by checking imagePath and Uri fields. An idea that came up in crc-org#212 (comment) was to refactor the StorageConfig struct to be extended by a DiskStorageConfig and a NetworkBlockStorageConfig. This PR refactors the code based on that suggestion. Signed-off-by: Luca Stocchi <[email protected]>
1 parent 4af22e9 commit 8df513f

File tree

4 files changed

+103
-71
lines changed

4 files changed

+103
-71
lines changed

‎pkg/config/json_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,7 @@ var jsonTests = map[string]jsonTest{
178178

179179
return vm
180180
},
181-
expectedJSON: `{"vcpus":3,"memoryBytes":4194304000,"bootloader":{"kind":"linuxBootloader","vmlinuzPath":"/vmlinuz","initrdPath":"/initrd","kernelCmdLine":"console=hvc0"},"devices":[{"kind":"virtioserial","logFile":"/virtioserial"},{"kind":"virtioinput","inputType":"keyboard"},{"kind":"virtiogpu","usesGUI":false,"width":800,"height":600},{"kind":"virtionet","nat":true,"macAddress":"00:11:22:33:44:55"},{"kind":"virtiorng"},{"kind":"virtioblk","devName":"virtio-blk","imagePath":"/virtioblk"},{"kind":"virtiosock","port":1234,"socketURL":"/virtiovsock"},{"kind":"virtiofs","mountTag":"tag","sharedDir":"/virtiofs"},{"kind":"usbmassstorage","devName":"usb-mass-storage","imagePath":"/usbmassstorage","readOnly":true},{"kind":"rosetta","mountTag":"vz-rosetta","installRosetta":false,"ignoreIfMissing":false},{"kind":"nbd", "devName":"nbd", "uri":"uri", "SynchronizationMode":"full","Timeout":1000000}]}`,
181+
expectedJSON: `{"vcpus":3,"memoryBytes":4194304000,"bootloader":{"kind":"linuxBootloader","vmlinuzPath":"/vmlinuz","initrdPath":"/initrd","kernelCmdLine":"console=hvc0"},"devices":[{"kind":"virtioserial","logFile":"/virtioserial"},{"kind":"virtioinput","inputType":"keyboard"},{"kind":"virtiogpu","usesGUI":false,"width":800,"height":600},{"kind":"virtionet","nat":true,"macAddress":"00:11:22:33:44:55"},{"kind":"virtiorng"},{"kind":"virtioblk","devName":"virtio-blk","imagePath":"/virtioblk"},{"kind":"virtiosock","port":1234,"socketURL":"/virtiovsock"},{"kind":"virtiofs","mountTag":"tag","sharedDir":"/virtiofs"},{"kind":"usbmassstorage","devName":"usb-mass-storage","imagePath":"/usbmassstorage","readOnly":true},{"kind":"rosetta","mountTag":"vz-rosetta","installRosetta":false,"ignoreIfMissing":false},{"kind":"nbd", "devName":"nbd", "uri":"uri", "DeviceIdentifier":"", "SynchronizationMode":"full","Timeout":1000000}]}`,
182182
},
183183
}
184184

@@ -317,7 +317,7 @@ var jsonStabilityTests = map[string]jsonStabilityTest{
317317
return nbd
318318
},
319319
skipFields: []string{"DevName", "ImagePath"},
320-
expectedJSON: `{"kind":"nbd","deviceIdentifier":"DeviceIdentifier","devName":"nbd","uri":"URI","readOnly":true,"SynchronizationMode":"SynchronizationMode","Timeout":2}`,
320+
expectedJSON: `{"kind":"nbd","DeviceIdentifier":"DeviceIdentifier","devName":"nbd","uri":"URI","readOnly":true,"SynchronizationMode":"SynchronizationMode","Timeout":2}`,
321321
},
322322
}
323323

‎pkg/config/virtio.go‎

Lines changed: 72 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ type VirtioVsock struct {
5959

6060
// VirtioBlk configures a disk device.
6161
type VirtioBlk struct {
62-
StorageConfig
62+
DiskStorageConfig
6363
DeviceIdentifier string `json:"deviceIdentifier,omitempty"`
6464
}
6565

@@ -82,7 +82,7 @@ type RosettaShare struct {
8282

8383
// NVMExpressController configures a NVMe controller in the guest
8484
type NVMExpressController struct {
85-
StorageConfig
85+
DiskStorageConfig
8686
}
8787

8888
// VirtioRng configures a random number generator (RNG) device.
@@ -117,7 +117,8 @@ const (
117117
)
118118

119119
type NetworkBlockDevice struct {
120-
VirtioBlk
120+
NetworkBlockStorageConfig
121+
DeviceIdentifier string
121122
Timeout time.Duration
122123
SynchronizationMode NBDSynchronizationMode
123124
}
@@ -479,8 +480,10 @@ func (dev *VirtioRng) FromOptions(options []option) error {
479480

480481
func nvmExpressControllerNewEmpty() *NVMExpressController {
481482
return &NVMExpressController{
482-
StorageConfig: StorageConfig{
483-
DevName: "nvme",
483+
DiskStorageConfig: DiskStorageConfig{
484+
StorageConfig: StorageConfig{
485+
DevName: "nvme",
486+
},
484487
},
485488
}
486489
}
@@ -496,8 +499,10 @@ func NVMExpressControllerNew(imagePath string) (*NVMExpressController, error) {
496499

497500
func virtioBlkNewEmpty() *VirtioBlk {
498501
return &VirtioBlk{
499-
StorageConfig: StorageConfig{
500-
DevName: "virtio-blk",
502+
DiskStorageConfig: DiskStorageConfig{
503+
StorageConfig: StorageConfig{
504+
DevName: "virtio-blk",
505+
},
501506
},
502507
DeviceIdentifier: "",
503508
}
@@ -527,11 +532,11 @@ func (dev *VirtioBlk) FromOptions(options []option) error {
527532
}
528533
}
529534

530-
return dev.StorageConfig.FromOptions(unhandledOpts)
535+
return dev.DiskStorageConfig.FromOptions(unhandledOpts)
531536
}
532537

533538
func (dev *VirtioBlk) ToCmdLine() ([]string, error) {
534-
cmdLine, err := dev.StorageConfig.ToCmdLine()
539+
cmdLine, err := dev.DiskStorageConfig.ToCmdLine()
535540
if err != nil {
536541
return []string{}, err
537542
}
@@ -681,12 +686,12 @@ func (dev *RosettaShare) FromOptions(options []option) error {
681686

682687
func networkBlockDeviceNewEmpty() *NetworkBlockDevice {
683688
return &NetworkBlockDevice{
684-
VirtioBlk: VirtioBlk{
689+
NetworkBlockStorageConfig: NetworkBlockStorageConfig{
685690
StorageConfig: StorageConfig{
686691
DevName: "nbd",
687692
},
688-
DeviceIdentifier: "",
689693
},
694+
DeviceIdentifier: "",
690695
Timeout: time.Duration(15000 * time.Millisecond), // set a default timeout to 15s
691696
SynchronizationMode: SynchronizationFullMode, // default mode to full
692697
}
@@ -707,14 +712,17 @@ func NetworkBlockDeviceNew(uri string, timeout uint32, synchronization NBDSynchr
707712
}
708713

709714
func (nbd *NetworkBlockDevice) ToCmdLine() ([]string, error) {
710-
cmdLine, err := nbd.VirtioBlk.ToCmdLine()
715+
cmdLine, err := nbd.NetworkBlockStorageConfig.ToCmdLine()
711716
if err != nil {
712717
return []string{}, err
713718
}
714719
if len(cmdLine) != 2 {
715720
return []string{}, fmt.Errorf("unexpected storage config commandline")
716721
}
717722

723+
if nbd.DeviceIdentifier != "" {
724+
cmdLine[1] = fmt.Sprintf("%s,deviceId=%s", cmdLine[1], nbd.DeviceIdentifier)
725+
}
718726
if nbd.Timeout.Milliseconds() > 0 {
719727
cmdLine[1] = fmt.Sprintf("%s,timeout=%d", cmdLine[1], nbd.Timeout.Milliseconds())
720728
}
@@ -729,6 +737,8 @@ func (nbd *NetworkBlockDevice) FromOptions(options []option) error {
729737
unhandledOpts := []option{}
730738
for _, option := range options {
731739
switch option.key {
740+
case "deviceId":
741+
nbd.DeviceIdentifier = option.value
732742
case "timeout":
733743
timeoutMS, err := strconv.ParseInt(option.value, 10, 32)
734744
if err != nil {
@@ -749,17 +759,19 @@ func (nbd *NetworkBlockDevice) FromOptions(options []option) error {
749759
}
750760
}
751761

752-
return nbd.VirtioBlk.FromOptions(unhandledOpts)
762+
return nbd.NetworkBlockStorageConfig.FromOptions(unhandledOpts)
753763
}
754764

755765
type USBMassStorage struct {
756-
StorageConfig
766+
DiskStorageConfig
757767
}
758768

759769
func usbMassStorageNewEmpty() *USBMassStorage {
760770
return &USBMassStorage{
761-
StorageConfig{
762-
DevName: "usb-mass-storage",
771+
DiskStorageConfig: DiskStorageConfig{
772+
StorageConfig: StorageConfig{
773+
DevName: "usb-mass-storage",
774+
},
763775
},
764776
}
765777
}
@@ -779,38 +791,66 @@ func (dev *USBMassStorage) SetReadOnly(readOnly bool) {
779791

780792
// StorageConfig configures a disk device.
781793
type StorageConfig struct {
782-
DevName string `json:"devName"`
794+
DevName string `json:"devName"`
795+
ReadOnly bool `json:"readOnly,omitempty"`
796+
}
797+
798+
type DiskStorageConfig struct {
799+
StorageConfig
783800
ImagePath string `json:"imagePath,omitempty"`
784-
URI string `json:"uri,omitempty"`
785-
ReadOnly bool `json:"readOnly,omitempty"`
786801
}
787802

788-
func (config *StorageConfig) ToCmdLine() ([]string, error) {
789-
if config.ImagePath != "" && config.URI != "" {
790-
return nil, fmt.Errorf("%s devices cannot have both path to a disk image and a uri to a remote block device", config.DevName)
803+
type NetworkBlockStorageConfig struct {
804+
StorageConfig
805+
URI string `json:"uri,omitempty"`
806+
}
807+
808+
func (config *DiskStorageConfig) ToCmdLine() ([]string, error) {
809+
if config.ImagePath == "" {
810+
return nil, fmt.Errorf("%s devices need the path to a disk image", config.DevName)
791811
}
792-
if config.ImagePath == "" && config.URI == "" {
793-
return nil, fmt.Errorf("%s devices need a path to a disk image or a uri to a remote block device", config.DevName)
812+
813+
value := fmt.Sprintf("%s,path=%s", config.DevName, config.ImagePath)
814+
815+
if config.ReadOnly {
816+
value += ",readonly"
794817
}
795-
var value string
796-
if config.ImagePath != "" {
797-
value = fmt.Sprintf("%s,path=%s", config.DevName, config.ImagePath)
818+
return []string{"--device", value}, nil
819+
}
820+
821+
func (config *DiskStorageConfig) FromOptions(options []option) error {
822+
for _, option := range options {
823+
switch option.key {
824+
case "path":
825+
config.ImagePath = option.value
826+
case "readonly":
827+
if option.value != "" {
828+
return fmt.Errorf("unexpected value for virtio-blk 'readonly' option: %s", option.value)
829+
}
830+
config.ReadOnly = true
831+
default:
832+
return fmt.Errorf("unknown option for %s devices: %s", config.DevName, option.key)
833+
}
798834
}
799-
if config.URI != "" {
800-
value = fmt.Sprintf("%s,uri=%s", config.DevName, config.URI)
835+
return nil
836+
}
837+
838+
func (config *NetworkBlockStorageConfig) ToCmdLine() ([]string, error) {
839+
if config.URI == "" {
840+
return nil, fmt.Errorf("%s devices need the uri to a remote block device", config.DevName)
801841
}
802842

843+
value := fmt.Sprintf("%s,uri=%s", config.DevName, config.URI)
844+
803845
if config.ReadOnly {
804846
value += ",readonly"
805847
}
806848
return []string{"--device", value}, nil
807849
}
808850

809-
func (config *StorageConfig) FromOptions(options []option) error {
851+
func (config *NetworkBlockStorageConfig) FromOptions(options []option) error {
810852
for _, option := range options {
811853
switch option.key {
812-
case "path":
813-
config.ImagePath = option.value
814854
case "uri":
815855
config.URI = option.value
816856
case "readonly":

‎pkg/config/virtio_test.go‎

Lines changed: 24 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,10 @@ var virtioDevTests = map[string]virtioDevTest{
2020
"NewVirtioBlk": {
2121
newDev: func() (VirtioDevice, error) { return VirtioBlkNew("/foo/bar") },
2222
expectedDev: &VirtioBlk{
23-
StorageConfig: StorageConfig{
24-
DevName: "virtio-blk",
23+
DiskStorageConfig: DiskStorageConfig{
24+
StorageConfig: StorageConfig{
25+
DevName: "virtio-blk",
26+
},
2527
ImagePath: "/foo/bar",
2628
},
2729
DeviceIdentifier: "",
@@ -38,8 +40,10 @@ var virtioDevTests = map[string]virtioDevTest{
3840
return dev, nil
3941
},
4042
expectedDev: &VirtioBlk{
41-
StorageConfig: StorageConfig{
42-
DevName: "virtio-blk",
43+
DiskStorageConfig: DiskStorageConfig{
44+
StorageConfig: StorageConfig{
45+
DevName: "virtio-blk",
46+
},
4347
ImagePath: "/foo/bar",
4448
},
4549
DeviceIdentifier: "test",
@@ -50,8 +54,10 @@ var virtioDevTests = map[string]virtioDevTest{
5054
"NewNVMe": {
5155
newDev: func() (VirtioDevice, error) { return NVMExpressControllerNew("/foo/bar") },
5256
expectedDev: &NVMExpressController{
53-
StorageConfig: StorageConfig{
54-
DevName: "nvme",
57+
DiskStorageConfig: DiskStorageConfig{
58+
StorageConfig: StorageConfig{
59+
DevName: "nvme",
60+
},
5561
ImagePath: "/foo/bar",
5662
},
5763
},
@@ -156,8 +162,10 @@ var virtioDevTests = map[string]virtioDevTest{
156162
"NewUSBMassStorage": {
157163
newDev: func() (VirtioDevice, error) { return USBMassStorageNew("/foo/bar") },
158164
expectedDev: &USBMassStorage{
159-
StorageConfig: StorageConfig{
160-
DevName: "usb-mass-storage",
165+
DiskStorageConfig: DiskStorageConfig{
166+
StorageConfig: StorageConfig{
167+
DevName: "usb-mass-storage",
168+
},
161169
ImagePath: "/foo/bar",
162170
},
163171
},
@@ -173,10 +181,12 @@ var virtioDevTests = map[string]virtioDevTest{
173181
return dev, err
174182
},
175183
expectedDev: &USBMassStorage{
176-
StorageConfig: StorageConfig{
177-
DevName: "usb-mass-storage",
184+
DiskStorageConfig: DiskStorageConfig{
185+
StorageConfig: StorageConfig{
186+
DevName: "usb-mass-storage",
187+
ReadOnly: true,
188+
},
178189
ImagePath: "/foo/bar",
179-
ReadOnly: true,
180190
},
181191
},
182192
expectedCmdLine: []string{"--device", "usb-mass-storage,path=/foo/bar,readonly"},
@@ -223,36 +233,18 @@ var virtioDevTests = map[string]virtioDevTest{
223233
return NetworkBlockDeviceNew("nbd://1.1.1.1:10000", 1000, SynchronizationNoneMode)
224234
},
225235
expectedDev: &NetworkBlockDevice{
226-
VirtioBlk: VirtioBlk{
236+
NetworkBlockStorageConfig: NetworkBlockStorageConfig{
227237
StorageConfig: StorageConfig{
228238
DevName: "nbd",
229-
URI: "nbd://1.1.1.1:10000",
230239
},
231-
DeviceIdentifier: "",
240+
URI: "nbd://1.1.1.1:10000",
232241
},
242+
DeviceIdentifier: "",
233243
Timeout: time.Duration(1000 * time.Millisecond),
234244
SynchronizationMode: SynchronizationNoneMode,
235245
},
236246
expectedCmdLine: []string{"--device", "nbd,uri=nbd://1.1.1.1:10000,timeout=1000,sync=none"},
237247
},
238-
"StorageConfigErrorImageUri": {
239-
newDev: func() (VirtioDevice, error) {
240-
return &StorageConfig{
241-
DevName: "dev",
242-
ImagePath: "path",
243-
URI: "uri",
244-
}, nil
245-
},
246-
errorMsg: "dev devices cannot have both path to a disk image and a uri to a remote block device",
247-
},
248-
"StorageConfigErrorNoImageOrUri": {
249-
newDev: func() (VirtioDevice, error) {
250-
return &StorageConfig{
251-
DevName: "dev",
252-
}, nil
253-
},
254-
errorMsg: "dev devices need a path to a disk image or a uri to a remote block device",
255-
},
256248
}
257249

258250
func testVirtioDev(t *testing.T, test *virtioDevTest) {

‎pkg/vf/virtio.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ type vzNetworkBlockDevice struct {
3535
}
3636

3737
func (dev *NVMExpressController) toVz() (vz.StorageDeviceConfiguration, error) {
38-
var storageConfig StorageConfig = StorageConfig(dev.StorageConfig)
38+
var storageConfig DiskStorageConfig = DiskStorageConfig(dev.DiskStorageConfig)
3939
attachment, err := storageConfig.toVz()
4040
if err != nil {
4141
return nil, err
@@ -60,7 +60,7 @@ func (dev *NVMExpressController) AddToVirtualMachineConfig(vmConfig *VirtualMach
6060
}
6161

6262
func (dev *VirtioBlk) toVz() (vz.StorageDeviceConfiguration, error) {
63-
var storageConfig StorageConfig = StorageConfig(dev.StorageConfig)
63+
var storageConfig DiskStorageConfig = DiskStorageConfig(dev.DiskStorageConfig)
6464
attachment, err := storageConfig.toVz()
6565
if err != nil {
6666
return nil, err
@@ -431,7 +431,7 @@ func AddToVirtualMachineConfig(vmConfig *VirtualMachineConfiguration, dev config
431431
}
432432
}
433433

434-
func (config *StorageConfig) toVz() (vz.StorageDeviceAttachment, error) {
434+
func (config *DiskStorageConfig) toVz() (vz.StorageDeviceAttachment, error) {
435435
if config.ImagePath == "" {
436436
return nil, fmt.Errorf("missing mandatory 'path' option for %s device", config.DevName)
437437
}
@@ -441,7 +441,7 @@ func (config *StorageConfig) toVz() (vz.StorageDeviceAttachment, error) {
441441
}
442442

443443
func (dev *USBMassStorage) toVz() (vz.StorageDeviceConfiguration, error) {
444-
var storageConfig StorageConfig = StorageConfig(dev.StorageConfig)
444+
var storageConfig DiskStorageConfig = DiskStorageConfig(dev.DiskStorageConfig)
445445
attachment, err := storageConfig.toVz()
446446
if err != nil {
447447
return nil, err
@@ -460,6 +460,6 @@ func (dev *USBMassStorage) AddToVirtualMachineConfig(vmConfig *VirtualMachineCon
460460
return nil
461461
}
462462

463-
type StorageConfig config.StorageConfig
463+
type DiskStorageConfig config.DiskStorageConfig
464464

465465
type USBMassStorage config.USBMassStorage

0 commit comments

Comments
 (0)