[libvirt] [PATCH 0/4] Misc travis improvements

[Daniel P . Berrangé]

Various improvements to travis coverage. The first patch was posted
separately yesterday too.

Daniel P. Berrangé (4):
  travis: add a scenario for running make distcheck
  travis: make all builds use VPATH
  travis: test upstart script handling on precise distro scenario
  travis: run 'make install' during build tests

 .travis.yml | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

-- 
2.14.3

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

[libvirt] [PATCH 4/4] travis: run 'make install' during build tests

[Daniel P . Berrangé]

Running 'make install' is important to catch some VPATH problems

Signed-off-by: Daniel P. Berrangé <berra...@redhat.com>
---
 .travis.yml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 0328fcb8f1..61f0e38d40 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -105,12 +105,12 @@ before_install:
   - if [ "$TRAVIS_OS_NAME" == "osx" ]; then brew update && brew upgrade && 
brew install rpcgen yajl; fi
 
 before_script:
-  - mkdir build && cd build && ../autogen.sh $CONFIGURE_ARGS
+  - mkdir build && cd build && ../autogen.sh $CONFIGURE_ARGS 
--prefix=$(pwd)/../vroot
 
 script:
   # Many unit tests still fail on macOS, and there are a bunch of issues with
   # syntax-check as well, so skip those steps on that platform for now
-  - make -j3 && if [ "$TRAVIS_OS_NAME" != "osx" ]; then make -j3 syntax-check 
&& make -j3 check; fi
+  - make -j3 && if [ "$TRAVIS_OS_NAME" != "osx" ]; then make -j3 syntax-check 
&& make -j3 check; fi && make -j3 install
 
 after_failure:
   - echo 
''
-- 
2.14.3

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

[libvirt] [PATCH 1/4] travis: add a scenario for running make distcheck

[Daniel P . Berrangé]

Running "make distcheck" ensures that we have CLEANFILES and uninstall
rules setup correctly, as well as validating VPATH builds succeeed.

Signed-off-by: Daniel P. Berrangé <berra...@redhat.com>
---
 .travis.yml | 5 +
 1 file changed, 5 insertions(+)

diff --git a/.travis.yml b/.travis.yml
index 3f26a1..4bdf034829 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -6,8 +6,13 @@ matrix:
   include:
 - compiler: gcc
   dist: precise
+# Special scenario to run distcheck, so we don't waste time duplicating
+# work in all the other scenarios. Doesn't work on precise due to the
+# CVE-2012-3386 flaw being present on that Ubuntu version
 - compiler: gcc
   dist: trusty
+  script:
+  - make -j3 distcheck
 - compiler: clang
   dist: precise
 - compiler: clang
-- 
2.14.3

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

[libvirt] [PATCH 3/4] travis: test upstart script handling on precise distro scenario

[Daniel P . Berrangé]

Signed-off-by: Daniel P. Berrangé <berra...@redhat.com>
---
 .travis.yml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/.travis.yml b/.travis.yml
index 41a293451c..0328fcb8f1 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -6,6 +6,8 @@ matrix:
   include:
 - compiler: gcc
   dist: precise
+  env:
+- CONFIGURE_ARGS=--with-init-script=upstart
 # Special scenario to run distcheck, so we don't waste time duplicating
 # work in all the other scenarios. Doesn't work on precise due to the
 # CVE-2012-3386 flaw being present on that Ubuntu version
@@ -103,7 +105,7 @@ before_install:
   - if [ "$TRAVIS_OS_NAME" == "osx" ]; then brew update && brew upgrade && 
brew install rpcgen yajl; fi
 
 before_script:
-  - mkdir build && cd build && ../autogen.sh
+  - mkdir build && cd build && ../autogen.sh $CONFIGURE_ARGS
 
 script:
   # Many unit tests still fail on macOS, and there are a bunch of issues with
-- 
2.14.3

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

[libvirt] [PATCH] make: fix another VPATH bug impacting install of sysconf files

[Daniel P . Berrangé]

Signed-off-by: Daniel P. Berrangé <berra...@redhat.com>
---

Pushed as build fix

 src/Makefile.am | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/Makefile.am b/src/Makefile.am
index 2166e17dbe..8ceeda5756 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -2833,7 +2833,7 @@ install-sysconfig:
for f in $(SYSCONF_FILES:%.sysconf=%) ; \
do \
  tgt=`basename $$f`; \
- $(INSTALL_SCRIPT) $$f.sysconf 
$(DESTDIR)$(sysconfdir)/sysconfig/$$tgt; \
+ $(INSTALL_SCRIPT) $(srcdir)/$$f.sysconf 
$(DESTDIR)$(sysconfdir)/sysconfig/$$tgt; \
done
 
 uninstall-sysconfig:
-- 
2.14.3

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

[libvirt] [PATCH jenkins-ci] Fix workspace for python check/rpm jobs

[Daniel P . Berrangé]

The check/rpm jobs were using a different workspace to the build job, so
never saw the correct content.

Signed-off-by: Daniel P. Berrangé <berra...@redhat.com>
---

Pushed as a build fix for virt-manager CI

 jobs/python-distutils.yaml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/jobs/python-distutils.yaml b/jobs/python-distutils.yaml
index 510769e..16eca1c 100644
--- a/jobs/python-distutils.yaml
+++ b/jobs/python-distutils.yaml
@@ -58,7 +58,7 @@
 name: '{name}-{branch}-py{pyver}-check'
 project-type: matrix
 description: '{title} Check (Python {pyver})'
-workspace: '{name}-{branch}'
+workspace: '{name}-{branch}-py{pyver}'
 child-workspace: '.'
 block-downstream: true
 block-upstream: true
@@ -97,7 +97,7 @@
 name: '{name}-{branch}-py{pyver}-rpm'
 project-type: matrix
 description: '{title} RPM (Python {pyver})'
-workspace: '{name}-{branch}'
+workspace: '{name}-{branch}-py{pyver}'
 child-workspace: '.'
 block-downstream: true
 block-upstream: true
-- 
2.14.3

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

Re: [libvirt] [jenkins-ci PATCH 2/2] jobs: Switch to github webhook instead of polling

[Daniel P . Berrangé]

On Thu, Jun 21, 2018 at 03:37:49PM +0200, Andrea Bolognani wrote:
> On Thu, 2018-06-21 at 14:21 +0200, Pavel Hrdina wrote:
> [...]
> > @@ -31,8 +31,7 @@
> >  triggers:
> >- reverse:
> >jobs: '{obj:parent_jobs}'
> > -  - pollscm:
> > -  cron: "H/20 * * * *"
> > +  - github
> 
> As noted in the review for the previous patch
> 
>   libosinfo: https://gitlab.com/libosinfo/libosinfo.git
>   osinfo-db-tools: https://gitlab.com/libosinfo/osinfo-db-tools.git
>   osinfo-db: https://gitlab.com/libosinfo/osinfo-db.git
>   virt-viewer: https://pagure.io/virt-viewer.git
> 
> are not hosted on GitHub, so the above won't work for them.
> 
> There's probably a way to make something like this work for GitLab
> too, but I'm not very confident about Pagure.
> 
> Perhaps we can convince the projects listed above to set up
> mirroring to GitHub...

I think its better if we just leave the jenkins config using pollscm
as it does today, but simply point it at an appropriate repo for
each project, instead of using github webhooks or the local cached
git.

This kind of need to mirror/move everything to github just to use
their unique features, is exactly the kind of embrace & extend that
makes github undesirable. Better to stick to core git functionality.

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

[libvirt] [PATCH] qemu: ensure FDs passed to QEMU for chardevs have correct SELinux labels

[Daniel P . Berrangé]

The UNIX socket FDs were we passing to QEMU inherited a label based on
libvirtd's context. QEMU is thus denied ability to access the UNIX
socket. We need to use the security manager to change our current
context temporarily when creating the UNIX socket FD.

