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@xxxxxxxxxx> > --- > 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.
