Mention where to obtain the index and how it's treated. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/libvirt-domain.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index fe4dab7cdf..ba30d18f65 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -12371,6 +12371,12 @@ int virDomainGetGuestInfo(virDomainPtr domain, * level is unset once the event fires. The event might not be delivered at all * if libvirtd was not running at the moment when the threshold was reached. * + * @dev can either be a disk target name (vda, sda) or disk target with index ( + * vda[4]). Without the index the top image in the backing chain will have the + * threshold set. The index corresponds to the 'index' attribute reported in the + * live VM XML for 'backingStore' or 'source' elements of a disk. If index is + * given the threshold is set for the corresponding image. + * * Hypervisors report the last written sector of an image in the bulk stats API * (virConnectGetAllDomainStats/virDomainListGetStats) as * "block.<num>.allocation" in the VIR_DOMAIN_STATS_BLOCK group. The current -- 2.26.2

Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 1 - src/qemu/qemu_domain.h | 1 - src/qemu/qemu_process.c | 2 +- 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 4a2daffc0a..3d136a6b8a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11515,7 +11515,6 @@ qemuDomainDiskLookupByNodename(virDomainDefPtr def, */ char * qemuDomainDiskBackingStoreGetName(virDomainDiskDefPtr disk, - virStorageSourcePtr src G_GNUC_UNUSED, unsigned int idx) { char *ret = NULL; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index e524fd0002..6944b37ff7 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -963,7 +963,6 @@ virDomainDiskDefPtr qemuDomainDiskLookupByNodename(virDomainDefPtr def, unsigned int *idx); char *qemuDomainDiskBackingStoreGetName(virDomainDiskDefPtr disk, - virStorageSourcePtr src, unsigned int idx); virStorageSourcePtr qemuDomainGetStorageSourceByDevstr(const char *devstr, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 70fc24b993..2ee778c606 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1514,7 +1514,7 @@ qemuProcessHandleBlockThreshold(qemuMonitorPtr mon G_GNUC_UNUSED, if (virStorageSourceIsLocalStorage(src)) path = src->path; - if ((dev = qemuDomainDiskBackingStoreGetName(disk, src, idx))) + if ((dev = qemuDomainDiskBackingStoreGetName(disk, idx))) event = virDomainEventBlockThresholdNewFromObj(vm, dev, path, threshold, excess); } -- 2.26.2

We can return the formatted string directly. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3d136a6b8a..cfdd9270da 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11517,14 +11517,10 @@ char * qemuDomainDiskBackingStoreGetName(virDomainDiskDefPtr disk, unsigned int idx) { - char *ret = NULL; - if (idx) - ret = g_strdup_printf("%s[%d]", disk->dst, idx); + return g_strdup_printf("%s[%d]", disk->dst, idx); else - ret = g_strdup(disk->dst); - - return ret; + return g_strdup(disk->dst); } -- 2.26.2

The index returned by qemuDomainDiskLookupByNodename is the position in the backing chain rather than the index we report in the XML. Since with -blockdev they differ now and additionally the disk source also has an index we need to fix the 'threshold' evens we report: 1) If it's the top level image we must always trigger the event without any suffix as we did until now 2) We must report the correct index 3) We must report the correct index also for the top level image, when blockdev is used. This means that we need to potentially emit 2 events, one for the device without the index and then when blockdev is used and the top level image has an idex we must do it also with the index. This will fix it for blockdev cases, while also not removing previous semantics. https://bugzilla.redhat.com/show_bug.cgi?id=1857204 Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_process.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2ee778c606..9b6dc8e68b 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1497,10 +1497,10 @@ qemuProcessHandleBlockThreshold(qemuMonitorPtr mon G_GNUC_UNUSED, void *opaque) { virQEMUDriverPtr driver = opaque; - virObjectEventPtr event = NULL; + virObjectEventPtr eventSource = NULL; + virObjectEventPtr eventDevice = NULL; virDomainDiskDefPtr disk; virStorageSourcePtr src; - unsigned int idx; const char *path = NULL; virObjectLock(vm); @@ -1509,18 +1509,28 @@ qemuProcessHandleBlockThreshold(qemuMonitorPtr mon G_GNUC_UNUSED, "threshold '%llu' exceeded by '%llu'", nodename, vm, vm->def->name, threshold, excess); - if ((disk = qemuDomainDiskLookupByNodename(vm->def, nodename, &src, &idx))) { - g_autofree char *dev = NULL; + if ((disk = qemuDomainDiskLookupByNodename(vm->def, nodename, &src, NULL))) { if (virStorageSourceIsLocalStorage(src)) path = src->path; - if ((dev = qemuDomainDiskBackingStoreGetName(disk, idx))) - event = virDomainEventBlockThresholdNewFromObj(vm, dev, path, - threshold, excess); + if (src == disk->src) { + g_autofree char *dev = qemuDomainDiskBackingStoreGetName(disk, 0); + + eventDevice = virDomainEventBlockThresholdNewFromObj(vm, dev, path, + threshold, excess); + } + + if (src->id != 0) { + g_autofree char *dev = qemuDomainDiskBackingStoreGetName(disk, src->id); + + eventSource = virDomainEventBlockThresholdNewFromObj(vm, dev, path, + threshold, excess); + } } virObjectUnlock(vm); - virObjectEventStateQueue(driver->domainEventState, event); + virObjectEventStateQueue(driver->domainEventState, eventDevice); + virObjectEventStateQueue(driver->domainEventState, eventSource); return 0; } -- 2.26.2

Top level image may get two events, one with the disk target (vda) and one with disk target with index (vda[3]) if the top level image has an index. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/libvirt-domain.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index ba30d18f65..6c5ff5b0db 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -12370,6 +12370,10 @@ int virDomainGetGuestInfo(virDomainPtr domain, * described by @dev is written beyond the set threshold level. The threshold * level is unset once the event fires. The event might not be delivered at all * if libvirtd was not running at the moment when the threshold was reached. + * Note that if the threshold level is reached for a top level image the event + * is emitted for @dev corresponding to the disk target, and may also be reported + * with @dev corresponding to the disk target with an index corresponding to the + * 'index' attribute of 'source' in the live VM XML if the atribute is present. * * @dev can either be a disk target name (vda, sda) or disk target with index ( * vda[4]). Without the index the top image in the backing chain will have the -- 2.26.2

All callers pass NULL as the value. Remove the argument. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 12 ++---------- src/qemu/qemu_domain.h | 3 +-- src/qemu/qemu_process.c | 4 ++-- 3 files changed, 5 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index cfdd9270da..64bd52c51f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11469,7 +11469,6 @@ qemuDomainNamespaceTeardownInput(virDomainObjPtr vm, * @def: domain definition to look for the disk * @nodename: block backend node name to find * @src: filled with the specific backing store element if provided - * @idx: index of @src in the backing chain, if provided * * Looks up the disk in the domain via @nodename and returns its definition. * Optionally fills @src and @idx if provided with the specific backing chain @@ -11478,24 +11477,17 @@ qemuDomainNamespaceTeardownInput(virDomainObjPtr vm, virDomainDiskDefPtr qemuDomainDiskLookupByNodename(virDomainDefPtr def, const char *nodename, - virStorageSourcePtr *src, - unsigned int *idx) + virStorageSourcePtr *src) { size_t i; - unsigned int srcindex; virStorageSourcePtr tmp = NULL; - if (!idx) - idx = &srcindex; - if (src) *src = NULL; - *idx = 0; - for (i = 0; i < def->ndisks; i++) { if ((tmp = virStorageSourceFindByNodeName(def->disks[i]->src, - nodename, idx))) { + nodename, NULL))) { if (src) *src = tmp; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 6944b37ff7..d75fbc0af3 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -959,8 +959,7 @@ int qemuDomainNamespaceTeardownInput(virDomainObjPtr vm, virDomainDiskDefPtr qemuDomainDiskLookupByNodename(virDomainDefPtr def, const char *nodename, - virStorageSourcePtr *src, - unsigned int *idx); + virStorageSourcePtr *src); char *qemuDomainDiskBackingStoreGetName(virDomainDiskDefPtr disk, unsigned int idx); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9b6dc8e68b..580abb0fb4 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -880,7 +880,7 @@ qemuProcessHandleIOError(qemuMonitorPtr mon G_GNUC_UNUSED, if (diskAlias) disk = qemuProcessFindDomainDiskByAliasOrQOM(vm, diskAlias, NULL); else if (nodename) - disk = qemuDomainDiskLookupByNodename(vm->def, nodename, NULL, NULL); + disk = qemuDomainDiskLookupByNodename(vm->def, nodename, NULL); else disk = NULL; @@ -1509,7 +1509,7 @@ qemuProcessHandleBlockThreshold(qemuMonitorPtr mon G_GNUC_UNUSED, "threshold '%llu' exceeded by '%llu'", nodename, vm, vm->def->name, threshold, excess); - if ((disk = qemuDomainDiskLookupByNodename(vm->def, nodename, &src, NULL))) { + if ((disk = qemuDomainDiskLookupByNodename(vm->def, nodename, &src))) { if (virStorageSourceIsLocalStorage(src)) path = src->path; -- 2.26.2

None of the callers actually use it. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 9 ++++----- src/util/virstoragefile.c | 16 +++------------- src/util/virstoragefile.h | 3 +-- 3 files changed, 8 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 64bd52c51f..ed7ec77ed4 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2498,15 +2498,15 @@ qemuDomainObjPrivateXMLParseBlockjobNodename(qemuBlockJobDataPtr job, return; if (job->disk && - (*src = virStorageSourceFindByNodeName(job->disk->src, nodename, NULL))) + (*src = virStorageSourceFindByNodeName(job->disk->src, nodename))) return; if (job->chain && - (*src = virStorageSourceFindByNodeName(job->chain, nodename, NULL))) + (*src = virStorageSourceFindByNodeName(job->chain, nodename))) return; if (job->mirrorChain && - (*src = virStorageSourceFindByNodeName(job->mirrorChain, nodename, NULL))) + (*src = virStorageSourceFindByNodeName(job->mirrorChain, nodename))) return; /* the node was in the XML but was not found in the job definitions */ @@ -11486,8 +11486,7 @@ qemuDomainDiskLookupByNodename(virDomainDefPtr def, *src = NULL; for (i = 0; i < def->ndisks; i++) { - if ((tmp = virStorageSourceFindByNodeName(def->disks[i]->src, - nodename, NULL))) { + if ((tmp = virStorageSourceFindByNodeName(def->disks[i]->src, nodename))) { if (src) *src = tmp; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 274883c4bd..00d8e16ef9 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -4589,33 +4589,23 @@ virStorageSourceIsRelative(virStorageSourcePtr src) * virStorageSourceFindByNodeName: * @top: backing chain top * @nodeName: node name to find in backing chain - * @index: if provided the index in the backing chain * * Looks up the given storage source in the backing chain and returns the - * pointer to it. If @index is passed then it's filled by the index in the - * backing chain. On failure NULL is returned and no error is reported. + * pointer to it. + * On failure NULL is returned and no error is reported. */ virStorageSourcePtr virStorageSourceFindByNodeName(virStorageSourcePtr top, - const char *nodeName, - unsigned int *idx) + const char *nodeName) { virStorageSourcePtr tmp; - if (idx) - *idx = 0; - for (tmp = top; virStorageSourceIsBacking(tmp); tmp = tmp->backingStore) { if ((tmp->nodeformat && STREQ(tmp->nodeformat, nodeName)) || (tmp->nodestorage && STREQ(tmp->nodestorage, nodeName))) return tmp; - - if (idx) - (*idx)++; } - if (idx) - *idx = 0; return NULL; } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index c68bdc9680..f73b3ee005 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -526,8 +526,7 @@ bool virStorageSourceIsRelative(virStorageSourcePtr src); virStorageSourcePtr virStorageSourceFindByNodeName(virStorageSourcePtr top, - const char *nodeName, - unsigned int *index) + const char *nodeName) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); void -- 2.26.2

When doing a block copy, there is another chain of images attached to a disk. Consider them as well when looking up a disk using nodename. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ed7ec77ed4..18fd445e30 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11492,6 +11492,14 @@ qemuDomainDiskLookupByNodename(virDomainDefPtr def, return def->disks[i]; } + + if (def->disks[i]->mirror && + (tmp = virStorageSourceFindByNodeName(def->disks[i]->mirror, nodename))) { + if (src) + *src = tmp; + + return def->disks[i]; + } } return NULL; -- 2.26.2

A disk can have a mirror, look also in it's backing chain. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 18fd445e30..ebf18a546e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11553,11 +11553,16 @@ qemuDomainGetStorageSourceByDevstr(const char *devstr, } if (idx == 0) - src = disk->src; - else - src = virStorageFileChainLookup(disk->src, NULL, NULL, idx, NULL); + return disk->src; + + if ((src = virStorageFileChainLookup(disk->src, NULL, NULL, idx, NULL))) + return src; - return src; + if (disk->mirror && + (src = virStorageFileChainLookup(disk->mirror, NULL, NULL, idx, NULL))) + return src; + + return NULL; } -- 2.26.2

On 7/15/20 8:10 AM, Peter Krempa wrote: Reviewed-by: Eric Blake <eblake(a)redhat.com&gt; -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org