Re: [libvirt] [PATCH v2 08/14] qemu_hotplug: standardize the names/args/calling of qemuDomainDetach*()

On Mon, Mar 25, 2019 at 13:24:30 -0400, Laine Stump wrote: > Most of these functions will soon contain only some setup for > detaching the device, not the detach code proper (since that code is > identical for these devices). Their device specific functions are all > being renamed to qemuDomainDetachPrep*(), where * is the > name of that device's data member in the virDomainDeviceDef > object. > > Since there will be other code in qemuDomainDetachDeviceLive() after > the calls to qemuDomainDetachPrep*() that could still fail, we no > longer directly set "ret" with the return code from > qemuDomainDetachPrep*() functions, but simply return -1 on > failure, and wait until the end of qemuDomainDetachDeviceLive() to set > ret = 0. > > Along with the rename, qemuDomainDetachPrep*() functions are also > given similar arglists, including an arg called "match" that points to I must say that doing this along with the rename did not help reviewability. The rename which includes whitespace change in the argument list obscures actual argument changes. I'd prefer if you did not do that in the future as I understand that splitting this patch would be an even bigger nightmare.

> the proto-object of the device we want to delete, and another arg > "detach" that is used to return a pointer to the actual object that > will be (for now *has been*) detached. To make sure these new args > aren't confused with existing local pointers that sometimes had the > same name (detach), the local pointer to the device is now named after > the device type ("controller", "disk", etc). These point to the same > place as (*detach)->data.blah, it's just easier on the eyes to have, > e.g., "disk->dst" rather than "(*detach)->data.disk-dst". > > Signed-off-by: Laine Stump <laine(a)laine.org&gt; > --- > > Change from V1: > g > * Renaming of Chr and Lease functions is now in a separate patch - 09/14. > > * "reverting name change" made in previous patch" that was pointed out > by Peter is eliminated - I removed the name change from the earlier > patch prior to pushing, thus simplifying both patches. > > * preserved NULL initialization of pointers that had it (no matter how unnecessary) > > * I *haven't* removed ret from qemuDomainDetachDeviceLive() as > suggested in review, since it's just going to be added back in Patch > 12/14 anyway. > > src/qemu/qemu_hotplug.c | 317 +++++++++++++++++++++++----------------- > 1 file changed, 182 insertions(+), 135 deletions(-) [...] > @@ -5773,13 +5774,17 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, > > > static int > -qemuDomainDetachWatchdog(virQEMUDriverPtr driver, > - virDomainObjPtr vm, > - virDomainWatchdogDefPtr dev, > - bool async) > +qemuDomainDetachPrepWatchdog(virQEMUDriverPtr driver, > + virDomainObjPtr vm, > + virDomainWatchdogDefPtr match, > + virDomainWatchdogDefPtr *detach, > + bool async) > { > int ret = -1; > - virDomainWatchdogDefPtr watchdog = vm->def->watchdog; > + virDomainWatchdogDefPtr watchdog; > + > + extra line > + *detach = watchdog = vm->def->watchdog; > > if (!watchdog) { > virReportError(VIR_ERR_DEVICE_MISSING, "%s", [...] > @@ -6206,6 +6218,7 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, > virQEMUDriverPtr driver, > bool async) > { > + virDomainDeviceDef detach = { .type = match->type }; > int ret = -1; > > switch ((virDomainDeviceType)match->type) { > @@ -6228,38 +6241,70 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, > * assure it is okay to detach the device. > */ > case VIR_DOMAIN_DEVICE_DISK: > - ret = qemuDomainDetachDeviceDiskLive(driver, vm, match, async); > + if (qemuDomainDetachPrepDisk(driver, vm, match->data.disk, > + &detach.data.disk, async) < 0) { > + return -1; > + } I'm not a fan of the curly braces on this single-line body but I was told that the coding style mandates it. Thus I'll not require a change here.

Also, all the functions should use the qemuHotplug prefix rather than qemuDomain, but given that the file is inconsistent I'm not going to insist.

ACK witht the extra line deleted. Please don't send further patches which mix function renames and argument list change.