[libvirt] [PATCH 0/2] Introduce virDomainDeviceIterate

A function for iterating over all devices [0] instead of just the ones with DeviceInfo. [0] Terms and conditions apply. Leases might be included as well. Ján Tomko (2): Introduce virDomainDeviceIterate Introduce DOMAIN_DEVICE_ITERATE_MISSING_INFO src/conf/domain_conf.c | 47 ++++++++++++++++++++++++++++++------------ src/conf/domain_conf.h | 3 +++ 2 files changed, 37 insertions(+), 13 deletions(-) -- 2.19.2

A counterpart to virDomainDeviceInfoIterate that will iterate over all devices, not just those with an info. Use it in places where we intend to process all devices with callbacks that do not depend on DeviceInfo being present: * virDomainDefPostParse * virDomainDefValidate Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/conf/domain_conf.c | 28 +++++++++++++++++++--------- src/conf/domain_conf.h | 3 +++ 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a3a514136b..fb1256f640 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4297,6 +4297,19 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def, } +int +virDomainDeviceIterate(virDomainDefPtr def, + virDomainDeviceInfoCallback cb, + void *opaque) +{ + return virDomainDeviceInfoIterateInternal(def, + cb, + DOMAIN_DEVICE_ITERATE_ALL_CONSOLES | + DOMAIN_DEVICE_ITERATE_GRAPHICS, + opaque); +} + + int virDomainDeviceInfoIterate(virDomainDefPtr def, virDomainDeviceInfoCallback cb, @@ -5793,10 +5806,9 @@ virDomainDefPostParse(virDomainDefPtr def, } /* iterate the devices */ - ret = virDomainDeviceInfoIterateInternal(def, - virDomainDefPostParseDeviceIterator, - DOMAIN_DEVICE_ITERATE_ALL_CONSOLES, - &data); + ret = virDomainDeviceIterate(def, + virDomainDefPostParseDeviceIterator, + &data); if (virDomainDefPostParseCheckFailure(def, parseFlags, ret) < 0) goto cleanup; @@ -6923,11 +6935,9 @@ virDomainDefValidate(virDomainDefPtr def, return -1; /* iterate the devices */ - if (virDomainDeviceInfoIterateInternal(def, - virDomainDefValidateDeviceIterator, - (DOMAIN_DEVICE_ITERATE_ALL_CONSOLES | - DOMAIN_DEVICE_ITERATE_GRAPHICS), - &data) < 0) + if (virDomainDeviceIterate(def, + virDomainDefValidateDeviceIterator, + &data) < 0) return -1; if (virDomainDefValidateInternal(def, xmlopt) < 0) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index fa0756b634..15ecdac6f7 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2869,6 +2869,9 @@ typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def, virDomainDeviceInfoPtr info, void *opaque); +int virDomainDeviceIterate(virDomainDefPtr def, + virDomainDeviceInfoCallback cb, + void *opaque); int virDomainDeviceInfoIterate(virDomainDefPtr def, virDomainDeviceInfoCallback cb, void *opaque); -- 2.19.2

On Tue, 2019-05-21 at 16:25 +0200, Ján Tomko wrote: [...]
@@ -5793,10 +5806,9 @@ virDomainDefPostParse(virDomainDefPtr def, }
/* iterate the devices */ - ret = virDomainDeviceInfoIterateInternal(def, - virDomainDefPostParseDeviceIterator, - DOMAIN_DEVICE_ITERATE_ALL_CONSOLES, - &data); + ret = virDomainDeviceIterate(def, + virDomainDefPostParseDeviceIterator, + &data);
Since we have no callers of the new function outside of domain_conf, we can leave it private. Actually, we can do better, by not creating the function in the first place and simply adding the missing flag to this single call to virDomainDeviceInfoIterateInternal(). NACK -- Andrea Bolognani / Red Hat / Virtualization

Rename the DOMAIN_DEVICE_ITERATE_GRAPHICS flag. It was introduced by commit dd45c2710f6fd2d4f8a47f97960532d0e0091e7d with the intention to run the Validate callback even on the graphics device. However, enumerating every single device in virDomainDeviceIterateFlags is unsustainable and what really was special about the graphics device was the lack of DeviceInfo. Rename the flag and iterate over more info-less devices. (and leases) Signed-off-by: Ján Tomko <jtomko@redhat.com> Inspired-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fb1256f640..0fe440a2cf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4077,7 +4077,7 @@ virDomainSkipBackcompatConsole(virDomainDefPtr def, enum { DOMAIN_DEVICE_ITERATE_ALL_CONSOLES = 1 << 0, - DOMAIN_DEVICE_ITERATE_GRAPHICS = 1 << 1 + DOMAIN_DEVICE_ITERATE_MISSING_INFO = 1 << 1, } virDomainDeviceIterateFlags; /* @@ -4243,15 +4243,26 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def, return rc; } - /* If the flag below is set, make sure @cb can handle @info being NULL, as - * graphics don't have any boot info */ - if (iteratorFlags & DOMAIN_DEVICE_ITERATE_GRAPHICS) { + /* If the flag below is set, make sure @cb can handle @info being NULL */ + if (iteratorFlags & DOMAIN_DEVICE_ITERATE_MISSING_INFO) { device.type = VIR_DOMAIN_DEVICE_GRAPHICS; for (i = 0; i < def->ngraphics; i++) { device.data.graphics = def->graphics[i]; if ((rc = cb(def, &device, NULL, opaque)) != 0) return rc; } + device.type = VIR_DOMAIN_DEVICE_LEASE; + for (i = 0; i < def->nleases; i++) { + device.data.lease = def->leases[i]; + if ((rc = cb(def, &device, NULL, opaque)) != 0) + return rc; + } + device.type = VIR_DOMAIN_DEVICE_IOMMU; + if (def->iommu) { + device.data.iommu = def->iommu; + if ((rc = cb(def, &device, NULL, opaque)) != 0) + return rc; + } } /* Coverity is not very happy with this - all dead_error_condition */ @@ -4305,7 +4316,7 @@ virDomainDeviceIterate(virDomainDefPtr def, return virDomainDeviceInfoIterateInternal(def, cb, DOMAIN_DEVICE_ITERATE_ALL_CONSOLES | - DOMAIN_DEVICE_ITERATE_GRAPHICS, + DOMAIN_DEVICE_ITERATE_MISSING_INFO, opaque); } -- 2.19.2

On Tue, 2019-05-21 at 16:26 +0200, Ján Tomko wrote:
Rename the DOMAIN_DEVICE_ITERATE_GRAPHICS flag. It was introduced by commit dd45c2710f6fd2d4f8a47f97960532d0e0091e7d with the intention to run the Validate callback even on the graphics device.
However, enumerating every single device in virDomainDeviceIterateFlags is unsustainable and what really was special about the graphics device was the lack of DeviceInfo.
Rename the flag and iterate over more info-less devices. (and leases)
Signed-off-by: Ján Tomko <jtomko@redhat.com> Inspired-by: Andrea Bolognani <abologna@redhat.com>
I very clearly voiced my disagreement with this approach, so please drop this tag before pushing. ACK -- Andrea Bolognani / Red Hat / Virtualization

On Wed, May 22, 2019 at 05:22:42PM +0200, Andrea Bolognani wrote:
On Tue, 2019-05-21 at 16:26 +0200, Ján Tomko wrote:
Rename the DOMAIN_DEVICE_ITERATE_GRAPHICS flag. It was introduced by commit dd45c2710f6fd2d4f8a47f97960532d0e0091e7d with the intention to run the Validate callback even on the graphics device.
However, enumerating every single device in virDomainDeviceIterateFlags is unsustainable and what really was special about the graphics device was the lack of DeviceInfo.
Rename the flag and iterate over more info-less devices. (and leases)
Signed-off-by: Ján Tomko <jtomko@redhat.com> Inspired-by: Andrea Bolognani <abologna@redhat.com>
I very clearly voiced my disagreement with this approach, so please drop this tag before pushing.
Done.
ACK
Jano
participants (2)
-
Andrea Bolognani
-
Ján Tomko