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 <[email protected]>
> ---
> 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.
