1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/4] qemu: Fix CPU model broken by older libvirt
  4. View patch

[libvirt] [PATCH 1/4] qemu: Separate CPU updating code from qemuProcessReconnect

Jiri Denemark posted 4 patches 7 years, 7 months ago
[libvirt] [PATCH 1/4] qemu: Separate CPU updating code from qemuProcessReconnect
Posted by Jiri Denemark 7 years, 7 months ago 
The new function is called qemuProcessRefreshCPU.

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---
 src/qemu/qemu_process.c | 66 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 23 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 0cb023095b..5ed6b68eb8 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6876,6 +6876,47 @@ qemuProcessRefreshDisks(virQEMUDriverPtr driver,
 }
 
 
+static int
+qemuProcessRefreshCPU(virQEMUDriverPtr driver,
+                      virDomainObjPtr vm)
+{
+    virCapsPtr caps = virQEMUDriverGetCapabilities(driver, false);
+    virCPUDefPtr host = NULL;
+    int ret = -1;
+
+    if (!virQEMUCapsGuestIsNative(caps->host.arch, vm->def->os.arch) ||
+        !caps->host.cpu ||
+        !vm->def->cpu)
+        return 0;
+
+    if (!caps)
+        goto cleanup;
+
+    /* If the domain with a host-model CPU was started by an old libvirt
+     * (< 2.3) which didn't replace the CPU with a custom one, let's do it now
+     * since the rest of our code does not really expect a host-model CPU in a
+     * running domain.
+     */
+    if (vm->def->cpu->mode == VIR_CPU_MODE_HOST_MODEL) {
+        if (!(host = virCPUCopyMigratable(caps->host.cpu->arch, caps->host.cpu)))
+            goto cleanup;
+
+        if (virCPUUpdate(vm->def->os.arch, vm->def->cpu, host) < 0)
+            goto cleanup;
+
+        if (qemuProcessUpdateCPU(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
+            goto cleanup;
+    }
+
+    ret = 0;
+
+ cleanup:
+    virCPUDefFree(host);
+    virObjectUnref(caps);
+    return ret;
+}
+
+
 struct qemuProcessReconnectData {
     virConnectPtr conn;
     virQEMUDriverPtr driver;
@@ -7042,29 +7083,8 @@ qemuProcessReconnect(void *opaque)
     ignore_value(qemuSecurityCheckAllLabel(driver->securityManager,
                                            obj->def));
 
-    /* If the domain with a host-model CPU was started by an old libvirt
-     * (< 2.3) which didn't replace the CPU with a custom one, let's do it now
-     * since the rest of our code does not really expect a host-model CPU in a
-     * running domain.
-     */
-    if (virQEMUCapsGuestIsNative(caps->host.arch, obj->def->os.arch) &&
-        caps->host.cpu &&
-        obj->def->cpu &&
-        obj->def->cpu->mode == VIR_CPU_MODE_HOST_MODEL) {
-        virCPUDefPtr host;
-
-        if (!(host = virCPUCopyMigratable(caps->host.cpu->arch, caps->host.cpu)))
-            goto error;
-
-        if (virCPUUpdate(obj->def->os.arch, obj->def->cpu, host) < 0) {
-            virCPUDefFree(host);
-            goto error;
-        }
-        virCPUDefFree(host);
-
-        if (qemuProcessUpdateCPU(driver, obj, QEMU_ASYNC_JOB_NONE) < 0)
-            goto error;
-    }
+    if (qemuProcessRefreshCPU(driver, obj) < 0)
+        goto error;
 
     if (qemuDomainRefreshVcpuInfo(driver, obj, QEMU_ASYNC_JOB_NONE, true) < 0)
         goto error;
-- 
2.14.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] qemu: Separate CPU updating code from qemuProcessReconnect
Posted by Marc Hartmayer 7 years, 7 months ago 
On Wed, Oct 11, 2017 at 12:11 PM +0200, Jiri Denemark <jdenemar@redhat.com> wrote:
> The new function is called qemuProcessRefreshCPU.
>
> Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> ---
>  src/qemu/qemu_process.c | 66 ++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 43 insertions(+), 23 deletions(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 0cb023095b..5ed6b68eb8 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -6876,6 +6876,47 @@ qemuProcessRefreshDisks(virQEMUDriverPtr driver,
>  }
>
>
> +static int
> +qemuProcessRefreshCPU(virQEMUDriverPtr driver,
> +                      virDomainObjPtr vm)
> +{
> +    virCapsPtr caps = virQEMUDriverGetCapabilities(driver, false);
> +    virCPUDefPtr host = NULL;
> +    int ret = -1;
> +
> +    if (!virQEMUCapsGuestIsNative(caps->host.arch, vm->def->os.arch) ||
> +        !caps->host.cpu ||
> +        !vm->def->cpu)
> +        return 0;
> +
> +    if (!caps)
> +        goto cleanup;

That's somehow weird...  We access 'caps->host.arch'/ 'caps->host.cpu'
and after that we're checking for a null pointer?!

> +
> +    /* If the domain with a host-model CPU was started by an old libvirt
> +     * (< 2.3) which didn't replace the CPU with a custom one, let's do it now
> +     * since the rest of our code does not really expect a host-model CPU in a
> +     * running domain.
> +     */
> +    if (vm->def->cpu->mode == VIR_CPU_MODE_HOST_MODEL) {
> +        if (!(host = virCPUCopyMigratable(caps->host.cpu->arch, caps->host.cpu)))
> +            goto cleanup;
> +
> +        if (virCPUUpdate(vm->def->os.arch, vm->def->cpu, host) < 0)
> +            goto cleanup;
> +
> +        if (qemuProcessUpdateCPU(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
> +            goto cleanup;
> +    }
> +
> +    ret = 0;
> +
> + cleanup:
> +    virCPUDefFree(host);
> +    virObjectUnref(caps);
> +    return ret;
> +}
> +
> +
>  struct qemuProcessReconnectData {
>      virConnectPtr conn;
>      virQEMUDriverPtr driver;
> @@ -7042,29 +7083,8 @@ qemuProcessReconnect(void *opaque)
>      ignore_value(qemuSecurityCheckAllLabel(driver->securityManager,
>                                             obj->def));
>
> -    /* If the domain with a host-model CPU was started by an old libvirt
> -     * (< 2.3) which didn't replace the CPU with a custom one, let's do it now
> -     * since the rest of our code does not really expect a host-model CPU in a
> -     * running domain.
> -     */
> -    if (virQEMUCapsGuestIsNative(caps->host.arch, obj->def->os.arch) &&
> -        caps->host.cpu &&
> -        obj->def->cpu &&
> -        obj->def->cpu->mode == VIR_CPU_MODE_HOST_MODEL) {
> -        virCPUDefPtr host;
> -
> -        if (!(host = virCPUCopyMigratable(caps->host.cpu->arch, caps->host.cpu)))
> -            goto error;
> -
> -        if (virCPUUpdate(obj->def->os.arch, obj->def->cpu, host) < 0) {
> -            virCPUDefFree(host);
> -            goto error;
> -        }
> -        virCPUDefFree(host);
> -
> -        if (qemuProcessUpdateCPU(driver, obj, QEMU_ASYNC_JOB_NONE) < 0)
> -            goto error;
> -    }
> +    if (qemuProcessRefreshCPU(driver, obj) < 0)
> +        goto error;
>
>      if (qemuDomainRefreshVcpuInfo(driver, obj, QEMU_ASYNC_JOB_NONE, true) < 0)
>          goto error;
> --
> 2.14.2
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] qemu: Separate CPU updating code from qemuProcessReconnect
Posted by Jiri Denemark 7 years, 7 months ago 
On Wed, Oct 11, 2017 at 19:42:36 +0200, Marc Hartmayer wrote:
> On Wed, Oct 11, 2017 at 12:11 PM +0200, Jiri Denemark <jdenemar@redhat.com> wrote:
> > The new function is called qemuProcessRefreshCPU.
> >
> > Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> > ---
> >  src/qemu/qemu_process.c | 66 ++++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 43 insertions(+), 23 deletions(-)
> >
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index 0cb023095b..5ed6b68eb8 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -6876,6 +6876,47 @@ qemuProcessRefreshDisks(virQEMUDriverPtr driver,
> >  }
> >
> >
> > +static int
> > +qemuProcessRefreshCPU(virQEMUDriverPtr driver,
> > +                      virDomainObjPtr vm)
> > +{
> > +    virCapsPtr caps = virQEMUDriverGetCapabilities(driver, false);
> > +    virCPUDefPtr host = NULL;
> > +    int ret = -1;
> > +
> > +    if (!virQEMUCapsGuestIsNative(caps->host.arch, vm->def->os.arch) ||
> > +        !caps->host.cpu ||
> > +        !vm->def->cpu)
> > +        return 0;
> > +
> > +    if (!caps)
> > +        goto cleanup;
> 
> That's somehow weird...  We access 'caps->host.arch'/ 'caps->host.cpu'
> and after that we're checking for a null pointer?!

Oops, yes. Consider the following patch squashed in:

diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c
index 5ed6b68eb8..8c33af28dd 100644
--- i/src/qemu/qemu_process.c
+++ w/src/qemu/qemu_process.c
@@ -6884,14 +6884,14 @@ qemuProcessRefreshCPU(virQEMUDriverPtr driver,
     virCPUDefPtr host = NULL;
     int ret = -1;
 
+    if (!caps)
+        return -1;
+
     if (!virQEMUCapsGuestIsNative(caps->host.arch, vm->def->os.arch) ||
         !caps->host.cpu ||
         !vm->def->cpu)
         return 0;
 
-    if (!caps)
-        goto cleanup;
-
     /* If the domain with a host-model CPU was started by an old libvirt
      * (< 2.3) which didn't replace the CPU with a custom one, let's do it now
      * since the rest of our code does not really expect a host-model CPU in a

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] qemu: Separate CPU updating code from qemuProcessReconnect
Posted by Pavel Hrdina 7 years, 7 months ago 
On Thu, Oct 12, 2017 at 09:18:36AM +0200, Jiri Denemark wrote:
> On Wed, Oct 11, 2017 at 19:42:36 +0200, Marc Hartmayer wrote:
> > On Wed, Oct 11, 2017 at 12:11 PM +0200, Jiri Denemark <jdenemar@redhat.com> wrote:
> > > The new function is called qemuProcessRefreshCPU.
> > >
> > > Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> > > ---
> > >  src/qemu/qemu_process.c | 66 ++++++++++++++++++++++++++++++++-----------------
> > >  1 file changed, 43 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > > index 0cb023095b..5ed6b68eb8 100644
> > > --- a/src/qemu/qemu_process.c
> > > +++ b/src/qemu/qemu_process.c
> > > @@ -6876,6 +6876,47 @@ qemuProcessRefreshDisks(virQEMUDriverPtr driver,
> > >  }
> > >
> > >
> > > +static int
> > > +qemuProcessRefreshCPU(virQEMUDriverPtr driver,
> > > +                      virDomainObjPtr vm)
> > > +{
> > > +    virCapsPtr caps = virQEMUDriverGetCapabilities(driver, false);
> > > +    virCPUDefPtr host = NULL;
> > > +    int ret = -1;
> > > +
> > > +    if (!virQEMUCapsGuestIsNative(caps->host.arch, vm->def->os.arch) ||
> > > +        !caps->host.cpu ||
> > > +        !vm->def->cpu)
> > > +        return 0;
> > > +
> > > +    if (!caps)
> > > +        goto cleanup;
> > 
> > That's somehow weird...  We access 'caps->host.arch'/ 'caps->host.cpu'
> > and after that we're checking for a null pointer?!
> 
> Oops, yes. Consider the following patch squashed in:
> 
> diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c
> index 5ed6b68eb8..8c33af28dd 100644
> --- i/src/qemu/qemu_process.c
> +++ w/src/qemu/qemu_process.c
> @@ -6884,14 +6884,14 @@ qemuProcessRefreshCPU(virQEMUDriverPtr driver,
>      virCPUDefPtr host = NULL;
>      int ret = -1;
>  
> +    if (!caps)
> +        return -1;
> +
>      if (!virQEMUCapsGuestIsNative(caps->host.arch, vm->def->os.arch) ||
>          !caps->host.cpu ||
>          !vm->def->cpu)
>          return 0;

This will leak the caps reference.

Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.