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@xxxxxxxxx>
---
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!
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
