On Mon, 2021-11-29 at 15:33 +0100, Martin Kletzander wrote:
> On Mon, Nov 29, 2021 at 09:06:57PM +0800, Luke Yue wrote:
> > 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,
>
> Not really into one, although if you figure out a way to make that
> easily possible feel free to go on, what I meant make them look and
> read
> the same.
>
> > 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.
> >
>
> Deduplicating the bigger function would be nice to have for the
> future,
> it does not need to happen together in this one. It will require
> some
> more work to make it "modular" enough to be used by various drivers.
OK, thanks, now I see.
