[libvirt] [PATCH v2 08/14] qemu_hotplug: standardize the names/args/calling of qemuDomainDetach*()
Laine Stump
laine at laine.org
Tue Mar 26 13:52:44 UTC 2019
On 3/26/19 8:40 AM, Peter Krempa wrote:
> 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.
Yeah, valid point. I just sometimes grow weary of changing a line in one
patch just to change the same line again another. But I guess I do that
enough anyway, so one more time shouldn't bother me :-P
I'll be sure to inflate my patch count as much as reasonably possible
next time!
>
>> 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 at laine.org>
>> ---
>>
>> 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.
I'm not a fan of *not* putting curly braces 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.
Good point. I'm guessing it's because many others of these functions
also started out in qemu_driver.c (and at a time when were less
consistent with function naming) and were moved to this file with no
name change.
A followup patch to make the names all consistent might be nice...
>
> ACK witht the extra line deleted. Please don't send further patches
> which mix function renames and argument list change.
>
Yes sir!
More information about the libvir-list
mailing list