1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] qemu: store the emulator name in the capabilities XML

Daniel P. Berrangé posted 1 patch 4 years, 4 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20191218140301.764782-1-berrange@redhat.com
src/qemu/qemu_capabilities.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
[libvirt] [PATCH] qemu: store the emulator name in the capabilities XML
Posted by Daniel P. Berrangé 4 years, 4 months ago 
We don't need this for any functional purpose, but when debugging hosts
it is useful to know what binary a given capabilities XML document is
associated with.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/qemu/qemu_capabilities.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 2223589058..7d47fa4d02 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -3852,6 +3852,7 @@ virQEMUCapsParseSEVInfo(virQEMUCapsPtr qemuCaps, xmlXPathContextPtr ctxt)
  * Parsing a doc that looks like
  *
  * <qemuCaps>
+ *   <emulator>/some/path</emulator>
  *   <qemuctime>234235253</qemuctime>
  *   <selfctime>234235253</selfctime>
  *   <selfvers>1002016</selfvers>
@@ -3895,6 +3896,18 @@ virQEMUCapsLoadCache(virArch hostArch,
         goto cleanup;
     }
 
+    if (!(str = virXPathString("string(./emulator)", ctxt))) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("missing emulator in QEMU capabilities cache"));
+        goto cleanup;
+    }
+    if (!STREQ(str, qemuCaps->binary)) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Expected caps for '%s' but saw '%s'"),
+                       qemuCaps->binary, str);
+        goto cleanup;
+    }
+    VIR_FREE(str);
     if (virXPathLongLong("string(./qemuctime)", ctxt, &l) < 0) {
         virReportError(VIR_ERR_XML_ERROR, "%s",
                        _("missing qemuctime in QEMU capabilities XML"));
@@ -4232,6 +4245,8 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps)
     virBufferAddLit(&buf, "<qemuCaps>\n");
     virBufferAdjustIndent(&buf, 2);
 
+    virBufferEscapeString(&buf, "<emulator>%s</emulator>\n",
+                          qemuCaps->binary);
     virBufferAsprintf(&buf, "<qemuctime>%llu</qemuctime>\n",
                       (long long)qemuCaps->ctime);
     virBufferAsprintf(&buf, "<selfctime>%llu</selfctime>\n",
-- 
2.23.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: store the emulator name in the capabilities XML
Posted by Daniel Henrique Barboza 4 years, 4 months ago 

On 12/18/19 11:03 AM, Daniel P. Berrangé wrote:
> We don't need this for any functional purpose, but when debugging hosts
> it is useful to know what binary a given capabilities XML document is
> associated with.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   src/qemu/qemu_capabilities.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 2223589058..7d47fa4d02 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -3852,6 +3852,7 @@ virQEMUCapsParseSEVInfo(virQEMUCapsPtr qemuCaps, xmlXPathContextPtr ctxt)
>    * Parsing a doc that looks like
>    *
>    * <qemuCaps>
> + *   <emulator>/some/path</emulator>
>    *   <qemuctime>234235253</qemuctime>
>    *   <selfctime>234235253</selfctime>
>    *   <selfvers>1002016</selfvers>
> @@ -3895,6 +3896,18 @@ virQEMUCapsLoadCache(virArch hostArch,
>           goto cleanup;
>       }
>   
> +    if (!(str = virXPathString("string(./emulator)", ctxt))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("missing emulator in QEMU capabilities cache"));
> +        goto cleanup;
> +    }
> +    if (!STREQ(str, qemuCaps->binary)) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Expected caps for '%s' but saw '%s'"),
> +                       qemuCaps->binary, str);
> +        goto cleanup;
> +    }


Looks like "make syntax-check" is not happy about this if statement:

