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@xxxxxxxxx>
> > ---
> > 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