Signed-off-by: Daniel P. Berrangé 
---
 src/qemu/qemu_command.c | 86 -
 src/qemu/qemu_command.h |  1 +
 src/qemu/qemu_process.c |  2 +
 3 files changed, 63 insertions(+), 26 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 1ffcb5b1ae..146f671663 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4931,6 +4931,7 @@ qemuOpenChrChardevUNIXSocket(const virDomainChrSourceDef 
*dev)
  * host side of the character device */
 static char *
 qemuBuildChrChardevStr(virLogManagerPtr logManager,
+   virSecurityManagerPtr secManager,
virCommandPtr cmd,
virQEMUDriverConfigPtr cfg,
const virDomainDef *def,
@@ -5065,7 +5066,13 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
 
 case VIR_DOMAIN_CHR_TYPE_UNIX:
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS)) {
+if (qemuSecuritySetSocketLabel(secManager, (virDomainDefPtr)def) < 
0)
+goto cleanup;
 int fd = qemuOpenChrChardevUNIXSocket(dev);
+if (qemuSecurityClearSocketLabel(secManager, (virDomainDefPtr)def) 
< 0) {
+VIR_FORCE_CLOSE(fd);
+goto cleanup;
+}
 if (fd < 0)
 goto cleanup;
 
@@ -5404,6 +5411,7 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
 
 static int
 qemuBuildMonitorCommandLine(virLogManagerPtr logManager,
+virSecurityManagerPtr secManager,
 virCommandPtr cmd,
 virQEMUDriverConfigPtr cfg,
 virDomainDefPtr def,
@@ -5414,7 +5422,8 @@ qemuBuildMonitorCommandLine(virLogManagerPtr logManager,
 if (!priv->monConfig)
 return 0;
 
-if (!(chrdev = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
+if (!(chrdev = qemuBuildChrChardevStr(logManager, secManager,
+  cmd, cfg, def,
   priv->monConfig, "monitor",
   priv->qemuCaps, true,
   priv->chardevStdioLogd)))
@@ -5533,6 +5542,7 @@ qemuBuildSclpDevStr(virDomainChrDefPtr dev)
 
 static int
 qemuBuildRNGBackendChrdevStr(virLogManagerPtr logManager,
+ virSecurityManagerPtr secManager,
  virCommandPtr cmd,
  virQEMUDriverConfigPtr cfg,
  const virDomainDef *def,
@@ -5550,7 +5560,8 @@ qemuBuildRNGBackendChrdevStr(virLogManagerPtr logManager,
 return 0;
 
 case VIR_DOMAIN_RNG_BACKEND_EGD:
-if (!(*chr = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
+if (!(*chr = qemuBuildChrChardevStr(logManager, secManager,
+cmd, cfg, def,
 rng->source.chardev,
 rng->info.alias, qemuCaps, true,
 chardevStdioLogd)))
@@ -5680,6 +5691,7 @@ qemuBuildRNGDevStr(const virDomainDef *def,
 
 static int
 qemuBuildRNGCommandLine(virLogManagerPtr logManager,
+virSecurityManagerPtr secManager,
 virCommandPtr cmd,
 virQEMUDriverConfigPtr cfg,
 const virDomainDef *def,
@@ -5702,7 +5714,7 @@ qemuBuildRNGCommandLine(virLogManagerPtr logManager,
 }
 
 /* possibly add character device for backend */
-if (qemuBuildRNGBackendChrdevStr(logManager, cmd, cfg, def,
+if (qemuBuildRNGBackendChrdevStr(logManager, secManager, cmd, cfg, def,
  rng, qemuCaps, ,
  chardevStdioLogd) < 0)
 return -1;
@@ -8135,6 +8147,7 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg,
 static int
 qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver,
   virLogManagerPtr logManager,
+  virSecurityManagerPtr secManager,
   virCommandPtr cmd,
   virDomainDefPtr def,
   virDomainNetDefPtr net,
@@ -8157,7 +8170,8 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver,
 
 switch ((virDomainChrType)net->data.vhostuser->type) {
 case VIR_DOMAIN_CHR_TYPE_UNIX:
-if (!(chardev = qemuBuildChrChardevStr(logManager, cmd, cfg, def,
+if (!(chardev =

Re: [libvirt] [PATCH v3 16/20] nwfilter: keep track of active filter bindings

[Daniel P . Berrangé]

On Mon, Jun 18, 2018 at 04:59:12PM -0400, John Ferlan wrote:
> 
> 
> On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
> > Currently the nwfilter driver does not keep any record of what filter
> > bindings it has active. This means that when it needs to recreate
> > filters, it has to rely on triggering callbacks provided by the virt
> > drivers. This introduces a hash table recording the virNWFilterBinding
> > objects so the driver has a record of all active filters.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  src/conf/virnwfilterobj.h  |  4 ++
> >  src/nwfilter/nwfilter_driver.c | 86 --
> >  2 files changed, 65 insertions(+), 25 deletions(-)
> > 
> 
> [...]
> 
> > diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
> > index 7202691646..2388e925fc 100644
> > --- a/src/nwfilter/nwfilter_driver.c
> > +++ b/src/nwfilter/nwfilter_driver.c
> > @@ -38,7 +38,6 @@
> >  #include "domain_conf.h"
> >  #include "domain_nwfilter.h"
> >  #include "nwfilter_driver.h"
> > -#include "virnwfilterbindingdef.h"
> >  #include "nwfilter_gentech_driver.h"
> >  #include "configmake.h"
> >  #include "virfile.h"
> > @@ -174,7 +173,6 @@ nwfilterStateInitialize(bool privileged,
> >  virStateInhibitCallback callback ATTRIBUTE_UNUSED,
> >  void *opaque ATTRIBUTE_UNUSED)
> >  {
> 
> [...]
> 
> >  
> >  if (virNWFilterObjListLoadAllConfigs(driver->nwfilters, 
> > driver->configDir) < 0)
> >  goto error;
> >  
> > +if (virNWFilterBindingObjListLoadAllConfigs(driver->bindings, 
> > driver->bindingDir) < 0)
> > +goto error;
> > +
> 
> Because of this... [1]
> 
> >  nwfilterDriverUnlock();
> >  
> >  return 0;
> >  
> 
> [...]
> 
> > @@ -647,13 +656,37 @@ nwfilterInstantiateFilter(const char *vmname,
> >const unsigned char *vmuuid,
> >virDomainNetDefPtr net)
> >  {
> > -virNWFilterBindingDefPtr binding;
> > +virNWFilterBindingObjPtr obj;
> > +virNWFilterBindingDefPtr def;
> >  int ret;
> >  
> > -if (!(binding = virNWFilterBindingDefForNet(vmname, vmuuid, net)))
> > +obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, 
> > net->ifname);
> > +if (obj) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > +   _("Filter already present for NIC %s"), 
> > net->ifname);
> 
> [1]
> If I stop at this patch, start a domain with a filter, then restart
> libvirtd, then that will cause a guest running with a filter to exit and
> not just disappear - as it can be restarted. The error is:
> 
> 2018-06-18 16:49:07.405+: 3978: error :
> nwfilterInstantiateFilter:666 : internal error: Filter already present
> for NIC vnet1
> 
> Because once I have this patch built/running the
> /var/run/libvirt/nwfilter-binding/vnet1.xml exists and get's loaded when
> virNWFilterBindingObjListLoadAllConfigs is called at StateInitialize.
> 
> I only saw this because I found later in patch 20 the failure comes from
> nwfilterBindingCreateXML instead when virDomainConfNWFilterInstantiate
> is called. I worked my way back to this point.
> 
> Not sure which would be the "best" solution at this point. Should we
> wait to do [1] until patch 20 or perhaps just not have an error here.

The current semantics are that nwfilterInstantiateFilter will not
report an error if the filter already exists, so I think I'll just
not report an error here. This method will go away anyway, so no
great loss.

> 
> NB: If the guest was running at a point up through patch 15 then it
> won't exit on the first libvirtd restart (since the obj dir doesn't
> exist), but the issue shows up on the 2nd restart.
> 
> In general the code is fine to me, but just need to handle this one
> issue in some way.

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

Re: [libvirt] [PATCH v3 14/20] conf: introduce a virNWFilterBindingObjPtr struct

[Daniel P . Berrangé]

On Mon, Jun 18, 2018 at 10:08:00AM -0400, John Ferlan wrote:
> 
> 
> On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
> > Introduce a new struct to act as the stateful owner of the
> > virNWFilterBindingDefPtr objects.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  src/conf/Makefile.inc.am |   2 +
> >  src/conf/virnwfilterbindingobj.c | 299 +++
> >  src/conf/virnwfilterbindingobj.h |  69 +++
> >  src/libvirt_private.syms |  14 ++
> >  4 files changed, 384 insertions(+)
> >  create mode 100644 src/conf/virnwfilterbindingobj.c
> >  create mode 100644 src/conf/virnwfilterbindingobj.h
> > 
> 
> While continuing, I tripped across this:
> 
> > +
> > +static virNWFilterBindingObjPtr
> > +virNWFilterBindingObjParseNode(xmlDocPtr doc,
> > +   xmlNodePtr root)
> > +{
> > +xmlXPathContextPtr ctxt = NULL;
> > +virNWFilterBindingObjPtr obj = NULL;
> > +
> > +if (STRNEQ((const char *)root->name, "filterbinding")) {
> 
> "filterbindingstatus"

Oh fun, will fix that.


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

Re: [libvirt] [PATCH v3 17/20] nwfilter: remove virt driver callback layer for rebuilding filters

[Daniel P . Berrangé]

On Mon, Jun 18, 2018 at 04:59:37PM -0400, John Ferlan wrote:
> 
> 
> On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
> > Now that the nwfilter driver keeps a list of bindings that it has
> > created, there is no need for the complex virt driver callbacks. It is
> > possible to simply iterate of the list of recorded filter bindings.
> > 
> > This means that rebuilding filters no longer has to acquire any locks on
> > the virDomainObj objects, as they're never touched.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  src/conf/nwfilter_conf.c   | 134 +++-
> >  src/conf/nwfilter_conf.h   |  51 +---
> >  src/conf/virnwfilterobj.c  |   4 +-
> >  src/libvirt_private.syms   |   7 +-
> >  src/lxc/lxc_driver.c   |  28 -
> >  src/nwfilter/nwfilter_driver.c |  21 ++--
> >  src/nwfilter/nwfilter_gentech_driver.c | 167 -
> >  src/nwfilter/nwfilter_gentech_driver.h |   4 +-
> >  src/qemu/qemu_driver.c |  25 
> >  src/uml/uml_driver.c   |  29 -
> >  10 files changed, 141 insertions(+), 329 deletions(-)
> > 
> 
> A diffstat that Jano usually applauds!  Miracles do happen when you
> close your eyes and say 3 times "I wish this code would just go away"
> ;-).  Still this is some of the most "entertaining code" - that now used
> to exist!  It seems I can dig up my nwfilter obj/hash code once this is
> in...
> 
> There's a couple nits below...
> 
> Reviewed-by: John Ferlan 
> 
> John
> 
> 
> > diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
> > index de26a6d034..5bb8a0c2e7 100644
> > --- a/src/conf/nwfilter_conf.c
> > +++ b/src/conf/nwfilter_conf.c
> 
> [...]
> 
> > +
> > +
> > +int
> > +virNWFilterTriggerRebuild(void)
> > +{
> > +return rebuildCallback(rebuildOpaque);
> 
> In the better safe than sorry - should we gate on "if
> (rebuildCallback)"?  I don't think there's a way into here with it being
> NULL, but those are always "famous last words".

Yeah ok.

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

Re: [libvirt] [PATCH v3 09/20] virsh: add nwfilter binding commands

[Daniel P . Berrangé]

On Sun, Jun 17, 2018 at 08:27:27AM -0400, John Ferlan wrote:
> 
> 
> On 06/14/2018 08:32 AM, Daniel P. Berrangé wrote:
> > $ virsh nwfilter-binding-list
> >  Port Dev  Filter
> > --
> >  vnet0 clean-traffic
> >  vnet1 clean-traffic
> > 
> > $ virsh nwfilter-binding-dumpxml vnet1
> > 
> >   
> > f25arm7
> > 12ac8b8c-4f23-4248-ae42-fdcd50c400fd
> >   
> >   
> >   
> >   
> > 
> >   
> > 
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  tools/virsh-completer.c |  45 ++
> >  tools/virsh-completer.h |   4 +
> >  tools/virsh-nwfilter.c  | 317 
> >  tools/virsh-nwfilter.h  |   8 +
> >  4 files changed, 374 insertions(+)
> > 
> 
> Will need virsh.pod changes to describe the new commands. I'm OK if you
> just post an update as a reply.

I'll send a further patch with POD docs

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

Re: [libvirt] [PATCH v3 11/20] nwfilter: convert IP address learning code to virNWFilterBindingDefPtr

[Daniel P . Berrangé]

On Sun, Jun 17, 2018 at 08:28:11AM -0400, John Ferlan wrote:
> 
> 
> On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
> > Use the virNWFilterBindingDefPTr struct in the IP address learning code
> > directly.
> > 
> > Reviewed-by: John Ferlan 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  src/nwfilter/nwfilter_gentech_driver.c |   7 +-
> >  src/nwfilter/nwfilter_learnipaddr.c| 106 +++--
> >  src/nwfilter/nwfilter_learnipaddr.h|   7 +-
> >  3 files changed, 32 insertions(+), 88 deletions(-)
> 
> 
> R-By still applies, but please let's...
> 
> > --- a/src/nwfilter/nwfilter_learnipaddr.c
> > +++ b/src/nwfilter/nwfilter_learnipaddr.c
> 
> [...]
> 
> > @@ -737,19 +724,14 @@ learnIPAddressThread(void *arg)
> >   */
> 
> Adjust the comments above here to replace the ifname, linkdev, macaddr,
> filtername, and filterparams with @binding

Ok will do.

> 
> >  int
> >  virNWFilterLearnIPAddress(virNWFilterTechDriverPtr techdriver,
> > -  const char *ifname,
> > +  virNWFilterBindingDefPtr binding,
> >int ifindex,
> > -  const char *linkdev,
> > -  const virMacAddr *macaddr,
> > -  const char *filtername,
> > -  virHashTablePtr filterparams,
> >virNWFilterDriverStatePtr driver,
> >int howDetect)
> 
> [...]

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

Re: [libvirt] [PATCH v3 19/20] nwfilter: wire up new APIs for creating and deleting nwfilter bindings

[Daniel P . Berrangé]

On Mon, Jun 18, 2018 at 04:59:50PM -0400, John Ferlan wrote:
> 
> 
> On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
> > This allows the virsh commands nwfilter-binding-create and
> > nwfilter-binding-delete to be used.
> > 
> > Note using these commands lets you delete filters that were
> > previously created automatically by the virt drivers, or add
> > filters for VM nics that were not there before. Generally it
> > is expected these new APIs will only be used by virt drivers.
> > It is the admin's responsibility to not shoot themselves in
> > the foot.
> 
> Can't wait to see QE get ahold of this ;-)
> 
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  src/nwfilter/nwfilter_driver.c | 79 ++
> >  1 file changed, 79 insertions(+)
> > 
> 
> I think with a couple of extra comments as described below, then this
> looks fine.  Not sure how the other point regarding calling CreateXML
> from the ConfNWFilterInstantiate path (and the reload of load all
> configs) will be handled, but I'm sure it'll be something handled in
> patch 16 and 20, so the comment below is just the "tracing" I did while
> reviewing...
> 
> Reviewed-by: John Ferlan 
> 
> John
> 
> > diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
> > index 6bfb584b09..2b6856a36c 100644
> > --- a/src/nwfilter/nwfilter_driver.c
> > +++ b/src/nwfilter/nwfilter_driver.c
> > @@ -788,6 +788,83 @@ nwfilterBindingGetXMLDesc(virNWFilterBindingPtr 
> > binding,
> >  }
> >  
> >  
> 
> Because "not everyone" reads the commit message that added this, I think
> adding a few comments here and for BindingDelete to essentially indicate
> the same as the commit message would be good.
> 
> > +static virNWFilterBindingPtr
> > +nwfilterBindingCreateXML(virConnectPtr conn,
> > + const char *xml,
> > + unsigned int flags)
> > +{
> > +virNWFilterBindingObjPtr obj;
> > +virNWFilterBindingDefPtr def;
> > +virNWFilterBindingPtr ret = NULL;
> > +
> > +virCheckFlags(0, NULL);
> > +
> > +def = virNWFilterBindingDefParseString(xml);
> > +if (!def)
> > +return NULL;
> > +
> > +if (virNWFilterBindingCreateXMLEnsureACL(conn, def) < 0)
> > +goto cleanup;
> > +
> > +obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, 
> > def->portdevname);
> > +if (obj) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > +   _("Filter already present for NIC %s"), 
> > def->portdevname);
> 
> Recall my point from patch 16 regarding the existence of the filter.
> From certain paths it's OK if it exists but once this becomes functional
> for the subsequent patch via virDomainConfNWFilterInstantiate, then the
> issue from patch 16 moves to patch 20 (e.g. guest not restarting).

In this case, I think I'll change the calling code so that it first checks
whether the filter exists or not, instead of unconditionally trying to
recreate it.


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

[libvirt] [PATCH] rpm: simplify applying of patches

[Daniel P . Berrangé]

The distros we support for RPM builds all have %autosetup support so we
can ditch the convoluted code for running git manually and use the RPM
defaults.

Signed-off-by: Daniel P. Berrangé 
---
 libvirt.spec.in | 38 +-
 1 file changed, 1 insertion(+), 37 deletions(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 19ae55cdaf..a2f3112a0b 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -993,43 +993,7 @@ Libvirt plugin for NSS for translating domain names into 
IP addresses.
 
 %prep
 
-%setup -q
-
-# Patches have to be stored in a temporary file because RPM has
-# a limit on the length of the result of any macro expansion;
-# if the string is longer, it's silently cropped
-%{lua:
-tmp = os.tmpname();
-f = io.open(tmp, "w+");
-count = 0;
-for i, p in ipairs(patches) do
-f:write(p.."\n");
-count = count + 1;
-end;
-f:close();
-print("PATCHCOUNT="..count.."\n")
-print("PATCHLIST="..tmp.."\n")
-}
-
-git init -q
-git config user.name rpm-build
-git config user.email rpm-build
-git config gc.auto 0
-git add .
-git commit -q -a --author 'rpm-build ' \
-   -m '%{name}-%{version} base'
-
-COUNT=$(grep '\.patch$' $PATCHLIST | wc -l)
-if [ $COUNT -ne $PATCHCOUNT ]; then
-echo "Found $COUNT patches in $PATCHLIST, expected $PATCHCOUNT"
-exit 1
-fi
-if [ $COUNT -gt 0 ]; then
-xargs git am <$PATCHLIST || exit 1
-fi
-echo "Applied $COUNT patches"
-rm -f $PATCHLIST
-rm -rf .git
+%autosetup -S git_am
 
 %build
 %if ! %{supported_platform}
-- 
2.17.1

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

[libvirt] [jenkins-ci PATCH] projects: restrict virt-viewer RPMs to Fedora >= 28

[Daniel P . Berrangé]

Signed-off-by: Daniel P. Berrangé 
---
 projects/virt-viewer.yaml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/projects/virt-viewer.yaml b/projects/virt-viewer.yaml
index 12335f3..25474f2 100644
--- a/projects/virt-viewer.yaml
+++ b/projects/virt-viewer.yaml
@@ -13,7 +13,9 @@
   parent_jobs: 'virt-viewer-master-syntax-check'
   - autotools-rpm-job:
   parent_jobs: 'virt-viewer-master-check'
-  machines: '{rpm_machines}'
+  machines:
+   - libvirt-fedora-28
+   - libvirt-fedora-rawhide
   - autotools-build-job:
   parent_jobs: 'libvirt-glib-master-build-mingw32'
   variant: -mingw32
-- 
2.17.1

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

Re: [libvirt] [PATCH for v4.6.0] cgroups: Don't leave stale pointers around after virCgroupFree

[Daniel P . Berrangé]

On Mon, Jul 30, 2018 at 12:47:37PM +0200, Erik Skultety wrote:
> On Mon, Jul 30, 2018 at 11:35:02AM +0100, Daniel P. Berrangé wrote:
> > On Mon, Jul 30, 2018 at 12:30:09PM +0200, Pavel Hrdina wrote:
> > > On Mon, Jul 30, 2018 at 09:58:41AM +0100, Daniel P. Berrangé wrote:
> > > > On Mon, Jul 30, 2018 at 09:48:51AM +0100, Daniel P. Berrangé wrote:
> > > > > On Mon, Jul 30, 2018 at 10:25:05AM +0200, Michal Privoznik wrote:
> > > > > > One of the attributes that original virCgroupFree() had was it
> > > > > > set passed pointer to NULL. For instance in the following code
> > > > > > the latter call would be practically a no-op:
> > > > > >
> > > > > >   virCgroupFree();
> > > > > >   virCgroupFree();
> > > > > >
> > > > > > However, this behaviour of the function was changed in
> > > > > > 0f80c71822d824 but corresponding 'var = NULL' lines were not
> > > > > > added leading to double free:
> > > > >
> > > > > Sigh, can we please just revert that change. It is going in 
> > > > > completely the
> > > > > oppposite of what we should be doing. We want to change more 
> > > > > functions to
> > > > > take a ptr to a ptr, precisely because it avoids this double-free 
> > > > > problem.
> > >
> > > I agree that this change was not correct and we should revert it, the
> > > ultimate goal should be to have all the *Free() functions to take
> > > double pointer and set the variable to NULL as well.
> > >
> > > > Even more crazy, this change was done so that VIR_DEFINE_AUTOPTR_FUNC()
> > > > could be used to define a free function which takes a ptr to a ptr.
> > > >
> > > > Both of these changes should be reverted, as the previously existing
> > > > virCgroupFree should be used as the attribute((cleanup)) function 
> > > > directly
> > > > with no wrapper created.
> > >
> > > We already had this discussion, there are two possibilities:
> > >
> > > 1) as the end goal simple wrapper function defined by MACRO:
> > >
> > > # define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> > > static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
> > > { \
> > > (func)(_ptr); \
> > > } \
> > >
> > > It's a static inline so it will disappear during compilation.  As it
> > > may look like unnecessary code there is one benefit while using
> > > VIR_AUTOPTR()
> > >
> > > 2) the second option is to have a macro that would replace
> > > __attribute__(cleanup()), for example:
> > >
> > > VIR_CLEANUP(func) __attribute__(cleanup(func))
> > >
> > >
> > > If you compare the usage:
> > >
> > > VIR_AUTOPTR(virCgroup) group = NULL;
> > >
> > > VIR_CLEANUP(virCgroupFree) virCgroupPtr group = NULL;
> > >
> > > I personally prefer the first option, it's cleaner and the line is
> > > shorter and it's perfectly readable even though some logic is hidden.
> > > The second usage has a benefit that the logic is not hidden and you can
> > > use any function and you don't have to define anything but the line can
> > > get really long and ugly as we have some really long names for Free
> > > functions and types.
> > >
> > > For example:
> > >
> > > VIR_AUTOPTR(virDomainGraphicsDef) def = NULL;
> > >
> > > VIR_CLEANUP(virDomainGraphicsDefFree) virDomainGraphicsDefPtr def = 
> > > NULL;
> > >
> > > And that's not even the longest free function.
> > >
> > > Since the wrapper defined by VIR_DEFINE_AUTOPTR_FUNC() will be expanded
> > > during compilation I prefer that option.  It would essentially work the
> > > same way is VIR_CLEANUP but with the benefit that the declaration line
> > > is short, nice and clean.
> >
> > I'm fine with either option, as long as we keep the ptr to a ptr aspect
> > of the original function. Probably a slight pref for the first option.
> 
> Like Pavel said, we already had this discussion which also wasn't moving much.
> At that time I said that having the free functions take a double pointer is 
> the
> ultimate goal which I completely agree with, however, we decided to go this 
> way
> instead in the meantime. Although we broke it this way, I still prefer 
> Michal's
> original fix rather than reverting Sukrit's change, because (even though
> broken), right now we're consistent in usage and then when we switch to double
> pointer Free() functions, we can use one of Pavel's suggestions, preferably 
> the
> first one.

The switch from ptr, to ptr-to-a-ptr is not something we can realistically
do in a "big bang". It would have to be an incremental change across the
code base, so we're going to have inconsistency no matter what.

In any case code robustness is more important than code style, so reverting
this is still the right thing todo. 

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

Re: [libvirt] [jenkins-ci PATCH] hosts_vars: drop virt-viewer from platforms with old spice-gtk

[Daniel P . Berrangé]

On Mon, Jul 30, 2018 at 12:17:56PM +0200, Andrea Bolognani wrote:
> On Mon, 2018-07-30 at 10:04 +0100, Daniel P. Berrangé wrote:
> > git master requires spice-gtk >= 0.35
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  guests/host_vars/libvirt-centos-7/main.yml| 1 -
> >  guests/host_vars/libvirt-debian-8/main.yml| 1 -
> >  guests/host_vars/libvirt-debian-9/main.yml| 1 -
> >  guests/host_vars/libvirt-debian-sid/main.yml  | 1 -
> >  guests/host_vars/libvirt-fedora-27/main.yml   | 1 -
> >  guests/host_vars/libvirt-freebsd-10/main.yml  | 1 -
> >  guests/host_vars/libvirt-freebsd-11/main.yml  | 1 -
> >  guests/host_vars/libvirt-freebsd-current/main.yml | 1 -
> >  guests/host_vars/libvirt-ubuntu-14/main.yml   | 1 -
> >  guests/host_vars/libvirt-ubuntu-16/main.yml   | 1 -
> >  guests/host_vars/libvirt-ubuntu-18/main.yml   | 1 -
> >  11 files changed, 11 deletions(-)
> 
> All FreeBSD versions have 0.35, so they shouldn't be dropped;
> Debian Sid is still on 0.34, but I expect it will get 0.35 at
> some point in the not-too-distant future so I wouldn't touch
> that either, since it doesn't affect CentOS CI either way.
> 
> More importantly: do we really want to stop building virt-viewer
> entirely on those platform just because a recent enough spice-gtk
> version is not available? Building without spice support still
> provides some amount of coverage (eg. VNC support), and the builds
> themselves don't fail, so I don't really see the upside of merging
> this.
> 
> My suggestion would be to just disable the virt-viewer-master-rpm
> job on Fedora 27 and CentOS 7 and call it a day.

Sigh, yes, I'm not thinking properly yet this morning 

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

Re: [libvirt] [PATCH for v4.6.0] cgroups: Don't leave stale pointers around after virCgroupFree

[Daniel P . Berrangé]

On Mon, Jul 30, 2018 at 12:30:09PM +0200, Pavel Hrdina wrote:
> On Mon, Jul 30, 2018 at 09:58:41AM +0100, Daniel P. Berrangé wrote:
> > On Mon, Jul 30, 2018 at 09:48:51AM +0100, Daniel P. Berrangé wrote:
> > > On Mon, Jul 30, 2018 at 10:25:05AM +0200, Michal Privoznik wrote:
> > > > One of the attributes that original virCgroupFree() had was it
> > > > set passed pointer to NULL. For instance in the following code
> > > > the latter call would be practically a no-op:
> > > > 
> > > >   virCgroupFree();
> > > >   virCgroupFree();
> > > > 
> > > > However, this behaviour of the function was changed in
> > > > 0f80c71822d824 but corresponding 'var = NULL' lines were not
> > > > added leading to double free:
> > > 
> > > Sigh, can we please just revert that change. It is going in completely the
> > > oppposite of what we should be doing. We want to change more functions to
> > > take a ptr to a ptr, precisely because it avoids this double-free problem.
> 
> I agree that this change was not correct and we should revert it, the
> ultimate goal should be to have all the *Free() functions to take
> double pointer and set the variable to NULL as well.
> 
> > Even more crazy, this change was done so that VIR_DEFINE_AUTOPTR_FUNC()
> > could be used to define a free function which takes a ptr to a ptr.
> > 
> > Both of these changes should be reverted, as the previously existing
> > virCgroupFree should be used as the attribute((cleanup)) function directly
> > with no wrapper created.
> 
> We already had this discussion, there are two possibilities:
> 
> 1) as the end goal simple wrapper function defined by MACRO:
> 
> # define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
> { \
> (func)(_ptr); \
> } \
> 
> It's a static inline so it will disappear during compilation.  As it
> may look like unnecessary code there is one benefit while using
> VIR_AUTOPTR()
> 
> 2) the second option is to have a macro that would replace
> __attribute__(cleanup()), for example:
> 
> VIR_CLEANUP(func) __attribute__(cleanup(func))
> 
> 
> If you compare the usage:
> 
> VIR_AUTOPTR(virCgroup) group = NULL;
> 
> VIR_CLEANUP(virCgroupFree) virCgroupPtr group = NULL;
> 
> I personally prefer the first option, it's cleaner and the line is
> shorter and it's perfectly readable even though some logic is hidden.
> The second usage has a benefit that the logic is not hidden and you can
> use any function and you don't have to define anything but the line can
> get really long and ugly as we have some really long names for Free
> functions and types.
> 
> For example:
> 
> VIR_AUTOPTR(virDomainGraphicsDef) def = NULL;
> 
> VIR_CLEANUP(virDomainGraphicsDefFree) virDomainGraphicsDefPtr def = NULL;
> 
> And that's not even the longest free function.
> 
> Since the wrapper defined by VIR_DEFINE_AUTOPTR_FUNC() will be expanded
> during compilation I prefer that option.  It would essentially work the
> same way is VIR_CLEANUP but with the benefit that the declaration line
> is short, nice and clean.

I'm fine with either option, as long as we keep the ptr to a ptr aspect
of the original function. Probably a slight pref for the first option.


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

Re: [libvirt] [RFC] secdrivers remembering original labels

[Daniel P . Berrangé]

On Fri, Jul 27, 2018 at 09:56:23AM +0200, Michal Privoznik wrote:
> Dear list,
> 
> we have this very old bug [1] that I just keep pushing in front of me. I
> made several attempts to fix it. However, none of them made into the
> tree. I guess it's time to have discussion what to do about it. IIRC,
> the algorithm that I implemented last was to keep original label in
> XATTRs (among with some ref counter) and the last one to restore the
> label will look there and find the original label. There was a problem
> with two libvirtds fighting over a file on shared FS.

IIRC there was a second problem around security of XATTRs. eg, if we set
an XATTR it was possible for QEMU to change it once we given QEMU privs
on the image. Thus a malicious QEMU can cause libvirtd to change the
image to an arbitrary user on shutdown.

I think it was possible to deal with that on Linux, because the kernel
hsa some xattr namespacing that is reserved for only root. This protection
is worthless though when using NFS images as you can't trust the remote NFS
client to honour the same restriction.

> So I guess my question is can we come up with a design that would work?
> Or at least work to the extent that we're satisfied with?
> 
> Personally, I like the XATTRs approach. And to resolve the NFS race we
> can invent yet another lockspace to guard labelling - I also have a bug
> for that [2] (although, I'm not that familiar with lockspaces). I guess
> doing disk metadata locking is not going to be trivial, is it?

Yeah, for sure we need two distinct locking regimes. Our current locking
aims to protect disk content, and the lifetime of the lock is the entire
duration of the QEMU process. For XATTRs, we only need write protection
for the short time window in which the security drivers execute, which
is at start, during hotplug/unplug, during shutdown, and finally migration.

I think we could achieve this with the virtlockd if we make it use the
same locking file, but a different byte offset within the file. Just
need to make sure it doesn't clash with the byte offset QEMU has chosen,
nor the current offset we use.

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

Re: [libvirt] [RFC] secdrivers remembering original labels

[Daniel P . Berrangé]

On Fri, Jul 27, 2018 at 01:27:40PM +0200, Michal Privoznik wrote:
> On 07/27/2018 11:01 AM, Daniel P. Berrangé wrote:
> > On Fri, Jul 27, 2018 at 09:56:23AM +0200, Michal Privoznik wrote:
> >> Dear list,
> >>
> >> we have this very old bug [1] that I just keep pushing in front of me. I
> >> made several attempts to fix it. However, none of them made into the
> >> tree. I guess it's time to have discussion what to do about it. IIRC,
> >> the algorithm that I implemented last was to keep original label in
> >> XATTRs (among with some ref counter) and the last one to restore the
> >> label will look there and find the original label. There was a problem
> >> with two libvirtds fighting over a file on shared FS.
> > 
> > IIRC there was a second problem around security of XATTRs. eg, if we set
> > an XATTR it was possible for QEMU to change it once we given QEMU privs
> > on the image. Thus a malicious QEMU can cause libvirtd to change the
> > image to an arbitrary user on shutdown.
> > 
> > I think it was possible to deal with that on Linux, because the kernel
> > hsa some xattr namespacing that is reserved for only root. 
> 
> Yes, there are basically 4 levels (defined by prefix of attribute name):
> 
>   security.
>   system.
>   trusted.
>   user.
> 
> For the first two there is no restriction on side of VFS (but there
> might be some coming from underlying FS and/or security module). The
> trusted. can be get/set only by CAP_SYS_ADMIN process. And finally user.
> can be set only one regular files (plus some other restrictions), but
> it's available for basically everybody.
> 
> IIRC, I used trusted. namespace because of CAP_SYS_ADMIN. Nota bene, if
> a file is stored on NFS and only CAP_SYS_ADMIN can change the
> attribute's value they are CAP_SYS_ADMIN already - they might change
> seclabel too. Why bother mangling libvirt's records. IOW, if somebody
> runs qemu with CAP_SYS_ADMIN all bets are off anyway.

Yep, if QEMU has CAP_SYS_ADMIN they're doomed no matter what.

> 
> This would of course mean that there's no label restore for session
> daemon, but that can hardly ever work. Do we even initialize DAC/SELinux
> drivers for session daemon?

The DAC driver doesn't work for session daemons (since you can't setuid
obviously), but the sVirt driver *does* work.

> > This protection
> > is worthless though when using NFS images as you can't trust the remote NFS
> > client to honour the same restriction.
> > 
> >> So I guess my question is can we come up with a design that would work?
> >> Or at least work to the extent that we're satisfied with?
> >>
> >> Personally, I like the XATTRs approach. And to resolve the NFS race we
> >> can invent yet another lockspace to guard labelling - I also have a bug
> >> for that [2] (although, I'm not that familiar with lockspaces). I guess
> >> doing disk metadata locking is not going to be trivial, is it?
> > 
> > Yeah, for sure we need two distinct locking regimes. Our current locking
> > aims to protect disk content, and the lifetime of the lock is the entire
> > duration of the QEMU process. For XATTRs, we only need write protection
> > for the short time window in which the security drivers execute, which
> > is at start, during hotplug/unplug, during shutdown, and finally migration.
> 
> I guess migration would be covered by start. Since these locks would be
> held only for very short period (basically they would be acquired just
> before we try to set seclabel and released after disk content lock is
> successfully acquired) they can be exclusive.
> 
> However, since we want to protect more than just disk seclabels, we need
> to acquire this new lock from more places.
> 
> > 
> > I think we could achieve this with the virtlockd if we make it use the
> > same locking file, but a different byte offset within the file. Just
> > need to make sure it doesn't clash with the byte offset QEMU has chosen,
> > nor the current offset we use.
> 
> Makes sense. So the first step is to introduce metadata locking. I'll
> look into that.

Yes, in fact the metdata locking is desirable even ignoring the original
task of restoring original disk labels. The metadata locking alone would
better protect against startup races between system libvirtds accessing
the same NFS.

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

Re: [libvirt] [PATCH RFC 0/2] util: Add VIR_ENUM_IMPL_LABEL

[Daniel P . Berrangé]

On Fri, Jul 27, 2018 at 11:17:13AM +0200, Michal Privoznik wrote:
> On 07/26/2018 07:49 PM, Cole Robinson wrote:
> > This series adds VIR_ENUM_IMPL_LABEL and a demo conversion.
> > 
> > VIR_ENUM_IMPL_LABEL allows passing in a string that briefly
> > describes the value the enum represents, which we use to
> > generate error messages for FromString and ToString
> > function failures.
> > 
> > This will allow us to drop a lot of code and unique strings
> > that need translating.
> > 
> > Patch 2 is an example usage, converting virDomainVirtType
> > enum. It's not a full conversion for this particular enum,
> > just a demo. The enum creation now looks like
> > 
> >   VIR_ENUM_IMPL_LABEL(virDomainVirt,
> >   "domain type",
> >   VIR_DOMAIN_VIRT_LAST, ...
> > 
> > FromString failure reports this error for value ''
> > 
> >   invalid argument: Unknown 'domain type' value ''
> > 
> > ToString failure reports this error for unknown type=83
> > 
> >   internal error: Unknown 'domain type' internal value '83'
> > 
> > 
> > Seems like a win to me but I'm interested in other opinions.
> > This could also be a good BiteSizedTask for converting existing
> > enums
> > 
> 
> Agreed.
> 
> > Cole Robinson (2):
> >   util: Add VIR_ENUM_IMPL_LABEL
> >   conf: Convert virDomainVirtType to VIR_ENUM_IMPL_LABEL
> > 
> >  src/conf/domain_conf.c | 10 ++
> >  src/util/virutil.c | 20 
> >  src/util/virutil.h | 15 ++-
> >  3 files changed, 28 insertions(+), 17 deletions(-)
> > 
> 
> I like this. You can count on my ACK. But we should probably let others
> chime in and express their preference.

I think its good. My only comment is whether we could come up with a way
to gradually convert each file, while still finishing up with VIR_ENUM_IMPL
as the macro name. A mass rename at the end is one option, but perhaps
theres a more clever approach ?


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

Re: [libvirt] [PATCH v3 1/3] qemuxml2argvtest: Set more fake drivers

[Daniel P . Berrangé]

On Fri, Jul 27, 2018 at 05:24:45PM +0200, Michal Privoznik wrote:
> So far we are setting only fake secret and storage drivers.
> Therefore if the code wants to call a public NWFilter API (like
> qemuBuildInterfaceCommandLine() and qemuBuildNetCommandLine() are
> doing) the virGetConnectNWFilter() function will try to actually
> spawn session daemon because there's no connection object set to
> handle NWFilter driver.
> 
> Even though I haven't experienced the same problem with the rest
> of the drivers (interface, network and node dev), the reasoning
> above can be applied to them as well.
> 
> At the same time, now that connection object is registered for
> the drivers, the public APIs will throw
> virReportUnsupportedError(). And since we don't provide any error
> func the error is printed to stderr. Fix this by setting dummy
> error func.

Is this last paragraph stale ? you're not seeing a dummy error
func in this patch ...

> 
> Signed-off-by: Michal Privoznik 
> ---
>  tests/qemuxml2argvtest.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 84117a3e63..8901c7bde4 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -471,6 +471,10 @@ testCompareXMLToArgv(const void *data)
>  conn->secretDriver = 
>  conn->storageDriver = 
>  
> +virSetConnectInterface(conn);
> +virSetConnectNetwork(conn);
> +virSetConnectNWFilter(conn);
> +virSetConnectNodeDev(conn);
>  virSetConnectSecret(conn);
>  virSetConnectStorage(conn);
>  
> -- 
> 2.16.4
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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

Re: [libvirt] [PATCH v2 RESEND 00/12] PCI passthrough support on s390

[Daniel P . Berrangé]

On Thu, Jul 26, 2018 at 01:47:52PM +0200, Andrea Bolognani wrote:
> On Thu, 2018-07-26 at 12:22 +0100, Daniel P. Berrangé wrote:
> > On Thu, Jul 26, 2018 at 07:17:03PM +0800, Yi Min Zhao wrote:
> > > 在 2018/7/26 下午7:00, Andrea Bolognani 写道:
> > > > From the test cases I see a zpci devices, with its own uid and fid,
> > > > is created for the pci-bridge as well... Is that intentional?
> > > 
> > > Firstly pci bridge can be auto-generated if a pci device is to be plugged 
> > > to
> > > non-existing pci bus.
> > > IIUC, pci-bridge is treated as a controller device in libvirt. So I think,
> > > it's pretty readable not only
> > > in libvirt xml but also in qtree, if we assign zpci device for it. 
> > > Otherwise
> > > address type of pci-bridge
> > > is pci type but has no uid and fid. Isn't it odd?
> 
> Everything about zPCI is odd ;)
> 
> I guess there's no harm in creating an additional zpci device,
> and as you say it will keep things a bit more consistent, which
> is good.
> 
> > From the libvirt side we must avoid any scenario where QEMU auto-adds
> > devices behind our back. If adding a device requires adding a controller
> > libvirt must do this explicitly and record it in the XML.
> 
> Definitely. My question was whether the corresponding zpci device
> should be created as well...

I'm not sure I understand it fully, but it sounds like  zpci devices are
providing info that is guest ABI sensitive, which would mean libvirt must
control and record it. So from that POV we should create zpci devices

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

Re: [libvirt] [PATCH v2 RESEND 00/12] PCI passthrough support on s390

[Daniel P . Berrangé]

On Thu, Jul 26, 2018 at 07:17:03PM +0800, Yi Min Zhao wrote:
> 
> 
> 在 2018/7/26 下午7:00, Andrea Bolognani 写道:
> > > > > > > > How does the pci-bridge controller show up in the guest, if at 
> > > > > > > > all?
> > > > > Qemu hides pci-bridge devices and just exposes pci devices to the 
> > > > > guest.
> > > > > In above example, indeed, qemu will generate a pci-bridge device and 
> > > > > it will
> > > > > be existing in pci topology. But the guest can't see it. This is very
> > > > > special.
> > > > Yeah, that's kinda problematic as it violates the principle of least
> > > > surprise... If s390 guests can only see a flat PCI topology, then we
> > > > should find a way to reject bridges altogether instead of allowing
> > > > the user to create them (or even create them automatically) only for
> > > > them not to show up in the guest.
> > > If we reject bridges, there would be only one pci bus that maximum
> > > 32 pci devices could be plugged to. This kind of limitation is more
> > > problematic IMO.
> > I see how that would be pretty limiting.
> > 
> > >From the test cases I see a zpci devices, with its own uid and fid,
> > is created for the pci-bridge as well... Is that intentional?
> > 
> Firstly pci bridge can be auto-generated if a pci device is to be plugged to
> non-existing pci bus.
> IIUC, pci-bridge is treated as a controller device in libvirt. So I think,
> it's pretty readable not only
> in libvirt xml but also in qtree, if we assign zpci device for it. Otherwise
> address type of pci-bridge
> is pci type but has no uid and fid. Isn't it odd?

>From the libvirt side we must avoid any scenario where QEMU auto-adds
devices behind our back. If adding a device requires adding a controller
libvirt must do this explicitly and record it in the XML.

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

Re: [libvirt] Entering freeze for libvirt-4.6.0

[Daniel P . Berrangé]

On Tue, Jul 31, 2018 at 02:04:40PM +0200, Martin Kletzander wrote:
> On Tue, Jul 31, 2018 at 10:26:41AM +0200, Michal Privoznik wrote:
> > On 07/30/2018 05:20 PM, Andrea Bolognani wrote:
> > > On Sat, 2018-07-28 at 21:56 +0800, Daniel Veillard wrote:
> > > > 
> > 
> > > 
> > > Unfortunately I've spotted an issue during my testing of rc1 today:
> > > with the libvirt_guest NSS module enabled, Evolution would crash a
> > > few seconds after being started. Here's the stack trace:
> > > 
> > >   #0  0x7fffe7b69ba5 in json_object_iter_next () at 
> > > /lib64/libjson-glib-1.0.so.0
> > >   #1  0x7fffad8e757b in virJSONValueFromJansson () at 
> > > /lib64/libnss_libvirt_guest.so.2
> > >   #2  0x7fffad8e75d8 in virJSONValueFromJansson () at 
> > > /lib64/libnss_libvirt_guest.so.2
> > >   #3  0x7fffad8e8994 in virJSONValueFromString () at 
> > > /lib64/libnss_libvirt_guest.so.2
> > >   #4  0x7fffad8ecb5a in virMacMapNew () at 
> > > /lib64/libnss_libvirt_guest.so.2
> > >   #5  0x7fffad8cc140 in findLease () at 
> > > /lib64/libnss_libvirt_guest.so.2
> > >   #6  0x7fffad8ccb1c in _nss_libvirt_guest_gethostbyname4_r () at 
> > > /lib64/libnss_libvirt_guest.so.2
> > >   #7  0x7fffeb2599d2 in gaih_inet.constprop () at /lib64/libc.so.6
> > >   #8  0x7fffeb25aab4 in getaddrinfo () at /lib64/libc.so.6
> > >   #9  0x71d41a04 in do_lookup_by_name () at /lib64/libgio-2.0.so.0
> > >   #10 0x71d3e937 in g_task_thread_pool_thread () at 
> > > /lib64/libgio-2.0.so.0
> > >   #11 0x75c39933 in g_thread_pool_thread_proxy () at 
> > > /lib64/libglib-2.0.so.0
> > >   #12 0x75c38f2a in g_thread_proxy () at /lib64/libglib-2.0.so.0
> > >   #13 0x76314594 in start_thread () at /lib64/libpthread.so.0
> > >   #14 0x7fffeb2700df in clone () at /lib64/libc.so.6
> > > 
> > > I've talked about it with a few colleagues and we believe the issue
> > > to be caused by jansson and json-glib both exporting a symbol called
> > > json_object_iter_next: Evolution itself (indirectly?) links against
> > > the latter library, so when the libvirt_guest NSS module is loaded
> > > and attempts to process JSON using the former, it picks up the wrong
> > > implementation, leading to a crash. gnome-boxes also crashes with
> > > the same stack trace.
> > 
> > Worse. querying gentoo portage I've found some important packages
> > requiring json-glib:
> > 
> > x11-libs/gtk
> > gnome-base/gnome-shell
> > 
> > So once users of these app update to latest libvirt they will see the
> > crashes.
> > 
> > > 
> > > It seems like a similar issue could affect any application linking
> > > both to libvirt and json-glib, regardless of whether or not the NSS
> > > plugin has been enabled, which is of course pretty bad.
> > 
> > Yes, any application can crash.
> > 
> > > 
> > > Unfortunately, I don't have any bright ideas on how to solve this,
> > > so anyone who might: please step forward! We're just a few days
> > > away from the next release, and if we can't figure out a way around
> > > this soon I'm afraid the only reasonable course of action would be
> > > to (temporarily) revert the switch from yajl to jansson.
> > > 
> > 
> > Well, what if we linked with jansson statically? I'm not sure if it is
> > possible (and have no idea how to achieve that), but what if our dynamic
> > libraries we produce already contained jansson and thus linker would not
> > even try to resolve json_* symbols.
> > 
> 
> It could "help" (quotes for all the disadvantages that approach has).  Not
> because it would not try to resolve it, but because we would have the `json_`
> symbols as 'local' thanks to our src/libvirt.syms.  If the lib was added to 
> our
> dynamic lib we would still need to use `-Bsymbolic-functions` so that our
> `json_` symbols don't call `json_` symbols from the dynamic one programs where
> it is loaded.  However that has some issues with `LD_PRELOAD`.

Note there is no jansson static build in Fedora or RHEL, so it is somewhat
academic right now.

> 
> Maybe we could utilize the `-Bgroup` linker option, although I'm not sure how
> that is supposed to be used.
> 
> In any case, this could be fixed in the respective libraries.  The reasoning
> behind it is that since C doesn't support namespaces we namespace functions 
> by a
> prefix (`vir` in libvirt), however that "namespace" needs to be unique.  They
> should switch to `jansson_` or `glib_json_` prefixes and maybe provide macros
> for the previous names:
> 
>  #define json_auto_t jansson_auto_t
>  ...
> 
> I know it sounds like too big of a deal, but that's what happens in C world.
> The same would happen if libvirt used `json-glib` and some application linking
> with libvirt would start using jansson (and also use some specific functions).
> Not that we were guarded against that now.  I'm not saying the release can go
> on, of course not, just that the ultimate fix is not something *we* should do.

Changing their API names isn't required 

[libvirt] [PATCH] util: avoid symbol clash between json libraries

[Daniel P . Berrangé]

The jansson and json-glib libraries both export symbols with a json_
name prefix and json_object_iter_next() clashes between them.

Unfortunately json_glib is linked in by GTK, so any app using GTK and
libvirt will get a clash, resulting in SEGV. This also affects the NSS
module provided by libvirt

Instead of directly linking to jansson, use dlopen() with the RTLD_LOCAL
flag which allows us to hide the symbols from the application that loads
libvirt or the NSS module.

Some preprocessor black magic and wrapper functions are used to redirect
calls into the dlopen resolved symbols.

Signed-off-by: Daniel P. Berrangé 
---
 libvirt.spec.in  |   2 +
 src/Makefile.am  |   3 +
 src/util/Makefile.inc.am |   3 +-
 src/util/virjson.c   |   9 +-
 src/util/virjsoncompat.c | 253 +++
 src/util/virjsoncompat.h |  86 +
 6 files changed, 354 insertions(+), 2 deletions(-)
 create mode 100644 src/util/virjsoncompat.c
 create mode 100644 src/util/virjsoncompat.h

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 4113579e47..cfe7ab8a09 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -898,6 +898,8 @@ Requires: ncurses
 Requires: gettext
 # Needed by virt-pki-validate script.
 Requires: gnutls-utils
+# We dlopen() it so need an explicit dep
+Requires: libjansson.so.4()(64bit)
 %if %{with_bash_completion}
 Requires: %{name}-bash-completion = %{version}-%{release}
 %endif
diff --git a/src/Makefile.am b/src/Makefile.am
index 83263e69e5..59ae7a2e79 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -690,6 +690,7 @@ libvirt_setuid_rpc_client_la_SOURCES = \
util/virhashcode.c \
util/virhostcpu.c \
util/virjson.c \
+   util/virjsoncompat.c \
util/virlog.c \
util/virobject.c \
util/virpidfile.c \
@@ -961,6 +962,8 @@ libvirt_nss_la_SOURCES = \
util/virhashcode.h \
util/virjson.c \
util/virjson.h \
+   util/virjsoncompat.c \
+   util/virjsoncompat.h \
util/virkmod.c \
util/virkmod.h \
util/virlease.c \
diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am
index 71b2b93c2d..8ef9ee1dfa 100644
--- a/src/util/Makefile.inc.am
+++ b/src/util/Makefile.inc.am
@@ -86,6 +86,8 @@ UTIL_SOURCES = \
util/viriscsi.h \
util/virjson.c \
util/virjson.h \
+   util/virjsoncompat.c \
+   util/virjsoncompat.h \
util/virkeycode.c \
util/virkeycode.h \
util/virkeyfile.c \
@@ -264,7 +266,6 @@ libvirt_util_la_CFLAGS = \
$(NULL)
 libvirt_util_la_LIBADD = \
$(CAPNG_LIBS) \
-   $(JANSSON_LIBS) \
$(LIBNL_LIBS) \
$(THREAD_LIBS) \
$(AUDIT_LIBS) \
diff --git a/src/util/virjson.c b/src/util/virjson.c
index 01a387b2f7..5bab662cd3 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -1437,7 +1437,8 @@ virJSONValueCopy(const virJSONValue *in)
 
 
 #if WITH_JANSSON
-# include 
+
+# include "virjsoncompat.h"
 
 static virJSONValuePtr
 virJSONValueFromJansson(json_t *json)
@@ -1524,6 +1525,9 @@ virJSONValueFromString(const char *jsonstring)
 size_t flags = JSON_REJECT_DUPLICATES |
JSON_DECODE_ANY;
 
+if (virJSONInitialize() < 0)
+return NULL;
+
 if (!(json = json_loads(jsonstring, flags, ))) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("failed to parse JSON %d:%d: %s"),
@@ -1630,6 +1634,9 @@ virJSONValueToString(virJSONValuePtr object,
 json_t *json;
 char *str = NULL;
 
+if (virJSONInitialize() < 0)
+return NULL;
+
 if (pretty)
 flags |= JSON_INDENT(2);
 else
diff --git a/src/util/virjsoncompat.c b/src/util/virjsoncompat.c
new file mode 100644
index 00..c317e50c32
--- /dev/null
+++ b/src/util/virjsoncompat.c
@@ -0,0 +1,253 @@
+/*
+ * virjsoncompat.c: JSON object parsing/formatting
+ *
+ * Copyright (C) 2018 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include 
+
+#include "virthread.h"
+#include "virerror.h"
+#define VIR_JSON_COMPAT_IMPL
+#include "virjsoncompat.h"
+
+#define VIR_FROM_THIS VIR_FROM_NONE
+
+#if WITH

Re: [libvirt] Entering freeze for libvirt-4.6.0

[Daniel P . Berrangé]

On Tue, Jul 31, 2018 at 12:18:16PM +0200, Ján Tomko wrote:
> On Tue, Jul 31, 2018 at 10:26:41AM +0200, Michal Privoznik wrote:
> > On 07/30/2018 05:20 PM, Andrea Bolognani wrote:
> > > On Sat, 2018-07-28 at 21:56 +0800, Daniel Veillard wrote:
> > > > 
> > 
> > > 
> > > Unfortunately I've spotted an issue during my testing of rc1 today:
> > > with the libvirt_guest NSS module enabled, Evolution would crash a
> > > few seconds after being started. Here's the stack trace:
> > > 
> > >   #0  0x7fffe7b69ba5 in json_object_iter_next () at 
> > > /lib64/libjson-glib-1.0.so.0
> > >   #1  0x7fffad8e757b in virJSONValueFromJansson () at 
> > > /lib64/libnss_libvirt_guest.so.2
> > >   #2  0x7fffad8e75d8 in virJSONValueFromJansson () at 
> > > /lib64/libnss_libvirt_guest.so.2
> > >   #3  0x7fffad8e8994 in virJSONValueFromString () at 
> > > /lib64/libnss_libvirt_guest.so.2
> > >   #4  0x7fffad8ecb5a in virMacMapNew () at 
> > > /lib64/libnss_libvirt_guest.so.2
> > >   #5  0x7fffad8cc140 in findLease () at 
> > > /lib64/libnss_libvirt_guest.so.2
> > >   #6  0x7fffad8ccb1c in _nss_libvirt_guest_gethostbyname4_r () at 
> > > /lib64/libnss_libvirt_guest.so.2
> > >   #7  0x7fffeb2599d2 in gaih_inet.constprop () at /lib64/libc.so.6
> > >   #8  0x7fffeb25aab4 in getaddrinfo () at /lib64/libc.so.6
> > >   #9  0x71d41a04 in do_lookup_by_name () at /lib64/libgio-2.0.so.0
> > >   #10 0x71d3e937 in g_task_thread_pool_thread () at 
> > > /lib64/libgio-2.0.so.0
> > >   #11 0x75c39933 in g_thread_pool_thread_proxy () at 
> > > /lib64/libglib-2.0.so.0
> > >   #12 0x75c38f2a in g_thread_proxy () at /lib64/libglib-2.0.so.0
> > >   #13 0x76314594 in start_thread () at /lib64/libpthread.so.0
> > >   #14 0x7fffeb2700df in clone () at /lib64/libc.so.6
> > > 
> > > I've talked about it with a few colleagues and we believe the issue
> > > to be caused by jansson and json-glib both exporting a symbol called
> > > json_object_iter_next: Evolution itself (indirectly?) links against
> 
> For the libraries installed on my Fedora 28, objdump shows
> json_object_iter_next as the only conflicting symbol, maybe we can
> get away with using a different iterator as a quick fix.
> 
> > > the latter library, so when the libvirt_guest NSS module is loaded
> > > and attempts to process JSON using the former, it picks up the wrong
> > > implementation, leading to a crash. gnome-boxes also crashes with
> > > the same stack trace.
> > 
> > Worse. querying gentoo portage I've found some important packages
> > requiring json-glib:
> > 
> > x11-libs/gtk
> > gnome-base/gnome-shell
> > 
> > So once users of these app update to latest libvirt they will see the
> > crashes.
> > 
> > > 
> > > It seems like a similar issue could affect any application linking
> > > both to libvirt and json-glib, regardless of whether or not the NSS
> > > plugin has been enabled, which is of course pretty bad.
> > 
> > Yes, any application can crash.
> > 
> > > 
> > > Unfortunately, I don't have any bright ideas on how to solve this,
> > > so anyone who might: please step forward! We're just a few days
> > > away from the next release, and if we can't figure out a way around
> > > this soon I'm afraid the only reasonable course of action would be
> > > to (temporarily) revert the switch from yajl to jansson.
> > > 
> > 
> > Well, what if we linked with jansson statically? I'm not sure if it is
> > possible (and have no idea how to achieve that), but what if our dynamic
> > libraries we produce already contained jansson and thus linker would not
> > even try to resolve json_* symbols.
> 
> 
> For the client library, we can just compile out JSON - it should not be
> needed for anything. And we can generate the data for libvirt_nss in
> some simpler format.

There's no such concept of 'client library', our libvirt.so library
is used in both client and server sides, providing the shared code
to both.

Changing the NSS format is doable, but not before release.

In fact bearing this problem in mind, I tend to think we should perhaps
make sure that the NSS library doesn't link to anything except glibc.
It can be loaded into any process running on the host, so the less we
load from it the safer we'll be.

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

Re: [libvirt] [PATCH] util: avoid symbol clash between json libraries

[Daniel P . Berrangé]

On Tue, Jul 31, 2018 at 06:07:20PM +0200, Ján Tomko wrote:
> On Tue, Jul 31, 2018 at 03:55:28PM +0100, Daniel P. Berrangé wrote:
> > The jansson and json-glib libraries both export symbols with a json_
> > name prefix and json_object_iter_next() clashes between them.
> > 
> > Unfortunately json_glib is linked in by GTK, so any app using GTK and
> 
> json-glib

Not fixed in v2, but fixed in my local copy now.

> > +#include 
> > +
> > +#include "virthread.h"
> > +#include "virerror.h"
> 
> > +#define VIR_JSON_COMPAT_IMPL
> > +#include "virjsoncompat.h"
> 
> virjsoncompat.h includes jansson.h unconditionally, so this fails to
> compile on a machine without jansson-devel:
> In file included from util/virjsoncompat.c:27:
> util/virjsoncompat.h:56:10: fatal error: jansson.h: No such file or directory
> #include 
>  ^~~

I've rearranged things and tested a mingw build now so this should
be ok in v2.

> > +#define LOAD(name) \
> > +do { \
> > +if (!(name ## _ptr = dlsym(handle, #name))) {  \
> > +virReportError(VIR_ERR_NO_SUPPORT,  \
> > +   _("missing symbol '%s' in libjansson.so.4: 
> > %s"), #name, dlerror()); \
> > +goto error; \
> 
> If you do
>  return -1;
> you can drop the error label.

Yes ok




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

Re: [libvirt] [PATCH] util: avoid symbol clash between json libraries

[Daniel P . Berrangé]

On Tue, Jul 31, 2018 at 05:57:21PM +0200, Andrea Bolognani wrote:
> On Tue, 2018-07-31 at 15:55 +0100, Daniel P. Berrangé wrote:
> [...]
> > +# We dlopen() it so need an explicit dep
> > +Requires: libjansson.so.4()(64bit)
> 
> Wouldn't requiring jansson be better here? I don't think many
> people are running libvirt on 32-bit machines these days, but
> the (64bit) part still looks kinda weird.

Yes, ok.

> > @@ -264,7 +266,6 @@ libvirt_util_la_CFLAGS = \
> > $(NULL)
> >  libvirt_util_la_LIBADD = \
> > $(CAPNG_LIBS) \
> > -   $(JANSSON_LIBS) \
> 
> You missed a couple of instances of $(JANSSON_LIBS), notably the
> one used to link the NSS plugin against it ;)
> 
> > @@ -1524,6 +1525,9 @@ virJSONValueFromString(const char *jsonstring)
> >  size_t flags = JSON_REJECT_DUPLICATES |
> > JSON_DECODE_ANY;
> >  
> > +if (virJSONInitialize() < 0)
> > +return NULL;
> 
> Shouldn't we rather virReportError() here? Or does it not matter
> since we do that inside virJSONInitialize() already?

That method is already reporting errors when needed.

> The lines above trigger a syntax-check failure; there are other
> issues as well, please make sure you address all of them.

Yeah done in v2.

> 
> > +fprintf(stderr, "Resolve %s to %p\n", #name, name ## _ptr); \
> 
> I guess the fprintf() is a leftover from development: please drop
> it to avoid stuff like

Opps.


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

Re: [libvirt] [PATCH] util: avoid symbol clash between json libraries

[Daniel P . Berrangé]

On Tue, Jul 31, 2018 at 11:28:02AM -0500, Eric Blake wrote:
> On 07/31/2018 09:55 AM, Daniel P. Berrangé wrote:
> > The jansson and json-glib libraries both export symbols with a json_
> > name prefix and json_object_iter_next() clashes between them.
> > 
> > Unfortunately json_glib is linked in by GTK, so any app using GTK and
> > libvirt will get a clash, resulting in SEGV. This also affects the NSS
> > module provided by libvirt
> > 
> > Instead of directly linking to jansson, use dlopen() with the RTLD_LOCAL
> > flag which allows us to hide the symbols from the application that loads
> > libvirt or the NSS module.
> > 
> > Some preprocessor black magic and wrapper functions are used to redirect
> > calls into the dlopen resolved symbols.
> 
> Would using dlmopen() instead of dlopen() make this task any easier?

Not available on *BSD

> 
> Otherwise, this looks reasonable to me, and is preferable to Jan's proposal
> to revert jansson support.
> 
> > +++ b/src/util/virjsoncompat.c
> 
> > +
> > +static int virJSONJanssonOnceInit(void)
> > +{
> > +void *handle = dlopen("libjansson.so.4", 
> > RTLD_LAZY|RTLD_LOCAL|RTLD_DEEPBIND|RTLD_NODELETE);
> 
> RTLD_DEEPBIND might be specific to glibc; is this going to cause any
> compilation issues on BSD machines?

Yeah it broke BSD, so removed in v2.

> > +int json_array_append_new_impl(json_t *array, json_t *value)
> > +{
> > +return json_array_append_new_ptr(array, value);
> > +}
> 
> Would it be possible with __typeof__ to write a macro to make this
> forwarding more compact (one line per stem, instead of open-coding each
> renamed function)?  Looking something like:
> 
> #define FORWARD(name, params, args) \
>   __typeof__(name # _impl) name # _impl params { \
> return name # _ptr args; \
>   }
> 
> FORWARD(json_array, (void), ())
> FORWARD(json_array_append_new, (json_t * array, json_t *value),
>  (array, value))
> 
> (hmm, that's still a bit verbose; I'm not sure if the preprocessor could be
> cajoled into even less repetition of parameter names)

I'm not too fussed about the verbosity as this is write-once code (i hope).

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

Re: [libvirt] [PATCH] util: avoid symbol clash between json libraries

[Daniel P . Berrangé]

On Wed, Aug 01, 2018 at 09:31:49AM +0200, Peter Krempa wrote:
> On Tue, Jul 31, 2018 at 15:55:28 +0100, Daniel Berrange wrote:
> > The jansson and json-glib libraries both export symbols with a json_
> > name prefix and json_object_iter_next() clashes between them.
> > 
> > Unfortunately json_glib is linked in by GTK, so any app using GTK and
> > libvirt will get a clash, resulting in SEGV. This also affects the NSS
> > module provided by libvirt
> > 
> > Instead of directly linking to jansson, use dlopen() with the RTLD_LOCAL
> > flag which allows us to hide the symbols from the application that loads
> > libvirt or the NSS module.
> > 
> > Some preprocessor black magic and wrapper functions are used to redirect
> > calls into the dlopen resolved symbols.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  libvirt.spec.in  |   2 +
> >  src/Makefile.am  |   3 +
> >  src/util/Makefile.inc.am |   3 +-
> >  src/util/virjson.c   |   9 +-
> >  src/util/virjsoncompat.c | 253 +++
> >  src/util/virjsoncompat.h |  86 +
> >  6 files changed, 354 insertions(+), 2 deletions(-)
> >  create mode 100644 src/util/virjsoncompat.c
> >  create mode 100644 src/util/virjsoncompat.h
> 
> [...]
> 
> > diff --git a/src/util/virjson.c b/src/util/virjson.c
> > index 01a387b2f7..5bab662cd3 100644
> > --- a/src/util/virjson.c
> > +++ b/src/util/virjson.c
> > @@ -1437,7 +1437,8 @@ virJSONValueCopy(const virJSONValue *in)
> >  
> >  
> >  #if WITH_JANSSON
> > -# include 
> > +
> > +# include "virjsoncompat.h"
> >  
> >  static virJSONValuePtr
> >  virJSONValueFromJansson(json_t *json)
> > @@ -1524,6 +1525,9 @@ virJSONValueFromString(const char *jsonstring)
> >  size_t flags = JSON_REJECT_DUPLICATES |
> > JSON_DECODE_ANY;
> >  
> > +if (virJSONInitialize() < 0)
> > +return NULL;
> > +
> >  if (!(json = json_loads(jsonstring, flags, ))) {
> >  virReportError(VIR_ERR_INTERNAL_ERROR,
> > _("failed to parse JSON %d:%d: %s"),
> > @@ -1630,6 +1634,9 @@ virJSONValueToString(virJSONValuePtr object,
> >  json_t *json;
> >  char *str = NULL;
> >  
> > +if (virJSONInitialize() < 0)
> > +return NULL;
> > +
> >  if (pretty)
> >  flags |= JSON_INDENT(2);
> >  else
> 
> [Note that some mails did not make it to the list so I don't know if
> this issue was raised.]
> 
> If we initialize this when doing JSON operations and the initialization
> fails we'll end up with libvirtd in a crippled state not able to use
> anything in qemu which is in my opinion VERY bad.
> 
> In addition it also kills all running VMs when reconnecting since
> monitor will just not work.
> 
> I think libvirtd should terminate right away if we can't initialize this
> shim rather than execute some other code.

We can do that by calling virJSONInitialize from libvirtd main. It should
never fail unless someone instaled libvirtd without jansson being present,
or if jansson broke its API.

> Additionally, why is this even necessary for libvirtd? Not that it would
> ever link with any clashing libraries.

It may not currently be neccessary for libvirtd, but I can't guarantee
that's always going to be the case. We link to so many libraries, we can
be surprised at anytime with something pulling in a clashing json impl.
The json-c library also includes symbols with a json_* prefix in their
name, so that's a 3d json impl which might clash.

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

Re: [libvirt] Availability of libvirt-4.6.0 Release Candidate 2

[Daniel P . Berrangé]

On Tue, Jul 31, 2018 at 03:54:43PM +0200, Bjoern Walk wrote:
> Bjoern Walk  [2018-07-31, 03:16PM +0200]:
> > I have not yet had the time to figure out what goes wrong, any ideas are 
> > welcome.
> 
> Ah, classic. The mocked virRandomBytes function is not endian-agnostic,
> generating a different interface name as expected by the test.

I'm not seeing why virRandomBytes is affected by endian-ness. It is simply
populating an array of bytes, so there's no endin issues to consider there.

Can you elaborate on the actual error messages you are getting from the
tests, and what aspect makes you think virRandomBytes is the problem ?

Seems more likely that it is something higher up the call stack.

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

Re: [libvirt] [PATCH 2/2] viriscsi: Request more random bits for interface name

[Daniel P . Berrangé]

On Wed, Aug 01, 2018 at 01:44:33PM +0200, Michal Privoznik wrote:
> In virStorageBackendCreateIfaceIQN() the virRandomBits() is
> called in order to use random bits to generate random name for
> new interface. However, virAsprintf() is expecting 32 bits and we
> are requesting only 30.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/util/viriscsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
> index 653b4fd932..f00aeb53a7 100644
> --- a/src/util/viriscsi.c
> +++ b/src/util/viriscsi.c
> @@ -221,7 +221,7 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn,
>  
>  if (virAsprintf(_ifacename,
>  "libvirt-iface-%08llx",
> -(unsigned long long)virRandomBits(30)) < 0)
> +(unsigned long long)virRandomBits(32)) < 0)
>  return -1;
>  
>  VIR_DEBUG("Attempting to create interface '%s' with IQN '%s'",

Reviewed-by: Daniel P. Berrangé 

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

Re: [libvirt] [PATCH 1/2] util: Don't overflow in virRandomBits

[Daniel P . Berrangé]

On Wed, Aug 01, 2018 at 01:44:32PM +0200, Michal Privoznik wrote:
> The function is supposed to return up to 64bit long integer. In
> order to do that it calls virRandomBytes() to fill the integer
> with random bytes and then masks out everything but requested
> bits. However, when doing that it shifts 1U and not 1ULL. So
> effectively, requesting 32 random bis or more always return 0
> which is not random enough.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/util/virrandom.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/util/virrandom.c b/src/util/virrandom.c
> index 01cc82a052..3c011a8615 100644
> --- a/src/util/virrandom.c
> +++ b/src/util/virrandom.c
> @@ -68,7 +68,7 @@ uint64_t virRandomBits(int nbits)
>  return 0;
>  }
>  
> -ret &= (1U << nbits) - 1;
> +    ret &= (1ULL << nbits) - 1;
>  return ret;
>  }

Reviewed-by: Daniel P. Berrangé 

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

Re: [libvirt] Entering freeze for libvirt-4.6.0

[Daniel P . Berrangé]

On Mon, Jul 30, 2018 at 05:20:01PM +0200, Andrea Bolognani wrote:
> It seems like a similar issue could affect any application linking
> both to libvirt and json-glib, regardless of whether or not the NSS
> plugin has been enabled, which is of course pretty bad.
> 
> Unfortunately, I don't have any bright ideas on how to solve this,
> so anyone who might: please step forward! We're just a few days
> away from the next release, and if we can't figure out a way around
> this soon I'm afraid the only reasonable course of action would be
> to (temporarily) revert the switch from yajl to jansson.

It turns out we're not the first people to hit this problem. NetworkManager
uses jansson in its libnm-core.so library, and that caused crashes[1] when it
was loaded into GNOME control center  which uses json-glib.

They came up with a clever but gross solution [2].

First stop linking to jansson at build time. Then have code that calls
dlopen(jansson.so), passing RTLD_LAZY | RTLD_LOCAL which avoids jansson
symbols polluting the entire application.  Now use dlsym()  to resolve
ach jansson symbol they need to use and store them in function pointer
variables. Their code can now indirect call jansson via these saved
pointers.  This sounds like a doable approach for this release at least,
while we consider whether there's a better option long term.


Regards,
Daniel

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1535905
[2] 
https://github.com/NetworkManager/NetworkManager/blob/master/libnm-core/nm-json.c
-- 
|: 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

Re: [libvirt] Entering freeze for libvirt-4.6.0

[Daniel P . Berrangé]

On Mon, Jul 30, 2018 at 05:20:01PM +0200, Andrea Bolognani wrote:
> On Sat, 2018-07-28 at 21:56 +0800, Daniel Veillard wrote:
> > so things looks pretty good for
> > me but please try it out on different systems and OSes.
> > 
> >   If everything goes well I will push rc2 on Tuesday targetting Thursday 
> > for the
> > final release (or Friday if I get stuck in travels).
> > 
> >   thanks in advance for trying it out !
> 
> Unfortunately I've spotted an issue during my testing of rc1 today:
> with the libvirt_guest NSS module enabled, Evolution would crash a
> few seconds after being started. Here's the stack trace:
> 
>   #0  0x7fffe7b69ba5 in json_object_iter_next () at 
> /lib64/libjson-glib-1.0.so.0
>   #1  0x7fffad8e757b in virJSONValueFromJansson () at 
> /lib64/libnss_libvirt_guest.so.2
>   #2  0x7fffad8e75d8 in virJSONValueFromJansson () at 
> /lib64/libnss_libvirt_guest.so.2
>   #3  0x7fffad8e8994 in virJSONValueFromString () at 
> /lib64/libnss_libvirt_guest.so.2
>   #4  0x7fffad8ecb5a in virMacMapNew () at 
> /lib64/libnss_libvirt_guest.so.2
>   #5  0x7fffad8cc140 in findLease () at /lib64/libnss_libvirt_guest.so.2
>   #6  0x7fffad8ccb1c in _nss_libvirt_guest_gethostbyname4_r () at 
> /lib64/libnss_libvirt_guest.so.2
>   #7  0x7fffeb2599d2 in gaih_inet.constprop () at /lib64/libc.so.6
>   #8  0x7fffeb25aab4 in getaddrinfo () at /lib64/libc.so.6
>   #9  0x71d41a04 in do_lookup_by_name () at /lib64/libgio-2.0.so.0
>   #10 0x71d3e937 in g_task_thread_pool_thread () at 
> /lib64/libgio-2.0.so.0
>   #11 0x75c39933 in g_thread_pool_thread_proxy () at 
> /lib64/libglib-2.0.so.0
>   #12 0x75c38f2a in g_thread_proxy () at /lib64/libglib-2.0.so.0
>   #13 0x76314594 in start_thread () at /lib64/libpthread.so.0
>   #14 0x7fffeb2700df in clone () at /lib64/libc.so.6
> 
> I've talked about it with a few colleagues and we believe the issue
> to be caused by jansson and json-glib both exporting a symbol called
> json_object_iter_next: Evolution itself (indirectly?) links against
> the latter library, so when the libvirt_guest NSS module is loaded
> and attempts to process JSON using the former, it picks up the wrong
> implementation, leading to a crash. gnome-boxes also crashes with
> the same stack trace.

Frustratingly both jansson and json-glib use the same 'json_' prefix
for all their functions, which was very unwise choice of namespaces.

Despite this by some miracle 'json_object_iter_next' is the only
one that clashes.

This is compounded by neither library making use of symbol versioning
which would have ensured they resolved to the correct libraries. We
should talk to them about adding versioning to avoid this problem.

I wondered if it is possible for libvirt to change its impl to avoid
calling json_object_iter_next, but it doesn't look practicall, as
'json_object_foreach' just uses the iter behind the scenes.

> It seems like a similar issue could affect any application linking
> both to libvirt and json-glib, regardless of whether or not the NSS
> plugin has been enabled, which is of course pretty bad.

Sure, it affects applications for any library libvirt links to
in fact. Most libraries use the library name as the base prefix
for their methods to get good namespacing. Unfortunately this
time both libraries used the generic 'json_' prefix.

I'm not so worried about this though, as the code paths taken
in the libvirt client shouldn't tickle the json parser. Of
course if jansson gets resolved before json-glib, we could
still break the apps own usage of json.

The NSS module is the big worry since that's loadable by any
process.

> Unfortunately, I don't have any bright ideas on how to solve this,
> so anyone who might: please step forward! We're just a few days
> away from the next release, and if we can't figure out a way around
> this soon I'm afraid the only reasonable course of action would be
> to (temporarily) revert the switch from yajl to jansson.

Or just document it as a known issue in the NSS module for now
and resolve in next release.

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

Re: [libvirt] [PATCH for v4.6.0 0/2] Fix some random problems

[Daniel P . Berrangé]

On Wed, Aug 01, 2018 at 02:57:07PM +0200, Bjoern Walk wrote:
> And here's the fix for the viriscsitest on big-endian machine like
> Daniel suggested.
> 
> Bjoern
> 
> -- 
> IBM Systems
> Linux on Z & Virtualization Development
> 
> IBM Deutschland Research & Development GmbH
> Schönaicher Str. 220, 71032 Böblingen
> Phone: +49 7031 16 1819
> 
> Vorsitzende des Aufsichtsrats: Martina Koederitz
> Geschäftsführung: Dirk Wittkopp
> Sitz der Gesellschaft: Böblingen
> Registergericht: Amtsgericht Stuttgart, HRB 243294 

> From d59b254294a90c5a9ca0fb6ad29465cd0950bb61 Mon Sep 17 00:00:00 2001
> From: Bjoern Walk 
> Date: Wed, 1 Aug 2018 14:48:47 +0200
> Subject: [PATCH] util: virrandom: make virRandomBits endian-safe
> 
> Make the generation of random bits in virRandomBits independent of the
> endianness of the running architecture.
> 
> This also solves problems with the mocked random byte generation on
> big-endian machines.
> 
> Suggested-by: Daniel P. Berrangé 
> Signed-off-by: Bjoern Walk 
> ---
>  src/util/virrandom.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/util/virrandom.c b/src/util/virrandom.c
> index 01cc82a0..a58ee97a 100644
> --- a/src/util/virrandom.c
> +++ b/src/util/virrandom.c
> @@ -34,6 +34,7 @@
>  # include 
>  #endif
>  
> +#include "virendian.h"
>  #include "virrandom.h"
>  #include "virthread.h"
>  #include "count-one-bits.h"
> @@ -60,16 +61,15 @@ VIR_LOG_INIT("util.random");
>   */
>  uint64_t virRandomBits(int nbits)
>  {
> -uint64_t ret = 0;
> +uint8_t ret[8];
>  
> -if (virRandomBytes((unsigned char *) , sizeof(ret)) < 0) {
> +if (virRandomBytes(ret, sizeof(ret)) < 0) {
>  /* You're already hosed, so this particular non-random value
>   * isn't any worse.  */
>  return 0;
>  }
>  
> -ret &= (1ULL << nbits) - 1;
> -return ret;
> +return virReadBufInt64LE(ret) & ((1ULL << nbits) - 1);
>  }

Reviewed-by: Daniel P. Berrangé 

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

Re: [libvirt] Availability of libvirt-4.6.0 Release Candidate 2

[Daniel P . Berrangé]

On Wed, Aug 01, 2018 at 01:10:08PM +0100, Daniel P. Berrangé wrote:
> On Wed, Aug 01, 2018 at 01:41:48PM +0200, Bjoern Walk wrote:
> > Daniel P. Berrangé  [2018-08-01, 11:51AM +0100]:
> > > On Tue, Jul 31, 2018 at 03:54:43PM +0200, Bjoern Walk wrote:
> > > > Bjoern Walk  [2018-07-31, 03:16PM +0200]:
> > > > > I have not yet had the time to figure out what goes wrong, any ideas 
> > > > > are welcome.
> > > > 
> > > > Ah, classic. The mocked virRandomBytes function is not endian-agnostic,
> > > > generating a different interface name as expected by the test.
> > > 
> > > I'm not seeing why virRandomBytes is affected by endian-ness. It is simply
> > > populating an array of bytes, so there's no endin issues to consider 
> > > there.
> > 
> > Ahem, we are writing linearly to a byte array, of course this is
> > dependend on the endianness of the architecture. The actual problem is
> > that the mocked version does _not_ provide a random value, but a
> > deterministic one, which is byte order reversed on big endianness.
> 
> There's no concept of reversed byte order when you're accessing an
> array of bytes.  If you write elements 0, 1, 2, ...7, etc and then
> the caller reads elements 0, 1,2, ..., 7 everything is fine.
> 
> Endianness only comes into play if you take that array of bytes
> and then cast/assign it to a larger type were endianess is
> relevant (eg int16/int32/int64)
> 
> eg
> 
>char bytes[8];
> 
>virRandomBytes(bytes, 8);
> 
>uint64_t val = (uint64_t)bytes;
> 
> the problem in such a case isn't virRandomBytes, it is the cast
> to the larger sized integer type.
> 
> > > Can you elaborate on the actual error messages you are getting from the
> > > tests, and what aspect makes you think virRandomBytes is the problem ?
> > 
> > The name of the interface is wrong compared to what is explicitly
> > expected in the test case:
> > 
> > 200 if (virAsprintf(_ifacename,
> > (gdb)
> > 205 VIR_DEBUG("Attempting to create interface '%s' with IQN '%s'",
> > (gdb) p temp_ifacename
> > $1 = 0x1014320 "libvirt-iface-04050607"
> > 
> > > Seems more likely that it is something higher up the call stack.
> 
> That looks like it uses virRandomBits rather than virRandomBytes.

Yes, it is virRandomBits that is the problem - it is taking a uint64_t
variable and casting it to a "unsigned char *", which is not an
endian safe thing todo.

We need to rewrite virRandomBits to do

   char val[8];

   virRandomBytes(val, sizeof(val));

   uint64_t ret =
  val[0] |
  ((uint64_t)val[1]) << 8 |
  ((uint64_t)val[2]) << 16 |
  ((uint64_t)val[3]) << 24 |
  ((uint64_t)val[4]) << 32 |
  ((uint64_t)val[5]) << 40 |
  ((uint64_t)val[6]) << 48 |
  ((uint64_t)val[7]) << 56 |

   return ret & ((1ULL << nbits) -1);

to make it endiansafe.

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

[libvirt] [PATCH 2/4] cpu: push more parsing logic into common code

[Daniel P . Berrangé]

The x86 and ppc impls both duplicate some logic when parsing CPU
features. Change the callback signature so that this duplication can be
pushed up a level to common code.

Signed-off-by: Daniel P. Berrangé 
---
 src/cpu/cpu_map.c   | 106 +++-
 src/cpu/cpu_map.h   |  22 ++---
 src/cpu/cpu_ppc64.c | 112 ++---
 src/cpu/cpu_x86.c   | 196 +---
 4 files changed, 155 insertions(+), 281 deletions(-)

diff --git a/src/cpu/cpu_map.c b/src/cpu/cpu_map.c
index 9e090919ed..17ed53fda6 100644
--- a/src/cpu/cpu_map.c
+++ b/src/cpu/cpu_map.c
@@ -35,31 +35,51 @@
 
 VIR_LOG_INIT("cpu.cpu_map");
 
-VIR_ENUM_IMPL(cpuMapElement, CPU_MAP_ELEMENT_LAST,
-"vendor",
-"feature",
-"model")
-
-
-static int load(xmlXPathContextPtr ctxt,
-cpuMapElement element,
-cpuMapLoadCallback callback,
-void *data)
+static int
+loadData(const char *mapfile,
+ xmlXPathContextPtr ctxt,
+ const char *xpath,
+ cpuMapLoadCallback callback,
+ void *data)
 {
 int ret = -1;
 xmlNodePtr ctxt_node;
 xmlNodePtr *nodes = NULL;
 int n;
+size_t i;
+int rv;
 
 ctxt_node = ctxt->node;
 
-n = virXPathNodeSet(cpuMapElementTypeToString(element), ctxt, );
-if (n < 0)
+n = virXPathNodeSet(xpath, ctxt, );
+if (n < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("cannot find '%s' in CPU map '%s'"), xpath, mapfile);
 goto cleanup;
+}
 
-if (n > 0 &&
-callback(element, ctxt, nodes, n, data) < 0)
+if (n > 0 && !callback) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Unexpected %s in CPU map '%s'"), xpath, mapfile);
 goto cleanup;
+}
+
+for (i = 0; i < n; i++) {
+xmlNodePtr old = ctxt->node;
+char *name = virXMLPropString(nodes[i], "name");
+if (!name) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("cannot find %s name in CPU map '%s'"), xpath, 
mapfile);
+goto cleanup;
+}
+VIR_DEBUG("Load %s name %s", xpath, name);
+ctxt->node = nodes[i];
+rv = callback(ctxt, name, data);
+ctxt->node = old;
+VIR_FREE(name);
+if (rv < 0)
+goto cleanup;
+}
 
 ret = 0;
 
@@ -72,13 +92,14 @@ static int load(xmlXPathContextPtr ctxt,
 
 static int
 cpuMapLoadInclude(const char *filename,
-  cpuMapLoadCallback cb,
+  cpuMapLoadCallback vendorCB,
+  cpuMapLoadCallback featureCB,
+  cpuMapLoadCallback modelCB,
   void *data)
 {
 xmlDocPtr xml = NULL;
 xmlXPathContextPtr ctxt = NULL;
 int ret = -1;
-int element;
 char *mapfile;
 
 if (!(mapfile = virFileFindResource(filename,
@@ -93,13 +114,14 @@ cpuMapLoadInclude(const char *filename,
 
 ctxt->node = xmlDocGetRootElement(xml);
 
-for (element = 0; element < CPU_MAP_ELEMENT_LAST; element++) {
-if (load(ctxt, element, cb, data) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("cannot parse CPU map '%s'"), mapfile);
-goto cleanup;
-}
-}
+if (loadData(mapfile, ctxt, "vendor", vendorCB, data) < 0)
+goto cleanup;
+
+if (loadData(mapfile, ctxt, "feature", featureCB, data) < 0)
+goto cleanup;
+
+if (loadData(mapfile, ctxt, "model", modelCB, data) < 0)
+goto cleanup;
 
 ret = 0;
 
@@ -113,7 +135,9 @@ cpuMapLoadInclude(const char *filename,
 
 
 static int loadIncludes(xmlXPathContextPtr ctxt,
-cpuMapLoadCallback callback,
+cpuMapLoadCallback vendorCB,
+cpuMapLoadCallback featureCB,
+cpuMapLoadCallback modelCB,
 void *data)
 {
 int ret = -1;
@@ -131,7 +155,7 @@ static int loadIncludes(xmlXPathContextPtr ctxt,
 for (i = 0; i < n; i++) {
 char *filename = virXMLPropString(nodes[i], "filename");
 VIR_DEBUG("Finding CPU map include '%s'", filename);
-if (cpuMapLoadInclude(filename, callback, data) < 0) {
+if (cpuMapLoadInclude(filename, vendorCB, featureCB, modelCB, data) < 
0) {
 VIR_FREE(filename);
 goto cleanup;
 }
@@ -149,7 +173,9 @@ static int loadIncludes(xmlXPathContextPtr ctxt,
 
 
 int cpuMapLoad(const char *arch,
-   cpuMapLoadCallback cb,
+   cpuMapLoadCallback vendorCB,
+   cpuMapLoadCallback featureCB,
+   cpuMapLoadCallback modelCB,
void *data)
 {
 xmlDocPtr xml = NULL;
@@ -157,7 +183,6 @@ i

[libvirt] [PATCH v2] util: avoid symbol clash between json libraries

[Daniel P . Berrangé]

The jansson and json-glib libraries both export symbols with a json_
name prefix and json_object_iter_next() clashes between them.

Unfortunately json_glib is linked in by GTK, so any app using GTK and
libvirt will get a clash, resulting in SEGV. This also affects the NSS
module provided by libvirt

Instead of directly linking to jansson, use dlopen() with the RTLD_LOCAL
flag which allows us to hide the symbols from the application that loads
libvirt or the NSS module.

Some preprocessor black magic and wrapper functions are used to redirect
calls into the dlopen resolved symbols.

Signed-off-by: Daniel P. Berrangé 
---
 libvirt.spec.in  |   2 +
 src/Makefile.am  |   5 +-
 src/util/Makefile.inc.am |   3 +-
 src/util/virjson.c   |   9 +-
 src/util/virjsoncompat.c | 274 +++
 src/util/virjsoncompat.h |  88 +
 6 files changed, 377 insertions(+), 4 deletions(-)
 create mode 100644 src/util/virjsoncompat.c
 create mode 100644 src/util/virjsoncompat.h

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 4113579e47..19ae55cdaf 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -898,6 +898,8 @@ Requires: ncurses
 Requires: gettext
 # Needed by virt-pki-validate script.
 Requires: gnutls-utils
+# We dlopen(libjansson.so.4), so need an explicit dep
+Requires: jansson
 %if %{with_bash_completion}
 Requires: %{name}-bash-completion = %{version}-%{release}
 %endif
diff --git a/src/Makefile.am b/src/Makefile.am
index 83263e69e5..4b6366bb4c 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -552,7 +552,6 @@ libvirt_admin_la_CFLAGS += \
 
 libvirt_admin_la_LIBADD += \
$(CAPNG_LIBS) \
-   $(JANSSON_LIBS) \
$(DEVMAPPER_LIBS) \
$(LIBXML_LIBS) \
$(SSH2_LIBS) \
@@ -690,6 +689,7 @@ libvirt_setuid_rpc_client_la_SOURCES = \
util/virhashcode.c \
util/virhostcpu.c \
util/virjson.c \
+   util/virjsoncompat.c \
util/virlog.c \
util/virobject.c \
util/virpidfile.c \
@@ -961,6 +961,8 @@ libvirt_nss_la_SOURCES = \
util/virhashcode.h \
util/virjson.c \
util/virjson.h \
+   util/virjsoncompat.c \
+   util/virjsoncompat.h \
util/virkmod.c \
util/virkmod.h \
util/virlease.c \
@@ -1001,7 +1003,6 @@ libvirt_nss_la_LDFLAGS = \
$(NULL)
 
 libvirt_nss_la_LIBADD = \
-   $(JANSSON_LIBS) \
$(NULL)
 endif WITH_NSS
 
diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am
index 71b2b93c2d..8ef9ee1dfa 100644
--- a/src/util/Makefile.inc.am
+++ b/src/util/Makefile.inc.am
@@ -86,6 +86,8 @@ UTIL_SOURCES = \
util/viriscsi.h \
util/virjson.c \
util/virjson.h \
+   util/virjsoncompat.c \
+   util/virjsoncompat.h \
util/virkeycode.c \
util/virkeycode.h \
util/virkeyfile.c \
@@ -264,7 +266,6 @@ libvirt_util_la_CFLAGS = \
$(NULL)
 libvirt_util_la_LIBADD = \
$(CAPNG_LIBS) \
-   $(JANSSON_LIBS) \
$(LIBNL_LIBS) \
$(THREAD_LIBS) \
$(AUDIT_LIBS) \
diff --git a/src/util/virjson.c b/src/util/virjson.c
index 01a387b2f7..5bab662cd3 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -1437,7 +1437,8 @@ virJSONValueCopy(const virJSONValue *in)
 
 
 #if WITH_JANSSON
-# include 
+
+# include "virjsoncompat.h"
 
 static virJSONValuePtr
 virJSONValueFromJansson(json_t *json)
@@ -1524,6 +1525,9 @@ virJSONValueFromString(const char *jsonstring)
 size_t flags = JSON_REJECT_DUPLICATES |
JSON_DECODE_ANY;
 
+if (virJSONInitialize() < 0)
+return NULL;
+
 if (!(json = json_loads(jsonstring, flags, ))) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("failed to parse JSON %d:%d: %s"),
@@ -1630,6 +1634,9 @@ virJSONValueToString(virJSONValuePtr object,
 json_t *json;
 char *str = NULL;
 
+if (virJSONInitialize() < 0)
+return NULL;
+
 if (pretty)
 flags |= JSON_INDENT(2);
 else
diff --git a/src/util/virjsoncompat.c b/src/util/virjsoncompat.c
new file mode 100644
index 00..6c853e9bb5
--- /dev/null
+++ b/src/util/virjsoncompat.c
@@ -0,0 +1,274 @@
+/*
+ * virjsoncompat.c: JSON object parsing/formatting
+ *
+ * Copyright (C) 2018 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Les

[libvirt] [PATCH] conf: rewrite filtering for capabilities lookup

[Daniel P . Berrangé]

The virCapabilitiesDomainDataLookupInternal() is given a list of
parameters representing the desired domain characteristics. It then has
to look throught the capabilities to identify an acceptable match.

The virCapsDomainDataCompare() method is used for filtering out
candidates which don't match the desired criteria. It is called
primarily from the innermost loops and as such is doing many repeated
checks. For example if architcture and os type were checked at the top
level loop the two inner loops could be avoided entirely. If emulator
and domain type were checked in the 2nd level loop the 3rd level loop
can be avoided too.

This change thus removes the virCapsDomainDataCompare() method and puts
suitable checks at the start of each loop to ensure it executes the
minimal number of loop iterations possible. The code becomes clearer to
understand as a nice side-effect.

Signed-off-by: Daniel P. Berrangé 
---
 src/conf/capabilities.c | 100 ++--
 1 file changed, 45 insertions(+), 55 deletions(-)

diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index 0f96500294..cfd5132329 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c
@@ -604,46 +604,6 @@ 
virCapabilitiesHostSecModelAddBaseLabel(virCapsHostSecModelPtr secmodel,
 return -1;
 }
 
-static bool
-virCapsDomainDataCompare(virCapsGuestPtr guest,
- virCapsGuestDomainPtr domain,
- virCapsGuestMachinePtr machine,
- int ostype,
- virArch arch,
- virDomainVirtType domaintype,
- const char *emulator,
- const char *machinetype)
-{
-const char *check_emulator = NULL;
-
-if (ostype != -1 && guest->ostype != ostype)
-return false;
-if ((arch != VIR_ARCH_NONE) && (guest->arch.id != arch))
-return false;
-
-if (domaintype != VIR_DOMAIN_VIRT_NONE &&
-(!domain || domain->type != domaintype))
-return false;
-
-if (emulator) {
-if (domain)
-check_emulator = domain->info.emulator;
-if (!check_emulator)
-check_emulator = guest->arch.defaultInfo.emulator;
-if (STRNEQ_NULLABLE(check_emulator, emulator))
-return false;
-}
-
-if (machinetype) {
-if (!machine)
-return false;
-if (STRNEQ(machine->name, machinetype) &&
-(STRNEQ_NULLABLE(machine->canonical, machinetype)))
-return false;
-}
-
-return true;
-}
 
 static virCapsDomainDataPtr
 virCapabilitiesDomainDataLookupInternal(virCapsPtr caps,
@@ -659,13 +619,45 @@ virCapabilitiesDomainDataLookupInternal(virCapsPtr caps,
 virCapsDomainDataPtr ret = NULL;
 size_t i, j, k;
 
+VIR_DEBUG("Lookup ostype=%d arch=%d domaintype=%d emulator=%s machine=%s",
+  ostype, arch, domaintype, NULLSTR(emulator), 
NULLSTR(machinetype));
 for (i = 0; i < caps->nguests; i++) {
 virCapsGuestPtr guest = caps->guests[i];
 
+if (ostype != -1 && guest->ostype != ostype) {
+VIR_DEBUG("Skip os type want=%d vs got=%d", ostype, guest->ostype);
+continue;
+}
+VIR_DEBUG("Match os type %d", ostype);
+
+if ((arch != VIR_ARCH_NONE) && (guest->arch.id != arch)) {
+VIR_DEBUG("Skip arch want=%d vs got=%d", arch, guest->arch.id);
+continue;
+}
+VIR_DEBUG("Match arch %d", arch);
+
 for (j = 0; j < guest->arch.ndomains; j++) {
 virCapsGuestDomainPtr domain = guest->arch.domains[j];
 virCapsGuestMachinePtr *machinelist;
 int nmachines;
+const char *check_emulator = NULL;
+
+if (domaintype != VIR_DOMAIN_VIRT_NONE &&
+(domain->type != domaintype)) {
+VIR_DEBUG("Skip domain type want=%d vs got=%d", domaintype, 
domain->type);
+continue;
+}
+VIR_DEBUG("Match domain type %d", domaintype);
+
+check_emulator = domain->info.emulator;
+if (!check_emulator)
+check_emulator = guest->arch.defaultInfo.emulator;
+if (emulator && STRNEQ_NULLABLE(check_emulator, emulator)) {
+VIR_DEBUG("Skip emulator got=%s vs want=%s",
+  emulator, NULLSTR(check_emulator));
+continue;
+}
+VIR_DEBUG("Match emulator %s", NULLSTR(emulator));
 
 if (domain->info.nmachines) {
 nmachines = domain->info.nmachines;
@@ -677,32 +669,29 @@ virCapabilitiesDomainDataLookupInternal(virCapsPtr caps,
 
 for (k = 0; k < 

Re: [libvirt] [PATCH] util: virrandom: make virRandomBits endian-safe

[Daniel P . Berrangé]

On Thu, Aug 02, 2018 at 01:12:02PM +0200, Pino Toscano wrote:
> On Thursday, 2 August 2018 12:28:45 CEST Daniel P. Berrangé wrote:
> > On Thu, Aug 02, 2018 at 10:17:32AM +0200, Bjoern Walk wrote:
> > > Pino Toscano  [2018-08-02, 10:02AM +0200]:
> > > > I do not think this patch is correct: we are dealing with random bytes,
> > > > so there is no "endianness" for them.
> > > 
> > > Well, it's not incorrect either, isn't it? I agree that endianness
> > > doesn't matter for random data, but in the same time, it doesn't hurt to
> > > have the same random data generated on big- and little-endian machines.
> 
> Not sure I understand -- since it's random data, you cannot have it
> "the same", no matter which endianness the machine has.
> 
> > > And it gives an easy and future-proof fix for the mocked tests.
> 
> IMHO every mocked test has its own behaviour, and thus the mocking
> needs to reflect that.
> 
> > Lets just modijfy tests/virrandommock.c to add mocking of virRandomBits
> > alongside virRandomBytes.
> 
> I don't see how it will help, since all virRandomBits does is calling
> virRandomBytes.

That's exactly what it would fix. A virRandomBits mock would simply
return a fixed number and not call the mocked virRandomBytes at all
thus avoiding the endianness problem

> I still did not see any complaints about my patch to fix viriscsitest
> (since the problem is specific for it), what about ACKing it then?

The virrandommock.c was intended to provide "stable" random numbers across
multiple tests. It didn't mock virRandomBits because it was mistakenly
assumed that mocking virRandomBytes was enough. Changing virscsitest just
fixes the one occurrance that happens to hit today - we want to fix the
general problem as virrandommock.c intended.


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

Re: [libvirt] [PATCH] tests: mock virRandomBits to make it endian stable

[Daniel P . Berrangé]

On Thu, Aug 02, 2018 at 08:17:25AM -0500, Eric Blake wrote:
> On 08/02/2018 06:37 AM, Daniel P. Berrangé wrote:
> > virRandomBits is implemented in terms of virRandomBytes. Although we
> > mock virRandomBytes to give a stable value, this is not sufficient to
> > make virRandomBits give a stable value. The result of virRandomBits will
> > vary depending on endianness. Thus we mock virRandomBits to return a
> > stable value directly.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> 
> > +uint64_t virRandomBits(int nbits)
> > +{
> > +/* Chosen by a fair roll of a 2^64 sided dice */
> 
> Where do I get my hands on one of those?  :)
> 
> Reviewed-by: Eric Blake 

Thanks, this is now pushed


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

[libvirt] [PATCH] qemu: ensure "pc" machine is always used as default if available

[Daniel P . Berrangé]

It is increasingly likely that some distro is going to change the
default "x86" machine type in QEMU from "pc" to "q35". This will
certainly break existing applications which write their XML on the
assumption that its using a "pc" machine by default. For example they'll
lack a IDE CDROM and get PCI-X instad of PCI which changes the topology
radically.

Libvirt promises to isolate applications from hypervisor changes that
may cause incompatibilities, so we must ensure that we always use the
"pc" machine type if it is available. Only use QEMU's own reported
default machine type if "pc" does not exist.

Note this change assumes there will always be a "pc" alias as long as a
versioned "pc-XXX" machine type exists. If QEMU were to ship a "pc-XXX"
machine type but not provide the "pc" alias, it is too hard to decide
which to default so. Versioned machine types are supposed to be
considered opaque strings, so we can't apply any sensible ordering
ourselves and QEMU isn't reporting the list of machines in any sensible
ordering itself.

Signed-off-by: Daniel P. Berrangé 
---
 src/qemu/qemu_capabilities.c | 19 +--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 0fb800589a..9eb58afef3 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2242,6 +2242,17 @@ virQEMUCapsProbeQMPMachineTypes(virQEMUCapsPtr qemuCaps,
 int ret = -1;
 size_t i;
 size_t defIdx = 0;
+ssize_t preferredIdx = -1;
+const char *preferredAlias = NULL;
+
+/* Historically QEMU defaulted to 'pc' machine type but in
+ * future might switch 'q35'. Such a change is considered
+ * an ABI break from lbivirt's POV, so we must be sure to
+ * keep 'pc' as default machine no matter what QEMU says.
+ */
+if (qemuCaps->arch == VIR_ARCH_X86_64 ||
+qemuCaps->arch == VIR_ARCH_I686)
+preferredAlias = "pc";
 
 if ((nmachines = qemuMonitorGetMachines(mon, )) < 0)
 return -1;
@@ -2263,12 +2274,16 @@ virQEMUCapsProbeQMPMachineTypes(virQEMUCapsPtr qemuCaps,
 mach->maxCpus = machines[i]->maxCpus;
 mach->hotplugCpus = machines[i]->hotplugCpus;
 
+if (STREQ_NULLABLE(mach->alias, preferredAlias))
+preferredIdx = qemuCaps->nmachineTypes - 1;
+
 if (machines[i]->isDefault)
 defIdx = qemuCaps->nmachineTypes - 1;
 }
 
-if (defIdx)
-virQEMUCapsSetDefaultMachine(qemuCaps, defIdx);
+if (preferredIdx == -1)
+preferredIdx = defIdx;
+virQEMUCapsSetDefaultMachine(qemuCaps, preferredIdx);
 
 ret = 0;
 
-- 
2.17.1

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

Re: [libvirt] Availability of libvirt-4.6.0 Release Candidate 2

[Daniel P . Berrangé]

On Wed, Aug 01, 2018 at 01:41:48PM +0200, Bjoern Walk wrote:
> Daniel P. Berrangé  [2018-08-01, 11:51AM +0100]:
> > On Tue, Jul 31, 2018 at 03:54:43PM +0200, Bjoern Walk wrote:
> > > Bjoern Walk  [2018-07-31, 03:16PM +0200]:
> > > > I have not yet had the time to figure out what goes wrong, any ideas 
> > > > are welcome.
> > > 
> > > Ah, classic. The mocked virRandomBytes function is not endian-agnostic,
> > > generating a different interface name as expected by the test.
> > 
> > I'm not seeing why virRandomBytes is affected by endian-ness. It is simply
> > populating an array of bytes, so there's no endin issues to consider there.
> 
> Ahem, we are writing linearly to a byte array, of course this is
> dependend on the endianness of the architecture. The actual problem is
> that the mocked version does _not_ provide a random value, but a
> deterministic one, which is byte order reversed on big endianness.

There's no concept of reversed byte order when you're accessing an
array of bytes.  If you write elements 0, 1, 2, ...7, etc and then
the caller reads elements 0, 1,2, ..., 7 everything is fine.

Endianness only comes into play if you take that array of bytes
and then cast/assign it to a larger type were endianess is
relevant (eg int16/int32/int64)

eg

   char bytes[8];

   virRandomBytes(bytes, 8);

   uint64_t val = (uint64_t)bytes;

the problem in such a case isn't virRandomBytes, it is the cast
to the larger sized integer type.

> > Can you elaborate on the actual error messages you are getting from the
> > tests, and what aspect makes you think virRandomBytes is the problem ?
> 
> The name of the interface is wrong compared to what is explicitly
> expected in the test case:
> 
> 200 if (virAsprintf(_ifacename,
> (gdb)
> 205 VIR_DEBUG("Attempting to create interface '%s' with IQN '%s'",
> (gdb) p temp_ifacename
> $1 = 0x1014320 "libvirt-iface-04050607"
> 
> > Seems more likely that it is something higher up the call stack.

That looks like it uses virRandomBits rather than virRandomBytes.


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

Re: [libvirt] [PATCH] util: virrandom: make virRandomBits endian-safe

[Daniel P . Berrangé]

On Thu, Aug 02, 2018 at 10:17:32AM +0200, Bjoern Walk wrote:
> Pino Toscano  [2018-08-02, 10:02AM +0200]:
> > I do not think this patch is correct: we are dealing with random bytes,
> > so there is no "endianness" for them.
> 
> Well, it's not incorrect either, isn't it? I agree that endianness
> doesn't matter for random data, but in the same time, it doesn't hurt to
> have the same random data generated on big- and little-endian machines.
> And it gives an easy and future-proof fix for the mocked tests.

Lets just modijfy tests/virrandommock.c to add mocking of virRandomBits
alongside virRandomBytes.

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

[libvirt] [PATCH 3/4] cpu: split PPC64 map data into separate files

[Daniel P . Berrangé]

Signed-off-by: Daniel P. Berrangé 
---
 src/cpu/cpu_map.xml | 41 +
 src/cpu/cpu_map_ppc64_POWER6.xml|  6 
 src/cpu/cpu_map_ppc64_POWER7.xml|  7 +
 src/cpu/cpu_map_ppc64_POWER8.xml|  8 +
 src/cpu/cpu_map_ppc64_POWER9.xml|  6 
 src/cpu/cpu_map_ppc64_POWERPC_e5500.xml |  6 
 src/cpu/cpu_map_ppc64_POWERPC_e6500.xml |  6 
 src/cpu/cpu_map_ppc64_vendors.xml   |  4 +++
 8 files changed, 50 insertions(+), 34 deletions(-)
 create mode 100644 src/cpu/cpu_map_ppc64_POWER6.xml
 create mode 100644 src/cpu/cpu_map_ppc64_POWER7.xml
 create mode 100644 src/cpu/cpu_map_ppc64_POWER8.xml
 create mode 100644 src/cpu/cpu_map_ppc64_POWER9.xml
 create mode 100644 src/cpu/cpu_map_ppc64_POWERPC_e5500.xml
 create mode 100644 src/cpu/cpu_map_ppc64_POWERPC_e6500.xml
 create mode 100644 src/cpu/cpu_map_ppc64_vendors.xml

diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml
index 9af190a579..e236c41733 100644
--- a/src/cpu/cpu_map.xml
+++ b/src/cpu/cpu_map.xml
@@ -2340,43 +2340,16 @@
   
 
   
-
-
-
+
 
 
-
-  
-  
-
-
-
-  
-  
-  
-
-
-
-  
-  
-  
-  
-
-
-
-  
-  
-
+
+
+
+
 
 
-
-  
-  
-
-
-
-  
-  
-
+
+
   
 
diff --git a/src/cpu/cpu_map_ppc64_POWER6.xml b/src/cpu/cpu_map_ppc64_POWER6.xml
new file mode 100644
index 00..00e27495f4
--- /dev/null
+++ b/src/cpu/cpu_map_ppc64_POWER6.xml
@@ -0,0 +1,6 @@
+
+  
+
+
+  
+
diff --git a/src/cpu/cpu_map_ppc64_POWER7.xml b/src/cpu/cpu_map_ppc64_POWER7.xml
new file mode 100644
index 00..a071481805
--- /dev/null
+++ b/src/cpu/cpu_map_ppc64_POWER7.xml
@@ -0,0 +1,7 @@
+
+  
+
+
+
+  
+
diff --git a/src/cpu/cpu_map_ppc64_POWER8.xml b/src/cpu/cpu_map_ppc64_POWER8.xml
new file mode 100644
index 00..64d96fc4c4
--- /dev/null
+++ b/src/cpu/cpu_map_ppc64_POWER8.xml
@@ -0,0 +1,8 @@
+
+  
+
+
+
+
+  
+
diff --git a/src/cpu/cpu_map_ppc64_POWER9.xml b/src/cpu/cpu_map_ppc64_POWER9.xml
new file mode 100644
index 00..149fcde924
--- /dev/null
+++ b/src/cpu/cpu_map_ppc64_POWER9.xml
@@ -0,0 +1,6 @@
+
+  
+
+
+  
+
diff --git a/src/cpu/cpu_map_ppc64_POWERPC_e5500.xml 
b/src/cpu/cpu_map_ppc64_POWERPC_e5500.xml
new file mode 100644
index 00..3d64c8926c
--- /dev/null
+++ b/src/cpu/cpu_map_ppc64_POWERPC_e5500.xml
@@ -0,0 +1,6 @@
+
+  
+
+
+  
+
diff --git a/src/cpu/cpu_map_ppc64_POWERPC_e6500.xml 
b/src/cpu/cpu_map_ppc64_POWERPC_e6500.xml
new file mode 100644
index 00..b0d1006076
--- /dev/null
+++ b/src/cpu/cpu_map_ppc64_POWERPC_e6500.xml
@@ -0,0 +1,6 @@
+
+  
+
+  
+  
+
diff --git a/src/cpu/cpu_map_ppc64_vendors.xml 
b/src/cpu/cpu_map_ppc64_vendors.xml
new file mode 100644
index 00..52ad45c0bd
--- /dev/null
+++ b/src/cpu/cpu_map_ppc64_vendors.xml
@@ -0,0 +1,4 @@
+
+  
+  
+
-- 
2.17.1

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

[libvirt] [PATCH 0/4] cpu: modularize the CPU map data file

[Daniel P . Berrangé]

Currently we have a cpu_map.xml file that contains all the features and
CPU models for all architectures in one place. I frequently find myself
wondering about the differences between CPU models, but it is hard to
compare them as the list of features is huge.

With this patch series we end up with a large set of small files, one
per named CPU model, along with one for the feature and vendor
definitions

   cpu_map_ppc64_POWER6.xml
   cpu_map_ppc64_POWER7.xml
   cpu_map_ppc64_POWER8.xml
   cpu_map_ppc64_POWER9.xml
   cpu_map_ppc64_POWERPC_e5500.xml
   cpu_map_ppc64_POWERPC_e6500.xml
   cpu_map_ppc64_vendors.xml
   cpu_map_x86_486.xml
   cpu_map_x86_athlon.xml
   cpu_map_x86_Broadwell-IBRS.xml
   cpu_map_x86_Broadwell-noTSX-IBRS.xml
   cpu_map_x86_Broadwell-noTSX.xml
   cpu_map_x86_Broadwell.xml
   cpu_map_x86_Conroe.xml
   cpu_map_x86_core2duo.xml
   cpu_map_x86_coreduo.xml
   cpu_map_x86_cpu64-rhel5.xml
   cpu_map_x86_cpu64-rhel6.xml
   cpu_map_x86_EPYC-IBRS.xml
   cpu_map_x86_EPYC.xml
   cpu_map_x86_features.xml
   cpu_map_x86_Haswell-IBRS.xml
   cpu_map_x86_Haswell-noTSX-IBRS.xml
   cpu_map_x86_Haswell-noTSX.xml
   cpu_map_x86_Haswell.xml
   cpu_map_x86_IvyBridge-IBRS.xml
   cpu_map_x86_IvyBridge.xml
   cpu_map_x86_kvm32.xml
   cpu_map_x86_kvm64.xml
   cpu_map_x86_n270.xml
   cpu_map_x86_Nehalem-IBRS.xml
   cpu_map_x86_Nehalem.xml
   cpu_map_x86_Opteron_G1.xml
   cpu_map_x86_Opteron_G2.xml
   cpu_map_x86_Opteron_G3.xml
   cpu_map_x86_Opteron_G4.xml
   cpu_map_x86_Opteron_G5.xml
   cpu_map_x86_Penryn.xml
   cpu_map_x86_pentium2.xml
   cpu_map_x86_pentium3.xml
   cpu_map_x86_pentiumpro.xml
   cpu_map_x86_pentium.xml
   cpu_map_x86_phenom.xml
   cpu_map_x86_qemu32.xml
   cpu_map_x86_qemu64.xml
   cpu_map_x86_SandyBridge-IBRS.xml
   cpu_map_x86_SandyBridge.xml
   cpu_map_x86_Skylake-Client-IBRS.xml
   cpu_map_x86_Skylake-Client.xml
   cpu_map_x86_Skylake-Server-IBRS.xml
   cpu_map_x86_Skylake-Server.xml
   cpu_map_x86_vendors.xml
   cpu_map_x86_Westmere-IBRS.xml
   cpu_map_x86_Westmere.xml

The main cpu_map.xml file is now just a list of 
statements to pull in the individual files

Now we can easily see the differences in each model:

$ diff cpu_map_x86_Broadwell.xml cpu_map_x86_Skylake-Client.xml
2,3c2,3
<   
< 
---
>   
> 
5a6
> 
8a10
> 
18a21
> 
30a34
> 
42a47
> 
56a62
> 
57a64
> 
    58a66,67
> 
> 

Daniel P. Berrangé (4):
  cpu: allow include files for CPU definition
  cpu: push more parsing logic into common code
  cpu: split PPC64 map data into separate files
  cpu: split x86 map data into separate files

 libvirt.spec.in  |2 +-
 mingw-libvirt.spec.in|4 +-
 src/Makefile.am  |2 +-
 src/cpu/cpu_map.c|  160 +-
 src/cpu/cpu_map.h|   22 +-
 src/cpu/cpu_map.xml  | 2415 +-
 src/cpu/cpu_map_ppc64_POWER6.xml |6 +
 src/cpu/cpu_map_ppc64_POWER7.xml |7 +
 src/cpu/cpu_map_ppc64_POWER8.xml |8 +
 src/cpu/cpu_map_ppc64_POWER9.xml |6 +
 src/cpu/cpu_map_ppc64_POWERPC_e5500.xml  |6 +
 src/cpu/cpu_map_ppc64_POWERPC_e6500.xml  |6 +
 src/cpu/cpu_map_ppc64_vendors.xml|4 +
 src/cpu/cpu_map_x86_486.xml  |7 +
 src/cpu/cpu_map_x86_Broadwell-IBRS.xml   |   61 +
 src/cpu/cpu_map_x86_Broadwell-noTSX-IBRS.xml |   59 +
 src/cpu/cpu_map_x86_Broadwell-noTSX.xml  |   58 +
 src/cpu/cpu_map_x86_Broadwell.xml|   60 +
 src/cpu/cpu_map_x86_Conroe.xml   |   33 +
 src/cpu/cpu_map_x86_EPYC-IBRS.xml|   73 +
 src/cpu/cpu_map_x86_EPYC.xml |   72 +
 src/cpu/cpu_map_x86_Haswell-IBRS.xml |   57 +
 src/cpu/cpu_map_x86_Haswell-noTSX-IBRS.xml   |   55 +
 src/cpu/cpu_map_x86_Haswell-noTSX.xml|   54 +
 src/cpu/cpu_map_x86_Haswell.xml  |   56 +
 src/cpu/cpu_map_x86_IvyBridge-IBRS.xml   |   51 +
 src/cpu/cpu_map_x86_IvyBridge.xml|   50 +
 src/cpu/cpu_map_x86_Nehalem-IBRS.xml |   38 +
 src/cpu/cpu_map_x86_Nehalem.xml  |   37 +
 src/cpu/cpu_map_x86_Opteron_G1.xml   |   31 +
 src/cpu/cpu_map_x86_Opteron_G2.xml   |   35 +
 src/cpu/cpu_map_x86_Opteron_G3.xml   |   40 +
 src/cpu/cpu_map_x86_Opteron_G4.xml   |   50 +
 src/cpu/cpu_map_x86_Opteron_G5.xml   |   53 +
 src/cpu/cpu_map_x86_Penryn.xml   |   35 +
 src/cpu/cpu_map_x86_SandyBridge-IBRS.xml |   45 +
 src/cpu/cpu_map_x86_SandyBridge.xml  |   44 +
 src/cpu/cpu_map_x86_Skylake-Client-IBRS.xml  |   70 +
 src/cpu/cpu_map_x86_Skylake-Client.xml   |   69 +
 src/cpu/cpu_map_x86_Skylake-Server-IBRS.xml  |   77 +
 src/cpu/cpu_map_x86_Skylake-Server.xml  

[libvirt] [PATCH 1/4] cpu: allow include files for CPU definition

[Daniel P . Berrangé]

Allow for syntax



to reference other files in the CPU database directory

Signed-off-by: Daniel P. Berrangé 
---
 libvirt.spec.in   |  2 +-
 mingw-libvirt.spec.in |  4 +--
 src/Makefile.am   |  2 +-
 src/cpu/cpu_map.c | 84 +--
 4 files changed, 86 insertions(+), 6 deletions(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 19ae55cdaf..b6745dbffa 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1856,7 +1856,7 @@ exit 0
 %{_datadir}/libvirt/schemas/storagepool.rng
 %{_datadir}/libvirt/schemas/storagevol.rng
 
-%{_datadir}/libvirt/cpu_map.xml
+%{_datadir}/libvirt/cpu_map*.xml
 
 %{_datadir}/libvirt/test-screenshot.png
 
diff --git a/mingw-libvirt.spec.in b/mingw-libvirt.spec.in
index cc1e619927..22fe7a000f 100644
--- a/mingw-libvirt.spec.in
+++ b/mingw-libvirt.spec.in
@@ -260,7 +260,7 @@ rm -rf 
$RPM_BUILD_ROOT%{mingw64_libexecdir}/libvirt-guests.sh
 %{mingw32_datadir}/libvirt/api/libvirt-qemu-api.xml
 %{mingw32_datadir}/libvirt/api/libvirt-admin-api.xml
 
-%{mingw32_datadir}/libvirt/cpu_map.xml
+%{mingw32_datadir}/libvirt/cpu_map*.xml
 
 %{mingw32_datadir}/libvirt/test-screenshot.png
 
@@ -347,7 +347,7 @@ rm -rf 
$RPM_BUILD_ROOT%{mingw64_libexecdir}/libvirt-guests.sh
 %{mingw64_datadir}/libvirt/api/libvirt-qemu-api.xml
 %{mingw64_datadir}/libvirt/api/libvirt-admin-api.xml
 
-%{mingw64_datadir}/libvirt/cpu_map.xml
+%{mingw64_datadir}/libvirt/cpu_map*.xml
 
 %{mingw64_datadir}/libvirt/test-screenshot.png
 
diff --git a/src/Makefile.am b/src/Makefile.am
index a4f213480e..11a7ac81e2 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -366,7 +366,7 @@ check-local: check-protocol check-symfile check-symsorting \
 
 
 
-pkgdata_DATA = cpu/cpu_map.xml
+pkgdata_DATA = $(wildcard $(srcdir)/cpu/cpu_map*.xml)
 
 EXTRA_DIST +=  $(pkgdata_DATA)
 
diff --git a/src/cpu/cpu_map.c b/src/cpu/cpu_map.c
index d263eb8cdd..9e090919ed 100644
--- a/src/cpu/cpu_map.c
+++ b/src/cpu/cpu_map.c
@@ -70,6 +70,83 @@ static int load(xmlXPathContextPtr ctxt,
 return ret;
 }
 
+static int
+cpuMapLoadInclude(const char *filename,
+  cpuMapLoadCallback cb,
+  void *data)
+{
+xmlDocPtr xml = NULL;
+xmlXPathContextPtr ctxt = NULL;
+int ret = -1;
+int element;
+char *mapfile;
+
+if (!(mapfile = virFileFindResource(filename,
+abs_topsrcdir "/src/cpu",
+PKGDATADIR)))
+return -1;
+
+VIR_DEBUG("Loading CPU map include from %s", mapfile);
+
+if (!(xml = virXMLParseFileCtxt(mapfile, )))
+goto cleanup;
+
+ctxt->node = xmlDocGetRootElement(xml);
+
+for (element = 0; element < CPU_MAP_ELEMENT_LAST; element++) {
+if (load(ctxt, element, cb, data) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("cannot parse CPU map '%s'"), mapfile);
+goto cleanup;
+}
+}
+
+ret = 0;
+
+ cleanup:
+xmlXPathFreeContext(ctxt);
+xmlFreeDoc(xml);
+VIR_FREE(mapfile);
+
+return ret;
+}
+
+
+static int loadIncludes(xmlXPathContextPtr ctxt,
+cpuMapLoadCallback callback,
+void *data)
+{
+int ret = -1;
+xmlNodePtr ctxt_node;
+xmlNodePtr *nodes = NULL;
+int n;
+size_t i;
+
+ctxt_node = ctxt->node;
+
+n = virXPathNodeSet("include", ctxt, );
+if (n < 0)
+goto cleanup;
+
+for (i = 0; i < n; i++) {
+char *filename = virXMLPropString(nodes[i], "filename");
+VIR_DEBUG("Finding CPU map include '%s'", filename);
+if (cpuMapLoadInclude(filename, callback, data) < 0) {
+VIR_FREE(filename);
+goto cleanup;
+}
+VIR_FREE(filename);
+}
+
+ret = 0;
+
+ cleanup:
+ctxt->node = ctxt_node;
+VIR_FREE(nodes);
+
+return ret;
+}
+
 
 int cpuMapLoad(const char *arch,
cpuMapLoadCallback cb,
@@ -88,7 +165,7 @@ int cpuMapLoad(const char *arch,
 PKGDATADIR)))
 return -1;
 
-VIR_DEBUG("Loading CPU map from %s", mapfile);
+VIR_DEBUG("Loading '%s' CPU map from %s", arch, mapfile);
 
 if (arch == NULL) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -122,11 +199,14 @@ int cpuMapLoad(const char *arch,
 for (element = 0; element < CPU_MAP_ELEMENT_LAST; element++) {
 if (load(ctxt, element, cb, data) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("cannot parse CPU map for %s architecture"), 
arch);
+   _("cannot parse CPU map '%s'"), mapfile);
 goto cleanup;
 }
 }
 
+if (loadIncludes(ctxt, cb, data) < 0)
+goto cleanup;
+
 ret = 0;
 
  cleanup:
-- 
2.17.1

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

Re: [libvirt] [PATCH] tests: viriscsitest: make it work on big endian archs

[Daniel P . Berrangé]

On Wed, Aug 01, 2018 at 03:08:07PM +0200, Pino Toscano wrote:
> viriscsitest tries to ensure the interface IQN used is a specific one,
> checking later on that it is the same all during the whole test.  Since
> the IQN generation involves random bytes, viriscsitest got a fake
> virRandomBytes from the virrandommock helper library, setting static
> values.  virRandomBits(), called by virStorageBackendCreateIfaceIQN(),
> always requests 8 random bytes, chopping off the ones not requested by
> the caller -- this meant that on big endian machines it would chop bytes
> from the wrong size of the data buffer, and thus not returning the
> expected numbers.
> 
> As a fix, do not rely on the mock virRandomBytes, but provide an own
> version of it: this version will fill the values in the expected order,
> depending on the endianness of the system.  This way, the result of
> virStorageBackendCreateIfaceIQN() will be what the test actually
> expects.
> 
> Signed-off-by: Pino Toscano 

Mailman is having deliver problems so you might not have received
the patch that was sent earlier for this problem:

https://www.redhat.com/archives/libvir-list/2018-August/msg00039.html

I prefer that approach since it avoids the endianness problems for
all future usage too.


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

[libvirt] [PATCH] tests: mock virRandomBits to make it endian stable

[Daniel P . Berrangé]

virRandomBits is implemented in terms of virRandomBytes. Although we
mock virRandomBytes to give a stable value, this is not sufficient to
make virRandomBits give a stable value. The result of virRandomBits will
vary depending on endianness. Thus we mock virRandomBits to return a
stable value directly.

Signed-off-by: Daniel P. Berrangé 
---
 tests/virrandommock.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/tests/virrandommock.c b/tests/virrandommock.c
index 99a55a576a..3079b8bacb 100644
--- a/tests/virrandommock.c
+++ b/tests/virrandommock.c
@@ -44,6 +44,14 @@ virRandomBytes(unsigned char *buf,
 return 0;
 }
 
+uint64_t virRandomBits(int nbits)
+{
+/* Chosen by a fair roll of a 2^64 sided dice */
+uint64_t ret = 0x0706050403020100;
+if (nbits < 64)
+ret &= ((1ULL << nbits) - 1);
+return ret;
+}
 
 int virRandomGenerateWWN(char **wwn,
  const char *virt_type ATTRIBUTE_UNUSED)
-- 
2.17.1

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

Re: [libvirt] [PATCH for v4.6.0] cgroups: Don't leave stale pointers around after virCgroupFree

[Daniel P . Berrangé]

On Mon, Jul 30, 2018 at 10:25:05AM +0200, Michal Privoznik wrote:
> One of the attributes that original virCgroupFree() had was it
> set passed pointer to NULL. For instance in the following code
> the latter call would be practically a no-op:
> 
>   virCgroupFree();
>   virCgroupFree();
> 
> However, this behaviour of the function was changed in
> 0f80c71822d824 but corresponding 'var = NULL' lines were not
> added leading to double free:

Sigh, can we please just revert that change. It is going in completely the
oppposite of what we should be doing. We want to change more functions to
take a ptr to a ptr, precisely because it avoids this double-free problem.

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

Re: [libvirt] [PATCH for v4.6.0] cgroups: Don't leave stale pointers around after virCgroupFree

[Daniel P . Berrangé]

On Mon, Jul 30, 2018 at 09:48:51AM +0100, Daniel P. Berrangé wrote:
> On Mon, Jul 30, 2018 at 10:25:05AM +0200, Michal Privoznik wrote:
> > One of the attributes that original virCgroupFree() had was it
> > set passed pointer to NULL. For instance in the following code
> > the latter call would be practically a no-op:
> > 
> >   virCgroupFree();
> >   virCgroupFree();
> > 
> > However, this behaviour of the function was changed in
> > 0f80c71822d824 but corresponding 'var = NULL' lines were not
> > added leading to double free:
> 
> Sigh, can we please just revert that change. It is going in completely the
> oppposite of what we should be doing. We want to change more functions to
> take a ptr to a ptr, precisely because it avoids this double-free problem.

Even more crazy, this change was done so that VIR_DEFINE_AUTOPTR_FUNC()
could be used to define a free function which takes a ptr to a ptr.

Both of these changes should be reverted, as the previously existing
virCgroupFree should be used as the attribute((cleanup)) function directly
with no wrapper created.

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

[libvirt] [jenkins-ci PATCH] hosts_vars: drop virt-viewer from platforms with old spice-gtk

[Daniel P . Berrangé]

git master requires spice-gtk >= 0.35

Signed-off-by: Daniel P. Berrangé 
---
 guests/host_vars/libvirt-centos-7/main.yml| 1 -
 guests/host_vars/libvirt-debian-8/main.yml| 1 -
 guests/host_vars/libvirt-debian-9/main.yml| 1 -
 guests/host_vars/libvirt-debian-sid/main.yml  | 1 -
 guests/host_vars/libvirt-fedora-27/main.yml   | 1 -
 guests/host_vars/libvirt-freebsd-10/main.yml  | 1 -
 guests/host_vars/libvirt-freebsd-11/main.yml  | 1 -
 guests/host_vars/libvirt-freebsd-current/main.yml | 1 -
 guests/host_vars/libvirt-ubuntu-14/main.yml   | 1 -
 guests/host_vars/libvirt-ubuntu-16/main.yml   | 1 -
 guests/host_vars/libvirt-ubuntu-18/main.yml   | 1 -
 11 files changed, 11 deletions(-)

diff --git a/guests/host_vars/libvirt-centos-7/main.yml 
b/guests/host_vars/libvirt-centos-7/main.yml
index c147830..19e905e 100644
--- a/guests/host_vars/libvirt-centos-7/main.yml
+++ b/guests/host_vars/libvirt-centos-7/main.yml
@@ -12,4 +12,3 @@ projects:
   - libvirt-sandbox
   - osinfo-db
   - osinfo-db-tools
-  - virt-viewer
diff --git a/guests/host_vars/libvirt-debian-8/main.yml 
b/guests/host_vars/libvirt-debian-8/main.yml
index 954a037..62abfd3 100644
--- a/guests/host_vars/libvirt-debian-8/main.yml
+++ b/guests/host_vars/libvirt-debian-8/main.yml
@@ -10,4 +10,3 @@ projects:
   - libvirt-tck
   - osinfo-db
   - osinfo-db-tools
-  - virt-viewer
diff --git a/guests/host_vars/libvirt-debian-9/main.yml 
b/guests/host_vars/libvirt-debian-9/main.yml
index a588c09..032572f 100644
--- a/guests/host_vars/libvirt-debian-9/main.yml
+++ b/guests/host_vars/libvirt-debian-9/main.yml
@@ -13,4 +13,3 @@ projects:
   - osinfo-db
   - osinfo-db-tools
   - virt-manager
-  - virt-viewer
diff --git a/guests/host_vars/libvirt-debian-sid/main.yml 
b/guests/host_vars/libvirt-debian-sid/main.yml
index a588c09..032572f 100644
--- a/guests/host_vars/libvirt-debian-sid/main.yml
+++ b/guests/host_vars/libvirt-debian-sid/main.yml
@@ -13,4 +13,3 @@ projects:
   - osinfo-db
   - osinfo-db-tools
   - virt-manager
-  - virt-viewer
diff --git a/guests/host_vars/libvirt-fedora-27/main.yml 
b/guests/host_vars/libvirt-fedora-27/main.yml
index 00b0848..bc13420 100644
--- a/guests/host_vars/libvirt-fedora-27/main.yml
+++ b/guests/host_vars/libvirt-fedora-27/main.yml
@@ -14,4 +14,3 @@ projects:
   - osinfo-db
   - osinfo-db-tools
   - virt-manager
-  - virt-viewer
diff --git a/guests/host_vars/libvirt-freebsd-10/main.yml 
b/guests/host_vars/libvirt-freebsd-10/main.yml
index 2ad4584..903dd45 100644
--- a/guests/host_vars/libvirt-freebsd-10/main.yml
+++ b/guests/host_vars/libvirt-freebsd-10/main.yml
@@ -14,4 +14,3 @@ projects:
   - osinfo-db
   - osinfo-db-tools
   - virt-manager
-  - virt-viewer
diff --git a/guests/host_vars/libvirt-freebsd-11/main.yml 
b/guests/host_vars/libvirt-freebsd-11/main.yml
index 2ad4584..903dd45 100644
--- a/guests/host_vars/libvirt-freebsd-11/main.yml
+++ b/guests/host_vars/libvirt-freebsd-11/main.yml
@@ -14,4 +14,3 @@ projects:
   - osinfo-db
   - osinfo-db-tools
   - virt-manager
-  - virt-viewer
diff --git a/guests/host_vars/libvirt-freebsd-current/main.yml 
b/guests/host_vars/libvirt-freebsd-current/main.yml
index 2ad4584..903dd45 100644
--- a/guests/host_vars/libvirt-freebsd-current/main.yml
+++ b/guests/host_vars/libvirt-freebsd-current/main.yml
@@ -14,4 +14,3 @@ projects:
   - osinfo-db
   - osinfo-db-tools
   - virt-manager
-  - virt-viewer
diff --git a/guests/host_vars/libvirt-ubuntu-14/main.yml 
b/guests/host_vars/libvirt-ubuntu-14/main.yml
index 839c668..8b59152 100644
--- a/guests/host_vars/libvirt-ubuntu-14/main.yml
+++ b/guests/host_vars/libvirt-ubuntu-14/main.yml
@@ -7,4 +7,3 @@ projects:
   - libvirt-tck
   - osinfo-db
   - osinfo-db-tools
-  - virt-viewer
diff --git a/guests/host_vars/libvirt-ubuntu-16/main.yml 
b/guests/host_vars/libvirt-ubuntu-16/main.yml
index a588c09..032572f 100644
--- a/guests/host_vars/libvirt-ubuntu-16/main.yml
+++ b/guests/host_vars/libvirt-ubuntu-16/main.yml
@@ -13,4 +13,3 @@ projects:
   - osinfo-db
   - osinfo-db-tools
   - virt-manager
-  - virt-viewer
diff --git a/guests/host_vars/libvirt-ubuntu-18/main.yml 
b/guests/host_vars/libvirt-ubuntu-18/main.yml
index a588c09..032572f 100644
--- a/guests/host_vars/libvirt-ubuntu-18/main.yml
+++ b/guests/host_vars/libvirt-ubuntu-18/main.yml
@@ -13,4 +13,3 @@ projects:
   - osinfo-db
   - osinfo-db-tools
   - virt-manager
-  - virt-viewer
-- 
2.17.1

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

[libvirt] [PATCH] conf: don't use virDomainVirtType in struct field

[Daniel P . Berrangé]

Use of enum types for struct fields is generally avoided since it causes
warnings if the compiler assumes the enum is unsigned. For example

  commit 8e2982b5767a25e5da6533c65bfdc648c95b3c69
  Author: Cole Robinson 
  Date:   Tue Jul 24 16:27:54 2018 -0400

conf: Clean up virDomainDefParseCaps

Introduced a line:

  if ((def->virtType = virDomainVirtTypeFromString(virttype)) < 0) {

which causes a build failure with CLang

  conf/domain_conf.c:19143:65: error: comparison of unsigned enum expression < 
0 is always false [-Werror,-Wtautological-compare]

as the compiler is free to optimize away the "< 0" check due to the
assumption that the enum type is unsigned and always in range.

Signed-off-by: Daniel P. Berrangé 
---
 src/conf/domain_conf.c  | 2 +-
 src/conf/domain_conf.h  | 2 +-
 src/qemu/qemu_command.c | 2 +-
 src/qemu/qemu_process.c | 4 +++-
 4 files changed, 6 insertions(+), 4 deletions(-)

Pushing as a build fix for CLang


diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c27c874d9e..f94a90fbcc 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15075,7 +15075,7 @@ virDomainVideoDefaultRAM(const virDomainDef *def,
 int
 virDomainVideoDefaultType(const virDomainDef *def)
 {
-switch (def->virtType) {
+switch ((virDomainVirtType)def->virtType) {
 case VIR_DOMAIN_VIRT_TEST:
 case VIR_DOMAIN_VIRT_XEN:
 if (def->os.type == VIR_DOMAIN_OSTYPE_XEN ||
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index a804e86f6c..c1dfa37fdf 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2386,7 +2386,7 @@ struct _virDomainVirtioOptions {
 typedef struct _virDomainDef virDomainDef;
 typedef virDomainDef *virDomainDefPtr;
 struct _virDomainDef {
-virDomainVirtType virtType;
+int virtType; /* enum virDomainVirtType */
 int id;
 unsigned char uuid[VIR_UUID_BUFLEN];
 
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ae45c45b7f..d148db90fa 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7163,7 +7163,7 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
 virCommandAddArg(cmd, "-machine");
 virBufferAdd(, def->os.machine, -1);
 
-switch (def->virtType) {
+switch ((virDomainVirtType)def->virtType) {
 case VIR_DOMAIN_VIRT_QEMU:
 virBufferAddLit(, ",accel=tcg");
 break;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 27bd8b9465..c4e33723d1 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -7191,6 +7191,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 virCapsPtr caps = NULL;
 bool active = false;
+virDomainVirtType virtType;
 
 VIR_DEBUG("Beginning VM attach process");
 
@@ -7342,8 +7343,9 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
 goto exit_monitor;
 if (qemuMonitorGetStatus(priv->mon, , ) < 0)
 goto exit_monitor;
-if (qemuMonitorGetVirtType(priv->mon, >def->virtType) < 0)
+if (qemuMonitorGetVirtType(priv->mon, ) < 0)
 goto exit_monitor;
+vm->def->virtType = virtType;
 if (qemuDomainObjExitMonitor(driver, vm) < 0)
 goto error;
 
-- 
2.17.1

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

Re: [libvirt] [PATCH] conf: fix virtType FromString check

[Daniel P . Berrangé]

On Thu, Jul 26, 2018 at 11:30:07AM -0400, Cole Robinson wrote:
> In the domain definition, virtType is virDomainVirtType which is
> unsigned, so comparing against -1 doesn't work. Cast to int first
> 
> Fixes 8e2982b5767
> 
> Signed-off-by: Cole Robinson 
> ---
> This regressed in the past as well, when virtType was changed to
> virDomainVirtType in 7383b8cc068, fixed by the follow up 5e06a4f063.
> It's strange that virDomainVirtType is unsigned but VirtTypeFromString
> can return -1... it should probably either work like virArch, or we
> should switch virDomainVirtType to int in the DomainDef, but that
> requires fixing a few other places too
> 
>  src/conf/domain_conf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index c27c874d9e..30806d1e59 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -19140,7 +19140,7 @@ virDomainDefParseCaps(virDomainDefPtr def,
> "%s", _("missing domain type attribute"));
>  goto cleanup;
>  }
> -if ((def->virtType = virDomainVirtTypeFromString(virttype)) < 0) {
> +if ((int)(def->virtType = virDomainVirtTypeFromString(virttype)) < 0) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("invalid domain type %s"), virttype);
>  goto cleanup;

I've already pushed the fix for this problem, changing "virtType"
back to an int.

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

Re: [libvirt] [RFC] secdrivers remembering original labels

[Daniel P . Berrangé]

On Mon, Aug 06, 2018 at 10:02:44AM +0200, Michal Prívozník wrote:
> On 07/27/2018 11:01 AM, Daniel P. Berrangé wrote:
> > On Fri, Jul 27, 2018 at 09:56:23AM +0200, Michal Privoznik wrote:
> >> Dear list,
> >>
> >> we have this very old bug [1] that I just keep pushing in front of me. I
> >> made several attempts to fix it. However, none of them made into the
> >> tree. I guess it's time to have discussion what to do about it. IIRC,
> >> the algorithm that I implemented last was to keep original label in
> >> XATTRs (among with some ref counter) and the last one to restore the
> >> label will look there and find the original label. There was a problem
> >> with two libvirtds fighting over a file on shared FS.
> > 
> > IIRC there was a second problem around security of XATTRs. eg, if we set
> > an XATTR it was possible for QEMU to change it once we given QEMU privs
> > on the image. Thus a malicious QEMU can cause libvirtd to change the
> > image to an arbitrary user on shutdown.
> > 
> > I think it was possible to deal with that on Linux, because the kernel
> > hsa some xattr namespacing that is reserved for only root. This protection
> > is worthless though when using NFS images as you can't trust the remote NFS
> > client to honour the same restriction.
> > 
> >> So I guess my question is can we come up with a design that would work?
> >> Or at least work to the extent that we're satisfied with?
> >>
> >> Personally, I like the XATTRs approach. And to resolve the NFS race we
> >> can invent yet another lockspace to guard labelling - I also have a bug
> >> for that [2] (although, I'm not that familiar with lockspaces). I guess
> >> doing disk metadata locking is not going to be trivial, is it?
> > 
> > Yeah, for sure we need two distinct locking regimes. Our current locking
> > aims to protect disk content, and the lifetime of the lock is the entire
> > duration of the QEMU process. For XATTRs, we only need write protection
> > for the short time window in which the security drivers execute, which
> > is at start, during hotplug/unplug, during shutdown, and finally migration.
> > 
> > I think we could achieve this with the virtlockd if we make it use the
> > same locking file, but a different byte offset within the file. Just
> > need to make sure it doesn't clash with the byte offset QEMU has chosen,
> > nor the current offset we use.
> 
> So do we really need virtlockd for this? I mean, the whole purpose of it
> is to hold locks so that libvirtd can restart independently, without
> losing the lock attached. However, since the metadata lock will be held
> only for a fraction of a second and will be not held through more than a
> single API aren't we just fine with libvirtd holding the lock? The way I
> imagine this to work is the following:

There were two reasons for virtlockd. The first was the persistent holding
of locks, but the other reasons is that POSIX fcntl() locks are horrific.
If one thread has an FD open with a lock held, if another thread opens and
closes the same underlying file, the first thread's lock will get dropped.

We can't be confident that other threads won't open the file, so the only
way to be safe was to put locking in to a separate process where we know
exactly what all threads are doing.


> Frankly, I've started implementing this with virtlockd already, but the
> changes I made so far simply do not feel right, e.g. I have to change
> lock_protocol.x so that virLockSpaceProtocolAcquireResourceArgs has this
> 'type' argument which tells virtlockd which byte we want to lock.

We could perhaps make use of the "flags" field, giving a flg to identify
the lockspace to use. This could be turned into an offset server side.

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

Re: [libvirt] [PATCH] virrandom: Avoid undefined behaviour in virRandomBits

[Daniel P . Berrangé]

On Thu, Aug 02, 2018 at 09:35:49AM +0200, Michal Privoznik wrote:
> If nbits is 64 (or greater) then shifting 1ULL left is undefined.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/util/virrandom.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/util/virrandom.c b/src/util/virrandom.c
> index 3c011a8615..7915f6531e 100644
> --- a/src/util/virrandom.c
> +++ b/src/util/virrandom.c
> @@ -68,7 +68,9 @@ uint64_t virRandomBits(int nbits)
>  return 0;
>  }
>  
> -ret &= (1ULL << nbits) - 1;
> +if (nbits < 64)
> +ret &= (1ULL << nbits) - 1;
> +
>  return ret;
>  }

Reviewed-by: Daniel P. Berrangé 

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

Re: [libvirt] [PATCH] remote: daemon: Make sure that JSON symbols are properly loaded at startup

[Daniel P . Berrangé]

On Wed, Aug 01, 2018 at 12:53:24PM +0200, Peter Krempa wrote:
> Explicitly call virJSONInitialize at startup of the libvirt daemon so
> that we are sure that the symbols in the compat library are properly
> loaded. This will prevent any random failure from happening later on
> when the daemon would want to use the JSON parser.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/libvirt_private.syms   | 4 
>  src/remote/remote_daemon.c | 4 
>  2 files changed, 8 insertions(+)

For sake of historical record

Reviewed-by: Daniel P. Berrangé 

but due to mailing list problems, I acked it via IRC for Peter and we pushed
before release.

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

Re: [libvirt] [PATCH 0/1] bhyve: Make LPC slot number configurable

[Daniel P . Berrangé]

On Fri, Aug 10, 2018 at 06:11:57PM +0300, Ivan Mishonov wrote:
> I'd like to hear Roman's opinion on this too since he wrote the initial
> implementation. As for the command line arguments I was looking at
>  since it's doing exactly the same thing and I thought it
> would be nice to be consistent with it

It would still be reasonable to allow  for arbitrary
passthrough of new features which have no XML defined for them. I just
think it is reasonable to model these two example explicitly.

The namespaced passthrough is intended for short term hacks primarily.

> On 08/10/2018 05:57 PM, Daniel P. Berrangé wrote:
> > On Fri, Aug 10, 2018 at 05:47:40PM +0300, Ivan Mishonov wrote:
> > > Yes, this is totally doable. I just don't know if it's a good idea to add 
> > > a
> > > new device type specifically for bhyve LPC and nothing else. Even if we do
> > > it like this I'll still have to send another patch including the bhyve XML
> > > namespace as we need to be able to pass extra command line options to the
> > > bhyve process related to unimplemented MSRs on AMD Zen systems. I thought
> > > I'd do the 2 things in a similar manner as both of them are strictly bhyve
> > > specific
> > IMHO  the LPC thing is definitely in scope for correct modelling in
> > the XML.
> > 
> > For the MSRs option, it is probable we'd consider that in scope as
> > well. Currently KVM has a global "ignore unknown msrs" option in the
> > kernel module, but I think it is conceptually reasonable to expect
> > that to be settable on a per-VM basis.
> > 
> > Probably would do the MSRs thing as a  flag, as we stuff
> > lots of random feature toggles under there
> > 
> > Regards,
> > Daniel
> 

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

Re: [libvirt] [PATCH 0/1] bhyve: Make LPC slot number configurable

[Daniel P . Berrangé]

On Fri, Aug 10, 2018 at 05:47:40PM +0300, Ivan Mishonov wrote:
> Yes, this is totally doable. I just don't know if it's a good idea to add a
> new device type specifically for bhyve LPC and nothing else. Even if we do
> it like this I'll still have to send another patch including the bhyve XML
> namespace as we need to be able to pass extra command line options to the
> bhyve process related to unimplemented MSRs on AMD Zen systems. I thought
> I'd do the 2 things in a similar manner as both of them are strictly bhyve
> specific

IMHO  the LPC thing is definitely in scope for correct modelling in
the XML.

For the MSRs option, it is probable we'd consider that in scope as
well. Currently KVM has a global "ignore unknown msrs" option in the
kernel module, but I think it is conceptually reasonable to expect
that to be settable on a per-VM basis.

Probably would do the MSRs thing as a  flag, as we stuff
lots of random feature toggles under there

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

Re: [libvirt] [PATCH 0/1] bhyve: Make LPC slot number configurable

[Daniel P . Berrangé]

On Fri, Aug 10, 2018 at 06:36:46PM +0300, Ivan Mishonov wrote:
> Yes, that makes sense. I'll try to find some time next week to redo my code
> and send another patch. Since my time for working on libvirt is very limited
> can you confirm that the LPC configuration should look like this:
> 
>    
>    
>    
> 
> Also can you send me an example of how you imagine the  setting
> for unimplemented MSRs?

Leading from this:

  https://libvirt.org/formatdomain.html#elementsFeatures

I would perhaps think

   
  ...
  
  ...
   


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

Re: [libvirt] [PATCH] network: restrict usage of port management APIs

[Daniel P . Berrangé]

On Wed, Aug 08, 2018 at 11:41:23PM -0400, Laine Stump wrote:
> On 08/08/2018 11:46 AM, Daniel P. Berrangé wrote:
> > The port allocation APIs are currently called unconditionally for all
> > types of NIC, but (mostly) only do anything for NICs with type=network.
> >
> > The exception is the port allocate API which does some validation even
> > for NICs with type!=network. Relying on this validation is flawed,
> > however, since the network driver may not even be installed, so virt
> > drivers must not delegation validation to it for NICs with
> > type!=network.
> 
> Although I never thought through all the minute details to the end (and
> didn't need to because ,,,AllocateActualDevice() wasn't in a public
> API), I had always figured that these calls into the network driver
> would be where, eventually, we would do all of the plumbing for a
> network device, like creating the tap/macvtap device during startup, and
> disconnecting/deleting devices during domain shutdown. (it also kind of
> makes sense for nwfilters to be added/removed by the network driver
> during these APIs, since the filterref is in the  definition).
> 
> That's the reason for the unconditional calls.
> 
> (one of the "minute details" that I hadn't thought about was the fact
> that we probably can't do *all* of the plumbing at one time - at the
> very least we need to have one API call for creating the devices and
> hooking them up, and another call that happens right before the guest
> CPUs are started (to bring everything online). Still, if we limit the
> netAllocate API to only being called for type='network' then we
> definitely will need an additional API that will likely be called
> unconditionally just after netAllocation is called just for type='network'.)

Yep, this is a bigger problem than I first considered. We have four
hypervisors using tap devices (qemu, lxc, libxl & bhyve) and all of
them have different requirements for the way the tap devices are
created. This is going to make it hard to delegate everything to the
network driver, as the work is hypervisor specific.

I'm trying to tackle this is distinct achieveable phases. Even the
current AllocateActualDevice() method is doing too much at the same
time. For example, it deals with allocating the resources inside the
network driver (ie reserving a VF or hostdev), as well as populating
the virDomainNetActualDef struct, and invoking hook scripts. This
makes it too tangled up with the hyervisor drivers - for example
needing a full domain XML is very undesirable.

> BTW, once it is the responsibility of the network driver to create tap
> devices and connect them to bridges, the network driver will no longer
> be optional (unless we want to duplicate the code in the hypervisor
> drivers), but that is the price we have to pay in order to give
> unprivileged libvirt the ability to use any type of network device.

Even if the network driver can create tap devices, I'd still want to
deal with type=network vs type=bridge separately for access control
purposes. It is desirable to be able to have ACLs on the opening operation
and the natural place to attach the ACL is against the virNetworkPtr
object. We have no object to represent standalone bridge devices outside
of a virNetworkPtr, so nothing to attach an ACL to for creating tap
devices.


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

Re: [libvirt] [PATCH] conf: rewrite filtering for capabilities lookup

[Daniel P . Berrangé]

ping

On Fri, Aug 03, 2018 at 03:28:16PM +0100, Daniel P. Berrangé wrote:
> The virCapabilitiesDomainDataLookupInternal() is given a list of
> parameters representing the desired domain characteristics. It then has
> to look throught the capabilities to identify an acceptable match.
> 
> The virCapsDomainDataCompare() method is used for filtering out
> candidates which don't match the desired criteria. It is called
> primarily from the innermost loops and as such is doing many repeated
> checks. For example if architcture and os type were checked at the top
> level loop the two inner loops could be avoided entirely. If emulator
> and domain type were checked in the 2nd level loop the 3rd level loop
> can be avoided too.
> 
> This change thus removes the virCapsDomainDataCompare() method and puts
> suitable checks at the start of each loop to ensure it executes the
> minimal number of loop iterations possible. The code becomes clearer to
> understand as a nice side-effect.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/conf/capabilities.c | 100 ++--
>  1 file changed, 45 insertions(+), 55 deletions(-)
> 
> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> index 0f96500294..cfd5132329 100644
> --- a/src/conf/capabilities.c
> +++ b/src/conf/capabilities.c
> @@ -604,46 +604,6 @@ 
> virCapabilitiesHostSecModelAddBaseLabel(virCapsHostSecModelPtr secmodel,
>  return -1;
>  }
>  
> -static bool
> -virCapsDomainDataCompare(virCapsGuestPtr guest,
> - virCapsGuestDomainPtr domain,
> - virCapsGuestMachinePtr machine,
> - int ostype,
> - virArch arch,
> - virDomainVirtType domaintype,
> - const char *emulator,
> - const char *machinetype)
> -{
> -const char *check_emulator = NULL;
> -
> -if (ostype != -1 && guest->ostype != ostype)
> -return false;
> -if ((arch != VIR_ARCH_NONE) && (guest->arch.id != arch))
> -return false;
> -
> -if (domaintype != VIR_DOMAIN_VIRT_NONE &&
> -(!domain || domain->type != domaintype))
> -return false;
> -
> -if (emulator) {
> -if (domain)
> -check_emulator = domain->info.emulator;
> -if (!check_emulator)
> -check_emulator = guest->arch.defaultInfo.emulator;
> -if (STRNEQ_NULLABLE(check_emulator, emulator))
> -return false;
> -}
> -
> -if (machinetype) {
> -if (!machine)
> -return false;
> -if (STRNEQ(machine->name, machinetype) &&
> -(STRNEQ_NULLABLE(machine->canonical, machinetype)))
> -return false;
> -}
> -
> -return true;
> -}
>  
>  static virCapsDomainDataPtr
>  virCapabilitiesDomainDataLookupInternal(virCapsPtr caps,
> @@ -659,13 +619,45 @@ virCapabilitiesDomainDataLookupInternal(virCapsPtr caps,
>  virCapsDomainDataPtr ret = NULL;
>  size_t i, j, k;
>  
> +VIR_DEBUG("Lookup ostype=%d arch=%d domaintype=%d emulator=%s 
> machine=%s",
> +  ostype, arch, domaintype, NULLSTR(emulator), 
> NULLSTR(machinetype));
>  for (i = 0; i < caps->nguests; i++) {
>  virCapsGuestPtr guest = caps->guests[i];
>  
> +if (ostype != -1 && guest->ostype != ostype) {
> +VIR_DEBUG("Skip os type want=%d vs got=%d", ostype, 
> guest->ostype);
> +continue;
> +}
> +VIR_DEBUG("Match os type %d", ostype);
> +
> +if ((arch != VIR_ARCH_NONE) && (guest->arch.id != arch)) {
> +VIR_DEBUG("Skip arch want=%d vs got=%d", arch, guest->arch.id);
> +continue;
> +}
> +VIR_DEBUG("Match arch %d", arch);
> +
>  for (j = 0; j < guest->arch.ndomains; j++) {
>  virCapsGuestDomainPtr domain = guest->arch.domains[j];
>  virCapsGuestMachinePtr *machinelist;
>  int nmachines;
> +const char *check_emulator = NULL;
> +
> +if (domaintype != VIR_DOMAIN_VIRT_NONE &&
> +(domain->type != domaintype)) {
> +VIR_DEBUG("Skip domain type want=%d vs got=%d", domaintype, 
> domain->type);
> +continue;
> +}
> +VIR_DEBUG("Match domain type %d", domaintype);
> +
> +check_emulator = domain->info.emulator;
> +if (

Re: [libvirt] [PATCH libvirt-tck] Cleanup secret in disk encryption test

[Daniel P . Berrangé]

On Wed, Aug 08, 2018 at 02:44:54PM -0600, Jim Fehlig wrote:
> 100-disk-encryption.t does not undefine the secret it defines
> for the test disk, causing subsequent runs of the test to fail
> 
> not ok 1 - secret created
>  Failed test 'secret created'
>  at /usr/share/libvirt-tck/tests/qemu/100-disk-encryption.t line 65.
> died: Sys::Virt::Error (libvirt error code: 1, message: internal error: a
> secret with UUID 212c459b-b02c-41fc-8ae2-714cc31612c5 is already defined
> for use with /var/cache/libvirt-tck/300-disk-encryption/demo.qcow2
> 
> Signed-off-by: Jim Fehlig 
> ---
>  scripts/qemu/100-disk-encryption.t | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Daniel P. Berrangé 


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

[libvirt] [jenkins-ci PATCH] jobs: ensure rpmbuild purges $RPM-ROOT/BUILD/$PACKAGE

[Daniel P . Berrangé]

When using 'rpmbuild --rebuild',  $RPM-ROOT/BUILD/$PACKAGE is
automatically deleted on completion, but when using 'rpmbuild --ta' it
is not deleted. We need to pass --clean to get the desired behaviour.

This was not a visible problem in the past because "git clean -fdx"
would purge the directory, but since we use %autosetup now the directory
will contain a git repo which causes "git clean" to skip deletion:

  $ su - jenkins
  $ cd libvirt-master
  $ git clean -fdx
  Skipping repository build/rpmbuild/BUILD/libvirt-4.7.0

Signed-off-by: Daniel P. Berrangé 
---
 jobs/autotools.yaml| 2 +-
 jobs/perl-modulebuild.yaml | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/jobs/autotools.yaml b/jobs/autotools.yaml
index c1d0f27..f1ebf7b 100644
--- a/jobs/autotools.yaml
+++ b/jobs/autotools.yaml
@@ -172,7 +172,7 @@
   sed -i -e 's/BuildRequires: pkgconfig(libvirt.*).*//' {name}.spec
   rm -f *.tar.{archive_format}
   $MAKE dist
-  rpmbuild --define "_topdir `pwd`/rpmbuild" -ta 
{name}-*.tar.{archive_format}
+  rpmbuild --clean --define "_topdir `pwd`/rpmbuild" -ta 
{name}-*.tar.{archive_format}
 publishers:
   - email:
   recipients: '{obj:spam}'
diff --git a/jobs/perl-modulebuild.yaml b/jobs/perl-modulebuild.yaml
index 934b216..3f3f537 100644
--- a/jobs/perl-modulebuild.yaml
+++ b/jobs/perl-modulebuild.yaml
@@ -126,7 +126,7 @@
   sed -i -e 's/BuildRequires: *libvirt.*devel.*//' *.spec
   rm -f *.tar.{archive_format}
   perl Build dist
-  rpmbuild --define "_topdir `pwd`/rpmbuild" -ta *.tar.{archive_format}
+  rpmbuild --clean --define "_topdir `pwd`/rpmbuild" -ta 
*.tar.{archive_format}
 publishers:
   - email:
   recipients: '{obj:spam}'
-- 
2.17.1

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

Re: [libvirt] [jenkins-ci PATCH] projects: Don't use ~/rpmbuild for osinfo-db

[Daniel P . Berrangé]

On Thu, Aug 09, 2018 at 03:57:00PM +0200, Andrea Bolognani wrote:
> Commit 15a19dbc0b73 made sure all explicit calls use
> the current directory instead of ~/rpmbuild as workspace,
> but osinfo-db wasn't affected by the change since its
> build recipe calls the custom 'make rpm' target and
> continued storing build artifacts in ~/rpmbuild.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  projects/osinfo-db.yaml | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/projects/osinfo-db.yaml b/projects/osinfo-db.yaml
> index 141a373..d154c98 100644
> --- a/projects/osinfo-db.yaml
> +++ b/projects/osinfo-db.yaml
> @@ -19,7 +19,5 @@
>parent_jobs: 'osinfo-db-master-check'
>machines: '{rpm_machines}'
>command: |
> -rm -f *.tar.{archive_format}
> -$MAKE osinfo-db.spec
>  perl -i -p -e 's/BuildRequires: osinfo-db-tools.*//' 
> osinfo-db.spec
> -$MAKE rpm
> +rpmbuild --clean --define "_topdir `pwd`/rpmbuild" --define 
> "_sourcedir `pwd`" -ba osinfo-db.spec


Reviewed-by: Daniel P. Berrangé 

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

Re: [libvirt] [PATCH] network: restrict usage of port management APIs

[Daniel P . Berrangé]

On Thu, Aug 09, 2018 at 11:54:42AM -0400, Laine Stump wrote:
> On 08/09/2018 03:58 AM, Daniel P. Berrangé wrote:
> > On Wed, Aug 08, 2018 at 11:41:23PM -0400, Laine Stump wrote:
> >> On 08/08/2018 11:46 AM, Daniel P. Berrangé wrote:
> >>> The port allocation APIs are currently called unconditionally for all
> >>> types of NIC, but (mostly) only do anything for NICs with type=network.
> >>>
> >>> The exception is the port allocate API which does some validation even
> >>> for NICs with type!=network. Relying on this validation is flawed,
> >>> however, since the network driver may not even be installed, so virt
> >>> drivers must not delegation validation to it for NICs with
> >>> type!=network.
> >> Although I never thought through all the minute details to the end (and
> >> didn't need to because ,,,AllocateActualDevice() wasn't in a public
> >> API), I had always figured that these calls into the network driver
> >> would be where, eventually, we would do all of the plumbing for a
> >> network device, like creating the tap/macvtap device during startup, and
> >> disconnecting/deleting devices during domain shutdown. (it also kind of
> >> makes sense for nwfilters to be added/removed by the network driver
> >> during these APIs, since the filterref is in the  definition).
> >>
> >> That's the reason for the unconditional calls.
> >>
> >> (one of the "minute details" that I hadn't thought about was the fact
> >> that we probably can't do *all* of the plumbing at one time - at the
> >> very least we need to have one API call for creating the devices and
> >> hooking them up, and another call that happens right before the guest
> >> CPUs are started (to bring everything online). Still, if we limit the
> >> netAllocate API to only being called for type='network' then we
> >> definitely will need an additional API that will likely be called
> >> unconditionally just after netAllocation is called just for 
> >> type='network'.)
> > Yep, this is a bigger problem than I first considered. We have four
> > hypervisors using tap devices (qemu, lxc, libxl & bhyve) and all of
> > them have different requirements for the way the tap devices are
> > created. This is going to make it hard to delegate everything to the
> > network driver, as the work is hypervisor specific.
> 
> The differences between qemu and lxc were on my mind but, without
> looking, I had naively assumed that "all the others" (including libxl
> and bhve) were just doing something similar to qemu :-/

If anything libxl & bhyve are much closer to LXC in approach, in that
they want a persistent TAP device passed by name, not a transient TAP
device passed by FD.

> >> BTW, once it is the responsibility of the network driver to create tap
> >> devices and connect them to bridges, the network driver will no longer
> >> be optional (unless we want to duplicate the code in the hypervisor
> >> drivers), but that is the price we have to pay in order to give
> >> unprivileged libvirt the ability to use any type of network device.
> > Even if the network driver can create tap devices, I'd still want to
> > deal with type=network vs type=bridge separately for access control
> > purposes. It is desirable to be able to have ACLs on the opening operation
> > and the natural place to attach the ACL is against the virNetworkPtr
> > object. We have no object to represent standalone bridge devices outside
> > of a virNetworkPtr, so nothing to attach an ACL to for creating tap
> > devices.
> 
> So are you saying that the network driver simply wouldn't be able to
> create the tap devices for type=bridge (or type=direct)? I'm not exactly
> following you (although I don't doubt what you say).

That's the direction i'm leaning towards right now at least. Adding the
API is easy, I just don't know what useful security model I could give it.

Consider the original goal was to allow the unprivileged libvirtd to have
useful network connectivity for guests. This means we want unprivileged
libvirtd to be able to request a TAP device without requiring root password
auth.

I don't think it is viable to allow this for arbitrary bridge devices out
of the box.  It is, however, just about reasonable to allow unprivileged
libvirtd to request a connection to the virbr0 "default" virtual network
without root auth, as the connectivity provided by the dfault network
is quite tightly defined.

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

Re: [libvirt] [Qemu-devel] [PATCH 3/3] ui: remove support for SDL1.2 in favour of SDL2

[Daniel P . Berrangé]

On Wed, Aug 08, 2018 at 02:51:01PM +0100, Peter Maydell wrote:
> On 8 August 2018 at 11:49, Daniel P. Berrangé  wrote:
> > SDL1.2 was deprecated in the 2.12.0 release with:
> >
> >   commit e52c6ba34149b4f39c3fd60e59ee32b809db2bfa
> >   Author: Daniel P. Berrange 
> >   Date:   Mon Jan 15 14:25:33 2018 +
> >
> > ui: deprecate use of SDL 1.2 in favour of 2.0 series
> >
> > The SDL 2.0 release was made in Aug, 2013:
> >
> >   https://www.libsdl.org/release/
> >
> > That will soon be 4 + 1/2 years ago, which is enough time to consider
> > the 2.0 series widely supported.
> >
> > Thus we deprecate the SDL 1.2 support, which will allow us to delete it
> > in the last release of 2018. By this time, SDL 2.0 will be more than 5
> > years old.
> >
> > Signed-off-by: Daniel P. Berrange 
> > Reviewed-by: Marc-André Lureau 
> > Message-id: 20180115142533.24585-1-berra...@redhat.com
> > Signed-off-by: Gerd Hoffmann 
> >
> > It is thus able to be removed in the 3.1.0 release.
> 
> At least one of the BSD VMs in tests/vm/ is still using SDL1.2.
> I think we should update that VM before we drop SDL1.2 support.
> 
> (This is probably just a matter of updating the VM image that
> is currently stored on patchew somewhere. It would also be
> nice if our test infra here had a mechanism for regenerating
> that VM image from scratch rather than just being "here's a
> disk image blob...")

Oh, I can no idea the images used SDL1, because they're totally opaque
to our git repo :-( Just an external blob with no info about how they
were built. Preferrably we'd at least have a manifest for their contents
in git, even better if there's something to automate building them.

According to repology.org/metapackage/sdl2/versions all the *BSDs have
SDL2 available to use at least.

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

Re: [libvirt] [PATCH] qemu: ensure default machine types don't change if QEMU changes

[Daniel P . Berrangé]

On Wed, Aug 08, 2018 at 10:12:59AM +0200, Andrea Bolognani wrote:
> On Tue, 2018-08-07 at 16:01 +0100, Daniel P. Berrangé wrote:
> > On Tue, Aug 07, 2018 at 04:57:28PM +0200, Andrea Bolognani wrote:
> > > > > I wonder if we shouldn't just drop the default machine type handling
> > > > > altogether at this point, though.
> > > > 
> > > > That's impossible as it violates the back compatibility guarantee
> > > > and will certainly break applications
> > > 
> > > In abstract terms, given that the whole point of this exercise is
> > > shielding our users from changes in QEMU, why wouldn't we go the
> > > whole way and take QEMU defaults out of the picture entirely?
> > > 
> > > I just can't picture a scenario where ignoring the QEMU defaults
> > > would actually cause issues, since we're basically moving the
> > > defaults into libvirt with this commit... Can you describe such
> > > a scenario?
> > 
> > Someone could build a QEMU with the "pc" machine type deleted entirely,
> > in which case libvirt won't find the default. The best we can do there
> > is fallback to QEMU's own default.
> 
> Yeah, that's basically my point: wouldn't it be better to error
> out rather than silently pick up a different default which is not
> controlled by libvirt?
> 
> Of course that would be slightly annoying for people building their
> own QEMU binaries with machine types ripped out, but I can't
> imagine that population being very large and they're certainly
> capable enough to figure out a way around the error; most people
> will get their QEMU and libvirt through downstream distributions,
> and if the downstream changes the list of machines available in
> QEMU they should patch libvirt to update the table anyway.
> 
> Ultimately, users and applications should make sure they include
> an explicit machine type in the guest XML, so ideally in time this
> code path will not be hit at all.

Again, any change which mandates specifying a machine type in the XML
is a regression for libvirt's users. Even if there's no 'pc' machine
present in the QEMU binary we must continue to provide a default of
some kind. Sure some apps would break in this case regardless, but
other apps will still work. There's no valid reason why we should
break use of an XML config like this as it works fine with both
i440fx and q35 


  demo
  
hvm
  
  261072
  1
  

  
  


  
  
  


  



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

Re: [libvirt] [PATCH] qemu: ensure "pc" machine is always used as default if available

[Daniel P . Berrangé]

On Tue, Aug 07, 2018 at 03:53:53PM -0300, Eduardo Habkost wrote:
> On Tue, Aug 07, 2018 at 12:17:33PM +0100, Daniel P. Berrangé wrote:
> > On Fri, Aug 03, 2018 at 01:05:49PM -0300, Eduardo Habkost wrote:
> > > On Fri, Aug 03, 2018 at 01:59:47PM +0100, Daniel P. Berrangé wrote:
> > > > It is increasingly likely that some distro is going to change the
> > > > default "x86" machine type in QEMU from "pc" to "q35". This will
> > > > certainly break existing applications which write their XML on the
> > > > assumption that its using a "pc" machine by default. For example they'll
> > > > lack a IDE CDROM and get PCI-X instad of PCI which changes the topology
> > > > radically.
> > > > 
> > > > Libvirt promises to isolate applications from hypervisor changes that
> > > > may cause incompatibilities, so we must ensure that we always use the
> > > > "pc" machine type if it is available. Only use QEMU's own reported
> > > > default machine type if "pc" does not exist.
> > > > 
> > > > Note this change assumes there will always be a "pc" alias as long as a
> > > > versioned "pc-XXX" machine type exists. If QEMU were to ship a "pc-XXX"
> > > > machine type but not provide the "pc" alias, it is too hard to decide
> > > > which to default so. Versioned machine types are supposed to be
> > > > considered opaque strings, so we can't apply any sensible ordering
> > > > ourselves and QEMU isn't reporting the list of machines in any sensible
> > > > ordering itself.
> > > > 
> > > > Signed-off-by: Daniel P. Berrangé 
> > > 
> > > Won't this break qemuParseCommandLine() if it sees a QEMU binary
> > > running without "-machine"?  It will assume the QEMU default is
> > > "pc" but this may be not true.
> > 
> > If no -machine arg is present in ARGV, then the code will lookup the
> > default machine type for the emulator binary in the capabilities
> > record. So this should just "do the right thing" with my changes
> > in this patch.
> 
> I don't see how it would do the right thing.  e.g.: if we have a
> QEMU binary that defaults to "q35" and it is running without
> "-machine", it will be emulating a q35 machine, not i440fx.  But
> with this change qemuParseCommandLine() will incorrectly assume
> the existing process is emulating i440fx.
> 
> qemuParseCommandLine() calls:
> 
>   capsdata = virCapabilitiesDomainDataLookup(caps, def->os.type, def->os.arch,
>  def->virtType, NULL, NULL)
> 
> and as far as I can see, your patch will make
> qemuParseCommandLine() set capsdata->machinetype to a pc-i440fx-*
> machine, even if the QEMU binary defaults to something else.

Oh right, I mis-interpreted things :-(  This patch is pushed, so I'll think
about a followup fix.

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

[libvirt] [PATCH] network: restrict usage of port management APIs

[Daniel P . Berrangé]

The port allocation APIs are currently called unconditionally for all
types of NIC, but (mostly) only do anything for NICs with type=network.

The exception is the port allocate API which does some validation even
for NICs with type!=network. Relying on this validation is flawed,
however, since the network driver may not even be installed, so virt
drivers must not delegation validation to it for NICs with
type!=network.

This change allows us to report errors when the virtual network driver
is not registered.

Signed-off-by: Daniel P. Berrangé 
---
 src/conf/domain_conf.c  | 12 +---
 src/libxl/libxl_domain.c|  6 --
 src/libxl/libxl_driver.c|  9 ++---
 src/lxc/lxc_driver.c|  6 --
 src/lxc/lxc_process.c   | 10 --
 src/network/bridge_driver.c | 22 +++---
 src/qemu/qemu_hotplug.c | 17 +++--
 src/qemu/qemu_process.c |  9 ++---
 8 files changed, 59 insertions(+), 32 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index adcd8f41b9..e7d2acdcc9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -29947,13 +29947,11 @@ int
 virDomainNetAllocateActualDevice(virDomainDefPtr dom,
  virDomainNetDefPtr iface)
 {
-/* Just silently ignore if network driver isn't present. If something
- * has tried to use a NIC with type=network, other code will already
- * cause an error. This ensures type=bridge doesn't break when
- * network driver is compiled out.
- */
-if (!netAllocate)
-return 0;
+if (!netAllocate) {
+virReportError(VIR_ERR_NO_SUPPORT, "%s",
+   _("Virtual networking driver is not available"));
+return -1;
+}
 
 return netAllocate(dom, iface);
 }
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 2ab78ac9a5..c78d5ee96c 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -791,7 +791,8 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
 
 /* cleanup actual device */
 virDomainNetRemoveHostdev(vm->def, net);
-virDomainNetReleaseActualDevice(vm->def, net);
+if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK)
+virDomainNetReleaseActualDevice(vm->def, net);
 }
 }
 
@@ -948,7 +949,8 @@ libxlNetworkPrepareDevices(virDomainDefPtr def)
  * network's pool of devices, or resolve bridge device name
  * to the one defined in the network definition.
  */
-if (virDomainNetAllocateActualDevice(def, net) < 0)
+if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
+virDomainNetAllocateActualDevice(def, net) < 0)
 return -1;
 
 actualType = virDomainNetGetActualType(net);
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 5a5e792957..fb5f046ade 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -3264,7 +3264,8 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver,
  * network's pool of devices, or resolve bridge device name
  * to the one defined in the network definition.
  */
-if (virDomainNetAllocateActualDevice(vm->def, net) < 0)
+if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
+virDomainNetAllocateActualDevice(vm->def, net) < 0)
 goto cleanup;
 
 actualType = virDomainNetGetActualType(net);
@@ -3314,7 +3315,8 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver,
 vm->def->nets[vm->def->nnets++] = net;
 } else {
 virDomainNetRemoveHostdev(vm->def, net);
-virDomainNetReleaseActualDevice(vm->def, net);
+if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK)
+virDomainNetReleaseActualDevice(vm->def, net);
 }
 virObjectUnref(cfg);
 return ret;
@@ -3737,7 +3739,8 @@ libxlDomainDetachNetDevice(libxlDriverPrivatePtr driver,
  cleanup:
 libxl_device_nic_dispose();
 if (!ret) {
-virDomainNetReleaseActualDevice(vm->def, detach);
+if (detach->type == VIR_DOMAIN_NET_TYPE_NETWORK)
+virDomainNetReleaseActualDevice(vm->def, detach);
 virDomainNetRemove(vm->def, detachidx);
 }
 virObjectUnref(cfg);
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 8867645cdc..8729fc0174 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -3871,7 +3871,8 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn,
  * network's pool of devices, or resolve bridge device name
  * to the one defined in the network definition.
  */
-if (virDomainNetAllocateActualDevice(vm->def, net) < 0)
+if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
+virDomainNetAllocateActualDevice(vm->def, net) < 0)
 return -1;
 
 actualType = virDomainNetGetActualType(net);
@@ -4425,7 +4426,8 @@ lxcDomai

[libvirt] [PATCH] storage: tweak error message when skipping file

[Daniel P . Berrangé]

Signed-off-by: Daniel P. Berrangé 
---
 src/storage/storage_util.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index 715d5c2f88..42a9b6abf0 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -3609,8 +3609,8 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool)
 int err;
 
 if (virStringHasControlChars(ent->d_name)) {
-VIR_WARN("Ignoring file with control characters under '%s'",
- def->target.path);
+VIR_WARN("Ignoring file '%s' with control characters under '%s'",
+ ent->d_name, def->target.path);
 continue;
 }
 
-- 
2.17.1

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

Re: [libvirt] [PATCH] Fix link errors in tools/nss and tests

[Daniel P . Berrangé]

On Wed, Aug 08, 2018 at 11:00:01AM -0600, Jim Fehlig wrote:
> While local builds succeed fine, a build worker building in a chroot
> environment is encountering errors when linking some items in tools/nss
> and tests, e.g.
> 
> [  469s] libtool: link: gcc -shared  -fPIC -DPIC  -Wl,--whole-archive 
> nss/.libs/libnss_libvirt_impl.a -Wl,--no-whole-archive  -lpthread -lutil 
> -ltirpc  -fstack-protector-strong -grecord-gcc-switches -O2 
> -fstack-protector-strong -g -Wl,--version-script=./nss/libvirt_nss.syms 
> -Wl,-z -Wl,relro -Wl,-z -Wl,now -Wl,--no-copy-dt-needed-entries -Wl,-z 
> -Wl,defs -grecord-gcc-switches -O2 -fstack-protector-strong -g   -pthread 
> -Wl,-soname -Wl,libnss_libvirt.so.2 -o nss/.libs/libnss_libvirt.so.2
> [  469s] nss/.libs/libnss_libvirt_impl.a(libvirt_nss_la-virjsoncompat.o): In 
> function `virJSONJanssonOnce':
> [  469s] 
> /home/abuild/rpmbuild/BUILD/libvirt-4.6.0/src/util/virjsoncompat.c:63: 
> undefined reference to `dlopen'
> [  469s] 
> /home/abuild/rpmbuild/BUILD/libvirt-4.6.0/src/util/virjsoncompat.c:79: 
> undefined reference to `dlsym'
> ...
> 
> A similar problem was fixed in commit b018ada3 and inspires this fix.
> 
> Signed-off-by: Jim Fehlig 

Reviewed-by: Daniel P. Berrangé 


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

Re: [libvirt] [libvirt-glib][PATCH] Use new GObject define macros with private

[Daniel P . Berrangé]

On Wed, Aug 15, 2018 at 06:08:18PM +0200, Andrea Bolognani wrote:
> On Fri, 2018-08-10 at 10:38 +0100, Daniel P. Berrangé wrote:
> > On Fri, Aug 10, 2018 at 11:29:30AM +0200, Michal Privoznik wrote:
> > > G_ADD_PRIVATE was added in 2.38 and older functions are getting 
> > > deprecated:
> > > https://gitlab.gnome.org/GNOME/glib/merge_requests/7/commits
> > 
> > Our min glib2 version is currently 2.36, since that's what RHEL-7 has.
> 
> Uh? RHEL 7 has 2.54; Debian 8, the other pretty old platform
> we still target, has 2.42, so we're totally in the clear for
> making the switch...

Oh, the different between RHEL-7.0 and latest .x updates. I forget they
rebased glib2 RPM


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

Re: [libvirt] [PATCH v2 0/6] fdaskljrew

[Daniel P . Berrangé]

On Thu, Aug 16, 2018 at 11:11:26AM +0200, Simon Kobyda wrote:
> rpdwrewrewr

Please use a sensible subject line, and provide useful information about
the patch series in this cover letter.

> 
> Simon Kobyda (6):
>   vsh: Add API for printing tables.
>   virsh: Implement new table API for virsh list
>   vsh: Added tests
>   virsh: Implement vsh-table to iface-list
>   virsh: Implement vshTable API to net-list and net-dhcp-leases
>   virsh: Implement vshTable API to secret-list
> 
>  tests/Makefile.am|   8 +
>  tests/virshtest.c|  14 +-
>  tests/vshtabletest.c | 247 +
>  tools/Makefile.am|   4 +-
>  tools/virsh-domain-monitor.c |  43 ++--
>  tools/virsh-interface.c  |  27 ++-
>  tools/virsh-network.c|  50 +++--
>  tools/virsh-secret.c |  30 ++-
>  tools/vsh-table.c| 413 +++
>  tools/vsh-table.h|  42 
>  10 files changed, 817 insertions(+), 61 deletions(-)
>  create mode 100644 tests/vshtabletest.c
>  create mode 100644 tools/vsh-table.c
>  create mode 100644 tools/vsh-table.h
> 
> -- 
> 2.17.1
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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

Re: [libvirt] [PATCH 1/4] cpu: allow include files for CPU definition

[Daniel P . Berrangé]

On Tue, Aug 14, 2018 at 12:55:19PM +0200, Jiri Denemark wrote:
> On Wed, Aug 01, 2018 at 18:02:29 +0100, Daniel P. Berrangé wrote:
> > Allow for syntax
> > 
> > 
> 
> It seems the code should just work with
> 
> 
> 
> but Makefile.am and libvirt.spec would need some adjustment.
> 
> > to reference other files in the CPU database directory
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  libvirt.spec.in   |  2 +-
> >  mingw-libvirt.spec.in |  4 +--
> >  src/Makefile.am   |  2 +-
> >  src/cpu/cpu_map.c | 84 +--
> >  4 files changed, 86 insertions(+), 6 deletions(-)
> > 
> > diff --git a/libvirt.spec.in b/libvirt.spec.in
> > index 19ae55cdaf..b6745dbffa 100644
> > --- a/libvirt.spec.in
> > +++ b/libvirt.spec.in
> > @@ -1856,7 +1856,7 @@ exit 0
> >  %{_datadir}/libvirt/schemas/storagepool.rng
> >  %{_datadir}/libvirt/schemas/storagevol.rng
> >  
> > -%{_datadir}/libvirt/cpu_map.xml
> > +%{_datadir}/libvirt/cpu_map*.xml
> >  
> >  %{_datadir}/libvirt/test-screenshot.png
> >  
> > diff --git a/mingw-libvirt.spec.in b/mingw-libvirt.spec.in
> > index cc1e619927..22fe7a000f 100644
> > --- a/mingw-libvirt.spec.in
> > +++ b/mingw-libvirt.spec.in
> > @@ -260,7 +260,7 @@ rm -rf 
> > $RPM_BUILD_ROOT%{mingw64_libexecdir}/libvirt-guests.sh
> >  %{mingw32_datadir}/libvirt/api/libvirt-qemu-api.xml
> >  %{mingw32_datadir}/libvirt/api/libvirt-admin-api.xml
> >  
> > -%{mingw32_datadir}/libvirt/cpu_map.xml
> > +%{mingw32_datadir}/libvirt/cpu_map*.xml
> >  
> >  %{mingw32_datadir}/libvirt/test-screenshot.png
> >  
> > @@ -347,7 +347,7 @@ rm -rf 
> > $RPM_BUILD_ROOT%{mingw64_libexecdir}/libvirt-guests.sh
> >  %{mingw64_datadir}/libvirt/api/libvirt-qemu-api.xml
> >  %{mingw64_datadir}/libvirt/api/libvirt-admin-api.xml
> >  
> > -%{mingw64_datadir}/libvirt/cpu_map.xml
> > +%{mingw64_datadir}/libvirt/cpu_map*.xml
> >  
> >  %{mingw64_datadir}/libvirt/test-screenshot.png
> >  
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index a4f213480e..11a7ac81e2 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -366,7 +366,7 @@ check-local: check-protocol check-symfile 
> > check-symsorting \
> >  
> >  
> >  
> > -pkgdata_DATA = cpu/cpu_map.xml
> > +pkgdata_DATA = $(wildcard $(srcdir)/cpu/cpu_map*.xml)
> >  
> >  EXTRA_DIST +=  $(pkgdata_DATA)
> >  
> > diff --git a/src/cpu/cpu_map.c b/src/cpu/cpu_map.c
> > index d263eb8cdd..9e090919ed 100644
> > --- a/src/cpu/cpu_map.c
> > +++ b/src/cpu/cpu_map.c
> > @@ -70,6 +70,83 @@ static int load(xmlXPathContextPtr ctxt,
> ..
> > +static int loadIncludes(xmlXPathContextPtr ctxt,
> > +cpuMapLoadCallback callback,
> > +void *data)
> > +{
> > +int ret = -1;
> > +xmlNodePtr ctxt_node;
> > +xmlNodePtr *nodes = NULL;
> > +int n;
> > +size_t i;
> > +
> > +ctxt_node = ctxt->node;
> > +
> > +n = virXPathNodeSet("include", ctxt, );
> > +if (n < 0)
> > +goto cleanup;
> > +
> > +for (i = 0; i < n; i++) {
> > +char *filename = virXMLPropString(nodes[i], "filename");
> 
> This would be a nice candidate for VIR_AUTOFREE(char *) :-)

Since none of the src/cpu code has been switched to this style yet, its
probably best not to introduce it here. Rather wait for the bulk convert


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

Re: [libvirt] [RFC PATCH 01/17] qemu: setup shared memory without explicit numa configuration

[Daniel P . Berrangé]

On Fri, Jul 13, 2018 at 03:28:08PM +0200, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau 
> 
> When a domain is configured with 'shared' memory backing:
> 
>   
> 
>   
> 
> But no explicit NUMA configuration, let's configure a shared memory
> backend associated with default -numa.


> diff --git a/tests/qemuxml2argvdata/fd-memory-no-numa-topology.args 
> b/tests/qemuxml2argvdata/fd-memory-no-numa-topology.args
> index bd88daaa3b..400fb39cc6 100644
> --- a/tests/qemuxml2argvdata/fd-memory-no-numa-topology.args
> +++ b/tests/qemuxml2argvdata/fd-memory-no-numa-topology.args
> @@ -11,6 +11,10 @@ QEMU_AUDIO_DRV=none \
>  -m 14336 \
>  -mem-prealloc \
>  -smp 8,sockets=8,cores=1,threads=1 \
> +-object memory-backend-file,id=ram-node,\
> +mem-path=/var/lib/libvirt/qemu/ram/libvirt/qemu/-1-instance-0092/ram-node,\
> +share=yes,size=15032385536 \
> +-numa node,nodeid=0,memdev=ram-node \

I'm not at all convinced it is safe todo this. We've been burnt in the
past by adding  use of memory-backend objects causing migration to break


  commit f309db1f4d51009bad0d32e12efc75530b66836b
  Author: Michal Privoznik 
  Date:   Thu Dec 18 12:36:48 2014 +0100

qemu: Create memory-backend-{ram,file} iff needed

Libvirt BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1175397
QEMU BZ:https://bugzilla.redhat.com/show_bug.cgi?id=1170093

This change doesn't really feel like it is required either. If the
user wants NUMA, then the XML can just be written to request a NUMA
topology with a single node.  Better to be explicit in the XML rather
than silently adding things as a side effect

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

Re: [libvirt] [PATCH 0/7] Remove arbitrary limit on socket_id/core_id

[Daniel P . Berrangé]

On Tue, Aug 14, 2018 at 11:07:49AM +0200, Andrea Bolognani wrote:
> See patch 6 for the explanation; the previous 5 patches
> are cleanups.
> 
> I had to snip out some patches to comply with the message
> size limit for the list - ironic, I know. The full commits
> can be fetched from

Although the size limit is 300kb, I usually approve any genuine patches
that exceed that size - the limit is mostly there to stop people posting
large log files or screenshots, etc. So feel free to always send full
patchsets and let admins decide whether to approve or reject.

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

Re: [libvirt] [PATCH v3 1/3] vsh: Add API for printing tables.

[Daniel P . Berrangé]

On Thu, Aug 16, 2018 at 12:56:24PM +0200, Simon Kobyda wrote:
> It solves problems with alignment of columns. Width of each column
> is calculated by its biggest cell. Should solve unicode bug.
> In future, it may be implemented in virsh, virt-admin...
> 
> This API has 5 public functions:
> - vshTableNew - adds new table and defines its header
> - vshTableRowAppend - appends new row (for same number of columns as in
> header)
> - vshTablePrintToStdout
> - vshTablePrintToString
> - vshTableFree
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1574624
> https://bugzilla.redhat.com/show_bug.cgi?id=1584630
> 
> Signed-off-by: Simon Kobyda 
> ---
>  tools/Makefile.am |   4 +-
>  tools/vsh-table.c | 413 ++
>  tools/vsh-table.h |  42 +
>  3 files changed, 458 insertions(+), 1 deletion(-)
>  create mode 100644 tools/vsh-table.c
>  create mode 100644 tools/vsh-table.h
> 


> +/**
> + * vshTableGetColumnsWidths:
> + * @table: table
> + * @maxwidths: maximum count of characters for each columns
> + * @widths: count of characters for each cell in the table
> + *
> + * Fill passed @maxwidths and @widths arrays with maximum number
> + * of characters for columns and number of character per each
> + * table cell, respectively.
> + *
> + * Handle unicode strings (user must have multibyte locale)
> + */
> +static int
> +vshTableGetColumnsWidths(vshTablePtr table,
> + size_t *maxwidths,
> + size_t **widths,
> + bool header)
> +{
> +int ret = -1;
> +size_t i = 1;
> +size_t j;
> +size_t len;
> +int tmp;
> +wchar_t *wstr = NULL;
> +size_t wstrlen;
> +
> +if (header)
> +i = 0;
> +else
> +i = 1;
> +for (; i < table->nrows; i++) {
> +vshTableRowPtr row = table->rows[i];
> +
> +for (j = 0; j < row->ncells; j++) {
> +/* strlen should return maximum possible length needed */
> +wstrlen = strlen(row->cells[j]);
> +VIR_FREE(wstr);
> +if (VIR_ALLOC_N(wstr, wstrlen) < 0)
> +goto cleanup;
> +/* mbstowcs fails if machine is using singlebyte locale
> + * and user tries to convert unicode(multibyte)
> + * */
> +if (mbstowcs(wstr, row->cells[j], wstrlen) ==
> +(size_t) -1) {
> +len = wstrlen;
> +} else {
> +tmp = wcswidth(wstr, wstrlen);
> +if (tmp < 0)
> +goto cleanup;
> +len = (size_t)((unsigned)tmp);
> +}
> +widths[i][j] = len;
> +if (len > maxwidths[j])
> +maxwidths[j] = len;

After asking around I have found the right solution that we need to use
for measuring string width.  mbstowcs()/wcswidth() will get the answer
wrong wrt zero-width characters, combining characters, non-printable
characters, etc. We need to use the libunistring library:

  
https://www.gnu.org/software/libunistring/manual/libunistring.html#uniwidth_002eh


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

Re: [libvirt] [RFC PATCH 01/17] qemu: setup shared memory without explicit numa configuration

[Daniel P . Berrangé]

On Thu, Aug 16, 2018 at 07:32:57AM -0400, John Ferlan wrote:
> 
> 
> On 08/16/2018 06:31 AM, Marc-André Lureau wrote:
> > Hi
> > 
> > On Thu, Aug 16, 2018 at 4:35 AM, John Ferlan  wrote:
> >>
> >>
> >> On 07/13/2018 09:28 AM, marcandre.lur...@redhat.com wrote:
> >>> From: Marc-André Lureau 
> >>>
> >>> When a domain is configured with 'shared' memory backing:
> >>>
> >>>   
> >>> 
> >>>   
> >>>
> >>> But no explicit NUMA configuration, let's configure a shared memory
> >>> backend associated with default -numa.
> >>>
> >>> Signed-off-by: Marc-André Lureau 
> >>> ---
> >>>  src/qemu/qemu_command.c   | 100 --
> >>>  .../fd-memory-no-numa-topology.args   |   4 +
> >>>  2 files changed, 73 insertions(+), 31 deletions(-)
> >>>
> >>
> >> NUMA, memory backends, and hugepages - not in my wheelhouse of
> >> knowledge. Hopefully Michal and/or Pavel will take a look!
> >>
> >> Is it possible someone may not want this type of thing to happen? Is
> > 
> > I assume someone that sets 'shared' memory mode may consider this as a bug 
> > fix.
> > 
> >> there an upside or downside to this?  What happens "today" when not
> > 
> > You get non-shared memory
> > 
> 
> So today someone asks for "shared" and then end up with "non-shared"? I
> don't think that's apparent from the "access" description in:
> 
> https://libvirt.org/formatdomain.html#elementsMemoryBacking
> 
> Then again, not in my wheelhouse of knowledge, so maybe that's just one
> of those givens. Of course that perhaps goes to your first answer of
> this being a "bug fix". Not something that's apparent from the existing
> documentation or commit description though. This probably should have
> been it's own separate patch and not included in this series.

If we can't honour the "shared" request, we should make sure libvirt
reports an error and aborts startup.

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

[libvirt] [PATCH 0/3] Fix default machine when converting argv to xml

[Daniel P . Berrangé]

We must honour the QEMU built-in default machine when converting from argv

Daniel P. Berrangé (3):
  qemu: record the QEMU default machine in capabilities
  qemu: rename method for getting preferred machine type
  qemu: fix default machine for argv -> xml convertor

 src/qemu/qemu_capabilities.c  | 37 ++---
 src/qemu/qemu_capabilities.h  |  3 +-
 src/qemu/qemu_driver.c|  6 ++-
 src/qemu/qemu_parse_command.c | 34 ++--
 src/qemu/qemu_parse_command.h |  8 +++-
 tests/domaincapstest.c|  2 +-
 tests/qemuargv2xmldata/nomachine-aarch64.args | 11 -
 tests/qemuargv2xmldata/nomachine-aarch64.xml  | 40 ---
 tests/qemuargv2xmldata/nomachine-ppc64.xml|  4 +-
 tests/qemuargv2xmldata/nomachine-x86_64.xml   |  4 +-
 tests/qemuargv2xmldata/pseries-disk.xml   |  4 +-
 tests/qemuargv2xmldata/pseries-nvram.xml  |  4 +-
 tests/qemuargv2xmltest.c  | 18 -
 .../caps_1.5.3.x86_64.xml |  2 +-
 .../caps_1.6.0.x86_64.xml |  2 +-
 .../caps_1.7.0.x86_64.xml |  2 +-
 .../caps_2.1.1.x86_64.xml |  2 +-
 .../caps_2.10.0.ppc64.xml |  2 +-
 .../caps_2.10.0.s390x.xml |  2 +-
 .../caps_2.10.0.x86_64.xml|  2 +-
 .../caps_2.11.0.s390x.xml |  2 +-
 .../caps_2.11.0.x86_64.xml|  2 +-
 .../caps_2.12.0.ppc64.xml |  2 +-
 .../caps_2.12.0.s390x.xml |  2 +-
 .../caps_2.12.0.x86_64.xml|  2 +-
 .../caps_2.4.0.x86_64.xml |  2 +-
 .../caps_2.5.0.x86_64.xml |  2 +-
 .../qemucapabilitiesdata/caps_2.6.0.ppc64.xml |  2 +-
 .../caps_2.6.0.x86_64.xml |  2 +-
 .../qemucapabilitiesdata/caps_2.7.0.s390x.xml |  2 +-
 .../caps_2.7.0.x86_64.xml |  2 +-
 .../qemucapabilitiesdata/caps_2.8.0.s390x.xml |  2 +-
 .../caps_2.8.0.x86_64.xml |  2 +-
 .../qemucapabilitiesdata/caps_2.9.0.ppc64.xml |  2 +-
 .../qemucapabilitiesdata/caps_2.9.0.s390x.xml |  2 +-
 .../caps_2.9.0.x86_64.xml |  2 +-
 .../qemucapabilitiesdata/caps_3.0.0.ppc64.xml |  2 +-
 .../caps_3.0.0.x86_64.xml |  2 +-
 38 files changed, 116 insertions(+), 109 deletions(-)
 delete mode 100644 tests/qemuargv2xmldata/nomachine-aarch64.args
 delete mode 100644 tests/qemuargv2xmldata/nomachine-aarch64.xml

-- 
2.17.1

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

[libvirt] [PATCH 1/3] qemu: record the QEMU default machine in capabilities

[Daniel P . Berrangé]

We don't honour the QEMU default machine type anymore, always using the
libvirt chosen default instead. The QEMU argv parser, however, will need
to know the exacty QEMU default, so we must record that info.

Signed-off-by: Daniel P. Berrangé 
---
 src/qemu/qemu_capabilities.c | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index e6e199b2c6..a0a1b97f1d 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -515,6 +515,7 @@ struct virQEMUCapsMachineType {
 char *alias;
 unsigned int maxCpus;
 bool hotplugCpus;
+bool qemuDefault;
 };
 
 typedef struct _virQEMUCapsHostCPUData virQEMUCapsHostCPUData;
@@ -2324,8 +2325,10 @@ virQEMUCapsProbeQMPMachineTypes(virQEMUCapsPtr qemuCaps,
 preferredIdx = qemuCaps->nmachineTypes - 1;
 }
 
-if (machines[i]->isDefault)
+if (machines[i]->isDefault) {
+mach->qemuDefault = true;
 defIdx = qemuCaps->nmachineTypes - 1;
+}
 }
 
 /*
@@ -3355,7 +3358,7 @@ virQEMUCapsCachePrivFree(void *privData)
  *   ...
  *   
  *   ...
- *   
+ *   
  *   ...
  * 
  */
@@ -3520,6 +3523,11 @@ virQEMUCapsLoadCache(virArch hostArch,
 if (STREQ_NULLABLE(str, "yes"))
 qemuCaps->machineTypes[i].hotplugCpus = true;
 VIR_FREE(str);
+
+str = virXMLPropString(nodes[i], "default");
+if (STREQ_NULLABLE(str, "yes"))
+qemuCaps->machineTypes[i].qemuDefault = true;
+VIR_FREE(str);
 }
 }
 VIR_FREE(nodes);
@@ -3768,6 +3776,8 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps)
   qemuCaps->machineTypes[i].alias);
 if (qemuCaps->machineTypes[i].hotplugCpus)
 virBufferAddLit(, " hotplugCpus='yes'");
+if (qemuCaps->machineTypes[i].qemuDefault)
+virBufferAddLit(, " default='yes'");
 virBufferAsprintf(, " maxCpus='%u'/>\n",
   qemuCaps->machineTypes[i].maxCpus);
 }
-- 
2.17.1

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

[libvirt] [PATCH v2 1/8] cpu: allow include files for CPU definition

[Daniel P . Berrangé]

Allow for syntax



to reference other files in the CPU database directory

Signed-off-by: Daniel P. Berrangé 
---
 src/cpu/cpu_map.c | 87 +--
 1 file changed, 84 insertions(+), 3 deletions(-)

diff --git a/src/cpu/cpu_map.c b/src/cpu/cpu_map.c
index d263eb8cdd..bcd3e55417 100644
--- a/src/cpu/cpu_map.c
+++ b/src/cpu/cpu_map.c
@@ -1,7 +1,7 @@
 /*
  * cpu_map.c: internal functions for handling CPU mapping configuration
  *
- * Copyright (C) 2009-2010 Red Hat, Inc.
+ * Copyright (C) 2009-2018 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -70,6 +70,84 @@ static int load(xmlXPathContextPtr ctxt,
 return ret;
 }
 
+static int
+cpuMapLoadInclude(const char *filename,
+  cpuMapLoadCallback cb,
+  void *data)
+{
+xmlDocPtr xml = NULL;
+xmlXPathContextPtr ctxt = NULL;
+int ret = -1;
+int element;
+char *mapfile;
+
+if (!(mapfile = virFileFindResource(filename,
+abs_topsrcdir "/src/cpu",
+PKGDATADIR)))
+return -1;
+
+VIR_DEBUG("Loading CPU map include from %s", mapfile);
+
+if (!(xml = virXMLParseFileCtxt(mapfile, )))
+goto cleanup;
+
+ctxt->node = xmlDocGetRootElement(xml);
+
+for (element = 0; element < CPU_MAP_ELEMENT_LAST; element++) {
+if (load(ctxt, element, cb, data) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("cannot parse CPU map '%s'"), mapfile);
+goto cleanup;
+}
+}
+
+ret = 0;
+
+ cleanup:
+xmlXPathFreeContext(ctxt);
+xmlFreeDoc(xml);
+VIR_FREE(mapfile);
+
+return ret;
+}
+
+
+static int
+loadIncludes(xmlXPathContextPtr ctxt,
+ cpuMapLoadCallback callback,
+ void *data)
+{
+int ret = -1;
+xmlNodePtr ctxt_node;
+xmlNodePtr *nodes = NULL;
+int n;
+size_t i;
+
+ctxt_node = ctxt->node;
+
+n = virXPathNodeSet("include", ctxt, );
+if (n < 0)
+goto cleanup;
+
+for (i = 0; i < n; i++) {
+char *filename = virXMLPropString(nodes[i], "filename");
+VIR_DEBUG("Finding CPU map include '%s'", filename);
+if (cpuMapLoadInclude(filename, callback, data) < 0) {
+VIR_FREE(filename);
+goto cleanup;
+}
+VIR_FREE(filename);
+}
+
+ret = 0;
+
+ cleanup:
+ctxt->node = ctxt_node;
+VIR_FREE(nodes);
+
+return ret;
+}
+
 
 int cpuMapLoad(const char *arch,
cpuMapLoadCallback cb,
@@ -88,7 +166,7 @@ int cpuMapLoad(const char *arch,
 PKGDATADIR)))
 return -1;
 
-VIR_DEBUG("Loading CPU map from %s", mapfile);
+VIR_DEBUG("Loading '%s' CPU map from %s", NULLSTR(arch), mapfile);
 
 if (arch == NULL) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -122,11 +200,14 @@ int cpuMapLoad(const char *arch,
 for (element = 0; element < CPU_MAP_ELEMENT_LAST; element++) {
 if (load(ctxt, element, cb, data) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("cannot parse CPU map for %s architecture"), 
arch);
+   _("cannot parse CPU map '%s'"), mapfile);
 goto cleanup;
 }
 }
 
+if (loadIncludes(ctxt, cb, data) < 0)
+goto cleanup;
+
 ret = 0;
 
  cleanup:
-- 
2.17.1

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

[libvirt] [PATCH v2 6/8] cpu: split PPC64 map data into separate files

[Daniel P . Berrangé]

Signed-off-by: Daniel P. Berrangé 
---
 src/cpu_map/Makefile.inc.am |  7 +
 src/cpu_map/index.xml   | 41 +
 src/cpu_map/ppc64_POWER6.xml|  6 +
 src/cpu_map/ppc64_POWER7.xml|  7 +
 src/cpu_map/ppc64_POWER8.xml|  8 ++
 src/cpu_map/ppc64_POWER9.xml|  6 +
 src/cpu_map/ppc64_POWERPC_e5500.xml |  6 +
 src/cpu_map/ppc64_POWERPC_e6500.xml |  6 +
 src/cpu_map/ppc64_vendors.xml   |  4 +++
 9 files changed, 57 insertions(+), 34 deletions(-)
 create mode 100644 src/cpu_map/ppc64_POWER6.xml
 create mode 100644 src/cpu_map/ppc64_POWER7.xml
 create mode 100644 src/cpu_map/ppc64_POWER8.xml
 create mode 100644 src/cpu_map/ppc64_POWER9.xml
 create mode 100644 src/cpu_map/ppc64_POWERPC_e5500.xml
 create mode 100644 src/cpu_map/ppc64_POWERPC_e6500.xml
 create mode 100644 src/cpu_map/ppc64_vendors.xml

diff --git a/src/cpu_map/Makefile.inc.am b/src/cpu_map/Makefile.inc.am
index 91728b9200..64453cc384 100644
--- a/src/cpu_map/Makefile.inc.am
+++ b/src/cpu_map/Makefile.inc.am
@@ -2,6 +2,13 @@
 cpumapdir = $(pkgdatadir)/cpu_map
 cpumap_DATA = \
cpu_map/index.xml \
+   cpu_map/ppc64_vendors.xml \
+   cpu_map/ppc64_POWER7.xml \
+   cpu_map/ppc64_POWER9.xml \
+   cpu_map/ppc64_POWERPC_e6500.xml \
+   cpu_map/ppc64_POWER6.xml \
+   cpu_map/ppc64_POWER8.xml \
+   cpu_map/ppc64_POWERPC_e5500.xml \
$(NULL)
 
 EXTRA_DIST += $(cpumap_DATA)
diff --git a/src/cpu_map/index.xml b/src/cpu_map/index.xml
index 9af190a579..ce4f0204b0 100644
--- a/src/cpu_map/index.xml
+++ b/src/cpu_map/index.xml
@@ -2340,43 +2340,16 @@
   
 
   
-
-
-
+
 
 
-
-  
-  
-
-
-
-  
-  
-  
-
-
-
-  
-  
-  
-  
-
-
-
-  
-  
-
+
+
+
+
 
 
-
-  
-  
-
-
-
-  
-  
-
+
+
   
 
diff --git a/src/cpu_map/ppc64_POWER6.xml b/src/cpu_map/ppc64_POWER6.xml
new file mode 100644
index 00..00e27495f4
--- /dev/null
+++ b/src/cpu_map/ppc64_POWER6.xml
@@ -0,0 +1,6 @@
+
+  
+
+
+  
+
diff --git a/src/cpu_map/ppc64_POWER7.xml b/src/cpu_map/ppc64_POWER7.xml
new file mode 100644
index 00..a071481805
--- /dev/null
+++ b/src/cpu_map/ppc64_POWER7.xml
@@ -0,0 +1,7 @@
+
+  
+
+
+
+  
+
diff --git a/src/cpu_map/ppc64_POWER8.xml b/src/cpu_map/ppc64_POWER8.xml
new file mode 100644
index 00..64d96fc4c4
--- /dev/null
+++ b/src/cpu_map/ppc64_POWER8.xml
@@ -0,0 +1,8 @@
+
+  
+
+
+
+
+  
+
diff --git a/src/cpu_map/ppc64_POWER9.xml b/src/cpu_map/ppc64_POWER9.xml
new file mode 100644
index 00..149fcde924
--- /dev/null
+++ b/src/cpu_map/ppc64_POWER9.xml
@@ -0,0 +1,6 @@
+
+  
+
+
+  
+
diff --git a/src/cpu_map/ppc64_POWERPC_e5500.xml 
b/src/cpu_map/ppc64_POWERPC_e5500.xml
new file mode 100644
index 00..3d64c8926c
--- /dev/null
+++ b/src/cpu_map/ppc64_POWERPC_e5500.xml
@@ -0,0 +1,6 @@
+
+  
+
+
+  
+
diff --git a/src/cpu_map/ppc64_POWERPC_e6500.xml 
b/src/cpu_map/ppc64_POWERPC_e6500.xml
new file mode 100644
index 00..b0d1006076
--- /dev/null
+++ b/src/cpu_map/ppc64_POWERPC_e6500.xml
@@ -0,0 +1,6 @@
+
+  
+
+  
+  
+
diff --git a/src/cpu_map/ppc64_vendors.xml b/src/cpu_map/ppc64_vendors.xml
new file mode 100644
index 00..52ad45c0bd
--- /dev/null
+++ b/src/cpu_map/ppc64_vendors.xml
@@ -0,0 +1,4 @@
+
+  
+  
+
-- 
2.17.1

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

[libvirt] [PATCH v2 3/8] cpu: push more parsing logic into common code

[Daniel P . Berrangé]

The x86 and ppc impls both duplicate some logic when parsing CPU
features. Change the callback signature so that this duplication can be
pushed up a level to common code.

Signed-off-by: Daniel P. Berrangé 
---
 src/cpu/cpu_map.c   |  98 +-
 src/cpu/cpu_map.h   |  22 ++---
 src/cpu/cpu_ppc64.c | 112 ++---
 src/cpu/cpu_x86.c   | 196 +---
 4 files changed, 143 insertions(+), 285 deletions(-)

diff --git a/src/cpu/cpu_map.c b/src/cpu/cpu_map.c
index bcd3e55417..400e6f1427 100644
--- a/src/cpu/cpu_map.c
+++ b/src/cpu/cpu_map.c
@@ -35,31 +35,47 @@
 
 VIR_LOG_INIT("cpu.cpu_map");
 
-VIR_ENUM_IMPL(cpuMapElement, CPU_MAP_ELEMENT_LAST,
-"vendor",
-"feature",
-"model")
-
-
-static int load(xmlXPathContextPtr ctxt,
-cpuMapElement element,
-cpuMapLoadCallback callback,
-void *data)
+static int
+loadData(const char *mapfile,
+ xmlXPathContextPtr ctxt,
+ const char *element,
+ cpuMapLoadCallback callback,
+ void *data)
 {
 int ret = -1;
 xmlNodePtr ctxt_node;
 xmlNodePtr *nodes = NULL;
 int n;
+size_t i;
+int rv;
 
 ctxt_node = ctxt->node;
 
-n = virXPathNodeSet(cpuMapElementTypeToString(element), ctxt, );
-if (n < 0)
+if ((n = virXPathNodeSet(element, ctxt, )) < 0)
 goto cleanup;
 
-if (n > 0 &&
-callback(element, ctxt, nodes, n, data) < 0)
+if (n > 0 && !callback) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Unexpected element '%s' in CPU map '%s'"), element, 
mapfile);
 goto cleanup;
+}
+
+for (i = 0; i < n; i++) {
+xmlNodePtr old = ctxt->node;
+char *name = virXMLPropString(nodes[i], "name");
+if (!name) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("cannot find %s name in CPU map '%s'"), element, 
mapfile);
+goto cleanup;
+}
+VIR_DEBUG("Load %s name %s", element, name);
+ctxt->node = nodes[i];
+rv = callback(ctxt, name, data);
+ctxt->node = old;
+VIR_FREE(name);
+if (rv < 0)
+goto cleanup;
+}
 
 ret = 0;
 
@@ -72,13 +88,14 @@ static int load(xmlXPathContextPtr ctxt,
 
 static int
 cpuMapLoadInclude(const char *filename,
-  cpuMapLoadCallback cb,
+  cpuMapLoadCallback vendorCB,
+  cpuMapLoadCallback featureCB,
+  cpuMapLoadCallback modelCB,
   void *data)
 {
 xmlDocPtr xml = NULL;
 xmlXPathContextPtr ctxt = NULL;
 int ret = -1;
-int element;
 char *mapfile;
 
 if (!(mapfile = virFileFindResource(filename,
@@ -93,13 +110,14 @@ cpuMapLoadInclude(const char *filename,
 
 ctxt->node = xmlDocGetRootElement(xml);
 
-for (element = 0; element < CPU_MAP_ELEMENT_LAST; element++) {
-if (load(ctxt, element, cb, data) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("cannot parse CPU map '%s'"), mapfile);
-goto cleanup;
-}
-}
+if (loadData(mapfile, ctxt, "vendor", vendorCB, data) < 0)
+goto cleanup;
+
+if (loadData(mapfile, ctxt, "feature", featureCB, data) < 0)
+goto cleanup;
+
+if (loadData(mapfile, ctxt, "model", modelCB, data) < 0)
+goto cleanup;
 
 ret = 0;
 
@@ -114,7 +132,9 @@ cpuMapLoadInclude(const char *filename,
 
 static int
 loadIncludes(xmlXPathContextPtr ctxt,
- cpuMapLoadCallback callback,
+ cpuMapLoadCallback vendorCB,
+ cpuMapLoadCallback featureCB,
+ cpuMapLoadCallback modelCB,
  void *data)
 {
 int ret = -1;
@@ -132,7 +152,7 @@ loadIncludes(xmlXPathContextPtr ctxt,
 for (i = 0; i < n; i++) {
 char *filename = virXMLPropString(nodes[i], "filename");
 VIR_DEBUG("Finding CPU map include '%s'", filename);
-if (cpuMapLoadInclude(filename, callback, data) < 0) {
+if (cpuMapLoadInclude(filename, vendorCB, featureCB, modelCB, data) < 
0) {
 VIR_FREE(filename);
 goto cleanup;
 }
@@ -150,7 +170,9 @@ loadIncludes(xmlXPathContextPtr ctxt,
 
 
 int cpuMapLoad(const char *arch,
-   cpuMapLoadCallback cb,
+   cpuMapLoadCallback vendorCB,
+   cpuMapLoadCallback featureCB,
+   cpuMapLoadCallback modelCB,
void *data)
 {
 xmlDocPtr xml = NULL;
@@ -158,7 +180,6 @@ int cpuMapLoad(const char *arch,
 virBuffer buf = VIR_BUFFER_INITIALIZER;
 char *xpath = NULL;
 int ret = -1;
-int element;
 char *mapfile;
 
 if (!(mapfile = virFileFindResource("cpu

[libvirt] [PATCH v2 4/8] cpu: simplify failure cleanup paths

[Daniel P . Berrangé]

Get rid of the separate 'error:' label, so all code paths jump straight
to the 'cleanup:' label.

Signed-off-by: Daniel P. Berrangé 
---
 src/cpu/cpu_ppc64.c | 38 
 src/cpu/cpu_x86.c   | 71 -
 2 files changed, 49 insertions(+), 60 deletions(-)

diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c
index 75da5b77d8..fcba7e9b37 100644
--- a/src/cpu/cpu_ppc64.c
+++ b/src/cpu/cpu_ppc64.c
@@ -288,27 +288,28 @@ ppc64VendorParse(xmlXPathContextPtr ctxt ATTRIBUTE_UNUSED,
 {
 struct ppc64_map *map = data;
 struct ppc64_vendor *vendor;
+int ret = -1;
 
 if (VIR_ALLOC(vendor) < 0)
 return -1;
 
 if (VIR_STRDUP(vendor->name, name) < 0)
-goto error;
+goto cleanup;
 
 if (ppc64VendorFind(map, vendor->name)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("CPU vendor %s already defined"), vendor->name);
-goto error;
+goto cleanup;
 }
 
 if (VIR_APPEND_ELEMENT(map->vendors, map->nvendors, vendor) < 0)
-goto error;
+goto cleanup;
 
-return 0;
+ret = 0;
 
- error:
+ cleanup:
 ppc64VendorFree(vendor);
-return -1;
+return ret;
 }
 
 
@@ -327,15 +328,15 @@ ppc64ModelParse(xmlXPathContextPtr ctxt,
 int ret = -1;
 
 if (VIR_ALLOC(model) < 0)
-goto error;
+goto cleanup;
 
 if (VIR_STRDUP(model->name, name) < 0)
-goto error;
+goto cleanup;
 
 if (ppc64ModelFind(map, model->name)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("CPU model %s already defined"), model->name);
-goto error;
+goto cleanup;
 }
 
 if (virXPathBoolean("boolean(./vendor)", ctxt)) {
@@ -344,14 +345,14 @@ ppc64ModelParse(xmlXPathContextPtr ctxt,
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Invalid vendor element in CPU model %s"),
model->name);
-goto error;
+goto cleanup;
 }
 
 if (!(model->vendor = ppc64VendorFind(map, vendor))) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unknown vendor %s referenced by CPU model %s"),
vendor, model->name);
-goto error;
+goto cleanup;
 }
 }
 
@@ -359,11 +360,11 @@ ppc64ModelParse(xmlXPathContextPtr ctxt,
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Missing PVR information for CPU model %s"),
model->name);
-goto error;
+goto cleanup;
 }
 
 if (VIR_ALLOC_N(model->data.pvr, n) < 0)
-goto error;
+goto cleanup;
 
 model->data.len = n;
 
@@ -374,7 +375,7 @@ ppc64ModelParse(xmlXPathContextPtr ctxt,
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Missing or invalid PVR value in CPU model %s"),
model->name);
-goto error;
+goto cleanup;
 }
 model->data.pvr[i].value = pvr;
 
@@ -382,24 +383,21 @@ ppc64ModelParse(xmlXPathContextPtr ctxt,
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Missing or invalid PVR mask in CPU model %s"),
model->name);
-goto error;
+goto cleanup;
 }
 model->data.pvr[i].mask = pvr;
 }
 
 if (VIR_APPEND_ELEMENT(map->models, map->nmodels, model) < 0)
-goto error;
+goto cleanup;
 
 ret = 0;
 
  cleanup:
+ppc64ModelFree(model);
 VIR_FREE(vendor);
 VIR_FREE(nodes);
 return ret;
-
- error:
-ppc64ModelFree(model);
-goto cleanup;
 }
 
 
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index 73af9d0885..8e4a3d0f77 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -723,15 +723,15 @@ x86VendorParse(xmlXPathContextPtr ctxt,
 int ret = -1;
 
 if (VIR_ALLOC(vendor) < 0)
-goto error;
+goto cleanup;
 
 if (VIR_STRDUP(vendor->name, name) < 0)
-goto error;
+goto cleanup;
 
 if (x86VendorFind(map, vendor->name)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("CPU vendor %s already defined"), vendor->name);
-goto error;
+goto cleanup;
 }
 
 string = virXPathString("string(@string)", ctxt);
@@ -739,24 +739,21 @@ x86VendorParse(xmlXPathContextPtr ctxt,
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Missing vendor string for CPU vendor %s"),
vendor->name);
-goto error;
+goto cleanup;
 }
 
 if (virCPUx86VendorToCPUID(string, >cpuid) < 0)
-goto error;
+goto clean

[libvirt] [PATCH v2 8/8] xml: report the filename (if any) when parsing files

[Daniel P . Berrangé]

A generic "failed to parse xml document" message without telling us
which XML file failed is quite unhelpful.

Signed-off-by: Daniel P. Berrangé 
---
 src/util/virxml.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/util/virxml.c b/src/util/virxml.c
index a03a747e60..d1926f4605 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -847,7 +847,8 @@ virXMLParseHelper(int domcode,
 
 if (virGetLastErrorCode() == VIR_ERR_OK) {
 virGenericReportError(domcode, VIR_ERR_XML_ERROR,
-  "%s", _("failed to parse xml document"));
+  _("failed to parse xml document '%s'"),
+  filename ? filename : "[inline data]");
 }
 goto cleanup;
 }
-- 
2.17.1

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

[libvirt] [PATCH 2/3] qemu: rename method for getting preferred machine type

[Daniel P . Berrangé]

The virQEMUCapsGetDefaultMachine() method doesn't get QEMU's default
machine any more, instead it gets the historical default that libvirt
prefers for each arch. Rename it, so that the old name can be used for
getting QEMU's default.

Signed-off-by: Daniel P. Berrangé 
---
 src/qemu/qemu_capabilities.c | 8 ++--
 src/qemu/qemu_capabilities.h | 2 +-
 tests/domaincapstest.c   | 2 +-
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index a0a1b97f1d..4e4f732889 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -4876,7 +4876,7 @@ virQEMUCapsCacheLookupDefault(virFileCachePtr cache,
 goto cleanup;
 }
 } else {
-machine = virQEMUCapsGetDefaultMachine(qemuCaps);
+machine = virQEMUCapsGetPreferredMachine(qemuCaps);
 }
 
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
@@ -4935,8 +4935,12 @@ virQEMUCapsIsMachineSupported(virQEMUCapsPtr qemuCaps,
 }
 
 
+/*
+ * The preferred machine to use if none is listed explicitly
+ * Note that this may differ from QEMU's own default machine
+ */
 const char *
-virQEMUCapsGetDefaultMachine(virQEMUCapsPtr qemuCaps)
+virQEMUCapsGetPreferredMachine(virQEMUCapsPtr qemuCaps)
 {
 if (!qemuCaps->nmachineTypes)
 return NULL;
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 26813a908c..88e81be09b 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -605,7 +605,7 @@ bool virQEMUCapsSupportsGICVersion(virQEMUCapsPtr qemuCaps,
 bool virQEMUCapsIsMachineSupported(virQEMUCapsPtr qemuCaps,
const char *canonical_machine);
 
-const char *virQEMUCapsGetDefaultMachine(virQEMUCapsPtr qemuCaps);
+const char *virQEMUCapsGetPreferredMachine(virQEMUCapsPtr qemuCaps);
 
 int virQEMUCapsInitGuestFromBinary(virCapsPtr caps,
const char *binary,
diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c
index 06e77fd586..3b94cad223 100644
--- a/tests/domaincapstest.c
+++ b/tests/domaincapstest.c
@@ -168,7 +168,7 @@ fillQemuCaps(virDomainCapsPtr domCaps,
 
 if (!domCaps->machine &&
 VIR_STRDUP(domCaps->machine,
-   virQEMUCapsGetDefaultMachine(qemuCaps)) < 0)
+   virQEMUCapsGetPreferredMachine(qemuCaps)) < 0)
 goto cleanup;
 
 if (virQEMUCapsFillDomainCaps(caps, domCaps, qemuCaps,
-- 
2.17.1

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

[libvirt] [PATCH 3/3] qemu: fix default machine for argv -> xml convertor

[Daniel P . Berrangé]

Historically the argv -> xml convertor wanted the same default machine
as we'd set when parsing xml. The latter has now changed, however, to
use a default defined by libvirt. The former needs fixing to again
honour the default QEMU machine.

This exposed a bug in handling for the aarch64 target, as QEMU does not
define any default machine. Thus we should not having been accepting
argv without a -machine provided.

Signed-off-by: Daniel P. Berrangé 
---
 src/qemu/qemu_capabilities.c  | 17 +++-
 src/qemu/qemu_capabilities.h  |  1 +
 src/qemu/qemu_driver.c|  6 ++-
 src/qemu/qemu_parse_command.c | 34 ++--
 src/qemu/qemu_parse_command.h |  8 +++-
 tests/qemuargv2xmldata/nomachine-aarch64.args | 11 -
 tests/qemuargv2xmldata/nomachine-aarch64.xml  | 40 ---
 tests/qemuargv2xmldata/nomachine-ppc64.xml|  4 +-
 tests/qemuargv2xmldata/nomachine-x86_64.xml   |  4 +-
 tests/qemuargv2xmldata/pseries-disk.xml   |  4 +-
 tests/qemuargv2xmldata/pseries-nvram.xml  |  4 +-
 tests/qemuargv2xmltest.c  | 18 -
 .../caps_1.5.3.x86_64.xml |  2 +-
 .../caps_1.6.0.x86_64.xml |  2 +-
 .../caps_1.7.0.x86_64.xml |  2 +-
 .../caps_2.1.1.x86_64.xml |  2 +-
 .../caps_2.10.0.ppc64.xml |  2 +-
 .../caps_2.10.0.s390x.xml |  2 +-
 .../caps_2.10.0.x86_64.xml|  2 +-
 .../caps_2.11.0.s390x.xml |  2 +-
 .../caps_2.11.0.x86_64.xml|  2 +-
 .../caps_2.12.0.ppc64.xml |  2 +-
 .../caps_2.12.0.s390x.xml |  2 +-
 .../caps_2.12.0.x86_64.xml|  2 +-
 .../caps_2.4.0.x86_64.xml |  2 +-
 .../caps_2.5.0.x86_64.xml |  2 +-
 .../qemucapabilitiesdata/caps_2.6.0.ppc64.xml |  2 +-
 .../caps_2.6.0.x86_64.xml |  2 +-
 .../qemucapabilitiesdata/caps_2.7.0.s390x.xml |  2 +-
 .../caps_2.7.0.x86_64.xml |  2 +-
 .../qemucapabilitiesdata/caps_2.8.0.s390x.xml |  2 +-
 .../caps_2.8.0.x86_64.xml |  2 +-
 .../qemucapabilitiesdata/caps_2.9.0.ppc64.xml |  2 +-
 .../qemucapabilitiesdata/caps_2.9.0.s390x.xml |  2 +-
 .../caps_2.9.0.x86_64.xml |  2 +-
 .../qemucapabilitiesdata/caps_3.0.0.ppc64.xml |  2 +-
 .../caps_3.0.0.x86_64.xml |  2 +-
 37 files changed, 97 insertions(+), 104 deletions(-)
 delete mode 100644 tests/qemuargv2xmldata/nomachine-aarch64.args
 delete mode 100644 tests/qemuargv2xmldata/nomachine-aarch64.xml

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 4e4f732889..72b550ae16 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1625,6 +1625,7 @@ virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps)
 goto error;
 ret->machineTypes[i].maxCpus = qemuCaps->machineTypes[i].maxCpus;
 ret->machineTypes[i].hotplugCpus = 
qemuCaps->machineTypes[i].hotplugCpus;
+ret->machineTypes[i].qemuDefault = 
qemuCaps->machineTypes[i].qemuDefault;
 }
 
 if (VIR_ALLOC_N(ret->gicCapabilities, qemuCaps->ngicCapabilities) < 0)
@@ -2042,6 +2043,17 @@ const char 
*virQEMUCapsGetCanonicalMachine(virQEMUCapsPtr qemuCaps,
 return name;
 }
 
+const char *virQEMUCapsGetDefaultMachine(virQEMUCapsPtr qemuCaps)
+{
+size_t i;
+
+for (i = 0; i < qemuCaps->nmachineTypes; i++) {
+if (qemuCaps->machineTypes[i].qemuDefault)
+return qemuCaps->machineTypes[i].name;
+}
+
+return NULL;
+}
 
 int virQEMUCapsGetMachineMaxCpus(virQEMUCapsPtr qemuCaps,
  const char *name)
@@ -3776,10 +3788,11 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps)
   qemuCaps->machineTypes[i].alias);
 if (qemuCaps->machineTypes[i].hotplugCpus)
 virBufferAddLit(, " hotplugCpus='yes'");
+virBufferAsprintf(, " maxCpus='%u'",
+  qemuCaps->machineTypes[i].maxCpus);
 if (qemuCaps->machineTypes[i].qemuDefault)
 virBufferAddLit(, " default='yes'");
-virBufferAsprintf(, " maxCpus='%u'/>\n",
-  qemuCaps->machineTypes[i].maxCpus);
+virBufferAddLit(, "/>\n");
 }
 
 for (i = 0; i < qemuCaps->ngicCapabilities; i++) {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 88e81be09b..a410885215 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -558,6 +558,7 @@ bool virQEMUCapsIsCPUModeSupported(virQEMUCapsPtr qemuCaps,
virCPUMode mode);
 const char *virQEMUCapsGetCanonicalMachine(virQEMUCap

Re: [libvirt] [PATCH v3 1/3] vsh: Add API for printing tables.

[Daniel P . Berrangé]

On Thu, Aug 16, 2018 at 02:05:45PM +0200, Ján Tomko wrote:
> On Thu, Aug 16, 2018 at 12:56:24PM +0200, Simon Kobyda wrote:
> > It solves problems with alignment of columns. Width of each column
> > is calculated by its biggest cell. Should solve unicode bug.
> > In future, it may be implemented in virsh, virt-admin...
> > 
> > This API has 5 public functions:
> > - vshTableNew - adds new table and defines its header
> > - vshTableRowAppend - appends new row (for same number of columns as in
> > header)
> > - vshTablePrintToStdout
> > - vshTablePrintToString
> > - vshTableFree
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1574624
> > https://bugzilla.redhat.com/show_bug.cgi?id=1584630
> > 
> > Signed-off-by: Simon Kobyda 
> > ---
> > tools/Makefile.am |   4 +-
> > tools/vsh-table.c | 413 ++
> > tools/vsh-table.h |  42 +
> > 3 files changed, 458 insertions(+), 1 deletion(-)
> > create mode 100644 tools/vsh-table.c
> > create mode 100644 tools/vsh-table.h
> > 

> > +/**
> > + * vshTableGetColumnsWidths:
> > + * @table: table
> > + * @maxwidths: maximum count of characters for each columns
> > + * @widths: count of characters for each cell in the table
> > + *
> > + * Fill passed @maxwidths and @widths arrays with maximum number
> > + * of characters for columns and number of character per each
> > + * table cell, respectively.
> > + *
> > + * Handle unicode strings (user must have multibyte locale)
> > + */
> > +static int
> > +vshTableGetColumnsWidths(vshTablePtr table,
> > + size_t *maxwidths,
> > + size_t **widths,
> > + bool header)
> > +{
> > +int ret = -1;
> > +size_t i = 1;
> > +size_t j;
> > +size_t len;
> > +int tmp;
> > +wchar_t *wstr = NULL;
> > +size_t wstrlen;
> > +
> > +if (header)
> > +i = 0;
> > +else
> > +i = 1;
> > +for (; i < table->nrows; i++) {
> > +vshTableRowPtr row = table->rows[i];
> > +
> > +for (j = 0; j < row->ncells; j++) {
> 
> 
> > +/* strlen should return maximum possible length needed */
> > +wstrlen = strlen(row->cells[j]);
> > +VIR_FREE(wstr);
> > +if (VIR_ALLOC_N(wstr, wstrlen) < 0)
> > +goto cleanup;
> > +/* mbstowcs fails if machine is using singlebyte locale
> > + * and user tries to convert unicode(multibyte)
> > + * */
> > +if (mbstowcs(wstr, row->cells[j], wstrlen) ==
> > +(size_t) -1) {
> > +len = wstrlen;
> > +} else {
> > +tmp = wcswidth(wstr, wstrlen);
> > +if (tmp < 0)
> > +goto cleanup;
> > +len = (size_t)((unsigned)tmp);
> > +}
> 
> Whatever solution you use for converting a multi-byte string to a
> maximum count of characters, please put it in a separate function.
> That would make this function more readable.

Note that unfortnately libunistring is "GPLv2+ or LGPLv3+", so is not
compatible with libvirt.so

It is fine to use it from virsh since that's free to be GPLv2+ only,
but we mustn't link libunistring to libvirt.so.  So any helper func
would have to stay in the tools dir just for virsh/virt-admin


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

[libvirt] [PATCH v2 0/8] cpu: modularize the CPU map data file

[Daniel P . Berrangé]

Currently we have a cpu_map.xml file that contains all the features and
CPU models for all architectures in one place. I frequently find myself
wondering about the differences between CPU models, but it is hard to
compare them as the list of features is huge.

With this patch series we end up with a large set of small files, one
per named CPU model, along with one for the feature and vendor
definitions

   cpu_map/index.xml
   cpu_map/ppc64_POWER6.xml
   cpu_map/ppc64_POWER7.xml
   cpu_map/ppc64_POWER8.xml
   cpu_map/ppc64_POWER9.xml
   cpu_map/ppc64_POWERPC_e5500.xml
   cpu_map/ppc64_POWERPC_e6500.xml
   cpu_map/ppc64_vendors.xml
   cpu_map/x86_486.xml
   cpu_map/x86_athlon.xml
   cpu_map/x86_Broadwell-IBRS.xml
   cpu_map/x86_Broadwell-noTSX-IBRS.xml
   cpu_map/x86_Broadwell-noTSX.xml
   cpu_map/x86_Broadwell.xml
   cpu_map/x86_Conroe.xml
   cpu_map/x86_core2duo.xml
   cpu_map/x86_coreduo.xml
   cpu_map/x86_cpu64-rhel5.xml
   cpu_map/x86_cpu64-rhel6.xml
   cpu_map/x86_EPYC-IBRS.xml
   cpu_map/x86_EPYC.xml
   cpu_map/x86_features.xml
   cpu_map/x86_Haswell-IBRS.xml
   cpu_map/x86_Haswell-noTSX-IBRS.xml
   cpu_map/x86_Haswell-noTSX.xml
   cpu_map/x86_Haswell.xml
   cpu_map/x86_IvyBridge-IBRS.xml
   cpu_map/x86_IvyBridge.xml
   cpu_map/x86_kvm32.xml
   cpu_map/x86_kvm64.xml
   cpu_map/x86_n270.xml
   cpu_map/x86_Nehalem-IBRS.xml
   cpu_map/x86_Nehalem.xml
   cpu_map/x86_Opteron_G1.xml
   cpu_map/x86_Opteron_G2.xml
   cpu_map/x86_Opteron_G3.xml
   cpu_map/x86_Opteron_G4.xml
   cpu_map/x86_Opteron_G5.xml
   cpu_map/x86_Penryn.xml
   cpu_map/x86_pentium2.xml
   cpu_map/x86_pentium3.xml
   cpu_map/x86_pentiumpro.xml
   cpu_map/x86_pentium.xml
   cpu_map/x86_phenom.xml
   cpu_map/x86_qemu32.xml
   cpu_map/x86_qemu64.xml
   cpu_map/x86_SandyBridge-IBRS.xml
   cpu_map/x86_SandyBridge.xml
   cpu_map/x86_Skylake-Client-IBRS.xml
   cpu_map/x86_Skylake-Client.xml
   cpu_map/x86_Skylake-Server-IBRS.xml
   cpu_map/x86_Skylake-Server.xml
   cpu_map/x86_vendors.xml
   cpu_map/x86_Westmere-IBRS.xml
   cpu_map/x86_Westmere.xml

The main cpu_map/index.xml file is now just a list of 
statements to pull in the individual files

Now we can easily see the differences in each model:

$ diff cpu_map/x86_Broadwell.xml cpu_map/x86_Skylake-Client.xml
2,3c2,3
<   
< 
---
>   
> 
5a6
> 
8a10
> 
18a21
> 
30a34
> 
42a47
> 
56a62
> 
57a64
> 
58a66,67
> 
> 

Changed in v2:

 - Moved all XML files into a new src/cpu_map/ directory
 - Simplify the goto labels for error code paths.
 - Code style fixes

Daniel P. Berrangé (8):
  cpu: allow include files for CPU definition
  cpu: fix cleanup when signature parsing fails
  cpu: push more parsing logic into common code
  cpu: simplify failure cleanup paths
  cpu: move the CPU map data files into a src/cpu_map directory
  cpu: split PPC64 map data into separate files
  cpu: split x86 map data into separate files
  xml: report the filename (if any) when parsing files

 libvirt.spec.in  |2 +-
 mingw-libvirt.spec.in|4 +-
 src/Makefile.am  |7 +-
 src/cpu/cpu_map.c|  161 +-
 src/cpu/cpu_map.h|   22 +-
 src/cpu/cpu_map.xml  | 2382 --
 src/cpu/cpu_ppc64.c  |  142 +-
 src/cpu/cpu_x86.c|  255 +--
 src/cpu_map/Makefile.inc.am  |   61 +
 src/cpu_map/index.xml|   75 +
 src/cpu_map/ppc64_POWER6.xml |6 +
 src/cpu_map/ppc64_POWER7.xml |7 +
 src/cpu_map/ppc64_POWER8.xml |8 +
 src/cpu_map/ppc64_POWER9.xml |6 +
 src/cpu_map/ppc64_POWERPC_e5500.xml  |6 +
 src/cpu_map/ppc64_POWERPC_e6500.xml  |6 +
 src/cpu_map/ppc64_vendors.xml|4 +
 src/cpu_map/x86_486.xml  |7 +
 src/cpu_map/x86_Broadwell-IBRS.xml   |   61 +
 src/cpu_map/x86_Broadwell-noTSX-IBRS.xml |   59 +
 src/cpu_map/x86_Broadwell-noTSX.xml  |   58 +
 src/cpu_map/x86_Broadwell.xml|   60 +
 src/cpu_map/x86_Conroe.xml   |   33 +
 src/cpu_map/x86_EPYC-IBRS.xml|   73 +
 src/cpu_map/x86_EPYC.xml |   72 +
 src/cpu_map/x86_Haswell-IBRS.xml |   57 +
 src/cpu_map/x86_Haswell-noTSX-IBRS.xml   |   55 +
 src/cpu_map/x86_Haswell-noTSX.xml|   54 +
 src/cpu_map/x86_Haswell.xml  |   56 +
 src/cpu_map/x86_IvyBridge-IBRS.xml   |   51 +
 src/cpu_map/x86_IvyBridge.xml|   50 +
 src/cpu_map/x86_Nehalem-IBRS.xml |   38 +
 src/cpu_map/x86_Nehalem.xml  |   37 +
 src/cpu_map/x86_Opteron_G1.xml   |   31 +
 src/cpu_map/x86_Opteron_G2.xml   |   35 +
 src/cpu_map/x86_Opteron_G3.xml   |   40 +
 src/cpu_map/x86_Opteron

[libvirt] [PATCH v2 2/8] cpu: fix cleanup when signature parsing fails

[Daniel P . Berrangé]

Two pieces of code accidentally jumped to the wrong label when they
failed causing incorrect cleanup, returning a partially initialized
CPU model struct.

Signed-off-by: Daniel P. Berrangé 
---
 src/cpu/cpu_x86.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index 809da94117..a045a8280c 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -1242,7 +1242,7 @@ x86ModelParse(xmlXPathContextPtr ctxt,
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Invalid CPU signature family in model %s"),
model->name);
-goto cleanup;
+goto error;
 }
 
 rc = virXPathUInt("string(./signature/@model)", ctxt, );
@@ -1250,7 +1250,7 @@ x86ModelParse(xmlXPathContextPtr ctxt,
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Invalid CPU signature model in model %s"),
model->name);
-goto cleanup;
+goto error;
 }
 
 model->signature = x86MakeSignature(sigFamily, sigModel, 0);
-- 
2.17.1

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

[libvirt] [PATCH v2 5/8] cpu: move the CPU map data files into a src/cpu_map directory

[Daniel P . Berrangé]

In preparation for splitting up the CPU map data file, move it into a
dedicated directory of its own.

Signed-off-by: Daniel P. Berrangé 
---
 libvirt.spec.in|  2 +-
 mingw-libvirt.spec.in  |  4 ++--
 src/Makefile.am|  7 +--
 src/cpu/cpu_map.c  | 10 +-
 src/cpu_map/Makefile.inc.am|  7 +++
 src/{cpu/cpu_map.xml => cpu_map/index.xml} |  0
 6 files changed, 16 insertions(+), 14 deletions(-)
 create mode 100644 src/cpu_map/Makefile.inc.am
 rename src/{cpu/cpu_map.xml => cpu_map/index.xml} (100%)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 883c8a49e7..09f654b2ec 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1854,7 +1854,7 @@ exit 0
 %{_datadir}/libvirt/schemas/storagepool.rng
 %{_datadir}/libvirt/schemas/storagevol.rng
 
-%{_datadir}/libvirt/cpu_map.xml
+%{_datadir}/libvirt/cpu_map/*.xml
 
 %{_datadir}/libvirt/test-screenshot.png
 
diff --git a/mingw-libvirt.spec.in b/mingw-libvirt.spec.in
index cc1e619927..b28e40f7f7 100644
--- a/mingw-libvirt.spec.in
+++ b/mingw-libvirt.spec.in
@@ -260,7 +260,7 @@ rm -rf 
$RPM_BUILD_ROOT%{mingw64_libexecdir}/libvirt-guests.sh
 %{mingw32_datadir}/libvirt/api/libvirt-qemu-api.xml
 %{mingw32_datadir}/libvirt/api/libvirt-admin-api.xml
 
-%{mingw32_datadir}/libvirt/cpu_map.xml
+%{mingw32_datadir}/libvirt/cpu_map/*.xml
 
 %{mingw32_datadir}/libvirt/test-screenshot.png
 
@@ -347,7 +347,7 @@ rm -rf 
$RPM_BUILD_ROOT%{mingw64_libexecdir}/libvirt-guests.sh
 %{mingw64_datadir}/libvirt/api/libvirt-qemu-api.xml
 %{mingw64_datadir}/libvirt/api/libvirt-admin-api.xml
 
-%{mingw64_datadir}/libvirt/cpu_map.xml
+%{mingw64_datadir}/libvirt/cpu_map/*.xml
 
 %{mingw64_datadir}/libvirt/test-screenshot.png
 
diff --git a/src/Makefile.am b/src/Makefile.am
index db8c8ebd1a..2a3ed0d42d 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -100,6 +100,7 @@ man7_MANS =
 include util/Makefile.inc.am
 include conf/Makefile.inc.am
 include cpu/Makefile.inc.am
+include cpu_map/Makefile.inc.am
 include security/Makefile.inc.am
 include access/Makefile.inc.am
 include logging/Makefile.inc.am
@@ -364,12 +365,6 @@ check-local: check-protocol check-symfile check-symsorting 
\
 .PHONY: check-protocol $(PROTOCOL_STRUCTS:structs=struct)
 
 
-
-
-pkgdata_DATA = cpu/cpu_map.xml
-
-EXTRA_DIST +=  $(pkgdata_DATA)
-
 #
 #
 # Build up list of libvirt.la source files based on configure conditions
diff --git a/src/cpu/cpu_map.c b/src/cpu/cpu_map.c
index 400e6f1427..2079767df8 100644
--- a/src/cpu/cpu_map.c
+++ b/src/cpu/cpu_map.c
@@ -99,8 +99,8 @@ cpuMapLoadInclude(const char *filename,
 char *mapfile;
 
 if (!(mapfile = virFileFindResource(filename,
-abs_topsrcdir "/src/cpu",
-PKGDATADIR)))
+abs_topsrcdir "/src/cpu_map",
+PKGDATADIR "/cpu_map")))
 return -1;
 
 VIR_DEBUG("Loading CPU map include from %s", mapfile);
@@ -182,9 +182,9 @@ int cpuMapLoad(const char *arch,
 int ret = -1;
 char *mapfile;
 
-if (!(mapfile = virFileFindResource("cpu_map.xml",
-abs_topsrcdir "/src/cpu",
-PKGDATADIR)))
+if (!(mapfile = virFileFindResource("index.xml",
+abs_topsrcdir "/src/cpu_map",
+PKGDATADIR "/cpu_map")))
 return -1;
 
 VIR_DEBUG("Loading '%s' CPU map from %s", NULLSTR(arch), mapfile);
diff --git a/src/cpu_map/Makefile.inc.am b/src/cpu_map/Makefile.inc.am
new file mode 100644
index 00..91728b9200
--- /dev/null
+++ b/src/cpu_map/Makefile.inc.am
@@ -0,0 +1,7 @@
+
+cpumapdir = $(pkgdatadir)/cpu_map
+cpumap_DATA = \
+   cpu_map/index.xml \
+   $(NULL)
+
+EXTRA_DIST += $(cpumap_DATA)
diff --git a/src/cpu/cpu_map.xml b/src/cpu_map/index.xml
similarity index 100%
rename from src/cpu/cpu_map.xml
rename to src/cpu_map/index.xml
-- 
2.17.1

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

Re: [libvirt] [Qemu-devel] clean/simple Q35 support in libvirt+QEMU for guest OSes that don't support virtio-1.0

[Daniel P . Berrangé]

On Fri, Aug 17, 2018 at 12:35:11PM +0200, Andrea Bolognani wrote:
> On Fri, 2018-08-17 at 10:29 +0100, Daniel P. Berrangé wrote:
> > On Thu, Aug 16, 2018 at 06:20:29PM -0400, Laine Stump wrote:
> > > 5) Some guest OSes that we still want to support (and which would
> > > otherwise work okay on a Q35 virtual machine) have virtio drivers too
> > > old to support virtio-1.0 (CentOS6 and RHEL6 are examples of such OSes),
> > > but due to the chain of reasons listed above, the "standard" config for
> > > a Q35 guest generated by libvirt doesn't support virtio-0.9, hence
> > > doesn't support these guest OSes.
> > 
> > Note when talking about "support" you're really saying it from the
> > downstream vendor, specifically RHEL, POV. From upstream or Fedora POV
> > essentially all x86 OS ever made are in scope for running under QEMU
> > if suitable virtual hardware models have been provided. QEMU doesn't
> > maintain any whitelist of "supported" OS that differs from what is
> > technically capable of being run, in the way downstream vendors do.
> 
> Well, at least in the case of RHEL 6, "not supported" means that it
> will not boot at all on q35 with the default guest topology created
> by libvirt, so that's not really a downstream-only problem :)

I mean from an upstream POV we still support RHEL-6 fine in i440fx,
so there's no reason to particularly care about RHEL-6 with q35
upstream. It is only downstream decision to try to force it to
use q35, despite it not working right today.

> > > C) inside libvirt, the implementation of the "virtio-0.9" model is
> > > identical to "virtio", except that the VIR_PCI_CONNECT_TYPE flags for
> > > these devices contain VIR_PCI_CONNECT_TYPE_PCI rather than
> > > VIR_PCI_CONNECT_TYPE_PCIE, resulting in those devices being assigned to
> > > a legacy PCI slot, and thus they would be transitional mode by default.
> > 
> > For 'virtio-0.9' libvirt should set "disable-modern=yes" in QEMU args
> > 
> > For 'virtio-1.0' libvirt should set "disable-legacy=yes" in QEMU args
> 
> If we decide we want to explicitly spell out the options instead
> of relying on QEMU changing behavior based on the slot type, which
> is probably a good idea anyway, I think we should have
> 
>   virtio-0.9 => disable-legacy=no,disable-modern=no
>   virtio-1.0 => disable-legacy=yes,disable-modern=no
> 
> There's basically no reason to have a device legacy-only rather
> than transitional, and spelling out both options instead of only
> one of them just seems more robust.

>From a testing POV it is desirable to be able to have legacy-only.
There is also possibility that guest OS impl 1.0 in a buggy manner,
so forcing legacy only is desirable.

The existing device still already provides a transitional option
on i440fx, and on Q35 if you do explicit placement in a PCI slot.
So I don't think there's a good reason to have a second transitional
device type, especially if we're naming it virtio-0.9, it is rather
misleading if it would be in fact able to run virtio-1.0 mode.


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

Re: [libvirt] [Qemu-devel] clean/simple Q35 support in libvirt+QEMU for guest OSes that don't support virtio-1.0

[Daniel P . Berrangé]

On Thu, Aug 16, 2018 at 06:20:29PM -0400, Laine Stump wrote:
> Summary of the problem:
> 
> 1) We want to persuade libvirt+QEMU users to move away from the i440fx
> machinetype in favor of Q35. (NB: Someday this *might* lead to the
> ability to deprecate and even remove the 440fx machinetype, but even if
> that were to happen, it would be a *very long* time from now, so this
> discussion is *not* about that!)

There are plenty of OS that will never support Q35 and are still interesting
to use under Q35. The set which could use Q35, but lack virtio1.0 is fairly
small. So removal of i440fx is really only something for downstream KVM vendors
to consider. Those vendors only care about modern OS, but upstream is much
more open minded about what QEMU is used for, so I see it probably living
forever in upstream, or at least long enough that current maintaniers will
be retired ;-P.


> 2) When Q35 machinetype is used, libvirt assigns virtio devices to a
> slot on a PCI Express controller (because why have modern PCIe
> controllers/slots available but force everything onto clunky old legacy
> controllers?).
> 
> 3) When a virtio device is plugged into an Express controller, QEMU
> disables the device's IO port space, and it is put into "modern-only"
> mode (this is done to avoid a rapid exhaustion of limited IO port space).
> 
> 4) modern-only virtio devices won't work with a legacy (virtio-0.9-only)
> guest driver, because virtio-0.9 requires IO port space.
> 
> 5) Some guest OSes that we still want to support (and which would
> otherwise work okay on a Q35 virtual machine) have virtio drivers too
> old to support virtio-1.0 (CentOS6 and RHEL6 are examples of such OSes),
> but due to the chain of reasons listed above, the "standard" config for
> a Q35 guest generated by libvirt doesn't support virtio-0.9, hence
> doesn't support these guest OSes.

Note when talking about "support" you're really saying it from the
downstream vendor, specifically RHEL, POV. From upstream or Fedora POV
essentially all x86 OS ever made are in scope for running under QEMU
if suitable virtual hardware models have been provided. QEMU doesn't
maintain any whitelist of "supported" OS that differs from what is
technically capable of being run, in the way downstream vendors do.

> And here's a list of possible solutions to this problem (note that
> "consumers" means management applications such as OpenStack, oVirt,
> virt-manager, virt-install, gnome-boxes, etc. In all cases, it's assumed
> that the consumer's decision on the action to take will be based on
> information from libosinfo). For completeness, I've included even the
> possibilities that have been rejected, along with a brief synopsis of
> (at least part of) the reason for rejection:
> 
>   (1) Add some way libvirt consumers can ask libvirt to place
>   virtio devices on a legacy pci slot instead of pcie when
>   the machinetype is q35 (qemu sets virtio devices in legacy
>   PCI slots to transitional mode, so io port space is enabled
>   and virtio-0.0 drivers will work).
> 
>   This has been proposed on libvir-list, but rejected. Here is
>   the most elquently stated reasoning for the rejection I could
>   find (with thanks to Dan Berrange):
> 
>  The domain XML is a way to express the configuration
>  of the guest virtual machine.  What we're talking about
>  here is a policy tunable for an internal libvirt QEMU
>  driver algorithm, as so does not belong anywhere in the
>  domain XML.

Indeed, that's a guiding principal in general, not just for this PCI
question.

>   (2) Add full-blown pci enumeration support to all libvirt consumers
>   (i.e. they will need to build a model of the PCI bus topology
>   of each guest, and keep track of which addresses are in use).
>   They can then manually place virtio devices on legacy pci slots
>   (again, triggering transitional mode) when the intended guest
>   OS doesn't support virtio-0.9.
> 
>   (This is seen as requiring too much duplicated effort for
>   development and support/maintenance, since up until now libvirt
>   has been the single point of action for PCI address assignment
>   (well, QEMU can do it too, but for > 10 years libvirt has
>   *always* provided full PCI addresses for all devices)

It really depends on the scope of the mgmt app - at some point the mgmt
apps needs to take charge to some degree if it has particular ideas
about how a machine should look. Libvirt's placement strategy is a good
default for 95% of use cases, but it'll never be 100%. An example is
setting up a particular PCI topology that is guest NUMA node aware,
using expander buses.

So some apps might take this option, but in the common case it is
undesirable.

>   (3) Add virtio-1.0 support to all guest OSes. If this is done,
>   existing libvirt configs will work.
> 
>   (Aside from the difficulty of backporting, and the fact that
>

Re: [libvirt] [RFC] secdrivers remembering original labels

[Daniel P . Berrangé]

On Mon, Aug 06, 2018 at 12:01:03PM +0200, Michal Prívozník wrote:
> On 08/06/2018 10:30 AM, Daniel P. Berrangé wrote:
> 
> >>
> >> So do we really need virtlockd for this? I mean, the whole purpose of it
> >> is to hold locks so that libvirtd can restart independently, without
> >> losing the lock attached. However, since the metadata lock will be held
> >> only for a fraction of a second and will be not held through more than a
> >> single API aren't we just fine with libvirtd holding the lock? The way I
> >> imagine this to work is the following:
> > 
> > There were two reasons for virtlockd. The first was the persistent holding
> > of locks, but the other reasons is that POSIX fcntl() locks are horrific.
> > If one thread has an FD open with a lock held, if another thread opens and
> > closes the same underlying file, the first thread's lock will get dropped.
> > 
> > We can't be confident that other threads won't open the file, so the only
> > way to be safe was to put locking in to a separate process where we know
> > exactly what all threads are doing.
> 
> Ah. That's terrible. But IIUC avoidable by using OFDs instead (which are
> not available on BSD I presume).

Yep, OFD is a (nice) Linux-ism but only exists in pretty recent Linux,
and no non-Linux OS, so we can't rely on it.

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

[libvirt] [PATCH] qemu: ensure default machine types don't change if QEMU changes

[Daniel P . Berrangé]

It is increasingly likely that some distro is going to change the
default "x86" machine type in QEMU from "pc" to "q35". This will
certainly break existing applications which write their XML on the
assumption that it is using a "pc" machine by default. For example they'll
lack a IDE CDROM and get PCIe instead of PCI which changes the topology
radically.

Libvirt promises to isolate applications from hypervisor changes that
may cause incompatibilities, so we must ensure that we always use the
"pc" machine type if it is available. Only use QEMU's own reported
default machine type if "pc" does not exist.

This issue is not x86-only, other arches are liable t change their
default machine, while some arches don't report any default at all
causing libvirt to pick the first machine in the list. Thus to
guarantee stability to applications, declare a preferred default
machine for all architectures we currently support with QEMU.

Note this change assumes there will always be a "pc" alias as long as a
versioned "pc-XXX" machine type exists. If QEMU were to ship a "pc-XXX"
machine type but not provide the "pc" alias, it is too hard to decide
which to default so. Versioned machine types are supposed to be
considered opaque strings, so we can't apply any sensible ordering
ourselves and QEMU isn't reporting the list of machines in any sensible
ordering itself.

Signed-off-by: Daniel P. Berrangé 
---
 src/qemu/qemu_capabilities.c | 79 ++--
 1 file changed, 76 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 0fb800589a..045e2bd489 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2233,6 +2233,61 @@ virQEMUCapsProbeQMPDevices(virQEMUCapsPtr qemuCaps,
 }
 
 
+/* Historically QEMU x86 targets defaulted to 'pc' machine type but
+ * in future x86_64 might switch 'q35'. Such a change is considered
+ * an ABI break from libvirt's POV. Other QEMU targets may not declare
+ * a default macine at all, causing libvirt to use the first reported
+ * machinbe in the list.
+ *
+ * Here we record a preferred default machine for all arches, so
+ * that we're not vulnerable to changes in QEMU defaults or machine
+ * list ordering.
+ */
+static const char *preferredMachines[VIR_ARCH_LAST] =
+{
+[VIR_ARCH_ALPHA] = "clipper",
+[VIR_ARCH_ARMV6L] = NULL, /* No QEMU impl */
+[VIR_ARCH_ARMV7L] = "integratorcp",
+[VIR_ARCH_ARMV7B] = "integratorcp",
+
+[VIR_ARCH_AARCH64] = "integratorcp",
+[VIR_ARCH_CRIS] = "axis-dev88",
+[VIR_ARCH_I686] = "pc",
+[VIR_ARCH_ITANIUM] = NULL, /* doesn't exist in QEMU any more */
+[VIR_ARCH_LM32] = "lm32-evr",
+
+[VIR_ARCH_M68K] = "mcf5208evb",
+[VIR_ARCH_MICROBLAZE] = "petalogix-s3adsp1800",
+[VIR_ARCH_MICROBLAZEEL] = "petalogix-s3adsp1800",
+[VIR_ARCH_MIPS] = "malta",
+[VIR_ARCH_MIPSEL] = "malta",
+
+[VIR_ARCH_MIPS64] = "malta",
+[VIR_ARCH_MIPS64EL] = "malta",
+[VIR_ARCH_OR32] = "or1k-sim",
+[VIR_ARCH_PARISC] = NULL, /* No QEMU impl */
+[VIR_ARCH_PARISC64] = NULL, /* No QEMU impl */
+
+[VIR_ARCH_PPC] = "g3beige",
+[VIR_ARCH_PPCLE] = "g3beige",
+[VIR_ARCH_PPC64] = "pseries",
+[VIR_ARCH_PPC64LE] = "pseries",
+[VIR_ARCH_PPCEMB] = "bamboo",
+
+[VIR_ARCH_S390] = NULL, /* No QEMU impl*/
+[VIR_ARCH_S390X] = "s390-ccw-virtio",
+[VIR_ARCH_SH4] = "shix",
+[VIR_ARCH_SH4EB] = "shix",
+[VIR_ARCH_SPARC] = "SS-5",
+
+[VIR_ARCH_SPARC64] = "sun4u",
+[VIR_ARCH_UNICORE32] = "puv3",
+[VIR_ARCH_X86_64] = "pc",
+[VIR_ARCH_XTENSA] = "sim",
+[VIR_ARCH_XTENSAEB] = "sim",
+
+};
+
 static int
 virQEMUCapsProbeQMPMachineTypes(virQEMUCapsPtr qemuCaps,
 qemuMonitorPtr mon)
@@ -2241,7 +2296,9 @@ virQEMUCapsProbeQMPMachineTypes(virQEMUCapsPtr qemuCaps,
 int nmachines = 0;
 int ret = -1;
 size_t i;
-size_t defIdx = 0;
+ssize_t defIdx = -1;
+ssize_t preferredIdx = -1;
+const char *preferredMachine = preferredMachines[qemuCaps->arch];
 
 if ((nmachines = qemuMonitorGetMachines(mon, )) < 0)
 return -1;
@@ -2263,12 +2320,28 @@ virQEMUCapsProbeQMPMachineTypes(virQEMUCapsPtr qemuCaps,
 mach->maxCpus = machines[i]->maxCpus;
 mach->hotplugCpus = machines[i]->hotplugCpus;
 
+if (preferredMachine &&
+(STREQ_NULLABLE(mach->alias, preferredMachine) ||
+ STREQ(mach->name, preferredMachine)))
+preferredIdx = qemuCaps->nmachineTypes - 1;
+
 if (machi

Re: [libvirt] [PATCH] qemu: ensure "pc" machine is always used as default if available

[Daniel P . Berrangé]

On Mon, Aug 06, 2018 at 11:38:06AM +0200, Andrea Bolognani wrote:
> On Fri, 2018-08-03 at 13:59 +0100, Daniel P. Berrangé wrote:
> > It is increasingly likely that some distro is going to change the
> > default "x86" machine type in QEMU from "pc" to "q35". This will
> > certainly break existing applications which write their XML on the
> > assumption that its using a "pc" machine by default. For example they'll
> 
> s/its/it's/
> 
> > lack a IDE CDROM and get PCI-X instad of PCI which changes the topology
> 
> s/PCI-X instad/PCIe instead/
> 
> [...]
> > +/* Historically QEMU defaulted to 'pc' machine type but in
> > + * future might switch 'q35'. Such a change is considered
> > + * an ABI break from lbivirt's POV, so we must be sure to
> 
> s/lbivirt/libvirt/
> 
> > + * keep 'pc' as default machine no matter what QEMU says.
> > + */
> > +if (qemuCaps->arch == VIR_ARCH_X86_64 ||
> > +qemuCaps->arch == VIR_ARCH_I686)
> > +preferredAlias = "pc";
> 
> You can use ARCH_IS_X86() here.
> 
> Since we're shielding users from changes in the default x86
> machine type, I think it would make sense to do the same for other
> architectures at the same time: for example, ppc64 should prefer
> pseries, s390 should prefer s390-ccw-virtio and so on.
> 
> I wonder how to handle architectures where QEMU never declared a
> default machine type, such as aarch64 and riscv64, though: I think
> it would make sense to prefer the virt machine type there, but I'm
> not entirely sure that wouldn't cause any breakages either...

Existing libvirt behaviour is that we'll pick the first reported machine
type, so we have to preserve that.


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