../src/qemu/qemu_capabilities.c:3904:    if (!STREQ(str, qemuCaps->binary)) {

build-aux/syntax-check.mk: Use STRNEQ instead of !STREQ and STREQ instead of !STRNEQ
make: *** [../build-aux/syntax-check.mk:1104: sc_prohibit_not_streq] Error 1
make: *** Waiting for unfinished jobs....
8.84 prohibit_backslash_alignment
8.96 prohibit_canonicalize_without_use



This patch also broke 6 tests in 'make check', at least here in my env:

FAIL: domaincapstest
FAIL: qemuxml2argvtest
FAIL: qemuxml2xmltest
FAIL: qemublocktest
FAIL: qemusecuritytest
FAIL: qemucapabilitiestest



Thanks,


DHB




> +    VIR_FREE(str);
>       if (virXPathLongLong("string(./qemuctime)", ctxt, &l) < 0) {
>           virReportError(VIR_ERR_XML_ERROR, "%s",
>                          _("missing qemuctime in QEMU capabilities XML"));
> @@ -4232,6 +4245,8 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps)
>       virBufferAddLit(&buf, "<qemuCaps>\n");
>       virBufferAdjustIndent(&buf, 2);
>   
> +    virBufferEscapeString(&buf, "<emulator>%s</emulator>\n",
> +                          qemuCaps->binary);
>       virBufferAsprintf(&buf, "<qemuctime>%llu</qemuctime>\n",
>                         (long long)qemuCaps->ctime);
>       virBufferAsprintf(&buf, "<selfctime>%llu</selfctime>\n",
> 


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: store the emulator name in the capabilities XML
Posted by Michal Prívozník 4 years, 4 months ago 
On 12/18/19 3:03 PM, Daniel P. Berrangé wrote:
> We don't need this for any functional purpose, but when debugging hosts
> it is useful to know what binary a given capabilities XML document is
> associated with.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/qemu/qemu_capabilities.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 2223589058..7d47fa4d02 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -3852,6 +3852,7 @@ virQEMUCapsParseSEVInfo(virQEMUCapsPtr qemuCaps, xmlXPathContextPtr ctxt)
>   * Parsing a doc that looks like
>   *
>   * <qemuCaps>
> + *   <emulator>/some/path</emulator>
>   *   <qemuctime>234235253</qemuctime>
>   *   <selfctime>234235253</selfctime>
>   *   <selfvers>1002016</selfvers>
> @@ -3895,6 +3896,18 @@ virQEMUCapsLoadCache(virArch hostArch,
>          goto cleanup;
>      }
>  
> +    if (!(str = virXPathString("string(./emulator)", ctxt))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("missing emulator in QEMU capabilities cache"));
> +        goto cleanup;

Since no caps stored on a disk have this, this change will trigger full
caps reprobe. I'm not saying it's a bad thing, just so that we are aware
of this.

> +    }
> +    if (!STREQ(str, qemuCaps->binary)) {

Use STRNEQ() instead, please, to make syntax-check happy.

> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Expected caps for '%s' but saw '%s'"),
> +                       qemuCaps->binary, str);
> +        goto cleanup;
> +    }
> +    VIR_FREE(str);
>      if (virXPathLongLong("string(./qemuctime)", ctxt, &l) < 0) {
>          virReportError(VIR_ERR_XML_ERROR, "%s",
>                         _("missing qemuctime in QEMU capabilities XML"));
> @@ -4232,6 +4245,8 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps)
>      virBufferAddLit(&buf, "<qemuCaps>\n");
>      virBufferAdjustIndent(&buf, 2);
>  
> +    virBufferEscapeString(&buf, "<emulator>%s</emulator>\n",
> +                          qemuCaps->binary);
>      virBufferAsprintf(&buf, "<qemuctime>%llu</qemuctime>\n",
>                        (long long)qemuCaps->ctime);
>      virBufferAsprintf(&buf, "<selfctime>%llu</selfctime>\n",
> 

What I'm missing here is change to our tests/qemucapabilitiesdata/*.xml
that would introduce the <emulator/> to each one of them.

Otherwise the patch looks good. This is something I wanted a long time ago.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: store the emulator name in the capabilities XML
Posted by Daniel P. Berrangé 4 years, 4 months ago 
On Wed, Dec 18, 2019 at 03:41:15PM +0100, Michal Prívozník wrote:
> On 12/18/19 3:03 PM, Daniel P. Berrangé wrote:
> > We don't need this for any functional purpose, but when debugging hosts
> > it is useful to know what binary a given capabilities XML document is
> > associated with.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  src/qemu/qemu_capabilities.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index 2223589058..7d47fa4d02 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -3852,6 +3852,7 @@ virQEMUCapsParseSEVInfo(virQEMUCapsPtr qemuCaps, xmlXPathContextPtr ctxt)
> >   * Parsing a doc that looks like
> >   *
> >   * <qemuCaps>
> > + *   <emulator>/some/path</emulator>
> >   *   <qemuctime>234235253</qemuctime>
> >   *   <selfctime>234235253</selfctime>
> >   *   <selfvers>1002016</selfvers>
> > @@ -3895,6 +3896,18 @@ virQEMUCapsLoadCache(virArch hostArch,
> >          goto cleanup;
> >      }
> >  
> > +    if (!(str = virXPathString("string(./emulator)", ctxt))) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                       _("missing emulator in QEMU capabilities cache"));
> > +        goto cleanup;
> 
> Since no caps stored on a disk have this, this change will trigger full
> caps reprobe. I'm not saying it's a bad thing, just so that we are aware
> of this.

We reprobe any time libvirtd itself changes its modification
time so all those existing caps are already invalidated.

> > +    virBufferEscapeString(&buf, "<emulator>%s</emulator>\n",
> > +                          qemuCaps->binary);
> >      virBufferAsprintf(&buf, "<qemuctime>%llu</qemuctime>\n",
> >                        (long long)qemuCaps->ctime);
> >      virBufferAsprintf(&buf, "<selfctime>%llu</selfctime>\n",
> > 
> 
> What I'm missing here is change to our tests/qemucapabilitiesdata/*.xml
> that would introduce the <emulator/> to each one of them.

Sigh, yes, I knew there was something I forgot todo yesterday when
writing this.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.