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

[Martin Kletzander]

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 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,

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.
-------------- 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/20211129/7099c7f4/attachment-0001.sig>