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

[Laine Stump]

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!