Re: [PATCH] qemuDomainDiskChangeSupported: Add missing iothreads check

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

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

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

Attachment: signature.asc
Description: PGP signature