[PATCH] qemuDomainDiskChangeSupported: Add missing iothreads check
GSList of iothreads is not allowed to be changed while the virtual machine is running. Resolves: https://issues.redhat.com/browse/RHEL-23607 Signed-off-by: Adam Julis <ajulis@redhat.com> --- While the qemuDomainDiskChangeSupported() design primarily uses its macros (CHECK_EQ and CHECK_STREQ_NULLABLE), the logic for comparing 2 GSList of iothreads could perhaps be extracted into a separate function (e.g. IothreadsGslistCompare(GSList *first, GSList *second)). I am absolutely not sure about this idea so feel free to comment. src/qemu/qemu_domain.c | 53 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 298f4bfb9e..2b5222c685 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8505,6 +8505,59 @@ qemuDomainDiskChangeSupported(virDomainDiskDef *disk, CHECK_EQ(discard, "discard", true); CHECK_EQ(iothread, "iothread", true); + /* compare list of iothreads, no change allowed */ + if (orig_disk->iothreads != disk->iothreads) { + GSList *old; + GSList *new = disk->iothreads; + bool print_err = true; + + for (old = orig_disk->iothreads; old; old = old->next) { + virDomainDiskIothreadDef *orig = old->data; + virDomainDiskIothreadDef *update; + print_err = false; + + if (new == NULL) { + print_err = true; + break; + } + + update = new->data; + + if (orig->id != update->id) { + print_err = true; + break; + } + + if (orig->nqueues != update->nqueues) { + print_err = true; + break; + } + + if (orig->nqueues != 0) { + ssize_t i = 0; + + while (i < orig->nqueues) { + if (orig->queues[i] != update->queues[i]) { + print_err = true; + break; + } + } + } + + new = new->next; + if (new) + print_err = true; + } + + if (print_err) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("cannot modify field '%1$s' (or it's parts) of the disk"), + "iothreads"); + return false; + } + } + + CHECK_STREQ_NULLABLE(domain_name, "backenddomain"); -- 2.45.2
On Fri, Jul 26, 2024 at 10:52:18AM +0200, Adam Julis wrote:
GSList of iothreads is not allowed to be changed while the virtual machine is running.
Resolves: https://issues.redhat.com/browse/RHEL-23607 Signed-off-by: Adam Julis <ajulis@redhat.com> --- While the qemuDomainDiskChangeSupported() design primarily uses its macros (CHECK_EQ and CHECK_STREQ_NULLABLE), the logic for comparing 2 GSList of iothreads could perhaps be extracted into a separate function (e.g. IothreadsGslistCompare(GSList *first, GSList *second)). I am absolutely not sure about this idea so feel free to comment.
Since we are doing this in one place and glib does not have such function I'd say leave it. What I'd probably extract into its own function now that I'm looking into the patch is something like virDomainDiskIothreadDefEquals() which you could run in a loop like this: for (old = orig_disk->iothreads, new = orig_disk->iothreads; old && new; old = old->next, new = new->next) { if (!virDomainDiskIothreadDefEquals(old->data, new->data)) break; } if (old || new) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("cannot modify field '%1$s' (or it's parts) of the disk"), "iothreads"); return false; } I understand it might be less readable for some, but the suggested extracted function above will be way more concise since you can just return false instead of `{ print_err = true; break; }`.
src/qemu/qemu_domain.c | 53 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 298f4bfb9e..2b5222c685 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8505,6 +8505,59 @@ qemuDomainDiskChangeSupported(virDomainDiskDef *disk, CHECK_EQ(discard, "discard", true); CHECK_EQ(iothread, "iothread", true);
+ /* compare list of iothreads, no change allowed */ + if (orig_disk->iothreads != disk->iothreads) {
Since the new disk will be parsed separately the pointers will never be the same, unless I missed some part of the calling flow in which case it is updated with the old information, but I don't think so.
+ GSList *old; + GSList *new = disk->iothreads; + bool print_err = true; + + for (old = orig_disk->iothreads; old; old = old->next) { + virDomainDiskIothreadDef *orig = old->data; + virDomainDiskIothreadDef *update; + print_err = false; + + if (new == NULL) { + print_err = true; + break; + } + + update = new->data; + + if (orig->id != update->id) { + print_err = true; + break; + } + + if (orig->nqueues != update->nqueues) { + print_err = true; + break; + } + + if (orig->nqueues != 0) { + ssize_t i = 0; + + while (i < orig->nqueues) { + if (orig->queues[i] != update->queues[i]) { + print_err = true; + break; + } + } + } + + new = new->next; + if (new) + print_err = true; + } + + if (print_err) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("cannot modify field '%1$s' (or it's parts) of the disk"), + "iothreads"); + return false; + } + } + + CHECK_STREQ_NULLABLE(domain_name, "backenddomain");
-- 2.45.2
On Fri, Jul 26, 2024 at 10:52:18 +0200, Adam Julis wrote:
GSList of iothreads is not allowed to be changed while the virtual machine is running.
Resolves: https://issues.redhat.com/browse/RHEL-23607 Signed-off-by: Adam Julis <ajulis@redhat.com> --- While the qemuDomainDiskChangeSupported() design primarily uses its macros (CHECK_EQ and CHECK_STREQ_NULLABLE), the logic for comparing 2 GSList of iothreads could perhaps be extracted into a separate function (e.g. IothreadsGslistCompare(GSList *first, GSList *second)). I am absolutely not sure about this idea so feel free to comment.
src/qemu/qemu_domain.c | 53 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 298f4bfb9e..2b5222c685 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8505,6 +8505,59 @@ qemuDomainDiskChangeSupported(virDomainDiskDef *disk, CHECK_EQ(discard, "discard", true); CHECK_EQ(iothread, "iothread", true);
+ /* compare list of iothreads, no change allowed */
This is a bit too long for the existing function and thus should be put into it's own: static bool qemuDomainDiskChangeSupportedIothreads(virDomainDiskDef *disk, virDomainDiskDef *orig_disk)
+ if (orig_disk->iothreads != disk->iothreads) {
These are pointers which will be equal only if both are NULL. While this is technically correct it's confusing at first glance. Since the loop below needs to handle mismatched-lenght GSlists anyways there's no need to do this check before.
+ GSList *old; + GSList *new = disk->iothreads; + bool print_err = true; + + for (old = orig_disk->iothreads; old; old = old->next) { + virDomainDiskIothreadDef *orig = old->data; + virDomainDiskIothreadDef *update; + print_err = false; + + if (new == NULL) { + print_err = true; + break; + }
The two styles of iterating through the two GSlists are also confusing at the first glance. In order to make the code more readable I'll change it to a uniform style.
+ + update = new->data; + + if (orig->id != update->id) { + print_err = true; + break; + } + + if (orig->nqueues != update->nqueues) { + print_err = true; + break; + } + + if (orig->nqueues != 0) { + ssize_t i = 0;
'nqueues' is a 'size_t' so no need for ssize_t.
+ + while (i < orig->nqueues) { + if (orig->queues[i] != update->queues[i]) { + print_err = true; + break; + }
This loop doesn't have an increment, so it'll loop forever if the first entry matches.
+ } + } + + new = new->next; + if (new) + print_err = true; + } + + if (print_err) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("cannot modify field '%1$s' (or it's parts) of the disk"), + "iothreads");
This is translation unfriendly: https://www.libvirt.org/coding-style.html#error-message-format I'll post a v2.
participants (3)
-
Adam Julis -
Martin Kletzander -
Peter Krempa