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