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

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(a)gmail.com&gt; > > --- > > 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.