Re: [libvirt] [PATCH 10/13] qemu_driver: always check whether iothread is used by disk or not

[Peter Krempa]

On Fri, Feb 17, 2017 at 15:49:14 +0100, Pavel Hrdina wrote:
> If virDomainDelIOThread API was called with VIR_DOMAIN_AFFECT_LIVE
> and VIR_DOMAIN_AFFECT_CONFIG and both XML were already a different
> it could result in removing iothread from config XML even if there
> was a disk using that iothread.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_driver.c | 37 +
>  1 file changed, 25 insertions(+), 12 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 189c10ead5..1a7cc12874 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5740,6 +5740,25 @@ qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver,
>  }
>  
>  static int
> +qemuDomainDelIOThreadCheck(virDomainDefPtr def,
> +   unsigned int iothread_id)
> +{
> +size_t i;
> +
> +for (i = 0; i < def->ndisks; i++) {
> +if (def->disks[i]->iothread == iothread_id) {
> +virReportError(VIR_ERR_INVALID_ARG,
> +   _("cannot remove IOThread %u since it "
> + "is being used by disk '%s'"),
> +   iothread_id, def->disks[i]->dst);
> +return -1;
> +}
> +}
> +
> +return 0;
> +}
> +
> +static int
>  qemuDomainChgIOThread(virQEMUDriverPtr driver,
>virDomainObjPtr vm,
>unsigned int iothread_id,
> @@ -5773,6 +5792,9 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver,
>  if (qemuDomainHotplugAddIOThread(driver, vm, iothread_id) < 0)
>  goto endjob;
>  } else {
> +if (qemuDomainDelIOThreadCheck(def, iothread_id) < 0)
> +goto endjob;
> +
>  if (qemuDomainHotplugDelIOThread(driver, vm, iothread_id) < 0)
>  goto endjob;
>  }
> @@ -5797,6 +5819,9 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver,
>  goto endjob;
>  }
>  
> +if (qemuDomainDelIOThreadCheck(persistentDef, iothread_id) < 0)
> +goto endjob;

This check should be done prior to modifying the live state so that you
don't modify it and the API then returns failure.

On the other hand ... It's not worse than it was. Fixing the corner case
would require also moving the call to virDomainIOThreadIDFind.

Consider this as an ACK

Peter


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 10/13] qemu_driver: always check whether iothread is used by disk or not

[Pavel Hrdina]

If virDomainDelIOThread API was called with VIR_DOMAIN_AFFECT_LIVE
and VIR_DOMAIN_AFFECT_CONFIG and both XML were already a different
it could result in removing iothread from config XML even if there
was a disk using that iothread.

Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_driver.c | 37 +
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 189c10ead5..1a7cc12874 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5740,6 +5740,25 @@ qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver,
 }
 
 static int
+qemuDomainDelIOThreadCheck(virDomainDefPtr def,
+   unsigned int iothread_id)
+{
+size_t i;
+
+for (i = 0; i < def->ndisks; i++) {
+if (def->disks[i]->iothread == iothread_id) {
+virReportError(VIR_ERR_INVALID_ARG,
+   _("cannot remove IOThread %u since it "
+ "is being used by disk '%s'"),
+   iothread_id, def->disks[i]->dst);
+return -1;
+}
+}
+
+return 0;
+}
+
+static int
 qemuDomainChgIOThread(virQEMUDriverPtr driver,
   virDomainObjPtr vm,
   unsigned int iothread_id,
@@ -5773,6 +5792,9 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver,
 if (qemuDomainHotplugAddIOThread(driver, vm, iothread_id) < 0)
 goto endjob;
 } else {
+if (qemuDomainDelIOThreadCheck(def, iothread_id) < 0)
+goto endjob;
+
 if (qemuDomainHotplugDelIOThread(driver, vm, iothread_id) < 0)
 goto endjob;
 }
@@ -5797,6 +5819,9 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver,
 goto endjob;
 }
 
+if (qemuDomainDelIOThreadCheck(persistentDef, iothread_id) < 0)
+goto endjob;
+
 virDomainIOThreadIDDel(persistentDef, iothread_id);
 }
 
@@ -5855,7 +5880,6 @@ qemuDomainDelIOThread(virDomainPtr dom,
 virQEMUDriverPtr driver = dom->conn->privateData;
 virDomainObjPtr vm = NULL;
 int ret = -1;
-size_t i;
 
 virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
   VIR_DOMAIN_AFFECT_CONFIG, -1);
@@ -5872,17 +5896,6 @@ qemuDomainDelIOThread(virDomainPtr dom,
 if (virDomainDelIOThreadEnsureACL(dom->conn, vm->def, flags) < 0)
goto cleanup;
 
-/* If there is a disk using the IOThread to be removed, then fail. */
-for (i = 0; i < vm->def->ndisks; i++) {
-if (vm->def->disks[i]->iothread == iothread_id) {
-virReportError(VIR_ERR_INVALID_ARG,
-   _("cannot remove IOThread %u since it "
- "is being used by disk '%s'"),
-   iothread_id, vm->def->disks[i]->dst);
-goto cleanup;
-}
-}
-
 ret = qemuDomainChgIOThread(driver, vm, iothread_id, false, flags);
 
  cleanup:
-- 
2.11.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list