[libvirt] [PATCH 13/21] qemu_hotplug: move qemuDomainDetachDeviceLive() to qemu_hotplug.c

[Peter Krempa]

On Fri, Mar 22, 2019 at 09:36:31 -0400, Laine Stump wrote:
> On 3/22/19 7:36 AM, Peter Krempa wrote:
> > On Thu, Mar 21, 2019 at 18:28:53 -0400, Laine Stump wrote:
> > > This function is going to take on some of the functionality of its
> > > subordinate functions, which all live in qemu_hotplug.c.
> > > 
> > > Signed-off-by: Laine Stump <laine at laine.org>
> > > ---
> > >   src/qemu/qemu_driver.c  |  95 -----------------------------
> > >   src/qemu/qemu_hotplug.c | 129 +++++++++++++++++++++++++++++++++++-----
> > >   src/qemu/qemu_hotplug.h |  68 ++++++---------------
> > >   3 files changed, 133 insertions(+), 159 deletions(-)
> > > 
> > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > > index a16eab5467..0b80004c6e 100644
> > > --- a/src/qemu/qemu_driver.c
> > > +++ b/src/qemu/qemu_driver.c
> > > @@ -8004,101 +8004,6 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
> > >       return ret;
> > >   }
> > > -static int
> > > -qemuDomainDetachDeviceControllerLive(virQEMUDriverPtr driver,
> > > -                                     virDomainObjPtr vm,
> > > -                                     virDomainDeviceDefPtr dev,
> > > -                                     bool async)
> > > -{
> > > -    virDomainControllerDefPtr cont = dev->data.controller;
> > > -    int ret = -1;
> > > -
> > > -    switch (cont->type) {
> > > -    case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
> > > -        ret = qemuDomainDetachControllerDevice(driver, vm, dev, async);
> > > -        break;
> > > -    default :
> > > -        virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> > > -                       _("'%s' controller cannot be hot unplugged."),
> > > -                       virDomainControllerTypeToString(cont->type));
> > > -    }
> > > -    return ret;
> > > -}
> > This function is not just moved ...
> > 
> > [...]
> > 
> > >   static int
> > >   qemuDomainChangeDiskLive(virDomainObjPtr vm,
> > > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > > index 1f96f56942..c7aba74c6b 100644
> > > --- a/src/qemu/qemu_hotplug.c
> > > +++ b/src/qemu/qemu_hotplug.c
> > > @@ -5540,10 +5540,11 @@ static bool qemuDomainControllerIsBusy(virDomainObjPtr vm,
> > >       }
> > >   }
> > > -int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
> > > -                                     virDomainObjPtr vm,
> > > -                                     virDomainDeviceDefPtr dev,
> > > -                                     bool async)
> > > +static int
> > > +qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
> > > +                                 virDomainObjPtr vm,
> > > +                                 virDomainDeviceDefPtr dev,
> > > +                                 bool async)
> > ... but replaced. This is a separate semantic change.
> 
> 
> No, those are two different functions. In the original code, you had this
> call chain:
> 
> 
> qemuDomainDetachDeviceLive
>   qemuDomainDetachDeviceControllerLive
>     qemuDomainDetachControllerDevice
> 
> and after the patch you have the same call chain. In a later patch, I merge qemuDomainDetachDeviceControllerLive and qemuDomainDeviceControllerDevice into a single function, but in this patch it's pure movement.

Okay then. I'd split out the change to exported functions so that it's
indeed just moving the described functions ...

> 
> > 

[...]

> > >   {
> > > +                                   virDomainObjPtr vm,
> > > +                                   virDomainDeviceDefPtr dev,
> > > +                                   bool async);
> > These header movements don't belong to this patch.
> 
> 
> Yeah, I suppose. Would you accept them in the same patch where I remove the
> prototypes for the functions that have become static? Or must that be
> separate as well?

I don't think you need to move them if you then delete them later.
Anyways, if you want to keep them in sync, then move the headers in the
patch which is moving the code. If there's more to move than just the
headers for functions which are moved it'll be better to do a separate
patch to clean up the headers.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190322/3948332f/attachment-0001.sig>