[PATCH v3 02/12] domain_driver: extract DetachXXXDeviceConfig related functions and use them

[Luke Yue]

On Fri, 2021-11-26 at 16:43 +0100, Martin Kletzander wrote:
> On Wed, Nov 10, 2021 at 10:24:21PM +0800, Luke Yue wrote:
> > libxl / lxc / qemu drivers share some common codes in their
> > DomainDetachDeviceConfig functions, so extract them to
> > domain_driver and
> > reuse them.
> > 
> 
> I would argue that these specific functions are actually dealing just
> with the domain definition which is done mostly in domain_conf.c, so
> I
> would pick that place.
> 

Thanks for the review! No problem, I would move the code to
domain_conf.c in next version.

> > At the same time, this will enable test driver to test these
> > functions
> > with virshtest in the future.
> > 
> > Signed-off-by: Luke Yue <lukedyue at gmail.com>
> > ---
> > Not pretty sure whether this is a proper way to make these
> > functions
> > reusable, maybe there is a more elegant choice.
> 
> The patch does a good job in deduplicating some of the code and
> except
> the file placement I think the overall approach is fine.
> 
> However unfortunately the functions are just copy-paste from various
> places and they could be cleaned up to look the same.  For example...
> 
> > ---
> > src/hypervisor/domain_driver.c | 266
> > +++++++++++++++++++++++++++++++++
> > src/hypervisor/domain_driver.h |  41 +++++
> > src/libvirt_private.syms       |  14 ++
> > src/libxl/libxl_driver.c       |  41 +----
> > src/lxc/lxc_driver.c           |  37 +----
> > src/qemu/qemu_driver.c         | 123 +++------------
> > 6 files changed, 356 insertions(+), 166 deletions(-)
> > 
> > diff --git a/src/hypervisor/domain_driver.c
> > b/src/hypervisor/domain_driver.c
> > index 31737b0f4a..01ecb4e30e 100644
> > --- a/src/hypervisor/domain_driver.c
> > +++ b/src/hypervisor/domain_driver.c
> > @@ -644,3 +644,269 @@
> > virDomainDriverGetIOThreadsConfig(virDomainDef *targetDef,
> > 
> >     return ret;
> > }
> > +
> > +
> > +int
> > +virDomainDriverDetachDiskDeviceConfig(virDomainDef *vmdef,
> > +                                      virDomainDeviceDef *dev)
> > +{
> > +    virDomainDiskDef *disk;
> > +    virDomainDiskDef *det_disk;
> > +
> > +    disk = dev->data.disk;
> > +    if (!(det_disk = virDomainDiskRemoveByName(vmdef, disk->dst)))
> > {
> > +        virReportError(VIR_ERR_DEVICE_MISSING, _("no target device
> > %s"),
> > +                       disk->dst);
> > +        return -1;
> > +    }
> > +    virDomainDiskDefFree(det_disk);
> > +
> > +    return 0;
> 
> This function uses temporary variables for everything and adds an
> error
> message (which could be moved to the virDomainDiskRemoveByName() as
> all
> callers do report the same error message when it is not found.  Other
> functions do essentially the same job, but have random comments, some
> have extra helper variables. All that could be nicely unified and it
> would read so much nicer.
> 
> What I really like about this change is that apart from roughly two
> device types, like video and chardev, this function could be
> de-duplicated as well.  Adding a new functionality to that de-
> duplicated
> function would then add that functionality into all the drivers using
> this function.  Of course the special devices would need some xmlopt
> callbacks or something that drivers can modify themselves for such
> reasons, but such change would be really, really helpful, especially
> in
> the long term.

So if I understand you correctly, I will unified all these functions
into one, just like the original ones for QEMU / lxc / libxl, but this
one is for all, and maybe I should create a new enum for flags of
detachable device that different hypervisor support, and xmlopt etc for
special devices. I will do that in next version.

Thanks,
Luke