5:31 p.m.
John Levon (2):
test_driver: provide basic disk hotplug support
test_driver: provide basic disk hotunplug support
src/test/test_driver.c | 276 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 273 insertions(+), 3 deletions(-)
--
2.34.1
5:31 p.m.
New subject: [PATCH 1/2] test_driver: provide basic disk hotplug support
Add some basic plumbing, based on the qemu driver.
Signed-off-by: John Levon <john.levon(a)nutanix.com>
---
src/test/test_driver.c | 162 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 161 insertions(+), 1 deletion(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 03afe8af8d..b7e36e8451 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -10223,6 +10223,158 @@ testDomainAttachHostDevice(testDriver *driver,
}
+/**
+ * @def: Domain definition
+ * @info: Domain device info
+ *
+ * Using the device info, find the controller related to the
+ * device by index and use that controller to return the model.
+ *
+ * Returns the model if found, -1 if not with an error message set
+ */
+static int
+testDomainFindSCSIControllerModel(const virDomainDef *def,
+ virDomainDeviceInfo *info)
+{
+ virDomainControllerDef *cont;
+
+ if (!(cont = virDomainDeviceFindSCSIController(def, &info->addr.drive))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unable to find a SCSI controller for idx=%1$d"),
+ info->addr.drive.controller);
+ return -1;
+ }
+
+ return cont->model;
+}
+
+
+static int
+testAssignDeviceDiskAlias(virDomainDef *def,
+ virDomainDiskDef *disk)
+{
+ const char *prefix = virDomainDiskBusTypeToString(disk->bus);
+ int controllerModel = -1;
+
+ if (!disk->info.alias) {
+ if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) {
+ if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
+ controllerModel = testDomainFindSCSIControllerModel(def,
+ &disk->info);
+ if (controllerModel < 0)
+ return -1;
+ }
+
+ if (disk->bus != VIR_DOMAIN_DISK_BUS_SCSI ||
+ controllerModel == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC) {
+ disk->info.alias = g_strdup_printf("%s%d-%d-%d", prefix,
+ disk->info.addr.drive.controller,
+ disk->info.addr.drive.bus,
+ disk->info.addr.drive.unit);
+ } else {
+ disk->info.alias = g_strdup_printf("%s%d-%d-%d-%d", prefix,
+ disk->info.addr.drive.controller,
+ disk->info.addr.drive.bus,
+ disk->info.addr.drive.target,
+ disk->info.addr.drive.unit);
+ }
+ } else {
+ int idx = virDiskNameToIndex(disk->dst);
+ disk->info.alias = g_strdup_printf("%s-disk%d", prefix, idx);
+ }
+ }
+
+ return 0;
+}
+
+
+static int
+testDomainAttachDeviceDiskLiveInternal(testDriver *driver G_GNUC_UNUSED,
+ virDomainObj *vm,
+ virDomainDeviceDef *dev)
+{
+ size_t i;
+ virDomainDiskDef *disk = dev->data.disk;
+ int ret = -1;
+
+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("floppy device hotplug isn't supported"));
+ return -1;
+ }
+
+ if (virDomainDiskTranslateSourcePool(disk) < 0)
+ goto cleanup;
+
+ for (i = 0; i < vm->def->ndisks; i++) {
+ if (virDomainDiskDefCheckDuplicateInfo(vm->def->disks[i], disk) < 0)
+ goto cleanup;
+ }
+
+ switch (disk->bus) {
+ case VIR_DOMAIN_DISK_BUS_USB:
+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("disk device='lun' is not supported for usb
bus"));
+ goto cleanup;
+ }
+
+ break;
+
+ case VIR_DOMAIN_DISK_BUS_VIRTIO:
+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("cdrom device with virtio bus isn't
supported"));
+ goto cleanup;
+ }
+ break;
+
+ case VIR_DOMAIN_DISK_BUS_SCSI:
+ case VIR_DOMAIN_DISK_BUS_IDE:
+ case VIR_DOMAIN_DISK_BUS_FDC:
+ case VIR_DOMAIN_DISK_BUS_XEN:
+ case VIR_DOMAIN_DISK_BUS_UML:
+ case VIR_DOMAIN_DISK_BUS_SATA:
+ case VIR_DOMAIN_DISK_BUS_SD:
+ case VIR_DOMAIN_DISK_BUS_NONE:
+ case VIR_DOMAIN_DISK_BUS_LAST:
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ _("disk bus '%1$s' cannot be hotplugged."),
+ virDomainDiskBusTypeToString(disk->bus));
+ goto cleanup;
+ }
+
+ if (testAssignDeviceDiskAlias(vm->def, disk) < 0)
+ goto cleanup;
+
+ virDomainDiskInsert(vm->def, disk);
+
+ ret = 0;
+
+ cleanup:
+ return ret;
+}
+
+
+/**
+ * testDomainAttachDeviceDiskLive:
+ * @driver: test driver struct
+ * @vm: domain object
+ * @dev: device to attach (expected type is DISK)
+ *
+ * Attach a new disk or in case of cdroms/floppies change the media in the drive.
+ * This function handles all the necessary steps to attach a new storage source
+ * to the VM.
+ */
+static int
+testDomainAttachDeviceDiskLive(testDriver *driver,
+ virDomainObj *vm,
+ virDomainDeviceDef *dev)
+{
+ return testDomainAttachDeviceDiskLiveInternal(driver, vm, dev);
+}
+
+
static int
testDomainAttachDeviceLive(virDomainObj *vm,
virDomainDeviceDef *dev,
@@ -10251,8 +10403,16 @@ testDomainAttachDeviceLive(virDomainObj *vm,
}
break;
- case VIR_DOMAIN_DEVICE_NONE:
case VIR_DOMAIN_DEVICE_DISK:
+ testDomainObjCheckDiskTaint(vm, dev->data.disk);
+ ret = testDomainAttachDeviceDiskLive(driver, vm, dev);
+ if (!ret) {
+ alias = dev->data.disk->info.alias;
+ dev->data.disk = NULL;
+ }
+ break;
+
+ case VIR_DOMAIN_DEVICE_NONE:
case VIR_DOMAIN_DEVICE_LEASE:
case VIR_DOMAIN_DEVICE_FS:
case VIR_DOMAIN_DEVICE_INPUT:
--
2.34.1
2:14 a.m.
New subject: [PATCH 1/2] test_driver: provide basic disk hotplug support
On 11/1/24 23:31, John Levon wrote:
Add some basic plumbing, based on the qemu driver.
Signed-off-by: John Levon <john.levon(a)nutanix.com>
---
src/test/test_driver.c | 162 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 161 insertions(+), 1 deletion(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 03afe8af8d..b7e36e8451 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
+
+/**
+ * testDomainAttachDeviceDiskLive:
+ * @driver: test driver struct
+ * @vm: domain object
+ * @dev: device to attach (expected type is DISK)
+ *
+ * Attach a new disk or in case of cdroms/floppies change the media in the drive.
+ * This function handles all the necessary steps to attach a new storage source
+ * to the VM.
+ */
+static int
+testDomainAttachDeviceDiskLive(testDriver *driver,
+ virDomainObj *vm,
+ virDomainDeviceDef *dev)
+{
+ return testDomainAttachDeviceDiskLiveInternal(driver, vm, dev);
You've removed too much. As comment above says, and per
virDomainAttachDevice() documentation, there is one (arguably weird) use
of this function: to change media in CDROM/FLOPPY devices.
+}
+
+
Michal
5:31 p.m.
New subject: [PATCH 2/2] test_driver: provide basic disk hotunplug support
Signed-off-by: John Levon <john.levon(a)nutanix.com>
---
src/test/test_driver.c | 114 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 112 insertions(+), 2 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index b7e36e8451..789b1a2222 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -10741,6 +10741,85 @@ testDomainDetachPrepNet(virDomainObj *vm,
}
+static int
+testFindDisk(virDomainDef *def, const char *dst)
+{
+ size_t i;
+
+ for (i = 0; i < def->ndisks; i++) {
+ if (STREQ(def->disks[i]->dst, dst))
+ return i;
+ }
+
+ return -1;
+}
+
+static int
+testDomainDetachPrepDisk(virDomainObj *vm,
+ virDomainDiskDef *match,
+ virDomainDiskDef **detach)
+{
+ virDomainDiskDef *disk;
+ int idx;
+
+ if ((idx = testFindDisk(vm->def, match->dst)) < 0) {
+ virReportError(VIR_ERR_DEVICE_MISSING,
+ _("disk %1$s not found"), match->dst);
+ return -1;
+ }
+ *detach = disk = vm->def->disks[idx];
+
+ switch ((virDomainDiskDevice) disk->device) {
+ case VIR_DOMAIN_DISK_DEVICE_DISK:
+ case VIR_DOMAIN_DISK_DEVICE_LUN:
+
+ switch (disk->bus) {
+ case VIR_DOMAIN_DISK_BUS_VIRTIO:
+ case VIR_DOMAIN_DISK_BUS_USB:
+ case VIR_DOMAIN_DISK_BUS_SCSI:
+ break;
+
+ case VIR_DOMAIN_DISK_BUS_IDE:
+ case VIR_DOMAIN_DISK_BUS_FDC:
+ case VIR_DOMAIN_DISK_BUS_XEN:
+ case VIR_DOMAIN_DISK_BUS_UML:
+ case VIR_DOMAIN_DISK_BUS_SATA:
+ case VIR_DOMAIN_DISK_BUS_SD:
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("This type of disk cannot be hot unplugged"));
+ return -1;
+
+ case VIR_DOMAIN_DISK_BUS_NONE:
+ case VIR_DOMAIN_DISK_BUS_LAST:
+ default:
+ virReportEnumRangeError(virDomainDiskBus, disk->bus);
+ return -1;
+ }
+ break;
+
+ case VIR_DOMAIN_DISK_DEVICE_CDROM:
+ if (disk->bus == VIR_DOMAIN_DISK_BUS_USB ||
+ disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
+ break;
+ }
+ G_GNUC_FALLTHROUGH;
+
+ case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ _("disk device type '%1$s' cannot be
detached"),
+ virDomainDiskDeviceTypeToString(disk->device));
+ return -1;
+
+ case VIR_DOMAIN_DISK_DEVICE_LAST:
+ default:
+ virReportEnumRangeError(virDomainDiskDevice, disk->device);
+ return -1;
+ }
+
+ return 0;
+}
+
+
static int
testDomainRemoveHostDevice(testDriver *driver G_GNUC_UNUSED,
virDomainObj *vm,
@@ -10810,6 +10889,28 @@ testDomainRemoveNetDevice(testDriver *driver G_GNUC_UNUSED,
return 0;
}
+
+static int
+testDomainRemoveDiskDevice(testDriver *driver G_GNUC_UNUSED,
+ virDomainObj *vm,
+ virDomainDiskDef *disk)
+{
+ size_t i;
+
+ VIR_DEBUG("Removing disk %s from domain %p %s",
+ disk->info.alias, vm, vm->def->name);
+
+ for (i = 0; i < vm->def->ndisks; i++) {
+ if (vm->def->disks[i] == disk) {
+ virDomainDiskRemove(vm->def, i);
+ break;
+ }
+ }
+
+ virDomainDiskDefFree(disk);
+ return 0;
+}
+
static int
testDomainRemoveDevice(testDriver *driver,
virDomainObj *vm,
@@ -10835,8 +10936,11 @@ testDomainRemoveDevice(testDriver *driver,
if (testDomainRemoveHostDevice(driver, vm, dev->data.hostdev) < 0)
return -1;
break;
- case VIR_DOMAIN_DEVICE_NONE:
case VIR_DOMAIN_DEVICE_DISK:
+ if (testDomainRemoveDiskDevice(driver, vm, dev->data.disk) < 0)
+ return -1;
+ break;
+ case VIR_DOMAIN_DEVICE_NONE:
case VIR_DOMAIN_DEVICE_LEASE:
case VIR_DOMAIN_DEVICE_FS:
case VIR_DOMAIN_DEVICE_INPUT:
@@ -10900,8 +11004,14 @@ testDomainDetachDeviceLive(testDriver *driver,
}
break;
- case VIR_DOMAIN_DEVICE_NONE:
case VIR_DOMAIN_DEVICE_DISK:
+ if (testDomainDetachPrepDisk(vm, match->data.disk,
+ &detach.data.disk) < 0) {
+ return -1;
+ }
+ break;
+
+ case VIR_DOMAIN_DEVICE_NONE:
case VIR_DOMAIN_DEVICE_LEASE:
case VIR_DOMAIN_DEVICE_FS:
case VIR_DOMAIN_DEVICE_INPUT:
--
2.34.1
2:14 a.m.
New subject: [PATCH 2/2] test_driver: provide basic disk hotunplug support
On 11/1/24 23:31, John Levon wrote:
Signed-off-by: John Levon <john.levon(a)nutanix.com>
---
src/test/test_driver.c | 114 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 112 insertions(+), 2 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index b7e36e8451..789b1a2222 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -10741,6 +10741,85 @@ testDomainDetachPrepNet(virDomainObj *vm,
}
+static int
+testFindDisk(virDomainDef *def, const char *dst)
+{
+ size_t i;
+
+ for (i = 0; i < def->ndisks; i++) {
+ if (STREQ(def->disks[i]->dst, dst))
+ return i;
+ }
+
+ return -1;
+}
+
+static int
+testDomainDetachPrepDisk(virDomainObj *vm,
+ virDomainDiskDef *match,
+ virDomainDiskDef **detach)
+{
+ virDomainDiskDef *disk;
+ int idx;
+
+ if ((idx = testFindDisk(vm->def, match->dst)) < 0) {
+ virReportError(VIR_ERR_DEVICE_MISSING,
+ _("disk %1$s not found"), match->dst);
+ return -1;
+ }
+ *detach = disk = vm->def->disks[idx];
So idx is there only to access vm->def->disks array? Well, the same
result can be achieved using virDomainDiskByTarget().
Oh, and I'd set *detach only after all checks passed, i.e. right before
return 0.
Michal
3:50 a.m.
On Tue, Nov 12, 2024 at 09:14:22AM +0100, Michal Prívozník wrote:
On 11/1/24 23:31, John Levon wrote:
> John Levon (2):
> test_driver: provide basic disk hotplug support
> test_driver: provide basic disk hotunplug support
>
> src/test/test_driver.c | 276 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 273 insertions(+), 3 deletions(-)
>
Sorry for late review. I'm fixing all the small nits found during review
and merging.
Thank you! Comments below just for clarification.
On Tue, Nov 12, 2024 at 09:14:25AM +0100, Michal Prívozník wrote:
> +/**
> + * testDomainAttachDeviceDiskLive:
> + * @driver: test driver struct
> + * @vm: domain object
> + * @dev: device to attach (expected type is DISK)
> + *
> + * Attach a new disk or in case of cdroms/floppies change the media in the drive.
> + * This function handles all the necessary steps to attach a new storage source
> + * to the VM.
> + */
> +static int
> +testDomainAttachDeviceDiskLive(testDriver *driver,
> + virDomainObj *vm,
> + virDomainDeviceDef *dev)
> +{
> + return testDomainAttachDeviceDiskLiveInternal(driver, vm, dev);
You've removed too much. As comment above says, and per
virDomainAttachDevice() documentation, there is one (arguably weird) use
of this function: to change media in CDROM/FLOPPY devices.
I think you mean I should have left in this:
10377 /* this API overloads media change semantics on disk hotplug
10378 * for devices supporting media changes */
10379 if ((disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM ||
10380 disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) &&
10381 (orig_disk = virDomainDiskByTarget(vm->def, disk->dst))) {
10382 virObjectUnref(orig_disk->src);
10383 orig_disk->src = g_steal_pointer(&disk->src);
10384 virDomainDiskDefFree(disk);
10385 return 0;
10386 }
I didn't copy it across simply because I wasn't in a position to test it, but if
you think it's probably fine there anyway, then great.
On Tue, Nov 12, 2024 at 09:14:30AM +0100, Michal Prívozník wrote:
> + if ((idx = testFindDisk(vm->def, match->dst)) < 0)
{
> + virReportError(VIR_ERR_DEVICE_MISSING,
> + _("disk %1$s not found"), match->dst);
> + return -1;
> + }
> + *detach = disk = vm->def->disks[idx];
So idx is there only to access vm->def->disks array? Well, the same
result can be achieved using virDomainDiskByTarget().
Oh, and I'd set *detach only after all checks passed, i.e. right before
return 0.
Sure. Both of these are like this in qemuDomainDetachPrepDisk(), that's all.
regards
john
2:14 a.m.
On 11/1/24 23:31, John Levon wrote:
John Levon (2):
test_driver: provide basic disk hotplug support
test_driver: provide basic disk hotunplug support
src/test/test_driver.c | 276 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 273 insertions(+), 3 deletions(-)
Sorry for late review. I'm fixing all the small nits found during review
and merging.
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
Michal
11
days inactive
22
days old
6 comments
2 participants
participants (2)
-
John Levon -
Michal Prívozník