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
