[libvirt] [PATCH v4 2/2] libvirtd: fix crash on termination

[John Ferlan]

From: Nikolay Shirokovskiy 

The problem is incorrect order of qemu driver shutdown and shutdown
of netserver threads that serve client requests (thru qemu driver
particularly).

Net server threads are shutdown upon dispose which is triggered
by last daemon object unref at the end of main function. At the same
time qemu driver is shutdown earlier in virStateCleanup. As a result
netserver threads see invalid driver object in the middle of request
processing.

Let's move shutting down netserver threads earlier to virNetDaemonClose.

Note: order of last daemon unref and virStateCleanup
is introduced in 85c3a182 for a valid reason.

Signed-off-by: John Ferlan 
---
 src/rpc/virnetdaemon.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index d970c47ad4..7cb3214166 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -882,6 +882,8 @@ virNetDaemonClose(virNetDaemonPtr dmn)
 virObjectLock(dmn);
 
 virHashForEach(dmn->servers, daemonServerClose, NULL);
+virHashRemoveAll(dmn->servers);
+dmn->servers = NULL;
 
 virObjectUnlock(dmn);
 }
-- 
2.13.6

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

[libvirt] [PATCH v4 1/2] rpc: When adding srv to dmn servers, need to add ref

[John Ferlan]

From: Nikolay Shirokovskiy 

Commit id '252610f7d' modified net server management to use a
hash table to store/manage the various servers; however, during
virNetDaemonAddServerPostExec an @srv object is created, added
to the dmn->servers hash table, but did not increment the object
refcnt like was done during virNetDaemonAddServer as if @srv
were being created for the first time.

Without the proper ref a subsequent virObjectUnref done during
virNetDaemonDispose when virHashFree calls the hash table entry
@dataFree function virObjectFreeHashData could inadvertently free
the memory before some caller is really done with it or cause the
caller to attempt to free something that's already been freed (and
it no longer necessarily owns).

Additionally, since commit id 'f08b1c58f3' removed the @srv
from virLockManager in favor of using virNetDaemonGetServer
which creates a ref on @srv, we need to modify the lock_manager
code in order to properly handle and dispose the references
to the @srv object allowing either the cleanup processing or
the virNetDaemonDispose processing to remove the last ref to
the object.

Signed-off-by: John Ferlan 
---
 src/locking/lock_daemon.c | 19 ++-
 src/rpc/virnetdaemon.c|  2 ++
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index fe3eaf9032..ee2c35fdb8 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -1312,14 +1312,14 @@ int main(int argc, char **argv) {
 srv = virNetDaemonGetServer(lockDaemon->dmn, "virtlockd");
 if ((rv = virLockDaemonSetupNetworkingSystemD(srv)) < 0) {
 ret = VIR_LOCK_DAEMON_ERR_NETWORK;
-goto cleanup;
+goto cleanup_srvref;
 }
 
 /* Only do this, if systemd did not pass a FD */
 if (rv == 0 &&
 virLockDaemonSetupNetworkingNative(srv, sock_file) < 0) {
 ret = VIR_LOCK_DAEMON_ERR_NETWORK;
-goto cleanup;
+goto cleanup_srvref;
 }
 } else if (rv == 1) {
 srv = virNetDaemonGetServer(lockDaemon->dmn, "virtlockd");
@@ -1333,7 +1333,7 @@ int main(int argc, char **argv) {
 
 if ((virLockDaemonSetupSignals(lockDaemon->dmn)) < 0) {
 ret = VIR_LOCK_DAEMON_ERR_SIGNAL;
-goto cleanup;
+goto cleanup_srvref;
 }
 
 if (!(lockProgram = virNetServerProgramNew(VIR_LOCK_SPACE_PROTOCOL_PROGRAM,
@@ -1341,14 +1341,17 @@ int main(int argc, char **argv) {
virLockSpaceProtocolProcs,
virLockSpaceProtocolNProcs))) {
 ret = VIR_LOCK_DAEMON_ERR_INIT;
-goto cleanup;
+goto cleanup_srvref;
 }
 
 if (virNetServerAddProgram(srv, lockProgram) < 0) {
 ret = VIR_LOCK_DAEMON_ERR_INIT;
-goto cleanup;
+goto cleanup_srvref;
 }
 
+/* Completed srv usage from virNetDaemonGetServer */
+virObjectUnref(srv);
+
 /* Disable error func, now logging is setup */
 virSetErrorFunc(NULL, virLockDaemonErrorHandler);
 
@@ -1403,4 +1406,10 @@ int main(int argc, char **argv) {
  no_memory:
 VIR_ERROR(_("Can't allocate memory"));
 exit(EXIT_FAILURE);
+
+ cleanup_srvref:
+/* Unref for virNetDaemonGetServer ref - still have "our" ref from
+ * allocation and possibly a ref for being in netserver hash table */
+virObjectUnref(srv);
+goto cleanup;
 }
diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index e3b9390af2..d970c47ad4 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -313,6 +313,8 @@ virNetDaemonAddServerPostExec(virNetDaemonPtr dmn,
 if (virHashAddEntry(dmn->servers, serverName, srv) < 0)
 goto error;
 
+virObjectRef(srv);
+
 virJSONValueFree(object);
 virObjectUnlock(dmn);
 return srv;
-- 
2.13.6

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

[libvirt] [PATCH v4 0/2] daemon: fix termination/reload issues

[John Ferlan]

This is a repost/fixup of Nikolay's v3:

https://www.redhat.com/archives/libvir-list/2017-October/msg01089.html

The primary difference here is a reorder of the patches to perform
the proper refcnt operations before reordering the shutdown path in
order to clean up servers out of the hash table sooner than later.
In particular, the lock_manager sequencing in order to make sure that
the virObjectUnref is done "orderly".

Nikolay - if you're fine with these changes let me know and I can
then push them (unless of course someone else ACK's before that).
Hopefully the change to lock_daemon makes sense.

Theoretically spaking the virHashFree in virNetDaemonDispose is
probably now superfluous since patch 2/2 will remove all the servers
much sooner, but let's keep it there just to be safe!

Nikolay Shirokovskiy (2):
  rpc: When adding srv to dmn servers, need to add ref
  libvirtd: fix crash on termination

 src/locking/lock_daemon.c | 19 ++-
 src/rpc/virnetdaemon.c|  4 
 2 files changed, 18 insertions(+), 5 deletions(-)

-- 
2.13.6

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

[libvirt] [PATCH go-xml] Add address support for memory device

[zhenwei.pi]

Add Base element for DomainAddress.
Add address element for DomainMemorydev.
Add test code for new DomainMemorydev.

Signed-off-by: zhenwei.pi 
---
 domain.go  | 2 ++
 domain_test.go | 9 +
 2 files changed, 11 insertions(+)

diff --git a/domain.go b/domain.go
index bacab11..3d9404f 100644
--- a/domain.go
+++ b/domain.go
@@ -297,6 +297,7 @@ type DomainAddress struct {
Function   *HexUint `xml:"function,attr"`
Target *uint`xml:"target,attr"`
Unit   *uint`xml:"unit,attr"`
+   Base   *HexUint `xml:"base,attr"`
 }
 
 type DomainConsole struct {
@@ -450,6 +451,7 @@ type DomainMemorydev struct {
Model   string `xml:"model,attr"`
Access  string `xml:"access,attr"`
Target  *DomainMemorydevTarget `xml:"target"`
+   Address *DomainAddress `xml:"address"`
 }
 
 type DomainDeviceList struct {
diff --git a/domain_test.go b/domain_test.go
index dbebe42..cbc5d7f 100644
--- a/domain_test.go
+++ b/domain_test.go
@@ -66,6 +66,9 @@ var vcpuId0 uint = 0
 var vcpuOrder0 uint = 1
 var vcpuId1 uint = 1
 
+var memorydevAddressSlot HexUint = 0
+var memorydevAddressBase HexUint = 4294967296
+
 var domainTestData = []struct {
Object   Document
Expected []string
@@ -385,6 +388,11 @@ var domainTestData = []struct {
Value: 0,
},
},
+   Address: {
+   Type: "dimm",
+   Slot: 
,
+   Base: 
,
+   },
},
},
},
@@ -434,6 +442,7 @@ var domainTestData = []struct {
`1`,
`0`,
`  `,
+   `  `,
``,
`  `,
``,
-- 
2.7.4

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

[libvirt] 答复: [PATCH v2] qemu: change monitor.sock from /var/lib/libvirt/qemu/domain-*** to /var/run/libvirt/qemu/domain-***

[Caoxinhua]

Yes,In qemuxml2argvtest case, mymain call qemuTestDriverInit to init qemu 
driver, qemuTestDriverInit call mkdtemp() to init stateDir, so stateDir is 
randomly.  But '-monitor' part of the command line must be a const value. Can I 
use a const value "/tmp/lib" to instead of random value at qemuxml2argvtest?

-邮件原件-
发件人: Michal Privoznik [mailto:mpriv...@redhat.com] 
发送时间: 2017年10月27日 0:17
收件人: Caoxinhua; libvir-list@redhat.com; jfer...@redhat.com; 
mklet...@redhat.com; berra...@redhat.com
抄送: Yanqiangjun; Huangweidong (C); Wangjing (King, Euler); weifuqiang
主题: Re: [libvirt] [PATCH v2] qemu: change monitor.sock from 
/var/lib/libvirt/qemu/domain-*** to /var/run/libvirt/qemu/domain-***

On 10/16/2017 04:08 AM, xinhua.Cao wrote:
> directory /var/lib alway is Persistence directory, but in redhat system, 
> /var/run is memory directory.
> our running domain xml is saved at /var/run/libvirt/qemu. so if we 
> cold reset system, the /var/run/libvirt/qemu directory is clear, but 
> /var/lib/libvirt/qemu/domain-*** is saved. so there have same 
> /var/lib/libvirt/qemu/domain-*** directory will be left over at system cold 
> reset.
> ---
>  src/qemu/qemu_domain.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 
> ed27a91..3e6fe9b 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1674,7 +1674,7 @@ qemuDomainSetPrivatePaths(virQEMUDriverPtr driver,
>  goto cleanup;
>  
>  if (!priv->libDir &&
> -virAsprintf(>libDir, "%s/domain-%s", cfg->libDir, domname) < 0)
> +virAsprintf(>libDir, "%s/domain-%s", cfg->stateDir, 
> + domname) < 0)
>  goto cleanup;
>  
>  if (!priv->channelTargetDir &&
> 

Almost. I see a problem with this patch. Problem is that qemuxml2argvtest needs 
to be updated. Which is not trivial because state dir is a tempdir (mkdtemp()), 
and thus '-monitor' part of the command line changes with each test invocation.

Michal

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

Re: [libvirt] [PATCH v5 4/4] xlconfigtest: add tests for numa cell sibling distances

[Jim Fehlig]

On 10/12/2017 01:31 PM, Wim Ten Have wrote:

From: Wim ten Have 

Test a bidirectional xen-xl domxml to and from native for numa
support administration as brought under this patch series.

Signed-off-by: Wim ten Have 
---
  .../test-fullvirt-vnuma-autocomplete.cfg   | 26 +++
  .../test-fullvirt-vnuma-autocomplete.xml   | 85 ++
  .../test-fullvirt-vnuma-nodistances.cfg| 26 +++
  .../test-fullvirt-vnuma-nodistances.xml| 53 ++
  .../test-fullvirt-vnuma-partialdist.cfg| 26 +++
  .../test-fullvirt-vnuma-partialdist.xml| 60 +++
  tests/xlconfigdata/test-fullvirt-vnuma.cfg | 26 +++
  tests/xlconfigdata/test-fullvirt-vnuma.xml | 81 +
  tests/xlconfigtest.c   |  6 ++


Cool! Thanks for the various configurations to test all the hairy parsing code 
:-).

Reviewed-by: Jim Fehlig 

BTW I should have mentioned it while reviewing 3/4, but we should also have 
tests for the libxl_domain_config generator, now that we have a test suite for 
that. See tests/libxlxml2domconfigtest.c.


Regards,
Jim


  9 files changed, 389 insertions(+)
  create mode 100644 tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.cfg
  create mode 100644 tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.xml
  create mode 100644 tests/xlconfigdata/test-fullvirt-vnuma-nodistances.cfg
  create mode 100644 tests/xlconfigdata/test-fullvirt-vnuma-nodistances.xml
  create mode 100644 tests/xlconfigdata/test-fullvirt-vnuma-partialdist.cfg
  create mode 100644 tests/xlconfigdata/test-fullvirt-vnuma-partialdist.xml
  create mode 100644 tests/xlconfigdata/test-fullvirt-vnuma.cfg
  create mode 100644 tests/xlconfigdata/test-fullvirt-vnuma.xml

diff --git a/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.cfg 
b/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.cfg
new file mode 100644
index 0..edba69a17
--- /dev/null
+++ b/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.cfg
@@ -0,0 +1,26 @@
+name = "XenGuest2"
+uuid = "c7a5fdb2-cdaf-9455-926a-d65c16db1809"
+maxmem = 8192
+memory = 8192
+vcpus = 12
+pae = 1
+acpi = 1
+apic = 1
+viridian = 0
+rtc_timeoffset = 0
+localtime = 0
+on_poweroff = "destroy"
+on_reboot = "restart"
+on_crash = "restart"
+device_model = "/usr/lib/xen/bin/qemu-system-i386"
+sdl = 0
+vnc = 1
+vncunused = 1
+vnclisten = "127.0.0.1"
+vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000" ]
+parallel = "none"
+serial = "none"
+builder = "hvm"
+boot = "d"
+vnuma = [ [ "pnode=0", "size=2048", "vcpus=0,11", "vdistances=10,21,31,41,51,61" ], [ "pnode=1", "size=2048", "vcpus=1,10", "vdistances=21,10,21,31,41,51" ], [ "pnode=2", "size=2048", "vcpus=2,9", 
"vdistances=31,21,10,21,31,41" ], [ "pnode=3", "size=2048", "vcpus=3,8", "vdistances=41,31,21,10,21,31" ], [ "pnode=4", "size=2048", "vcpus=4,7", "vdistances=51,41,31,21,10,21" ], [ "pnode=5", "size=2048", 
"vcpus=5-6", "vdistances=61,51,41,31,21,10" ] ]
+disk = [ 
"format=raw,vdev=hda,access=rw,backendtype=phy,target=/dev/HostVG/XenGuest2" ]
diff --git a/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.xml 
b/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.xml
new file mode 100644
index 0..e3639eb04
--- /dev/null
+++ b/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.xml
@@ -0,0 +1,85 @@
+
+  XenGuest2
+  c7a5fdb2-cdaf-9455-926a-d65c16db1809
+  8388608
+  8388608
+  12
+  
+hvm
+/usr/lib/xen/boot/hvmloader
+
+  
+  
+
+
+
+  
+  
+
+  
+
+  
+  
+  
+  
+  
+
+  
+  
+
+  
+  
+  
+  
+
+  
+  
+
+  
+  
+  
+
+  
+  
+
+  
+  
+
+  
+  
+
+  
+
+  
+  
+
+  
+  
+  destroy
+  restart
+  restart
+  
+/usr/lib/xen/bin/qemu-system-i386
+
+  
+  
+  
+  
+
+
+
+  
+  
+  
+  
+
+
+
+
+  
+
+
+  
+
+  
+
diff --git a/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.cfg 
b/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.cfg
new file mode 100644
index 0..8186edfee
--- /dev/null
+++ b/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.cfg
@@ -0,0 +1,26 @@
+name = "XenGuest2"
+uuid = "c7a5fdb2-cdaf-9455-926a-d65c16db1809"
+maxmem = 8192
+memory = 8192
+vcpus = 8
+pae = 1
+acpi = 1
+apic = 1
+viridian = 0
+rtc_timeoffset = 0
+localtime = 0
+on_poweroff = "destroy"
+on_reboot = "restart"
+on_crash = "restart"
+device_model = "/usr/lib/xen/bin/qemu-system-i386"
+sdl = 0
+vnc = 1
+vncunused = 1
+vnclisten = "127.0.0.1"
+vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000" ]
+parallel = "none"
+serial = 

Re: [libvirt] [PATCH v5 3/4] libxl: vnuma support

[Jim Fehlig]

On 10/12/2017 01:31 PM, Wim Ten Have wrote:

From: Wim ten Have 

This patch generates a NUMA distance-aware libxl description from the
information extracted from a NUMA distance-aware libvirt XML file.

By default, if no NUMA node distance information is supplied in the
libvirt XML file, this patch uses the distances 10 for local and 20
for remote nodes/sockets."


Spurious " at the end of above sentence.


Signed-off-by: Wim ten Have 
---
Changes on v1:
- Fix function calling (coding) standards.
- Use virReportError(...) under all failing circumstances.
- Reduce redundant (format->parse) code sorting bitmaps.
- Avoid non GNU shorthand notations, difficult code practice.
Changes on v2:
- Have autonomous defaults applied from virDomainNumaGetNodeDistance.
- Automatically make Xen driver simulate vnuma pnodes on non NUMA h/w.
- Compute 'memory unit=' making it the sum of all node memory.
Changes on v4:
- Escape virDomainNumaGetNodeCount() if effective.
---
  src/libxl/libxl_conf.c   | 121 +++
  src/libxl/libxl_driver.c |   3 +-
  2 files changed, 123 insertions(+), 1 deletion(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 34233a955..adac6c19c 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -374,6 +374,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
def->features[VIR_DOMAIN_FEATURE_APIC] ==
VIR_TRISTATE_SWITCH_ON);
  libxl_defbool_set(_info->u.hvm.acpi,
+  virDomainNumaGetNodeCount(def->numa) > 0 || >   
  def->features[VIR_DOMAIN_FEATURE_ACPI] ==
VIR_TRISTATE_SWITCH_ON);


This doesn't seem right. At a minimum we should ensure 
def->features[VIR_DOMAIN_FEATURE_ACPI] is set to ON when a vnuma configuration 
is defined. I think libxlDomainDefPostParse is a better place to do that.


  
@@ -618,6 +619,121 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,

  return 0;
  }
  
+#ifdef LIBXL_HAVE_VNUMA

+static int
+libxlMakeVnumaList(virDomainDefPtr def,
+   libxl_ctx *ctx,
+   libxl_domain_config *d_config)
+{
+int ret = -1;
+size_t i, j;
+size_t nr_nodes;
+size_t num_vnuma;
+bool simulate = false;
+virBitmapPtr bitmap = NULL;
+virDomainNumaPtr numa = def->numa;
+libxl_domain_build_info *b_info = _config->b_info;
+libxl_physinfo physinfo;
+libxl_vnode_info *vnuma_nodes = NULL;
+
+if (!numa)
+return 0;
+
+num_vnuma = virDomainNumaGetNodeCount(numa);
+if (!num_vnuma)
+return 0;
+
+libxl_physinfo_init();
+if (libxl_get_physinfo(ctx, ) < 0) {
+libxl_physinfo_dispose();
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("libxl_get_physinfo_info failed"));
+return -1;
+}
+nr_nodes = physinfo.nr_nodes;
+libxl_physinfo_dispose();
+
+if (num_vnuma > nr_nodes) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Number of configured numa cells %zu exceeds the physical 
available nodes %zu, guest simulates numa"),
+   num_vnuma, nr_nodes);


It seems this should be VIR_WARN instead of virReportError, afterall an error is 
not returned.



+simulate = true;
+}
+
+/*
+ * allocate the vnuma_nodes for assignment under b_info.
+ */
+if (VIR_ALLOC_N(vnuma_nodes, num_vnuma) < 0)
+return -1;
+
+/*
+ * parse the vnuma vnodes data.
+ */
+for (i = 0; i < num_vnuma; i++) {
+int cpu;
+libxl_bitmap vcpu_bitmap;
+libxl_vnode_info *p = _nodes[i];
+
+libxl_vnode_info_init(p);
+
+/* pnode */
+p->pnode = simulate ? 0 : i;


So any time the number of vnuma nodes exceeds the number of physical nodes, all 
vnuma nodes are confined to physical node 0? Does xl behave this way too?



+
+/* memory size */
+p->memkb = virDomainNumaGetNodeMemorySize(numa, i);
+
+/* vcpus */
+bitmap = virDomainNumaGetNodeCpumask(numa, i);
+if (bitmap == NULL) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("vnuma sibling %zu missing vcpus set"), i);
+goto cleanup;
+}
+
+if ((cpu = virBitmapNextSetBit(bitmap, -1)) < 0)
+goto cleanup;
+
+libxl_bitmap_init(_bitmap);
+if (libxl_cpu_bitmap_alloc(ctx, _bitmap, b_info->max_vcpus)) {
+virReportOOMError();
+goto cleanup;
+}
+
+do {
+libxl_bitmap_set(_bitmap, cpu);
+} while ((cpu = virBitmapNextSetBit(bitmap, cpu)) >= 0);
+
+libxl_bitmap_copy_alloc(ctx, >vcpus, _bitmap);
+libxl_bitmap_dispose(_bitmap);
+
+/* vdistances */
+if (VIR_ALLOC_N(p->distances, num_vnuma) < 0)
+goto cleanup;
+p->num_distances = 

Re: [libvirt] [PATCH v5 2/4] xenconfig: add domxml conversions for xen-xl

[Jim Fehlig]

On 10/12/2017 01:31 PM, Wim Ten Have wrote:

From: Wim ten Have 

This patch converts NUMA configurations between the Xen libxl
configuration file format and libvirt's XML format.

XML HVM domain on a 4 node (2 cores/socket) configuration:

   
 
   
 
   
   
   
   
 
   
   
 
   
   
   
   
 
   
   
 
   
   
   
   
 
   
   
 
   
   
   
   
 
   
 
   

Xen xl.cfg domain configuration:

   vnuma = [["pnode=0","size=2048","vcpus=0-1","vdistances=10,21,31,21"],
["pnode=1","size=2048","vcpus=2-3","vdistances=21,10,21,31"],
["pnode=2","size=2048","vcpus=4-5","vdistances=31,21,10,21"],
["pnode=3","size=2048","vcpus=6-7","vdistances=21,31,21,10"]]

If there is no XML  description amongst the  data the
conversion schema from xml to native will generate 10 for local and 20
for all remote instances.


Does xl have the same behavior? E.g. with

vnuma = [["pnode=0","size=2048","vcpus=0-1"],
 ["pnode=1","size=2048","vcpus=2-3"],
 ["pnode=2","size=2048","vcpus=4-5"],
 ["pnode=3","size=2048","vcpus=6-7"]]

will distances be 10 for local and 20 for remote nodes?


Signed-off-by: Wim ten Have 
---
Changes on v2:
- Reduce the indentation level under xenParseXLVnuma().
Changes on v3:
- Add the Numa core split functions required to interface.
---
  src/conf/numa_conf.c | 138 
  src/conf/numa_conf.h |  20 +++
  src/libvirt_private.syms |   5 +
  src/xenconfig/xen_xl.c   | 333 +++
  4 files changed, 496 insertions(+)

diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
index 00a3343fb..b513b1de1 100644
--- a/src/conf/numa_conf.c
+++ b/src/conf/numa_conf.c
@@ -1144,6 +1144,133 @@ virDomainNumaGetNodeCount(virDomainNumaPtr numa)
  }
  
  
+size_t

+virDomainNumaSetNodeCount(virDomainNumaPtr numa, size_t nmem_nodes)
+{
+if (!nmem_nodes) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Cannot set an empty mem_nodes set"));
+return 0;
+}
+
+if (numa->mem_nodes) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Cannot alter an existing mem_nodes set"));
+return 0;
+}
+
+if (VIR_ALLOC_N(numa->mem_nodes, nmem_nodes) < 0)
+return 0;
+
+numa->nmem_nodes = nmem_nodes;
+
+return numa->nmem_nodes;
+}
+
+size_t
+virDomainNumaGetNodeDistance(virDomainNumaPtr numa,
+ size_t node,
+ size_t cellid)
+{
+virDomainNumaDistancePtr distances = NULL;
+
+if (node < numa->nmem_nodes)
+distances = numa->mem_nodes[node].distances;
+
+/*
+ * Present the configured distance value. If
+ * out of range or not available set the platform
+ * defined default for local and remote nodes.
+ */
+if (!distances ||
+!distances[cellid].value ||
+!numa->mem_nodes[node].ndistances)
+return (node == cellid) ? \
+  LOCAL_DISTANCE : REMOTE_DISTANCE;


This return statement will fit within 80 columns on a single line. (Side note: 
lines <= 80 columns is not strictly enforced in libvirt.)



+
+return distances[cellid].value;
+}
+
+
+int
+virDomainNumaSetNodeDistance(virDomainNumaPtr numa,
+ size_t node,
+ size_t cellid,
+ unsigned int value)
+{
+virDomainNumaDistancePtr distances;
+
+if (node >= numa->nmem_nodes) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Argument 'node' %zu outranges "
+ "defined number of NUMA nodes"),
+   node);
+return -1;
+}
+
+distances = numa->mem_nodes[node].distances;
+if (!distances ||
+cellid >= numa->mem_nodes[node].ndistances) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("Arguments under memnode element do not "
+ "correspond with existing guest's NUMA cell"));
+return -1;
+}
+
+/*
+ * Advanced Configuration and Power Interface
+ * Specification version 6.1. Chapter 5.2.17
+ * System Locality Distance Information Table
+ * ... Distance values of 0-9 are reserved.
+ */
+if (value < LOCAL_DISTANCE ||
+value > UNREACHABLE) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Distance value of %d is in 0-9 reserved range"),


Should the '0-9' be dropped? E.g. value could be > UNREACHABLE.


+   value);
+return -1;
+}
+
+if (value == LOCAL_DISTANCE && node != cellid) {
+

[libvirt] [PATCH 04/12] qemu: Generalize APPEND macro in qemuMonitorJSONSetMigrationParams

[Jiri Denemark]

The APPEND macro is now be usable for any type.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_monitor_json.c | 21 -
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 76fc84ed0..218bbd8bd 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2739,21 +2739,24 @@ qemuMonitorJSONSetMigrationParams(qemuMonitorPtr mon,
 if (!(args = virJSONValueNewObject()))
 goto cleanup;
 
-#define APPEND(VAR, FIELD)  \
+#define APPEND(VALID, API, VAR, FIELD)  \
 do {\
-if (params->VAR ## _set &&  \
-virJSONValueObjectAppendNumberInt(args, FIELD,  \
-  params->VAR) < 0) \
+if (VALID && API(args, FIELD, params->VAR) < 0) \
 goto cleanup;   \
 } while (0)
 
-APPEND(compressLevel, "compress-level");
-APPEND(compressThreads, "compress-threads");
-APPEND(decompressThreads, "decompress-threads");
-APPEND(cpuThrottleInitial, "cpu-throttle-initial");
-APPEND(cpuThrottleIncrement, "cpu-throttle-increment");
+#define APPEND_INT(VAR, FIELD)  \
+APPEND(params->VAR ## _set, \
+   virJSONValueObjectAppendNumberInt, VAR, FIELD)
+
+APPEND_INT(compressLevel, "compress-level");
+APPEND_INT(compressThreads, "compress-threads");
+APPEND_INT(decompressThreads, "decompress-threads");
+APPEND_INT(cpuThrottleInitial, "cpu-throttle-initial");
+APPEND_INT(cpuThrottleIncrement, "cpu-throttle-increment");
 
 #undef APPEND
+#undef APPEND_INT
 
 if (params->migrateTLSAlias &&
 virJSONValueObjectAppendString(args, "tls-creds",
-- 
2.14.3

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

[libvirt] [PATCH 06/12] qemu: Drop giant if statement from qemuMonitorSetMigrationParams

[Jiri Denemark]

The check can be easily replaced with a simple test in the JSON
implementation and we don't need to update it every time a new parameter
is added.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_monitor.c  | 9 -
 src/qemu/qemu_monitor_json.c | 5 +
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index dd9d64a20..71069827e 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2618,15 +2618,6 @@ qemuMonitorSetMigrationParams(qemuMonitorPtr mon,
 
 QEMU_CHECK_MONITOR_JSON(mon);
 
-if (!params->compressLevel_set &&
-!params->compressThreads_set &&
-!params->decompressThreads_set &&
-!params->cpuThrottleInitial_set &&
-!params->cpuThrottleIncrement_set &&
-!params->migrateTLSAlias &&
-!params->migrateTLSHostname)
-return 0;
-
 return qemuMonitorJSONSetMigrationParams(mon, params);
 }
 
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 826133543..d3c37ded8 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2765,6 +2765,11 @@ qemuMonitorJSONSetMigrationParams(qemuMonitorPtr mon,
 #undef APPEND_INT
 #undef APPEND_STR
 
+if (virJSONValueObjectKeysNumber(args) == 0) {
+ret = 0;
+goto cleanup;
+}
+
 if (virJSONValueObjectAppend(cmd, "arguments", args) < 0)
 goto cleanup;
 args = NULL;
-- 
2.14.3

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

[libvirt] [PATCH 00/12] qemu: Add support for more migration parameters

[Jiri Denemark]

QEMU is transforming existing special migration parameters (those which
need dedicated QMP commands to be set or queried) into proper parameters
handled by query-migrate-parameters and migrate-set-parameters. Even
though we may still want to use the existing commands adding support for
tha transformed parameters will help us clean all of them before
migration and reset them to their original values after a failed
migration. Thus we wouldn't need to add more ad-hoc code which resets
some of them and ignores some others.

Jiri Denemark (12):
  qemu: Generalize PARSE macro in qemuMonitorJSONGetMigrationParams
  qemu: Use macro for parsing string migration parameters
  qemu: Use macro for parsing ull migration parameters
  qemu: Generalize APPEND macro in qemuMonitorJSONSetMigrationParams
  qemu: Use macro for setting string migration parameters
  qemu: Drop giant if statement from qemuMonitorSetMigrationParams
  qemumonitorjsontest: Rename 1st CHECK macro in migration params test
  qemumonitorjsontest: Rename 2nd CHECK macro in migration params test
  qemu: Add support for setting downtime-limit migration parameter
  qemu: Rename TLS related migration parameters
  qemu: Add support for max-bandwidth migration parameter
  qemu: Add support for block-incremental migration parameter

 src/qemu/qemu_migration.c|  23 +-
 src/qemu/qemu_monitor.c  |  20 +++--
 src/qemu/qemu_monitor.h  |  10 -
 src/qemu/qemu_monitor_json.c | 104 +++
 tests/qemumonitorjsontest.c  |  46 ---
 5 files changed, 123 insertions(+), 80 deletions(-)

-- 
2.14.3

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

[libvirt] [PATCH 03/12] qemu: Use macro for parsing ull migration parameters

[Jiri Denemark]

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_monitor_json.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index cb0bb0d0d..76fc84ed0 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2687,6 +2687,9 @@ qemuMonitorJSONGetMigrationParams(qemuMonitorPtr mon,
 #define PARSE_INT(VAR, FIELD)   \
 PARSE_SET(virJSONValueObjectGetNumberInt, VAR, FIELD)
 
+#define PARSE_ULONG(VAR, FIELD) \
+PARSE_SET(virJSONValueObjectGetNumberUlong, VAR, FIELD)
+
 #define PARSE_STR(VAR, FIELD)   \
 do {\
 const char *str;\
@@ -2703,15 +2706,13 @@ qemuMonitorJSONGetMigrationParams(qemuMonitorPtr mon,
 PARSE_INT(cpuThrottleIncrement, "cpu-throttle-increment");
 PARSE_STR(migrateTLSAlias, "tls-creds");
 PARSE_STR(migrateTLSHostname, "tls-hostname");
+PARSE_ULONG(downtimeLimit, "downtime-limit");
 
 #undef PARSE_SET
 #undef PARSE_INT
+#undef PARSE_ULONG
 #undef PARSE_STR
 
-if (virJSONValueObjectGetNumberUlong(result, "downtime-limit",
- >downtimeLimit) == 0)
-params->downtimeLimit_set = true;
-
 ret = 0;
  cleanup:
 virJSONValueFree(cmd);
-- 
2.14.3

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

[libvirt] [PATCH 11/12] qemu: Add support for max-bandwidth migration parameter

[Jiri Denemark]

We already support several ways of setting migration bandwidth and this
is not adding another one. With this patch we are able to read and write
this parameter using query-migrate-parameters and migrate-set-parameters
in one call with all other parameters.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_monitor.c  | 3 ++-
 src/qemu/qemu_monitor.h  | 3 +++
 src/qemu/qemu_monitor_json.c | 2 ++
 tests/qemumonitorjsontest.c  | 2 ++
 4 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 3e2c69a9a..04b18baf9 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2607,13 +2607,14 @@ qemuMonitorSetMigrationParams(qemuMonitorPtr mon,
 VIR_DEBUG("compressLevel=%d:%d compressThreads=%d:%d "
   "decompressThreads=%d:%d cpuThrottleInitial=%d:%d "
   "cpuThrottleIncrement=%d:%d tlsCreds=%s tlsHostname=%s "
-  "downtimeLimit=%d:%llu",
+  "maxBandwidth=%d:%llu downtimeLimit=%d:%llu",
   params->compressLevel_set, params->compressLevel,
   params->compressThreads_set, params->compressThreads,
   params->decompressThreads_set, params->decompressThreads,
   params->cpuThrottleInitial_set, params->cpuThrottleInitial,
   params->cpuThrottleIncrement_set, params->cpuThrottleIncrement,
   NULLSTR(params->tlsCreds), NULLSTR(params->tlsHostname),
+  params->maxBandwidth_set, params->maxBandwidth,
   params->downtimeLimit_set, params->downtimeLimit);
 
 QEMU_CHECK_MONITOR_JSON(mon);
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index e123baaae..7836dd332 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -628,6 +628,9 @@ struct _qemuMonitorMigrationParams {
 char *tlsCreds;
 char *tlsHostname;
 
+bool maxBandwidth_set;
+unsigned long long maxBandwidth;
+
 bool downtimeLimit_set;
 unsigned long long downtimeLimit;
 };
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 9f238bc30..115610e50 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2706,6 +2706,7 @@ qemuMonitorJSONGetMigrationParams(qemuMonitorPtr mon,
 PARSE_INT(cpuThrottleIncrement, "cpu-throttle-increment");
 PARSE_STR(tlsCreds, "tls-creds");
 PARSE_STR(tlsHostname, "tls-hostname");
+PARSE_ULONG(maxBandwidth, "max-bandwidth");
 PARSE_ULONG(downtimeLimit, "downtime-limit");
 
 #undef PARSE_SET
@@ -2764,6 +2765,7 @@ qemuMonitorJSONSetMigrationParams(qemuMonitorPtr mon,
 APPEND_INT(cpuThrottleIncrement, "cpu-throttle-increment");
 APPEND_STR(tlsCreds, "tls-creds");
 APPEND_STR(tlsHostname, "tls-hostname");
+APPEND_ULONG(maxBandwidth, "max-bandwidth");
 APPEND_ULONG(downtimeLimit, "downtime-limit");
 
 #undef APPEND
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index aa5da8be9..488c79cc3 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -1804,6 +1804,7 @@ 
testQemuMonitorJSONqemuMonitorJSONGetMigrationParams(const void *data)
"\"cpu-throttle-initial\": 20,"
"\"tls-creds\": \"tls0\","
"\"tls-hostname\": \"\","
+   "\"max-bandwidth\": 1234567890,"
"\"downtime-limit\": 500"
"}"
"}") < 0) {
@@ -1855,6 +1856,7 @@ 
testQemuMonitorJSONqemuMonitorJSONGetMigrationParams(const void *data)
 CHECK_INT(cpuThrottleIncrement, "cpu-throttle-increment", 10);
 CHECK_STR(tlsCreds, "tls-creds", "tls0");
 CHECK_STR(tlsHostname, "tls-hostname", "");
+CHECK_ULONG(maxBandwidth, "max-bandwidth", 1234567890ULL);
 CHECK_ULONG(downtimeLimit, "downtime-limit", 500ULL);
 
 #undef CHECK_NUM
-- 
2.14.3

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

[libvirt] [PATCH 09/12] qemu: Add support for setting downtime-limit migration parameter

[Jiri Denemark]

We already support setting the maximum downtime with a dedicated
virDomainMigrateSetMaxDowntime API. This patch does not implement
another way of setting the downtime by adding a new public migration
parameter. It just makes sure any parameter we are able to get from a
QEMU monitor by query-migrate-parameters can be passed back to QEMU via
migrate-set-parameters.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_monitor.c  | 5 +++--
 src/qemu/qemu_monitor_json.c | 6 ++
 tests/qemumonitorjsontest.c  | 8 +++-
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 71069827e..c88726735 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2607,14 +2607,15 @@ qemuMonitorSetMigrationParams(qemuMonitorPtr mon,
 VIR_DEBUG("compressLevel=%d:%d compressThreads=%d:%d "
   "decompressThreads=%d:%d cpuThrottleInitial=%d:%d "
   "cpuThrottleIncrement=%d:%d tlsAlias=%s "
-  "tlsHostname=%s",
+  "tlsHostname=%s downtimeLimit=%d:%llu",
   params->compressLevel_set, params->compressLevel,
   params->compressThreads_set, params->compressThreads,
   params->decompressThreads_set, params->decompressThreads,
   params->cpuThrottleInitial_set, params->cpuThrottleInitial,
   params->cpuThrottleIncrement_set, params->cpuThrottleIncrement,
   NULLSTR(params->migrateTLSAlias),
-  NULLSTR(params->migrateTLSHostname));
+  NULLSTR(params->migrateTLSHostname),
+  params->downtimeLimit_set, params->downtimeLimit);
 
 QEMU_CHECK_MONITOR_JSON(mon);
 
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index d3c37ded8..a2f3e3317 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2753,6 +2753,10 @@ qemuMonitorJSONSetMigrationParams(qemuMonitorPtr mon,
 APPEND(params->VAR, \
virJSONValueObjectAppendString, VAR, FIELD)
 
+#define APPEND_ULONG(VAR, FIELD)\
+APPEND(params->VAR ## _set, \
+   virJSONValueObjectAppendNumberUlong, VAR, FIELD)
+
 APPEND_INT(compressLevel, "compress-level");
 APPEND_INT(compressThreads, "compress-threads");
 APPEND_INT(decompressThreads, "decompress-threads");
@@ -2760,10 +2764,12 @@ qemuMonitorJSONSetMigrationParams(qemuMonitorPtr mon,
 APPEND_INT(cpuThrottleIncrement, "cpu-throttle-increment");
 APPEND_STR(migrateTLSAlias, "tls-creds");
 APPEND_STR(migrateTLSHostname, "tls-hostname");
+APPEND_ULONG(downtimeLimit, "downtime-limit");
 
 #undef APPEND
 #undef APPEND_INT
 #undef APPEND_STR
+#undef APPEND_ULONG
 
 if (virJSONValueObjectKeysNumber(args) == 0) {
 ret = 0;
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 2cefdcac9..cc55b0c43 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -1803,7 +1803,8 @@ 
testQemuMonitorJSONqemuMonitorJSONGetMigrationParams(const void *data)
"\"compress-level\": 1,"
"\"cpu-throttle-initial\": 20,"
"\"tls-creds\": \"tls0\","
-   "\"tls-hostname\": \"\""
+   "\"tls-hostname\": \"\","
+   "\"downtime-limit\": 500"
"}"
"}") < 0) {
 goto cleanup;
@@ -1830,6 +1831,9 @@ 
testQemuMonitorJSONqemuMonitorJSONGetMigrationParams(const void *data)
 #define CHECK_INT(VAR, FIELD, VALUE)\
 CHECK_NUM(VAR, FIELD, VALUE, "%d")
 
+#define CHECK_ULONG(VAR, FIELD, VALUE)  \
+CHECK_NUM(VAR, FIELD, VALUE, "%llu")
+
 #define CHECK_STR(VAR, FIELD, VALUE)\
 do {\
 if (!params.VAR) {  \
@@ -1851,9 +1855,11 @@ 
testQemuMonitorJSONqemuMonitorJSONGetMigrationParams(const void *data)
 CHECK_INT(cpuThrottleIncrement, "cpu-throttle-increment", 10);
 CHECK_STR(migrateTLSAlias, "tls-creds", "tls0");
 CHECK_STR(migrateTLSHostname, "tls-hostname", "");
+CHECK_ULONG(downtimeLimit, "downtime-limit", 500ULL);
 
 #undef CHECK_NUM
 #undef CHECK_INT
+#undef CHECK_ULONG
 #undef CHECK_STR
 
 ret = 0;
-- 
2.14.3

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

[libvirt] [PATCH 05/12] qemu: Use macro for setting string migration parameters

[Jiri Denemark]

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_monitor_json.c | 17 +++--
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 218bbd8bd..826133543 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2749,24 +2749,21 @@ qemuMonitorJSONSetMigrationParams(qemuMonitorPtr mon,
 APPEND(params->VAR ## _set, \
virJSONValueObjectAppendNumberInt, VAR, FIELD)
 
+#define APPEND_STR(VAR, FIELD)  \
+APPEND(params->VAR, \
+   virJSONValueObjectAppendString, VAR, FIELD)
+
 APPEND_INT(compressLevel, "compress-level");
 APPEND_INT(compressThreads, "compress-threads");
 APPEND_INT(decompressThreads, "decompress-threads");
 APPEND_INT(cpuThrottleInitial, "cpu-throttle-initial");
 APPEND_INT(cpuThrottleIncrement, "cpu-throttle-increment");
+APPEND_STR(migrateTLSAlias, "tls-creds");
+APPEND_STR(migrateTLSHostname, "tls-hostname");
 
 #undef APPEND
 #undef APPEND_INT
-
-if (params->migrateTLSAlias &&
-virJSONValueObjectAppendString(args, "tls-creds",
-   params->migrateTLSAlias) < 0)
-goto cleanup;
-
-if (params->migrateTLSHostname &&
-virJSONValueObjectAppendString(args, "tls-hostname",
-   params->migrateTLSHostname) < 0)
-goto cleanup;
+#undef APPEND_STR
 
 if (virJSONValueObjectAppend(cmd, "arguments", args) < 0)
 goto cleanup;
-- 
2.14.3

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

[libvirt] [PATCH 12/12] qemu: Add support for block-incremental migration parameter

[Jiri Denemark]

We handle incremental storage migration in a different way. The support
for this new (as of QEMU 2.10) parameter is only needed for full
coverage of migration parameters used by QEMU.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_monitor.c  |  6 --
 src/qemu/qemu_monitor.h  |  3 +++
 src/qemu/qemu_monitor_json.c | 10 ++
 tests/qemumonitorjsontest.c  |  8 +++-
 4 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 04b18baf9..611876ff8 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2607,7 +2607,8 @@ qemuMonitorSetMigrationParams(qemuMonitorPtr mon,
 VIR_DEBUG("compressLevel=%d:%d compressThreads=%d:%d "
   "decompressThreads=%d:%d cpuThrottleInitial=%d:%d "
   "cpuThrottleIncrement=%d:%d tlsCreds=%s tlsHostname=%s "
-  "maxBandwidth=%d:%llu downtimeLimit=%d:%llu",
+  "maxBandwidth=%d:%llu downtimeLimit=%d:%llu "
+  "blockIncremental=%d:%d",
   params->compressLevel_set, params->compressLevel,
   params->compressThreads_set, params->compressThreads,
   params->decompressThreads_set, params->decompressThreads,
@@ -2615,7 +2616,8 @@ qemuMonitorSetMigrationParams(qemuMonitorPtr mon,
   params->cpuThrottleIncrement_set, params->cpuThrottleIncrement,
   NULLSTR(params->tlsCreds), NULLSTR(params->tlsHostname),
   params->maxBandwidth_set, params->maxBandwidth,
-  params->downtimeLimit_set, params->downtimeLimit);
+  params->downtimeLimit_set, params->downtimeLimit,
+  params->blockIncremental_set, params->blockIncremental);
 
 QEMU_CHECK_MONITOR_JSON(mon);
 
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 7836dd332..f81fb7f2a 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -633,6 +633,9 @@ struct _qemuMonitorMigrationParams {
 
 bool downtimeLimit_set;
 unsigned long long downtimeLimit;
+
+bool blockIncremental_set;
+bool blockIncremental;
 };
 
 int qemuMonitorGetMigrationParams(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 115610e50..aa2599209 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2690,6 +2690,9 @@ qemuMonitorJSONGetMigrationParams(qemuMonitorPtr mon,
 #define PARSE_ULONG(VAR, FIELD) \
 PARSE_SET(virJSONValueObjectGetNumberUlong, VAR, FIELD)
 
+#define PARSE_BOOL(VAR, FIELD)  \
+PARSE_SET(virJSONValueObjectGetBoolean, VAR, FIELD)
+
 #define PARSE_STR(VAR, FIELD)   \
 do {\
 const char *str;\
@@ -2708,10 +2711,12 @@ qemuMonitorJSONGetMigrationParams(qemuMonitorPtr mon,
 PARSE_STR(tlsHostname, "tls-hostname");
 PARSE_ULONG(maxBandwidth, "max-bandwidth");
 PARSE_ULONG(downtimeLimit, "downtime-limit");
+PARSE_BOOL(blockIncremental, "block-incremental");
 
 #undef PARSE_SET
 #undef PARSE_INT
 #undef PARSE_ULONG
+#undef PARSE_BOOL
 #undef PARSE_STR
 
 ret = 0;
@@ -2758,6 +2763,10 @@ qemuMonitorJSONSetMigrationParams(qemuMonitorPtr mon,
 APPEND(params->VAR ## _set, \
virJSONValueObjectAppendNumberUlong, VAR, FIELD)
 
+#define APPEND_BOOL(VAR, FIELD) \
+APPEND(params->VAR ## _set, \
+   virJSONValueObjectAppendBoolean, VAR, FIELD)
+
 APPEND_INT(compressLevel, "compress-level");
 APPEND_INT(compressThreads, "compress-threads");
 APPEND_INT(decompressThreads, "decompress-threads");
@@ -2767,6 +2776,7 @@ qemuMonitorJSONSetMigrationParams(qemuMonitorPtr mon,
 APPEND_STR(tlsHostname, "tls-hostname");
 APPEND_ULONG(maxBandwidth, "max-bandwidth");
 APPEND_ULONG(downtimeLimit, "downtime-limit");
+APPEND_BOOL(blockIncremental, "block-incremental");
 
 #undef APPEND
 #undef APPEND_INT
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 488c79cc3..aa2f67947 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -1805,7 +1805,8 @@ 
testQemuMonitorJSONqemuMonitorJSONGetMigrationParams(const void *data)
"\"tls-creds\": \"tls0\","
"\"tls-hostname\": \"\","
"\"max-bandwidth\": 1234567890,"
-   "\"downtime-limit\": 500"
+   "\"downtime-limit\": 500,"
+   "\"block-incremental\": true"
"}"

[libvirt] [PATCH 02/12] qemu: Use macro for parsing string migration parameters

[Jiri Denemark]

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_monitor_json.c | 23 ---
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 16554d5b2..cb0bb0d0d 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2659,7 +2659,6 @@ qemuMonitorJSONGetMigrationParams(qemuMonitorPtr mon,
 virJSONValuePtr result;
 virJSONValuePtr cmd = NULL;
 virJSONValuePtr reply = NULL;
-const char *tlsStr = NULL;
 
 memset(params, 0, sizeof(*params));
 
@@ -2688,29 +2687,31 @@ qemuMonitorJSONGetMigrationParams(qemuMonitorPtr mon,
 #define PARSE_INT(VAR, FIELD)   \
 PARSE_SET(virJSONValueObjectGetNumberInt, VAR, FIELD)
 
+#define PARSE_STR(VAR, FIELD)   \
+do {\
+const char *str;\
+if ((str = virJSONValueObjectGetString(result, FIELD))) {   \
+if (VIR_STRDUP(params->VAR, str) < 0)   \
+goto cleanup;   \
+}   \
+} while (0)
+
 PARSE_INT(compressLevel, "compress-level");
 PARSE_INT(compressThreads, "compress-threads");
 PARSE_INT(decompressThreads, "decompress-threads");
 PARSE_INT(cpuThrottleInitial, "cpu-throttle-initial");
 PARSE_INT(cpuThrottleIncrement, "cpu-throttle-increment");
+PARSE_STR(migrateTLSAlias, "tls-creds");
+PARSE_STR(migrateTLSHostname, "tls-hostname");
 
 #undef PARSE_SET
 #undef PARSE_INT
+#undef PARSE_STR
 
 if (virJSONValueObjectGetNumberUlong(result, "downtime-limit",
  >downtimeLimit) == 0)
 params->downtimeLimit_set = true;
 
-if ((tlsStr = virJSONValueObjectGetString(result, "tls-creds"))) {
-if (VIR_STRDUP(params->migrateTLSAlias, tlsStr) < 0)
-goto cleanup;
-}
-
-if ((tlsStr = virJSONValueObjectGetString(result, "tls-hostname"))) {
-if (VIR_STRDUP(params->migrateTLSHostname, tlsStr) < 0)
-goto cleanup;
-}
-
 ret = 0;
  cleanup:
 virJSONValueFree(cmd);
-- 
2.14.3

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

[libvirt] [PATCH 01/12] qemu: Generalize PARSE macro in qemuMonitorJSONGetMigrationParams

[Jiri Denemark]

The macro (now called PARSE_SET) is now usable for any type which needs
a *_set bool for indicating a valid value.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_monitor_json.c | 21 -
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 05cc634d2..16554d5b2 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2679,20 +2679,23 @@ qemuMonitorJSONGetMigrationParams(qemuMonitorPtr mon,
 
 result = virJSONValueObjectGet(reply, "return");
 
-#define PARSE(VAR, FIELD)   \
+#define PARSE_SET(API, VAR, FIELD)  \
 do {\
-if (virJSONValueObjectGetNumberInt(result, FIELD,   \
-   >VAR) == 0)  \
+if (API(result, FIELD, >VAR) == 0)  \
 params->VAR ## _set = true; \
 } while (0)
 
-PARSE(compressLevel, "compress-level");
-PARSE(compressThreads, "compress-threads");
-PARSE(decompressThreads, "decompress-threads");
-PARSE(cpuThrottleInitial, "cpu-throttle-initial");
-PARSE(cpuThrottleIncrement, "cpu-throttle-increment");
+#define PARSE_INT(VAR, FIELD)   \
+PARSE_SET(virJSONValueObjectGetNumberInt, VAR, FIELD)
 
-#undef PARSE
+PARSE_INT(compressLevel, "compress-level");
+PARSE_INT(compressThreads, "compress-threads");
+PARSE_INT(decompressThreads, "decompress-threads");
+PARSE_INT(cpuThrottleInitial, "cpu-throttle-initial");
+PARSE_INT(cpuThrottleIncrement, "cpu-throttle-increment");
+
+#undef PARSE_SET
+#undef PARSE_INT
 
 if (virJSONValueObjectGetNumberUlong(result, "downtime-limit",
  >downtimeLimit) == 0)
-- 
2.14.3

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

[libvirt] [PATCH 10/12] qemu: Rename TLS related migration parameters

[Jiri Denemark]

The parameters used "migrate" prefix which is pretty redundant and
qemuMonitorMigrationParams structure is our internal representation of
QEMU migration parameters and it is supposed to use names which match
QEMU names.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_migration.c| 23 +++
 src/qemu/qemu_monitor.c  |  7 +++
 src/qemu/qemu_monitor.h  |  4 ++--
 src/qemu/qemu_monitor_json.c |  8 
 tests/qemumonitorjsontest.c  |  8 
 5 files changed, 24 insertions(+), 26 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index af744661f..e4e9e79cc 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -114,7 +114,7 @@ qemuMigrationCheckTLSCreds(virQEMUDriverPtr driver,
 goto cleanup;
 
 /* NB: Could steal NULL pointer too! Let caller decide what to do. */
-VIR_STEAL_PTR(priv->migTLSAlias, migParams.migrateTLSAlias);
+VIR_STEAL_PTR(priv->migTLSAlias, migParams.tlsCreds);
 
 ret = 0;
 
@@ -225,7 +225,7 @@ qemuMigrationAddTLSObjects(virQEMUDriverPtr driver,
 *tlsAlias, ) < 0)
 goto error;
 
-if (VIR_STRDUP(migParams->migrateTLSAlias, *tlsAlias) < 0)
+if (VIR_STRDUP(migParams->tlsCreds, *tlsAlias) < 0)
 goto error;
 
 return 0;
@@ -2349,8 +2349,8 @@ qemuMigrationParamsClear(qemuMonitorMigrationParamsPtr 
migParams)
 if (!migParams)
 return;
 
-VIR_FREE(migParams->migrateTLSAlias);
-VIR_FREE(migParams->migrateTLSHostname);
+VIR_FREE(migParams->tlsCreds);
+VIR_FREE(migParams->tlsHostname);
 }
 
 
@@ -2391,8 +2391,8 @@ qemuMigrationSetEmptyTLSParams(virQEMUDriverPtr driver,
if (!priv->migTLSAlias)
return 0;
 
-   if (VIR_STRDUP(migParams->migrateTLSAlias, "") < 0 ||
-   VIR_STRDUP(migParams->migrateTLSHostname, "") < 0)
+   if (VIR_STRDUP(migParams->tlsCreds, "") < 0 ||
+   VIR_STRDUP(migParams->tlsHostname, "") < 0)
return -1;
 
 return 0;
@@ -2508,8 +2508,8 @@ qemuMigrationResetTLS(virQEMUDriverPtr driver,
 qemuDomainDelTLSObjects(driver, vm, asyncJob, secAlias, tlsAlias);
 qemuDomainSecretInfoFree(>migSecinfo);
 
-if (VIR_STRDUP(migParams.migrateTLSAlias, "") < 0 ||
-VIR_STRDUP(migParams.migrateTLSHostname, "") < 0 ||
+if (VIR_STRDUP(migParams.tlsCreds, "") < 0 ||
+VIR_STRDUP(migParams.tlsHostname, "") < 0 ||
 qemuMigrationSetParams(driver, vm, asyncJob, ) < 0)
 goto cleanup;
 
@@ -2774,7 +2774,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
 goto stopjob;
 
 /* Force reset of 'tls-hostname', it's a source only parameter */
-if (VIR_STRDUP(migParams.migrateTLSHostname, "") < 0)
+if (VIR_STRDUP(migParams.tlsHostname, "") < 0)
 goto stopjob;
 
 } else {
@@ -3737,12 +3737,11 @@ qemuMigrationRun(virQEMUDriverPtr driver,
  * connect directly to the destination. */
 if (spec->destType == MIGRATION_DEST_CONNECT_HOST ||
 spec->destType == MIGRATION_DEST_FD) {
-if (VIR_STRDUP(migParams->migrateTLSHostname,
-   spec->dest.host.name) < 0)
+if (VIR_STRDUP(migParams->tlsHostname, spec->dest.host.name) < 0)
 goto error;
 } else {
 /* Be sure there's nothing from a previous migration */
-if (VIR_STRDUP(migParams->migrateTLSHostname, "") < 0)
+if (VIR_STRDUP(migParams->tlsHostname, "") < 0)
 goto error;
 }
 } else {
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index c88726735..3e2c69a9a 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2606,15 +2606,14 @@ qemuMonitorSetMigrationParams(qemuMonitorPtr mon,
 {
 VIR_DEBUG("compressLevel=%d:%d compressThreads=%d:%d "
   "decompressThreads=%d:%d cpuThrottleInitial=%d:%d "
-  "cpuThrottleIncrement=%d:%d tlsAlias=%s "
-  "tlsHostname=%s downtimeLimit=%d:%llu",
+  "cpuThrottleIncrement=%d:%d tlsCreds=%s tlsHostname=%s "
+  "downtimeLimit=%d:%llu",
   params->compressLevel_set, params->compressLevel,
   params->compressThreads_set, params->compressThreads,
   params->decompressThreads_set, params->decompressThreads,
   params->cpuThrottleInitial_set, params->cpuThrottleInitial,
   params->cpuThrottleIncrement_set, params->cpuThrottleIncrement,
-  NULLSTR(params->migrateTLSAlias),
-  NULLSTR(params->migrateTLSHostname),
+  NULLSTR(params->tlsCreds), NULLSTR(params->tlsHostname),
   params->downtimeLimit_set, params->downtimeLimit);
 
 QEMU_CHECK_MONITOR_JSON(mon);
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index bc8494fae..e123baaae 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -625,8 +625,8 @@ struct 

[libvirt] [PATCH 07/12] qemumonitorjsontest: Rename 1st CHECK macro in migration params test

[Jiri Denemark]

The first CHECK macro in the test is used for checking integer values.
Let's make it a bit more generic to be usable for any numeric type and
use it for a new CHECK_INT macro.

Signed-off-by: Jiri Denemark 
---
 tests/qemumonitorjsontest.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 4d3b738e5..1dcff6672 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -1813,7 +1813,7 @@ 
testQemuMonitorJSONqemuMonitorJSONGetMigrationParams(const void *data)
   ) < 0)
 goto cleanup;
 
-#define CHECK(VAR, FIELD, VALUE)\
+#define CHECK_NUM(VAR, FIELD, VALUE, FMT)   \
 do {\
 if (!params.VAR ## _set) {  \
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s is not set", FIELD); \
@@ -1821,19 +1821,23 @@ 
testQemuMonitorJSONqemuMonitorJSONGetMigrationParams(const void *data)
 }   \
 if (params.VAR != VALUE) {  \
 virReportError(VIR_ERR_INTERNAL_ERROR,  \
-   "Invalid %s: %d, expected %d",   \
+   "Invalid %s: " FMT ", expected " FMT,\
FIELD, params.VAR, VALUE);   \
 goto cleanup;   \
 }   \
 } while (0)
 
-CHECK(compressLevel, "compress-level", 1);
-CHECK(compressThreads, "compress-threads", 8);
-CHECK(decompressThreads, "decompress-threads", 2);
-CHECK(cpuThrottleInitial, "cpu-throttle-initial", 20);
-CHECK(cpuThrottleIncrement, "cpu-throttle-increment", 10);
+#define CHECK_INT(VAR, FIELD, VALUE)\
+CHECK_NUM(VAR, FIELD, VALUE, "%d")
 
-#undef CHECK
+CHECK_INT(compressLevel, "compress-level", 1);
+CHECK_INT(compressThreads, "compress-threads", 8);
+CHECK_INT(decompressThreads, "decompress-threads", 2);
+CHECK_INT(cpuThrottleInitial, "cpu-throttle-initial", 20);
+CHECK_INT(cpuThrottleIncrement, "cpu-throttle-increment", 10);
+
+#undef CHECK_NUM
+#undef CHECK_INT
 
 #define CHECK(VAR, FIELD, VALUE)\
 do {\
-- 
2.14.3

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

[libvirt] [PATCH 08/12] qemumonitorjsontest: Rename 2nd CHECK macro in migration params test

[Jiri Denemark]

The second CHECK macro was used for string parameters. Let's rename it
to CHECK_STR and move it up to have all checks in one place.

Signed-off-by: Jiri Denemark 
---
 tests/qemumonitorjsontest.c | 24 +++-
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 1dcff6672..2cefdcac9 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -1830,16 +1830,7 @@ 
testQemuMonitorJSONqemuMonitorJSONGetMigrationParams(const void *data)
 #define CHECK_INT(VAR, FIELD, VALUE)\
 CHECK_NUM(VAR, FIELD, VALUE, "%d")
 
-CHECK_INT(compressLevel, "compress-level", 1);
-CHECK_INT(compressThreads, "compress-threads", 8);
-CHECK_INT(decompressThreads, "decompress-threads", 2);
-CHECK_INT(cpuThrottleInitial, "cpu-throttle-initial", 20);
-CHECK_INT(cpuThrottleIncrement, "cpu-throttle-increment", 10);
-
-#undef CHECK_NUM
-#undef CHECK_INT
-
-#define CHECK(VAR, FIELD, VALUE)\
+#define CHECK_STR(VAR, FIELD, VALUE)\
 do {\
 if (!params.VAR) {  \
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s is not set", FIELD); \
@@ -1853,10 +1844,17 @@ 
testQemuMonitorJSONqemuMonitorJSONGetMigrationParams(const void *data)
 }   \
 } while (0)
 
-CHECK(migrateTLSAlias, "tls-creds", "tls0");
-CHECK(migrateTLSHostname, "tls-hostname", "");
+CHECK_INT(compressLevel, "compress-level", 1);
+CHECK_INT(compressThreads, "compress-threads", 8);
+CHECK_INT(decompressThreads, "decompress-threads", 2);
+CHECK_INT(cpuThrottleInitial, "cpu-throttle-initial", 20);
+CHECK_INT(cpuThrottleIncrement, "cpu-throttle-increment", 10);
+CHECK_STR(migrateTLSAlias, "tls-creds", "tls0");
+CHECK_STR(migrateTLSHostname, "tls-hostname", "");
 
-#undef CHECK
+#undef CHECK_NUM
+#undef CHECK_INT
+#undef CHECK_STR
 
 ret = 0;
 
-- 
2.14.3

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

Re: [libvirt] [REPOST PATCH 0/4] Add the ability to LUKS encrypt during LV creation

[John Ferlan]

ping?

tks

John

On 10/19/2017 09:56 AM, John Ferlan wrote:
> Repost to address merge conflict from commit id '0a294a8e2' which
> used if (virStorageSourceHasBacking(>target)) instead of
> if (vol->target.backingStore).
> 
> Original series:
> https://www.redhat.com/archives/libvir-list/2017-October/msg00340.html
> 
> 
> John Ferlan (4):
>   storage: Extract out the LVCREATE
>   storage: Introduce virStorageBackendCreateVolUsingQemuImg
>   storage: Allow creation of a LUKS using logical volume
>   docs: Add news article
> 
>  docs/news.xml | 13 ++
>  src/storage/storage_backend_logical.c | 75 
> ---
>  src/storage/storage_util.c| 42 
>  src/storage/storage_util.h|  8 
>  4 files changed, 105 insertions(+), 33 deletions(-)
> 

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

Re: [libvirt] [PATCH 0/5] Properly resize a local LUKS encrypted volume

[John Ferlan]

ping^2 ?

Tks,

John

On 10/17/2017 06:34 PM, John Ferlan wrote:
> 
> 
> ping?
> 
> Thanks
> 
> John
> 
> On 10/06/2017 02:13 PM, John Ferlan wrote:
>> The patches hopefully speak for themselves.
>>
>> John Ferlan (5):
>>   storage: Alter args to storageBackendResizeQemuImg
>>   storage: Add error path for virStorageBackendCreateQemuImgCmdFromVol
>>   storage: Alter storageBackendCreateQemuImgSecretObject args
>>   storage: Properly resize a local volume using LUKS
>>   docs: Add news article for bug fix
>>
>>  docs/news.xml  |   8 +++
>>  src/storage/storage_util.c | 133 
>> -
>>  2 files changed, 115 insertions(+), 26 deletions(-)
>>
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

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

Re: [libvirt] [PATCH] qemu: logrotate: drop minsize directive

[Jim Fehlig]

On 10/26/2017 11:14 AM, Daniel P. Berrange wrote:

On Thu, Oct 26, 2017 at 11:13:23AM -0600, Jim Fehlig wrote:

On 10/26/2017 12:48 AM, Daniel P. Berrange wrote:

On Wed, Oct 25, 2017 at 03:30:46PM -0600, Jim Fehlig wrote:

On a cloud host it is possible to create 100's of unique instances
per day, each leaving behind a /var/log/libvirt/qemu/instance-name.log
file that is < 100k. With the current 'minsize 100k' directive, these
files are never rotated and hence never removed. Over months of time,
tens of thousands of these files can accumulate on the host.

Dropping 'minsize 100k' allows rotating small files, which will
increase the number of log files, but 'rotate 4' ensures they will
be removed after a month.

Signed-off-by: Jim Fehlig 
---
   daemon/libvirtd.qemu.logrotate.in | 1 -
   1 file changed, 1 deletion(-)

diff --git a/daemon/libvirtd.qemu.logrotate.in 
b/daemon/libvirtd.qemu.logrotate.in
index 15cf019b2..cdb399ef2 100644
--- a/daemon/libvirtd.qemu.logrotate.in
+++ b/daemon/libvirtd.qemu.logrotate.in
@@ -5,5 +5,4 @@
   compress
   delaycompress
   copytruncate
-minsize 100k
   }


Reviewed-by: Daniel P. Berrange 


Only after pushing this did I remember there are logrotate files for other
hypervisor drivers: libxl, lxc, and uml. Would it be fine to push a followup
that removes minsize from those files as well?


Yep, that makes sense.


Thanks. Followup patch is now pushed.

Regards,
Jim

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

Re: [libvirt] [PATCH] qemu: logrotate: drop minsize directive

[Daniel P. Berrange]

On Thu, Oct 26, 2017 at 11:13:23AM -0600, Jim Fehlig wrote:
> On 10/26/2017 12:48 AM, Daniel P. Berrange wrote:
> > On Wed, Oct 25, 2017 at 03:30:46PM -0600, Jim Fehlig wrote:
> > > On a cloud host it is possible to create 100's of unique instances
> > > per day, each leaving behind a /var/log/libvirt/qemu/instance-name.log
> > > file that is < 100k. With the current 'minsize 100k' directive, these
> > > files are never rotated and hence never removed. Over months of time,
> > > tens of thousands of these files can accumulate on the host.
> > > 
> > > Dropping 'minsize 100k' allows rotating small files, which will
> > > increase the number of log files, but 'rotate 4' ensures they will
> > > be removed after a month.
> > > 
> > > Signed-off-by: Jim Fehlig 
> > > ---
> > >   daemon/libvirtd.qemu.logrotate.in | 1 -
> > >   1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/daemon/libvirtd.qemu.logrotate.in 
> > > b/daemon/libvirtd.qemu.logrotate.in
> > > index 15cf019b2..cdb399ef2 100644
> > > --- a/daemon/libvirtd.qemu.logrotate.in
> > > +++ b/daemon/libvirtd.qemu.logrotate.in
> > > @@ -5,5 +5,4 @@
> > >   compress
> > >   delaycompress
> > >   copytruncate
> > > -minsize 100k
> > >   }
> > 
> > Reviewed-by: Daniel P. Berrange 
> 
> Only after pushing this did I remember there are logrotate files for other
> hypervisor drivers: libxl, lxc, and uml. Would it be fine to push a followup
> that removes minsize from those files as well?

Yep, that makes sense.


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: logrotate: drop minsize directive

[Jim Fehlig]

On 10/26/2017 12:48 AM, Daniel P. Berrange wrote:

On Wed, Oct 25, 2017 at 03:30:46PM -0600, Jim Fehlig wrote:

On a cloud host it is possible to create 100's of unique instances
per day, each leaving behind a /var/log/libvirt/qemu/instance-name.log
file that is < 100k. With the current 'minsize 100k' directive, these
files are never rotated and hence never removed. Over months of time,
tens of thousands of these files can accumulate on the host.

Dropping 'minsize 100k' allows rotating small files, which will
increase the number of log files, but 'rotate 4' ensures they will
be removed after a month.

Signed-off-by: Jim Fehlig 
---
  daemon/libvirtd.qemu.logrotate.in | 1 -
  1 file changed, 1 deletion(-)

diff --git a/daemon/libvirtd.qemu.logrotate.in 
b/daemon/libvirtd.qemu.logrotate.in
index 15cf019b2..cdb399ef2 100644
--- a/daemon/libvirtd.qemu.logrotate.in
+++ b/daemon/libvirtd.qemu.logrotate.in
@@ -5,5 +5,4 @@
  compress
  delaycompress
  copytruncate
-minsize 100k
  }


Reviewed-by: Daniel P. Berrange 


Only after pushing this did I remember there are logrotate files for other 
hypervisor drivers: libxl, lxc, and uml. Would it be fine to push a followup 
that removes minsize from those files as well?


Regards,
Jim

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

Re: [libvirt] [PATCH v2 03/15] vbox: Cleanup partially-defined VM on failure

[Dawid Zamirski]

On Tue, 2017-10-24 at 15:35 -0400, Dawid Zamirski wrote:
> Since the VBOX API requires to register an initial VM before
> proceeding
> to attach any remaining devices to it, any failure to attach such
> devices should result in automatic cleanup of the initially
> registered
> VM so that the state of VBOX registry remains clean without any
> leftover
> "aborted" VMs in it. Failure to cleanup of such partial VMs results
> in a
> warning log so that actual define error stays on the top of the error
> stack.
> ---
>  src/vbox/vbox_common.c | 41 +++-
> -
>  1 file changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> index 92ee37164..812c940e6 100644
> --- a/src/vbox/vbox_common.c
> +++ b/src/vbox/vbox_common.c
> @@ -1853,6 +1853,8 @@ vboxDomainDefineXMLFlags(virConnectPtr conn,
> const char *xml, unsigned int flags
>  char uuidstr[VIR_UUID_STRING_BUFLEN];
>  virDomainPtr ret = NULL;
>  unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
> +bool machineReady = false;
> +
>  
>  virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
>  
> @@ -1862,12 +1864,12 @@ vboxDomainDefineXMLFlags(virConnectPtr conn,
> const char *xml, unsigned int flags
>  if (!data->vboxObj)
>  return ret;
>  
> -VBOX_IID_INITIALIZE();
>  if (!(def = virDomainDefParseString(xml, data->caps, data-
> >xmlopt,
>  NULL, parse_flags))) {
> -goto cleanup;
> +return ret;
>  }
>  
> +VBOX_IID_INITIALIZE();
>  virUUIDFormat(def->uuid, uuidstr);
>  
>  rc = gVBoxAPI.UIVirtualBox.CreateMachine(data, def, ,
> uuidstr);
> @@ -1959,30 +1961,41 @@ vboxDomainDefineXMLFlags(virConnectPtr conn,
> const char *xml, unsigned int flags
>  vboxAttachUSB(def, data, machine);
>  vboxAttachSharedFolder(def, data, machine);
>  
> -/* Save the machine settings made till now and close the
> - * session. also free up the mchiid variable used.
> +machineReady = true;
> +
> + cleanup:
> +/* Save the machine settings made till now, even when jumped
> here on error,
> + * as otherwise unregister won't cleanup properly. For example,
> it won't
> + * close media that were partially attached. The VBOX SDK docs
> say that
> + * unregister implicitly calls saveSettings but evidently it's
> not so...
>   */
>  rc = gVBoxAPI.UIMachine.SaveSettings(machine);

There's one more code path in this function that causes a segfault here
due to machine == NULL (e.g. when CreateMachine fails). I already have
patched this but I'll hold off with sending V3 until this series is
reviewed and feedback is available.

> 

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

Re: [libvirt] [PATCH v2] qemu: change monitor.sock from /var/lib/libvirt/qemu/domain-*** to /var/run/libvirt/qemu/domain-***

[Michal Privoznik]

On 10/16/2017 04:08 AM, xinhua.Cao wrote:
> directory /var/lib alway is Persistence directory, but in redhat system, 
> /var/run is memory directory.
> our running domain xml is saved at /var/run/libvirt/qemu. so if we cold reset 
> system,
> the /var/run/libvirt/qemu directory is clear, but 
> /var/lib/libvirt/qemu/domain-*** is saved. so there
> have same /var/lib/libvirt/qemu/domain-*** directory will be left over at 
> system cold reset.
> ---
>  src/qemu/qemu_domain.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index ed27a91..3e6fe9b 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1674,7 +1674,7 @@ qemuDomainSetPrivatePaths(virQEMUDriverPtr driver,
>  goto cleanup;
>  
>  if (!priv->libDir &&
> -virAsprintf(>libDir, "%s/domain-%s", cfg->libDir, domname) < 0)
> +virAsprintf(>libDir, "%s/domain-%s", cfg->stateDir, domname) < 
> 0)
>  goto cleanup;
>  
>  if (!priv->channelTargetDir &&
> 

Almost. I see a problem with this patch. Problem is that
qemuxml2argvtest needs to be updated. Which is not trivial because state
dir is a tempdir (mkdtemp()), and thus '-monitor' part of the command
line changes with each test invocation.

Michal

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

Re: [libvirt] [PATCH 12/12] qemu: domain: skip chain detection to end of backing chain

[John Ferlan]

On 10/20/2017 09:47 AM, Peter Krempa wrote:
> When a user provides the backing chain, we will not need to re-detect
> all the backing stores again, but should move to the end of the user
> specified chain. Additionally if a user provides a full terminated chain
> we should not attempt any further detection.
> ---
>  src/qemu/qemu_domain.c | 48 +++-
>  1 file changed, 39 insertions(+), 9 deletions(-)
> 

Looks like some magic combination of keys caused an premature send.

In any case, IDC if you change the noted disk->src to src, but figured
I'd ask anyway...

Reviewed-by: John Ferlan 


> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 3560cdd29..5973474ca 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -6030,27 +6030,57 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
>   bool report_broken)
>  {
>  virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> -int ret = 0;
> +virStorageSourcePtr src = disk->src;
> +int ret = -1;
>  uid_t uid;
>  gid_t gid;
> 
> -if (virStorageSourceIsEmpty(disk->src))
> +if (virStorageSourceIsEmpty(src)) {
> +ret = 0;
>  goto cleanup;
> +}
> 
>  if (virStorageSourceHasBacking(disk->src)) {
> -if (force_probe)
> -virStorageSourceBackingStoreClear(disk->src);
> -else
> -goto cleanup;
> +if (force_probe) {
> +virStorageSourceBackingStoreClear(src);
> +} else {
> +/* skip to the end of the chain */
> +while (virStorageSourceIsBacking(src)) {
> +if (report_broken &&
> +virStorageFileSupportsAccess(src)) {
> +
> +if (qemuDomainStorageFileInit(driver, vm, src, 
> disk->src) < 0)
> +goto cleanup;
> +
> +if (virStorageFileAccess(src, F_OK) < 0) {
> +virStorageFileReportBrokenChain(errno, src, 
> disk->src);
> +virStorageFileDeinit(src);
> +goto cleanup;
> +}
> +
> +virStorageFileDeinit(src);
> +}
> +src = src->backingStore;
> +}
> +}
>  }
> 
> -qemuDomainGetImageIds(cfg, vm, disk->src, NULL, , );
> +/* We skipped to the end of the chain. Skip detection if there's the
> + * terminator. (An allocated but empty backingStore) */
> +if (src->backingStore) {
> +ret = 0;
> +goto cleanup;
> +}
> +
> +qemuDomainGetImageIds(cfg, vm, src, disk->src, , );
> 
> -if (virStorageFileGetMetadata(disk->src,
> +if (virStorageFileGetMetadata(src,
>uid, gid,
>cfg->allowDiskFormatProbing,
>report_broken) < 0)
> -ret = -1;
> +goto cleanup;

This looks familiar to the recent upstream question...


> +
> +ret = 0;
> 
>   cleanup:
>  virObjectUnref(cfg);
> 

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

Re: [libvirt] [PATCH 12/12] qemu: domain: skip chain detection to end of backing chain

[John Ferlan]

On 10/20/2017 09:47 AM, Peter Krempa wrote:
> When a user provides the backing chain, we will not need to re-detect
> all the backing stores again, but should move to the end of the user
> specified chain. Additionally if a user provides a full terminated chain
> we should not attempt any further detection.
> ---
>  src/qemu/qemu_domain.c | 48 +++-
>  1 file changed, 39 insertions(+), 9 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 3560cdd29..5973474ca 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -6030,27 +6030,57 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
>   bool report_broken)
>  {
>  virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> -int ret = 0;
> +virStorageSourcePtr src = disk->src;
> +int ret = -1;
>  uid_t uid;
>  gid_t gid;
> 
> -if (virStorageSourceIsEmpty(disk->src))
> +if (virStorageSourceIsEmpty(src)) {
> +ret = 0;
>  goto cleanup;
> +}
> 
>  if (virStorageSourceHasBacking(disk->src)) {

could this one be just @src?

> -if (force_probe)
> -virStorageSourceBackingStoreClear(disk->src);
> -else
> -goto cleanup;
> +if (force_probe) {
> +virStorageSourceBackingStoreClear(src);
> +} else {
> +/* skip to the end of the chain */
> +while (virStorageSourceIsBacking(src)) {
> +if (report_broken &&
> +virStorageFileSupportsAccess(src)) {
> +
> +if (qemuDomainStorageFileInit(driver, vm, src, 
> disk->src) < 0)
> +goto cleanup;
> +
> +if (virStorageFileAccess(src, F_OK) < 0) {
> +virStorageFileReportBrokenChain(errno, src, 
> disk->src);
> +virStorageFileDeinit(src);
> +goto cleanup;
> +}
> +
> +virStorageFileDeinit(src);
> +}
> +src = src->backingStore;
> +}
> +}
>  }
> 
> -qemuDomainGetImageIds(cfg, vm, disk->src, NULL, , );
> +/* We skipped to the end of the chain. Skip detection if there's the
> + * terminator. (An allocated but empty backingStore) */
> +if (src->backingStore) {
> +ret = 0;
> +goto cleanup;
> +}
> +
> +qemuDomainGetImageIds(cfg, vm, src, disk->src, , );
> 
> -if (virStorageFileGetMetadata(disk->src,
> +if (virStorageFileGetMetadata(src,
>uid, gid,
>cfg->allowDiskFormatProbing,
>report_broken) < 0)
> -ret = -1;
> +goto cleanup;

> +
> +ret = 0;
> 
>   cleanup:
>  virObjectUnref(cfg);
> 

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

Re: [libvirt] [PATCH 11/12] qemu: domain: Prepare TLS data for the whole backing chain

[John Ferlan]

On 10/20/2017 09:47 AM, Peter Krempa wrote:
> Iterate through the backing chain when setting up TLS for disks.
> ---
>  src/qemu/qemu_domain.c | 41 ++---
>  1 file changed, 22 insertions(+), 19 deletions(-)
> 

So (for now) a VxHS device could be at some depth within the backing
chain and we'd need to make sure we can use TLS for it, but not
necessarily use TLS for all levels - is that the basic premise? I don't
even want to think about having different target TLS types within the
chain - pffttt, splat goes the brain.


Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 10/12] qemu: domain: Remove pointless alias check

[John Ferlan]

On 10/20/2017 09:47 AM, Peter Krempa wrote:
> When attaching the disks, aliases are always generated.
> ---
>  src/qemu/qemu_domain.c  | 8 
>  src/qemu/qemu_domain.h  | 3 +--
>  src/qemu/qemu_hotplug.c | 2 +-
>  src/qemu/qemu_process.c | 2 +-
>  4 files changed, 3 insertions(+), 12 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 09/12] qemu: domain: Destroy secrets for complete backing chain

[John Ferlan]

On 10/20/2017 09:47 AM, Peter Krempa wrote:
> ---
>  src/qemu/qemu_domain.c | 22 --
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 


Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 08/12] qemu: domain: Extract setup for disk source secrets

[John Ferlan]

On 10/20/2017 09:47 AM, Peter Krempa wrote:
> Separate it so that it deals only with single virStorageSource, so that
> it can later be reused for full backing chain support.
> 
> Two aliases are passed since authentication is more relevant to the
> 'storage backend' whereas encryption is more relevant to the protocol
> layer. When using node names, the aliases will be different.
> ---
>  src/qemu/qemu_domain.c | 49 +++--
>  1 file changed, 31 insertions(+), 18 deletions(-)
> 

FWIW: The @authalias would be the secret for the source to access the
server (RBD only right now, but the iSCSI patches are on list) while the
@encalias would be the LUKS secret.

Does the backing chain allow different "auth" sources for different
chain members? Mind boggling, but I guess possible.

I suppose it would also be possible that some member of the chain
doesn't use the auth, but rather it's a local LUKS encrypted file - is
that the essential goal?

I think perhaps it'd help to add some comments for
qemuDomainSecretStorageSourcePrepare in order to describe the parameters
and the "expectations" for varying levels of the chain.  There's also
some "assumptions" built into the hotplug code for at least the top level.

In general as long as you add a bit more details as to the parameter
expectations,

Reviewed-by: John Ferlan 

John

> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 24ed61bc2..4a2ba1761 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1368,27 +1368,19 @@ qemuDomainDiskHasEncryptionSecret(virStorageSourcePtr 
> src)
>  }
> 
> 
> -/* qemuDomainSecretDiskPrepare:
> - * @conn: Pointer to connection
> - * @priv: pointer to domain private object
> - * @disk: Pointer to a disk definition
> - *
> - * For the right disk, generate the qemuDomainSecretInfo structure.
> - *
> - * Returns 0 on success, -1 on failure
> - */
> -int
> -qemuDomainSecretDiskPrepare(virConnectPtr conn,
> -qemuDomainObjPrivatePtr priv,
> -virDomainDiskDefPtr disk)
> +static int
> +qemuDomainSecretStorageSourcePrepare(virConnectPtr conn,
> + qemuDomainObjPrivatePtr priv,
> + virStorageSourcePtr src,
> + const char *authalias,
> + const char *encalias)
>  {
> -virStorageSourcePtr src = disk->src;
>  qemuDomainStorageSourcePrivatePtr srcPriv;
> 
> -if (!(disk->src->privateData = qemuDomainStorageSourcePrivateNew()))
> +if (!(src->privateData = qemuDomainStorageSourcePrivateNew()))
>  return -1;
> 
> -srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
> +srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
> 
>  if (qemuDomainSecretDiskCapable(src)) {
>  virSecretUsageType usageType = VIR_SECRET_USAGE_TYPE_ISCSI;
> @@ -1397,7 +1389,7 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn,
>  usageType = VIR_SECRET_USAGE_TYPE_CEPH;
> 
>  if (!(srcPriv->secinfo =
> -  qemuDomainSecretInfoNew(conn, priv, disk->info.alias,
> +  qemuDomainSecretInfoNew(conn, priv, authalias,
>usageType, src->auth->username,
>>auth->seclookupdef, false)))
>return -1;
> @@ -1405,7 +1397,7 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn,
> 
>  if (qemuDomainDiskHasEncryptionSecret(src)) {
>  if (!(srcPriv->encinfo =
> -  qemuDomainSecretInfoNew(conn, priv, disk->info.alias,
> +  qemuDomainSecretInfoNew(conn, priv, encalias,
>VIR_SECRET_USAGE_TYPE_VOLUME, NULL,
>
> >encryption->secrets[0]->seclookupdef,
>true)))
> @@ -1416,6 +1408,27 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn,
>  }
> 
> 
> +/* qemuDomainSecretDiskPrepare:
> + * @conn: Pointer to connection
> + * @priv: pointer to domain private object
> + * @disk: Pointer to a disk definition
> + *
> + * For the right disk, generate the qemuDomainSecretInfo structure.
> + *
> + * Returns 0 on success, -1 on failure
> + */
> +
> +int
> +qemuDomainSecretDiskPrepare(virConnectPtr conn,
> +qemuDomainObjPrivatePtr priv,
> +virDomainDiskDefPtr disk)
> +{
> +return qemuDomainSecretStorageSourcePrepare(conn, priv, disk->src,
> +disk->info.alias,
> +disk->info.alias);
> +}
> +
> +
>  /* qemuDomainSecretHostdevDestroy:
>   * @disk: Pointer to a hostdev definition
>   *
> 

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

Re: [libvirt] [PATCH 07/12] qemu: domain: Simplify using DAC permissions of top of backing chain

[John Ferlan]

On 10/20/2017 09:47 AM, Peter Krempa wrote:
> qemuDomainGetImageIds and qemuDomainStorageFileInit are helpful when
> trying to access a virStorageSource from the qemu driver since they
> figure out the correct uid and gid for the image.
> 
> When accessing members of a backing chain the permissions for the top
> level would be used. To allow using specific permissions per backing
> chain level but still allow inheritance from the parent of the chain we
> need to add a new parameter to the image ID APIs.
> ---
>  src/qemu/qemu_domain.c | 13 ++---
>  src/qemu/qemu_domain.h |  3 ++-
>  src/qemu/qemu_driver.c |  6 +++---
>  3 files changed, 15 insertions(+), 7 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 06/12] security: selinux: Take parent security label into account

[John Ferlan]

On 10/20/2017 09:47 AM, Peter Krempa wrote:
> Until now we ignored user-provided backing chains and while detecting
> the code inherited labels of the parent device. With user provided
> chains we should keep this functionality, so label of the parent image
> in the backing chain will be applied if an image-specific label is not
> present.
> ---
>  src/security/security_selinux.c | 15 ---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 05/12] security: dac: Take parent security label into account

[John Ferlan]

On 10/20/2017 09:47 AM, Peter Krempa wrote:
> Until now we ignored user-provided backing chains and while detecting
> the code inherited labels of the parent device. With user provided
> chains we should keep this functionality, so label of the parent image
> in the backing chain will be applied if an image-specific label is not
> present.
> ---
>  src/security/security_dac.c | 38 +-
>  1 file changed, 29 insertions(+), 9 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] RFC: how to help to detect backing chain when no backing format info in images

[Nikolay Shirokovskiy]

On 26.10.2017 17:15, Peter Krempa wrote:
> On Thu, Oct 26, 2017 at 17:05:41 +0300, Nikolay Shirokovskiy wrote:
>> I create transient domain with disk based on qcow2 image with 2 backing 
>> images.
>> I specify qcow2 format explicitly for the top image and omit the backing
>> chain in xml for create becase libvirt does not utilize this data anyway.
>> Top image and its backing file don't have backing file format information 
>> and format autodetection is off by default thus I get wrong backing chain
>> for the disk. Second image have format raw and there is no third backing
>> image obviously.
> 
> You would have to enable format detection, but that's not safe.
> 
>>
>> How can I get correct backing chain in domain xml in this case? I don't
>> want to set backing file format on this images because they are backup
>> images and I don't want to touch them in any way. Can we add option
>> to create API not to drop backing chain info that I can specify
>> on start? (Drop is forced on start in qemuDomainDetermineDiskChain)
> 
> You'll have to wait until I send the patches to allow specifying the
> backing chain in the XML (some prequels were already posted with
> 'blockdev-add saga' in the subject)
> 
> With that you'll be able to declare the backing chain in the XML along
> with the format so libvirt will not have to detect the chain (thus no
> security hole and also it provides workaround).
> 
> I don't want to add any other workarounds for those since it would
> likely introduce security risks.
> 

Great! Thanx!

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

Re: [libvirt] [PATCH 2/2] virt-aa-helper-test: only fails go to stdout by default

[Jamie Strandboge]

On Thu, 2017-10-26 at 15:22 +0200, Christian Ehrhardt wrote:
> By Default (without -d) the tests will only print Failures.
> So a log should follow general "no message is a good message" style.
> 
> But the testfw checks always emit the skip info to stdout. Instead
> they should use the redirection that is controlled by -d.
> 
> This avoids mesages like the following to clutter the log:
>   Skipping FW AAVMF32 test. Could not find
> /usr/share/AAVMF/AAVMF32_CODE.fd
> 
> Signed-off-by: Christian Ehrhardt 
> ---
>  tests/virt-aa-helper-test | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
> index e837668..d2a557e 100755
> --- a/tests/virt-aa-helper-test
> +++ b/tests/virt-aa-helper-test
> @@ -155,7 +155,7 @@ testfw() {
>  -e "s,, type='pflash'>$fwpath,g" "$template_xml" > "$test_xml"
>  testme "0" "$title" "-r -u $valid_uuid" "$test_xml"
>  else
> -echo "Skipping FW $title test. Could not find $fwpath"
> +echo "Skipping FW $title test. Could not find $fwpath"
> >$output
>  fi
>  }

LGTM. Thanks!

-- 
Jamie Strandboge | http://www.canonical.com

signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] RFC: how to help to detect backing chain when no backing format info in images

[Peter Krempa]

On Thu, Oct 26, 2017 at 17:05:41 +0300, Nikolay Shirokovskiy wrote:
> I create transient domain with disk based on qcow2 image with 2 backing 
> images.
> I specify qcow2 format explicitly for the top image and omit the backing
> chain in xml for create becase libvirt does not utilize this data anyway.
> Top image and its backing file don't have backing file format information 
> and format autodetection is off by default thus I get wrong backing chain
> for the disk. Second image have format raw and there is no third backing
> image obviously.

You would have to enable format detection, but that's not safe.

> 
> How can I get correct backing chain in domain xml in this case? I don't
> want to set backing file format on this images because they are backup
> images and I don't want to touch them in any way. Can we add option
> to create API not to drop backing chain info that I can specify
> on start? (Drop is forced on start in qemuDomainDetermineDiskChain)

You'll have to wait until I send the patches to allow specifying the
backing chain in the XML (some prequels were already posted with
'blockdev-add saga' in the subject)

With that you'll be able to declare the backing chain in the XML along
with the format so libvirt will not have to detect the chain (thus no
security hole and also it provides workaround).

I don't want to add any other workarounds for those since it would
likely introduce security risks.


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

Re: [libvirt] [PATCH 04/12] security: selinux: Pass parent storage source into image labeling helper

[John Ferlan]

On 10/20/2017 09:47 AM, Peter Krempa wrote:
> virSecuritySELinuxSetImageLabelInternal assigns different labels to
> backing chain members than to the parent image. This was done via the
> 'first' flag. Convert it to passing in pointer to the parent
> virStorageSource. This will allow us to use the parent virStorageSource
> in further changes.
> ---
>  src/security/security_selinux.c | 11 ---
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 03/12] storage: Extract error reporting for broken chains

[John Ferlan]

On 10/20/2017 09:47 AM, Peter Krempa wrote:
> Simplify reporting the error if backing chain is broken for further
> callers by extracting it into a separate function.
> ---
>  src/storage/storage_source.c | 47 
> +++-
>  src/storage/storage_source.h |  4 
>  2 files changed, 37 insertions(+), 14 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 02/12] storage: Add feature check for storage file backend supporting access check

[John Ferlan]

On 10/20/2017 09:47 AM, Peter Krempa wrote:
> When the user provides backing chain, we don't need the full support for
> traversing the backing chain. This patch adds a feature check for the
> virStorageSourceAccess API.
> ---
>  src/storage/storage_source.c | 20 
>  src/storage/storage_source.h |  1 +
>  2 files changed, 21 insertions(+)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 1/2] virt-aa-helper: apparmor wildcards to forbidden chars

[Jamie Strandboge]

On Thu, 2017-10-26 at 15:22 +0200, Christian Ehrhardt wrote:
> Some globbing chars in the domain name could be used to break out of
> apparmor rules, so lets forbid these when in virt-aa-helper.
> 
> Also adding a test to ensure all those cases were detected as bad
> char.
> 
> Signed-off-by: Christian Ehrhardt 
> ---
>  src/security/virt-aa-helper.c |  2 +-
>  tests/virt-aa-helper-test | 20 
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-
> helper.c
> index ef1bf01..4f1c195 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -449,7 +449,7 @@ valid_name(const char *name)
>  {
>  /* just try to filter out any dangerous characters in the name
> that can be
>   * used to subvert the profile */
> -const char *bad = "/[]*";
> +const char *bad = "/[]{}?^,\"*";
>  
>  if (strlen(name) == 0)
>  return -1;
> diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
> index 0599046..e837668 100755
> --- a/tests/virt-aa-helper-test
> +++ b/tests/virt-aa-helper-test
> @@ -211,6 +211,26 @@ testme "1" "-c with no os.type" "-c -u
> $valid_uuid" "$test_xml"
>  sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
> "s,hvm,hvm_invalid,g" "$template_xml" > "$test_xml"
>  testme "1" "-c with invalid hvm" "-c -u $valid_uuid" "$test_xml"
>  
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,virt-
> aa-helper-test,virt-aa-helper-test-/,g" "$template_xml" > "$test_xml"
> +testme "1" "-c with invalid domain name char /" "-c -u $valid_uuid"
> "$test_xml"
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,virt-
> aa-helper-test,virt-aa-helper-test-[,g" "$template_xml" > "$test_xml"
> +testme "1" "-c with invalid domain name char [" "-c -u $valid_uuid"
> "$test_xml"
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,virt-
> aa-helper-test,virt-aa-helper-test-],g" "$template_xml" > "$test_xml"
> +testme "1" "-c with invalid domain name char ]" "-c -u $valid_uuid"
> "$test_xml"
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,virt-
> aa-helper-test,virt-aa-helper-test-{,g" "$template_xml" > "$test_xml"
> +testme "1" "-c with invalid domain name char {" "-c -u $valid_uuid"
> "$test_xml"
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,virt-
> aa-helper-test,virt-aa-helper-test-},g" "$template_xml" > "$test_xml"
> +testme "1" "-c with invalid domain name char }" "-c -u $valid_uuid"
> "$test_xml"
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,virt-
> aa-helper-test,virt-aa-helper-test-?,g" "$template_xml" > "$test_xml"
> +testme "1" "-c with invalid domain name char ?" "-c -u $valid_uuid"
> "$test_xml"
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,virt-
> aa-helper-test,virt-aa-helper-test-^,g" "$template_xml" > "$test_xml"
> +testme "1" "-c with invalid domain name char ^" "-c -u $valid_uuid"
> "$test_xml"
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,virt-
> aa-helper-test,virt-aa-helper-test-\,,g" "$template_xml" >
> "$test_xml"
> +testme "1" "-c with invalid domain name char ," "-c -u $valid_uuid"
> "$test_xml"
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,virt-
> aa-helper-test,virt-aa-helper-test-\",g" "$template_xml" >
> "$test_xml"
> +testme "1" "-c with invalid domain name char \"" "-c -u $valid_uuid"
> "$test_xml"
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,virt-
> aa-helper-test,virt-aa-helper-test-*,g" "$template_xml" > "$test_xml"
> +testme "1" "-c with invalid domain name char *" "-c -u $valid_uuid"
> "$test_xml"
>  
>  echo "Expected pass:" >$output
>  sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g"
> "$template_xml" > "$test_xml"

LGTM, thanks!

-- 
Jamie Strandboge | http://www.canonical.com

signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 01/12] storage: Extract common code to retrieve driver backend for support check

[John Ferlan]

On 10/20/2017 09:47 AM, Peter Krempa wrote:
> The 'file access' module of the storage driver has few feature checks to
> determine whether libvirt supports given storage driver method. The code
> to retrieve the driver struct needed for the check is the same so it can
> be extracted.
> ---
>  src/storage/storage_source.c | 43 +++
>  1 file changed, 19 insertions(+), 24 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

[libvirt] RFC: how to help to detect backing chain when no backing format info in images

[Nikolay Shirokovskiy]

I create transient domain with disk based on qcow2 image with 2 backing images.
I specify qcow2 format explicitly for the top image and omit the backing
chain in xml for create becase libvirt does not utilize this data anyway.
Top image and its backing file don't have backing file format information 
and format autodetection is off by default thus I get wrong backing chain
for the disk. Second image have format raw and there is no third backing
image obviously.

How can I get correct backing chain in domain xml in this case? I don't
want to set backing file format on this images because they are backup
images and I don't want to touch them in any way. Can we add option
to create API not to drop backing chain info that I can specify
on start? (Drop is forced on start in qemuDomainDetermineDiskChain)

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

Re: [libvirt] [PATCH] AppArmor: add rules needed with additional mediation features brought by Linux 4.14.

[Jamie Strandboge]

On Thu, 2017-10-26 at 08:39 -0500, Jamie Strandboge wrote:
> On Thu, 2017-10-26 at 10:22 +, intrigeri+libv...@boum.org wrote:
> > diff --git a/examples/apparmor/usr.sbin.libvirtd
> > b/examples/apparmor/usr.sbin.libvirtd
> > index 819068ffc3..eb24726e08 100644
> > --- a/examples/apparmor/usr.sbin.libvirtd
> > +++ b/examples/apparmor/usr.sbin.libvirtd
> > @@ -30,10 +30,13 @@
> ># Needed for vfio
> >capability sys_resource,
> >  
> > +  mount,
> > +
> 
> This is interesting since the Ubuntu profile is missing mount rules.
> What specific denials/libvirt actions prompted this rule?
> 
Responding to myself now that I read the SUSE bug. I actually suggest
using the fine-grained rules in the SUSE patch because it is much
easier to add more rules for more access than to take them away. These
rules are in the 'examples' directory so I think it is expected that a
distribution may need to tailor them from time to time (hopefully
upstreaming their changes! :).

-- 
Jamie Strandboge | http://www.canonical.com

signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] AppArmor: add rules needed with additional mediation features brought by Linux 4.14.

[Jamie Strandboge]

On Thu, 2017-10-26 at 10:22 +, intrigeri+libv...@boum.org wrote:
> From: intrigeri 
> 
> ---
>  examples/apparmor/libvirt-qemu  | 2 ++
>  examples/apparmor/usr.sbin.libvirtd | 6 ++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/examples/apparmor/libvirt-qemu
> b/examples/apparmor/libvirt-qemu
> index b341e31f42..5994a35042 100644
> --- a/examples/apparmor/libvirt-qemu
> +++ b/examples/apparmor/libvirt-qemu
> @@ -16,6 +16,8 @@
>network inet stream,
>network inet6 stream,
>  
> +  signal (receive) set=("term") peer=/usr/sbin/libvirtd,

I suggest this rule instead:

  signal (receive) peer=/usr/sbin/libvirtd,

ie, let libvirtd send any signals it wants to its VMs.

>/dev/net/tun rw,
>/dev/kvm rw,
>/dev/ptmx rw,
> diff --git a/examples/apparmor/usr.sbin.libvirtd
> b/examples/apparmor/usr.sbin.libvirtd
> index 819068ffc3..eb24726e08 100644
> --- a/examples/apparmor/usr.sbin.libvirtd
> +++ b/examples/apparmor/usr.sbin.libvirtd
> @@ -30,10 +30,13 @@
># Needed for vfio
>capability sys_resource,
>  
> +  mount,
> +

This is interesting since the Ubuntu profile is missing mount rules.
What specific denials/libvirt actions prompted this rule?

>network inet stream,
>network inet dgram,
>network inet6 stream,
>network inet6 dgram,
> +  network netlink raw,

This is fine.

>network packet dgram,
>network packet raw,
>  
> @@ -42,6 +45,9 @@
>ptrace (trace) peer=/usr/sbin/dnsmasq,
>ptrace (trace) peer=libvirt-*,
>  
> +  signal (send) set=("hup") peer=/usr/sbin/dnsmasq,

I suspect you are missing 'term' to support net-destroy. I suggest this
instead:

  signal (send) peer=/usr/sbin/dnsmasq,

Ie, let libvirtd send any signals to fully manage its dnsmasq. 

> +  signal (send) set=("term") peer=libvirt-*,

I suggest this instead:

  signal (send) peer=libvirt-*,

Ie, let libvirtd send any signals to its VMs.

I think you are missing this in libvirt-qemu:

  ptrace (readby, tracedby) peer=/usr/sbin/libvirtd,

and this in usr.sbin.libvirtd:

  ptrace (read, trace) peer=libvirt-*,

-- 
Jamie Strandboge | http://www.canonical.com

signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: treat iso images as raw

[Peter Krempa]

On Thu, Oct 26, 2017 at 16:22:37 +0300, Nikolay Shirokovskiy wrote:
> 
> 
> On 26.10.2017 16:18, Peter Krempa wrote:
> > On Thu, Oct 26, 2017 at 13:09:25 +0100, Daniel Berrange wrote:
> >> On Thu, Oct 26, 2017 at 03:04:08PM +0300, Nikolay Shirokovskiy wrote:
> >>> if image format probing is on and image format of iso file
> >>> is not specified qemu fail to start a domain or change disk
> >>> media giving errors like [1]. The problem is format is being
> >>> detected as 'iso' and qemu expect format to be raw for iso
> >>> images.
> >>>
> >>> It makes sense to me because iso refers to filesystem format
> >>> in image not image format itself. Thus let's just convert
> >>> iso to raw in case of qemu.
> >>>
> >>> There is a similar patch for storage pools - 0e5db762.
> >>>
> >>> [1] Unknown driver 'iso'
> >>>
> >>> ---
> >>>
> >>> ISO as image format was added right at the beginning by e266ded2f
> >>> without any further comments. Maybe we just can drop ISO from image
> >>> formats entirely as it is not image format or some hypervisors
> >>> treat it in a special way?
> >>
> >> Yeah, I'm inclined to say we can drop it. I don't recall either Xen or
> >> QEMU caring about an 'iso' disk format
> > 
> > The hypervisors probably don't care about this but the storage driver
> > may care (At least for display purposes).
> > 
> >>>  src/qemu/qemu_domain.c | 7 ++-
> >>>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> >>> index c7c9e94..3da9271 100644
> >>> --- a/src/qemu/qemu_domain.c
> >>> +++ b/src/qemu/qemu_domain.c
> >>> @@ -6023,8 +6023,13 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr 
> >>> driver,
> >>>  if (virStorageFileGetMetadata(disk->src,
> >>>uid, gid,
> >>>cfg->allowDiskFormatProbing,
> >>> -  report_broken) < 0)
> >>> +  report_broken) < 0) {
> >>>  ret = -1;
> >>> +goto cleanup;
> >>> +}
> >>> +
> >>> +if (virDomainDiskGetFormat(disk) == VIR_STORAGE_FILE_ISO)
> >>> +virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_RAW);
> > 
> > This is not the right place and also not the correct way. We should
> > reject using ISO as a format if qemu will not support it as an invalid
> > configuration rather than silently turn it into raw.
> 
> But I want to fix the case when iso format is autodetected not specified
> exlicitly. If we still want to fail in the latter case I guess I can
> add check that iso comes from autodetection.

I don't really think that such case is worth it. Running with format
detection enabled is inherently dangerous. We strictly discourage
running with format detection enabled.

Also the code should still reject the explicit configuration.



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

Re: [libvirt] [PATCH] qemu: treat iso images as raw

[Nikolay Shirokovskiy]

On 26.10.2017 16:18, Peter Krempa wrote:
> On Thu, Oct 26, 2017 at 13:09:25 +0100, Daniel Berrange wrote:
>> On Thu, Oct 26, 2017 at 03:04:08PM +0300, Nikolay Shirokovskiy wrote:
>>> if image format probing is on and image format of iso file
>>> is not specified qemu fail to start a domain or change disk
>>> media giving errors like [1]. The problem is format is being
>>> detected as 'iso' and qemu expect format to be raw for iso
>>> images.
>>>
>>> It makes sense to me because iso refers to filesystem format
>>> in image not image format itself. Thus let's just convert
>>> iso to raw in case of qemu.
>>>
>>> There is a similar patch for storage pools - 0e5db762.
>>>
>>> [1] Unknown driver 'iso'
>>>
>>> ---
>>>
>>> ISO as image format was added right at the beginning by e266ded2f
>>> without any further comments. Maybe we just can drop ISO from image
>>> formats entirely as it is not image format or some hypervisors
>>> treat it in a special way?
>>
>> Yeah, I'm inclined to say we can drop it. I don't recall either Xen or
>> QEMU caring about an 'iso' disk format
> 
> The hypervisors probably don't care about this but the storage driver
> may care (At least for display purposes).
> 
>>>  src/qemu/qemu_domain.c | 7 ++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>>> index c7c9e94..3da9271 100644
>>> --- a/src/qemu/qemu_domain.c
>>> +++ b/src/qemu/qemu_domain.c
>>> @@ -6023,8 +6023,13 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
>>>  if (virStorageFileGetMetadata(disk->src,
>>>uid, gid,
>>>cfg->allowDiskFormatProbing,
>>> -  report_broken) < 0)
>>> +  report_broken) < 0) {
>>>  ret = -1;
>>> +goto cleanup;
>>> +}
>>> +
>>> +if (virDomainDiskGetFormat(disk) == VIR_STORAGE_FILE_ISO)
>>> +virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_RAW);
> 
> This is not the right place and also not the correct way. We should
> reject using ISO as a format if qemu will not support it as an invalid
> configuration rather than silently turn it into raw.

But I want to fix the case when iso format is autodetected not specified
exlicitly. If we still want to fail in the latter case I guess I can
add check that iso comes from autodetection.

> 
> The only acceptable place to turn ISO -> RAW is when it's comming from
> the storage driver via 
> 

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

[libvirt] [PATCH 2/2] virt-aa-helper-test: only fails go to stdout by default

[Christian Ehrhardt]

By Default (without -d) the tests will only print Failures.
So a log should follow general "no message is a good message" style.

But the testfw checks always emit the skip info to stdout. Instead
they should use the redirection that is controlled by -d.

This avoids mesages like the following to clutter the log:
  Skipping FW AAVMF32 test. Could not find /usr/share/AAVMF/AAVMF32_CODE.fd

Signed-off-by: Christian Ehrhardt 
---
 tests/virt-aa-helper-test | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
index e837668..d2a557e 100755
--- a/tests/virt-aa-helper-test
+++ b/tests/virt-aa-helper-test
@@ -155,7 +155,7 @@ testfw() {
 -e "s,,$fwpath,g" "$template_xml" > "$test_xml"
 testme "0" "$title" "-r -u $valid_uuid" "$test_xml"
 else
-echo "Skipping FW $title test. Could not find $fwpath"
+echo "Skipping FW $title test. Could not find $fwpath" >$output
 fi
 }
 
-- 
2.7.4

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

[libvirt] [PATCH 0/2] virt-aa-helper cleanups

[Christian Ehrhardt]

As follow up to [1] some smaller extensions and fixups to virt-aa-helper
and its tests.

[1]: https://www.redhat.com/archives/libvir-list/2017-October/msg01161.html

Christian Ehrhardt (2):
  virt-aa-helper: apparmor wildcards to forbidden chars
  virt-aa-helper-test: only fails go to stdout by default

 src/security/virt-aa-helper.c |  2 +-
 tests/virt-aa-helper-test | 22 +-
 2 files changed, 22 insertions(+), 2 deletions(-)

-- 
2.7.4

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

[libvirt] [PATCH 1/2] virt-aa-helper: apparmor wildcards to forbidden chars

[Christian Ehrhardt]

Some globbing chars in the domain name could be used to break out of
apparmor rules, so lets forbid these when in virt-aa-helper.

Also adding a test to ensure all those cases were detected as bad char.

Signed-off-by: Christian Ehrhardt 
---
 src/security/virt-aa-helper.c |  2 +-
 tests/virt-aa-helper-test | 20 
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index ef1bf01..4f1c195 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -449,7 +449,7 @@ valid_name(const char *name)
 {
 /* just try to filter out any dangerous characters in the name that can be
  * used to subvert the profile */
-const char *bad = "/[]*";
+const char *bad = "/[]{}?^,\"*";
 
 if (strlen(name) == 0)
 return -1;
diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
index 0599046..e837668 100755
--- a/tests/virt-aa-helper-test
+++ b/tests/virt-aa-helper-test
@@ -211,6 +211,26 @@ testme "1" "-c with no os.type" "-c -u $valid_uuid" 
"$test_xml"
 sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e 
"s,hvm,hvm_invalid,g" "$template_xml" > "$test_xml"
 testme "1" "-c with invalid hvm" "-c -u $valid_uuid" "$test_xml"
 
+sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e 
"s,virt-aa-helper-test,virt-aa-helper-test-/,g" "$template_xml" > "$test_xml"
+testme "1" "-c with invalid domain name char /" "-c -u $valid_uuid" "$test_xml"
+sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e 
"s,virt-aa-helper-test,virt-aa-helper-test-[,g" "$template_xml" > "$test_xml"
+testme "1" "-c with invalid domain name char [" "-c -u $valid_uuid" "$test_xml"
+sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e 
"s,virt-aa-helper-test,virt-aa-helper-test-],g" "$template_xml" > "$test_xml"
+testme "1" "-c with invalid domain name char ]" "-c -u $valid_uuid" "$test_xml"
+sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e 
"s,virt-aa-helper-test,virt-aa-helper-test-{,g" "$template_xml" > "$test_xml"
+testme "1" "-c with invalid domain name char {" "-c -u $valid_uuid" "$test_xml"
+sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e 
"s,virt-aa-helper-test,virt-aa-helper-test-},g" "$template_xml" > "$test_xml"
+testme "1" "-c with invalid domain name char }" "-c -u $valid_uuid" "$test_xml"
+sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e 
"s,virt-aa-helper-test,virt-aa-helper-test-?,g" "$template_xml" > "$test_xml"
+testme "1" "-c with invalid domain name char ?" "-c -u $valid_uuid" "$test_xml"
+sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e 
"s,virt-aa-helper-test,virt-aa-helper-test-^,g" "$template_xml" > "$test_xml"
+testme "1" "-c with invalid domain name char ^" "-c -u $valid_uuid" "$test_xml"
+sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e 
"s,virt-aa-helper-test,virt-aa-helper-test-\,,g" "$template_xml" > "$test_xml"
+testme "1" "-c with invalid domain name char ," "-c -u $valid_uuid" "$test_xml"
+sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e 
"s,virt-aa-helper-test,virt-aa-helper-test-\",g" "$template_xml" > "$test_xml"
+testme "1" "-c with invalid domain name char \"" "-c -u $valid_uuid" 
"$test_xml"
+sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e 
"s,virt-aa-helper-test,virt-aa-helper-test-*,g" "$template_xml" > "$test_xml"
+testme "1" "-c with invalid domain name char *" "-c -u $valid_uuid" "$test_xml"
 
 echo "Expected pass:" >$output
 sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" "$template_xml" > 
"$test_xml"
-- 
2.7.4

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

Re: [libvirt] [PATCH 3/4] virt-aa-helper: allow spaces in vm names

[Christian Ehrhardt]

On Wed, Oct 25, 2017 at 8:48 PM, Jamie Strandboge 
wrote:

> On Wed, 2017-09-20 at 16:59 +0200, Christian Ehrhardt wrote:
> > libvirt allows spaces in vm names, there were issues in the past but
> > it
> > seems not removed so the assumption has to be that spaces are
> > continuing
> > to be allowed.
> >
> > Therefore virt-aa-helper should not reject spaces in vm names anymore
> > if
> > it is goign to be refused causing issues then the parser or xml
> > schema
> > should do so.
> > Apparmor rules are in quotes, so a space in a path based on the name
> > works.
> >
> > Signed-off-by: Christian Ehrhardt 
> > ---
> >  src/security/virt-aa-helper.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-
> > helper.c
> > index d1518ea..5f4519d 100644
> > --- a/src/security/virt-aa-helper.c
> > +++ b/src/security/virt-aa-helper.c
> > @@ -449,7 +449,7 @@ valid_name(const char *name)
> >  {
> >  /* just try to filter out any dangerous characters in the name
> > that can be
> >   * used to subvert the profile */
> > -const char *bad = " /[]*";
> > +const char *bad = "/[]*";
> >
> >  if (strlen(name) == 0)
> >  return -1;
>
> Your justification seems reasonable. It does mean that we'll need
> always quote rules that use def->name and looking at virt-aa-helper.c,
> that seems to be the case.
>
> All that said, I was surprised that tests/virt-aa-helper-test didn't
> need to be updated, but, indeed, this is a testing gap.
>
> +1 as is, but perhaps in a follow-up patch you could expand bad to be:
>
>   const char *bad = "/[]{}?^,\"*";
>

Yep, makes sense.
Found a (minor) cleanup along the way - patches will follow shortly as new
thread (no need to splice it into this thread I think)


> '{', '}', '?', '^', ',' and '"' are characters used in AARE (see
> 'Globbing' in 'man apparmor.d') and add tests to tests/virt-aa-helper-
> test for this.
>
> Thanks!
>
> --
> Jamie Strandboge | http://www.canonical.com




-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: treat iso images as raw

[Peter Krempa]

On Thu, Oct 26, 2017 at 13:09:25 +0100, Daniel Berrange wrote:
> On Thu, Oct 26, 2017 at 03:04:08PM +0300, Nikolay Shirokovskiy wrote:
> > if image format probing is on and image format of iso file
> > is not specified qemu fail to start a domain or change disk
> > media giving errors like [1]. The problem is format is being
> > detected as 'iso' and qemu expect format to be raw for iso
> > images.
> > 
> > It makes sense to me because iso refers to filesystem format
> > in image not image format itself. Thus let's just convert
> > iso to raw in case of qemu.
> > 
> > There is a similar patch for storage pools - 0e5db762.
> > 
> > [1] Unknown driver 'iso'
> > 
> > ---
> > 
> > ISO as image format was added right at the beginning by e266ded2f
> > without any further comments. Maybe we just can drop ISO from image
> > formats entirely as it is not image format or some hypervisors
> > treat it in a special way?
> 
> Yeah, I'm inclined to say we can drop it. I don't recall either Xen or
> QEMU caring about an 'iso' disk format

The hypervisors probably don't care about this but the storage driver
may care (At least for display purposes).

> >  src/qemu/qemu_domain.c | 7 ++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index c7c9e94..3da9271 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -6023,8 +6023,13 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
> >  if (virStorageFileGetMetadata(disk->src,
> >uid, gid,
> >cfg->allowDiskFormatProbing,
> > -  report_broken) < 0)
> > +  report_broken) < 0) {
> >  ret = -1;
> > +goto cleanup;
> > +}
> > +
> > +if (virDomainDiskGetFormat(disk) == VIR_STORAGE_FILE_ISO)
> > +virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_RAW);

This is not the right place and also not the correct way. We should
reject using ISO as a format if qemu will not support it as an invalid
configuration rather than silently turn it into raw.

The only acceptable place to turn ISO -> RAW is when it's comming from
the storage driver via 


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

Re: [libvirt] [PATCH v3 2/2] virtlogd: add missing netserver refcount increment on reload

[John Ferlan]

On 10/25/2017 05:48 AM, John Ferlan wrote:
> 
> 
> On 10/25/2017 05:15 AM, Nikolay Shirokovskiy wrote:
>>
>>
>> On 25.10.2017 12:06, John Ferlan wrote:
>>>
>>>
>>> On 10/24/2017 06:47 AM, Nikolay Shirokovskiy wrote:
 After virNetDaemonAddServerPostExec call in virtlogd we should have
 netserver refcount set to 2. One goes to netdaemon servers hashtable
 and one goes to virtlogd own reference to netserver. Let's add
 missing increment in virNetDaemonAddServerPostExec itself while holding
 daemon lock.

 We also have to unref new extra ref after virtlockd call to 
 virNetDaemonAddServerPostExec.
 ---
  src/locking/lock_daemon.c | 1 +
  src/rpc/virnetdaemon.c| 1 +
  2 files changed, 2 insertions(+)

 diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
 index fe3eaf9..41a06b2 100644
 --- a/src/locking/lock_daemon.c
 +++ b/src/locking/lock_daemon.c
 @@ -275,6 +275,7 @@ virLockDaemonNewPostExecRestart(virJSONValuePtr 
 object, bool privileged)
virLockDaemonClientFree,

 (void*)(intptr_t)(privileged ? 0x1 : 0x0
  goto error;
 +virObjectUnref(srv);
>>>
>>> I need to think a bit more about this one... different model in lockd
>>> vs. logd over @srv usage especially w/r/t this particular path.
>>>
>>> I see that in this path eventually something calls virNetDaemonGetServer
>>> instead of storing the @srv in order to add lockProgram... In any case,> I 
>>> guess I'd be worried/concerned that something could remove @srv
>>> causing the Hash table code to unref/delete the srv... Also, in the
>>> cleanup: path of main() wouldn't the Unref(srv) cause problems?
>>
>> But this unref only balance newly added ref. If there are other
>> problems with reference conting in virtlockd - its a different
>> concern.
>>
> 
> Ok - so what I've learned is that virLockDaemonPostExecRestart returns
> rv == 1 which indicates a successful restore resulting in a call to
> virNetDaemonGetServer which will do a virObjectRef anyway leaving us
> with (theoretically) 2 refs prior to the code that adds programs to @srv
> (and less concern from my part of the subsequent Unref in cleanup: here).
> 
> If we saved srv in lockd like logd does, then we wouldn't need to call
> virNetDaemonGetServer resulting in the same eventual result except for a
> window after the Unref here where the refcnt == 1 until
> virNetDaemonGetServer is run. Since I believe nothing could happen in
> between that would cause the Unref due to HashFree being called, then I
> think we're OK.
> 
> The concern being is if virNetDaemonGetServer didn't find the server,
> then @srv would be NULL and the subsequent call to
> virNetServerAddProgram for lockProgram would fail miserably, but I don't
> think that can happen theoretically at least!
> 
> John
> 
> 
>>>
>>> Again- need to think a bit longer.

I've been thinking more about this more...

Looking through history - I see commit id f08b1c58 removed @srv from
_virLockDaemon, but _virLogDaemon wasn't adjusted likewise.

I also see commit id 252610f7 which modified the virnetdaemon code to
use hash tables.  What I'm missing is why that patch didn't add the
virObjectRef as well since the @srv is placed into the hash table to
match the similar operation done for the virNetDaemonAddServer.

It seems when first written perhaps 252610f7 came first with f08b1c58
afterwards, but when committed it was in reverse order.

There's just something not quite right and I'm not quite sure what the
exact right patch(es) should be.

"Theoretically speaking"...

If we get a REF for alloc, then we need an UNREF
If we get a REF for hash, then we need an UNREF
If we get a REF for virNetDaemonGetServer, then we need an UNREF

Without the REF in PostExec, then LogD will have a problem if there's a
usage of logd->srv after the UNREF for the hash table happens. In
particular, the UNREF in virLogDaemonFree. However, for LockD there's
not a problem because there's REF's for GetServer and no UNREF on the
allocation. For the "non"-PostExec path - there's a leak of @srv
(theoretical) and what seems to be normal processing for the PostExec path.

With the REF in PostExec, LogD would be fixed; however, LockD then has a
leak because of the extra REF.


Pushing patch1 seems to solve at least the problem w/ the late/delayed
client access (as shown by the sleep() comments), but I fear it opens
the door for a LogD problem because hash table ref gets decremented sooner.

It's all very tricky and timing based.

Perhaps someone else wants to have a look too and provide some thoughts?

John

>>>
>>> John
>>>
  
  return lockd;
  
 diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
 index 495bc4b..f3e3ffe 100644
 --- a/src/rpc/virnetdaemon.c
 +++ b/src/rpc/virnetdaemon.c
 @@ -312,6 +312,7 @@ 

Re: [libvirt] [PATCH/QEMU] s390x/kvm: use cpu_model_available for guarded storage on compat machines

[Cornelia Huck]

On Fri, 20 Oct 2017 16:54:37 +0200
Christian Borntraeger  wrote:

> Starting a guest with
>
> hvm
>   
>   
> 
> on an IBM z14 results in
> 
> "qemu-system-s390x: Some features requested in the CPU model are not
> available in the configuration: gs"
> 
> This is because guarded storage is fenced for compat machines that did not 
> have
> guarded storage support, but libvirt expands the cpu model according to the
> latest available machine.
> 
> While this prevents future migration abort (by not starting the guest at all),
> not being able to start a "host-model" guest is very much unexpected.  As it
> turns out, even if we would modify libvirt to not expand the cpu model to
> contain "gs" for compat machines, it cannot guarantee that a migration will
> succeed. For example if the kernel changes its features (or the user has
> nested=1 on one host but not on the other) the migration will fail
> nevertheless.  So instead of fencing "gs" for machines <= 2.9 lets allow it 
> for
> all machine types that support the CPU model. This will make "host-model"
> runnable all the time, while relying on the CPU model to reject invalid
> migration attempts.
> 
> Suggested-by: David Hildenbrand 
> Signed-off-by: Christian Borntraeger 
> ---
>  hw/s390x/s390-virtio-ccw.c | 8 
>  include/hw/s390x/s390-virtio-ccw.h | 3 ---
>  target/s390x/kvm.c | 2 +-
>  3 files changed, 1 insertion(+), 12 deletions(-)

Acked-by: Cornelia Huck 

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

Re: [libvirt] [[RFC] 0/8] Implement async QEMU event handling in libvirtd.

[Daniel P. Berrange]

On Tue, Oct 24, 2017 at 10:34:53AM -0700, Prerna Saxena wrote:
> 
> As noted in
> https://www.redhat.com/archives/libvir-list/2017-May/msg00016.html
> libvirt-QEMU driver handles all async events from the main loop.
> Each event handling needs the per-VM lock to make forward progress. In
> the case where an async event is received for the same VM which has an
> RPC running, the main loop is held up contending for the same lock.
> 
> This impacts scalability, and should be addressed on priority.
> 
> Note that libvirt does have a 2-step deferred handling for a few event
> categories, but (1) That is insufficient since blockign happens before
> the handler could disambiguate which one needs to be posted to this
> other queue.
> (2) There needs to be homogeniety.
> 
> The current series builds a framework for recording and handling VM
> events.
> It initializes per-VM event queue, and a global event queue pointing to
> events from all the VMs. Event handling is staggered in 2 stages:
> - When an event is received, it is enqueued in the per-VM queue as well
>   as the global queues.
> - The global queue is built into the QEMU Driver as a threadpool
>   (currently with a single thread).
> - Enqueuing of a new event triggers the global event worker thread, which
>   then attempts to take a lock for this event's VM.
> - If the lock is available, the event worker runs the function handling
>   this event type. Once done, it dequeues this event from the global
>   as well as per-VM queues.
> - If the lock is unavailable(ie taken by RPC thread), the event worker 
>   thread leaves this as-is and picks up the next event.
> - Once the RPC thread completes, it looks for events pertaining to the
>   VM in the per-VM event queue. It then processes the events serially
>   (holding the VM lock) until there are no more events remaining for
>   this VM. At this point, the per-VM lock is relinquished.

One of the nice aspects of processing the QEMU events in the main event
loop is that handling of them is self-throttling. ie if one QEMU process
goes mental and issues lots of events, we'll spend alot of time processing
them all, but our memory usage is still bounded.

If we take events from the VM and put them on a queue that is processed
asynchronously, and the event processing thread gets stuck for some
reason, then libvirt will end up queuing an unbounded number of events.
This could cause considerable memory usage in libvirt. This could be
exploited by a malicious VM to harm libvirt. eg a malicious QEMU could
stop responding to monitor RPC calls, which would tie up the RPC threads
and handling of RPC calls. This would in turn prevent events being
processed due to being unable to acquire the state lock.  Now the VM
can flood libvirtd with events which will all be read off the wire
and queued in memory, potentially forever. So libvirt memory usage
will balloon to an arbitrary level.

Now, the current code isn't great when faced with malicious QEMU
because with the same approach, QEMU can cause libvirtd main event
loop to stall as you found. This feels less bad than unbounded
memory usage though - if libvirt uses lots of memory, this will
push other apps on the host into swap, and/or trigger the OOM
killer.

So I agree that we need to make event processing asynchronous from
the main loop. When doing that through, I think we need to put an
upper limit on the number of events we're willing to queue from a
VM. When we get that limit, we should update the monitor event loop
watch so that we stop reading further events, until existing events
have been processed.

The other attractive thing bout the way events currently work is that
it is also automatically avoids lock acquisition priority problems
wrt incoming RPC calls. ie, we are guaranteed that once thread
workers have finished all currnetly queued RPC calls, we will be able
to acquire the VM lock to process the event. All further RPC calls
on the wire won't be read off the wire until events have been
processed.

With events being processed asychronously from RPC calls, there is
a risk of starvation where the event loop thread constantly looses
the race to acquire the VM lock vs other incoming RPC calls. I guess
this is one reason why you choose to process pending events at the
end of the RPC call processing.

> Known issues/ caveats:
> - RPC handling time will become non-deterministic.
> - An event will only be "notified" to a client once the RPC for same VM 
> completes.

Yeah, these two scenarios are not very nice IMHO. I'm also pretty wary
of having 2 completely different places in which events are processed.
ie some events are processed by the dedicated event thread, while other
events are processed immediately after an RPC call. When you have these
kind of distinct code paths it tends to lead to bugs because one code
path will be taken most of the time, and the other code path only
taken in unusal circumstances (and is thus rarely tested and liable
to 

Re: [libvirt] [PATCH] qemu: treat iso images as raw

[Daniel P. Berrange]

On Thu, Oct 26, 2017 at 03:04:08PM +0300, Nikolay Shirokovskiy wrote:
> if image format probing is on and image format of iso file
> is not specified qemu fail to start a domain or change disk
> media giving errors like [1]. The problem is format is being
> detected as 'iso' and qemu expect format to be raw for iso
> images.
> 
> It makes sense to me because iso refers to filesystem format
> in image not image format itself. Thus let's just convert
> iso to raw in case of qemu.
> 
> There is a similar patch for storage pools - 0e5db762.
> 
> [1] Unknown driver 'iso'
> 
> ---
> 
> ISO as image format was added right at the beginning by e266ded2f
> without any further comments. Maybe we just can drop ISO from image
> formats entirely as it is not image format or some hypervisors
> treat it in a special way?

Yeah, I'm inclined to say we can drop it. I don't recall either Xen or
QEMU caring about an 'iso' disk format

> 
>  src/qemu/qemu_domain.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index c7c9e94..3da9271 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -6023,8 +6023,13 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
>  if (virStorageFileGetMetadata(disk->src,
>uid, gid,
>cfg->allowDiskFormatProbing,
> -  report_broken) < 0)
> +  report_broken) < 0) {
>  ret = -1;
> +goto cleanup;
> +}
> +
> +if (virDomainDiskGetFormat(disk) == VIR_STORAGE_FILE_ISO)
> +virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_RAW);
>  
>   cleanup:
>  virObjectUnref(cfg);
> -- 

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: treat iso images as raw

[Nikolay Shirokovskiy]

if image format probing is on and image format of iso file
is not specified qemu fail to start a domain or change disk
media giving errors like [1]. The problem is format is being
detected as 'iso' and qemu expect format to be raw for iso
images.

It makes sense to me because iso refers to filesystem format
in image not image format itself. Thus let's just convert
iso to raw in case of qemu.

There is a similar patch for storage pools - 0e5db762.

[1] Unknown driver 'iso'

---

ISO as image format was added right at the beginning by e266ded2f
without any further comments. Maybe we just can drop ISO from image
formats entirely as it is not image format or some hypervisors
treat it in a special way?

 src/qemu/qemu_domain.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c7c9e94..3da9271 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6023,8 +6023,13 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
 if (virStorageFileGetMetadata(disk->src,
   uid, gid,
   cfg->allowDiskFormatProbing,
-  report_broken) < 0)
+  report_broken) < 0) {
 ret = -1;
+goto cleanup;
+}
+
+if (virDomainDiskGetFormat(disk) == VIR_STORAGE_FILE_ISO)
+virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_RAW);
 
  cleanup:
 virObjectUnref(cfg);
-- 
1.8.3.1

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

Re: [libvirt] [PATCH] virt-aa-helper: grant locking permission on -f

[Michal Privoznik]

On 10/24/2017 04:54 PM, Christian Ehrhardt wrote:
> Hot-adding disks does not parse the full XML to generate apparmor rules.
> Instead it uses -f  to append a generic rule for that file path.
> 
> 580cdaa7: "virt-aa-helper: locking disk files for qemu 2.10" implemented
> the qemu 2.10 requirement to allow locking on disks images that are part of
> the domain xml.
> 
> But on attach-device a user will still trigger an apparmor deny by going
> through virt-aa-helper -f, to fix that add the lock "k" permission to the
> append file case of virt-aa-helper.
> 
> Signed-off-by: Christian Ehrhardt 
> ---
>  src/security/virt-aa-helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

ACKed and pushed.

Michal

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

Re: [libvirt] [PATCH] virt-aa-helper: fix libusb access to udev usb descriptions

[Michal Privoznik]

On 10/25/2017 02:42 PM, Christian Ehrhardt wrote:
> In bf3a4140 "virt-aa-helper: fix libusb access to udev usb data" the
> libusb access to properly detect the device/bus ids was fixed.
> 
> The path /run/udev/data/+usb* contains a subset of that information we
> already allow to be read and are currently not needed for the function
> qemu needs libusb for. But on the init of libusb all those files are
> still read so a lot of apparmor denials can be seen when using usb host
> devices, like:
>   apparmor="DENIED" operation="open" name="/run/udev/data/+usb:2-1.2:1.0"
>   comm="qemu-system-x86" requested_mask="r" denied_mask="r"
> 
> Today we could silence the warnings with a deny rule without breaking
> current use cases. But since the data in there is only a subset of those
> it can read already it is no additional information exposure. And on the
> other hand a future udev/libusb/qemu combination might need it so allow
> the access in the default apparmor profile.
> 
> Signed-off-by: Christian Ehrhardt 
> ---
>  examples/apparmor/libvirt-qemu | 1 +
>  1 file changed, 1 insertion(+)
> 

ACKed and pushed.

Michal

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

Re: [libvirt] [Qemu-devel] [PATCH/QEMU] s390x/kvm: use cpu_model_available for guarded storage on compat machines

[Halil Pasic]

On 10/26/2017 10:13 AM, Christian Borntraeger wrote:
> 
> 
> On 10/26/2017 01:35 AM, Halil Pasic wrote:
>  try the most interesting scenarios out.
>>
>> The idea of the patch is very clear, but I don't understand the bigger gs
>> feature context fully.
>>
>> From what I read in the code, the attempt to enable the gs capability in
>> the kernel is made regardless of the cpu model. If the attempt was
>> successful kvm_s390_get_gs will keep returning true. That would in turn
>> affect migration, as far as I see (usages of kvm_s390_get_gs). I could
>> not figure out how does gs being turned off via cpu-model (-cpu
>> z14,gs=off) does turn of gs support -- at least not the details. I wanted
>> to give a timely review, so I've limited myself there. 
> 
> When the CPU model turns off gs, facility bit 133 will be turned off.
> Then the kernel does the right thing, see priv.c handle_gs.
> 
> guarded storage is enabled lazily. Whenever the guest issues a Gs instruction
> we will get an exit and only enable GS if facility 133 is set.
> 
>>
>> From what I see, this patch does what it advertises, and since I think
>> it's the right thing to do in the current situation I gonna give it an:
>> Acked-by: Halil Pasic 
>>
>> At the same time, I would prefer the commit message being reworded. IMHO
>> this patch is a good stop-gap measure, but essentially it trades an
>> annoying and obvious bug for a subtle and hopefully painless one.
>>
>> Let me explain this last statement. For starters, I  do share some of the
>> concerns Boris has voiced.  I won't repeat those. Same goes for the
>> example Christian paraphrased previously, and the the fear of an implicit
>> requirement for having to support a Cartesian product of the advertised
>> machine-types and cpu-models (for each qemu binary).
> 
> I will try to come up with a patch description that explains the open issues
> and it will tell that additional might or might not be necessary depending
> on followup discussions.

I would be already happy with adding something like:
During the discussion enabling cpu-model features for preexisting
machine-types came out as at least controversial. We agreed to implement
this patch as a stop-gap measure for the next release. What is a clean
enough solution shall be decided without time pressure.

> I will schedule this patch for 2.11 then.
> 

Sounds like a plan.

Cheers,
Halil

> 

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

Re: [libvirt] Libvirt xl to xml converter only picks up first occurrence of an option

[Wei Liu]

On Wed, Oct 25, 2017 at 02:53:01PM -0600, Jim Fehlig wrote:
> On 10/20/2017 08:46 AM, Wei Liu wrote:
> > Hi Jim
> 
> Hi Wei,
> 
> Sorry for the delay. Catching up on mail after some days off...
> 
> > I discovered that libvirt's native config file to xml converter for
> > libxl only pick up the first occurrence of an option.
> > 
> > For example in a xl cfg file:
> > 
> > extra = "abc"
> > ...
> > extra = "def"
> > 
> > xl will pick up "def" because that shows up later and takes precedence,
> > while the converter picks up "abc".
> > 
> > I think this is a bug in the converter and should be fixed.
> 
> Thanks for the report. I took a quick peek at libvirt's generic config
> parser and afaict it only searches for the first occurrence of a setting.
> The parser does support flags though, so we could add something like
> VIR_CONF_FLAG_{FIRST,LAST}_DUPLICATE. (Better name suggestions welcomed.)
> 

I'm normally very bad at naming things so I will refrain from making
suggestions.

> I've opened a bug report against openSUSE Factory to track this
> 
> https://bugzilla.opensuse.org/show_bug.cgi?id=1065118
> 

Thanks!

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

Re: [libvirt] [PATCH] conf: Avoid leaking blockers from virDomainCapsCPUModel

[Pavel Hrdina]

On Thu, Oct 26, 2017 at 12:22:59PM +0200, Jiri Denemark wrote:
> When adding CPU usability blockers I forgot to properly free them when
> in virDomainCapsCPUModelsDispose.
> 
> Reported-by: Marc Hartmayer 
> Signed-off-by: Jiri Denemark 
> ---
>  src/conf/domain_capabilities.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Pavel Hrdina 


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

Re: [libvirt] [PATCH] spec: Enable unit tests during build

[Michal Privoznik]

On 10/20/2017 01:51 PM, mka...@redhat.com wrote:
> From: Marek Kasik 
> 
> Enable unit tests so that we can catch some problems soon enough
> before the package gets to the users.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1502639
> ---
>  libvirt-glib.spec.in | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/libvirt-glib.spec.in b/libvirt-glib.spec.in
> index a1ca11f..7191862 100644
> --- a/libvirt-glib.spec.in
> +++ b/libvirt-glib.spec.in
> @@ -116,6 +116,9 @@ rm -f $RPM_BUILD_ROOT%{_libdir}/libvirt-gobject-1.0.la
>  
>  %find_lang %{name}
>  
> +%check
> +%__make %{?_smp_mflags} check

Should we fail the build if 'make check' fails? E.g.

%check
if ! make %{?_smp_mflags} check; then
  cat tests/test-suite.log
  exit 1
fi

Also, please use libvirt-glib for $SUBJ prefix.

Michal

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

Re: [libvirt] [PATCH 0/2] Further Increase default file handle limits

[Michal Privoznik]

On 10/18/2017 11:19 AM, Christian Ehrhardt wrote:
> In 27cd7635 new default limits were set:
>   Author: Daniel P. Berrange 
>   Date:   Wed Mar 15 16:51:51 2017 +
> 
> Increase default file handle limits for daemons
> 
> But I faced some constraints with these values and think it is time to
> reconsider the defaults to only have to be tweaked in really uncommon cases.
> 
> Christian Ehrhardt (2):
>   Increase default file handle limits for virtlogd
>   Increase default file handle limits for virtlockd
> 
>  src/locking/virtlockd.service.in | 4 ++--
>  src/logging/virtlogd.service.in  | 6 --
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 

ACKed and pushed.

Michal

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

Re: [libvirt] [PATCH v2 02/22] conf: Add usability blockers to virDomainCapsCPUModel

[Jiri Denemark]

On Wed, Oct 25, 2017 at 10:56:51 +0200, Marc Hartmayer wrote:
> On Fri, Oct 13, 2017 at 08:14 PM +0200, Jiri Denemark  
> wrote:
> > When a hypervisor marks a CPU model as unusable on the current host, it
> > may also give us a list of features which prevent the model from being
> > usable. Storing this list in virDomainCapsCPUModel will help the CPU
> > driver with creating a host-model CPU configuration.
> >
> > Signed-off-by: Jiri Denemark 
> > Reviewed-by: John Ferlan 
...
> > diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h
> > index 82183c4524..8c71dec21e 100644
> > --- a/src/conf/domain_capabilities.h
> > +++ b/src/conf/domain_capabilities.h
> > @@ -116,6 +116,7 @@ typedef virDomainCapsCPUModel *virDomainCapsCPUModelPtr;
> >  struct _virDomainCapsCPUModel {
> >  char *name;
> >  virDomainCapsCPUUsable usable;
> > +char **blockers; /* NULL-terminated list of usability blockers */
> >  };
> 
> I know this is an "old" thread and already pushed. But I think you have
> to free the blockers list in virDomainCapsCPUModelsDispose as well. No?

Oops, you're right. I've just sent a patch to fix this. Thanks.

Jirka

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

[libvirt] [PATCH] conf: Avoid leaking blockers from virDomainCapsCPUModel

[Jiri Denemark]

When adding CPU usability blockers I forgot to properly free them when
in virDomainCapsCPUModelsDispose.

Reported-by: Marc Hartmayer 
Signed-off-by: Jiri Denemark 
---
 src/conf/domain_capabilities.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index 729d905e2..7f96ff386 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -94,8 +94,10 @@ virDomainCapsCPUModelsDispose(void *obj)
 virDomainCapsCPUModelsPtr cpuModels = obj;
 size_t i;
 
-for (i = 0; i < cpuModels->nmodels; i++)
+for (i = 0; i < cpuModels->nmodels; i++) {
 VIR_FREE(cpuModels->models[i].name);
+virStringListFree(cpuModels->models[i].blockers);
+}
 
 VIR_FREE(cpuModels->models);
 }
-- 
2.14.3

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

[libvirt] [PATCH] AppArmor: add rules needed with additional mediation features brought by Linux 4.14.

[intrigeri+libvirt]

From: intrigeri 

---
 examples/apparmor/libvirt-qemu  | 2 ++
 examples/apparmor/usr.sbin.libvirtd | 6 ++
 2 files changed, 8 insertions(+)

diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
index b341e31f42..5994a35042 100644
--- a/examples/apparmor/libvirt-qemu
+++ b/examples/apparmor/libvirt-qemu
@@ -16,6 +16,8 @@
   network inet stream,
   network inet6 stream,
 
+  signal (receive) set=("term") peer=/usr/sbin/libvirtd,
+
   /dev/net/tun rw,
   /dev/kvm rw,
   /dev/ptmx rw,
diff --git a/examples/apparmor/usr.sbin.libvirtd 
b/examples/apparmor/usr.sbin.libvirtd
index 819068ffc3..eb24726e08 100644
--- a/examples/apparmor/usr.sbin.libvirtd
+++ b/examples/apparmor/usr.sbin.libvirtd
@@ -30,10 +30,13 @@
   # Needed for vfio
   capability sys_resource,
 
+  mount,
+
   network inet stream,
   network inet dgram,
   network inet6 stream,
   network inet6 dgram,
+  network netlink raw,
   network packet dgram,
   network packet raw,
 
@@ -42,6 +45,9 @@
   ptrace (trace) peer=/usr/sbin/dnsmasq,
   ptrace (trace) peer=libvirt-*,
 
+  signal (send) set=("hup") peer=/usr/sbin/dnsmasq,
+  signal (send) set=("term") peer=libvirt-*,
+
   # Very lenient profile for libvirtd since we want to first focus on confining
   # the guests. Guests will have a very restricted profile.
   / r,
-- 
2.15.0.rc2

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

[libvirt] [PATCH v2] AppArmor: add rules needed with additional mediation features brought by Linux 4.14.

[intrigeri+libvirt]

[PATCH v2] AppArmor: add rules needed with additional mediation features

Changes since v1:

 - remove unneeded "network unix" rules added by v1: they were only
   needed due to a bug in apparmor_parser, that was fixed in AppArmor
   2.11.1 since then;
 - move the "network netlink raw" rule to honor previous sorting.

Note that the "mount" rule is very broad. It could be replaced with
a set of more specific rules in the future. A draft is available on
https://bugzilla.opensuse.org/show_bug.cgi?id=1065123, that should be
tested on various distros and configurations before it is submitted
upstream. But let's not block on this and focus first on avoiding
breakage when distros ship Linux 4.14: this is not a regression given
so far we had no mount mediation at all (except in Ubuntu that carries
out-of-tree patches).

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

[libvirt] [PATCH] docs: Drop mention that WWN for disks must be unique

[Peter Krempa]

For multipath disks it might be useful to have the same WWN for multiple
disks. It's the users choice to do so. Since we dropped the check that
disallows using duplicate WWNs drop the docs as well.

https://bugzilla.redhat.com/show_bug.cgi?id=1464975
---

Pushed as trivial.

 docs/formatdomain.html.in | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 4609e2ec2..4f28dce35 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -3184,8 +3184,7 @@
   wwn
   If present, this element specifies the WWN (World Wide Name)
 of a virtual hard disk or CD-ROM drive. It must be composed
-of 16 hexadecimal digits and must be unique (at least among
-disks of a single domain)
+of 16 hexadecimal digits.
 Since 0.10.1
   
   vendor
-- 
2.13.6

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

Re: [libvirt] [Qemu-devel] [PATCH/QEMU] s390x/kvm: use cpu_model_available for guarded storage on compat machines

[Christian Borntraeger]

On 10/26/2017 01:35 AM, Halil Pasic wrote:
 try the most interesting scenarios out.
> 
> The idea of the patch is very clear, but I don't understand the bigger gs
> feature context fully.
> 
> From what I read in the code, the attempt to enable the gs capability in
> the kernel is made regardless of the cpu model. If the attempt was
> successful kvm_s390_get_gs will keep returning true. That would in turn
> affect migration, as far as I see (usages of kvm_s390_get_gs). I could
> not figure out how does gs being turned off via cpu-model (-cpu
> z14,gs=off) does turn of gs support -- at least not the details. I wanted
> to give a timely review, so I've limited myself there. 

When the CPU model turns off gs, facility bit 133 will be turned off.
Then the kernel does the right thing, see priv.c handle_gs.

guarded storage is enabled lazily. Whenever the guest issues a Gs instruction
we will get an exit and only enable GS if facility 133 is set.

> 
> From what I see, this patch does what it advertises, and since I think
> it's the right thing to do in the current situation I gonna give it an:
> Acked-by: Halil Pasic 
> 
> At the same time, I would prefer the commit message being reworded. IMHO
> this patch is a good stop-gap measure, but essentially it trades an
> annoying and obvious bug for a subtle and hopefully painless one.
> 
> Let me explain this last statement. For starters, I  do share some of the
> concerns Boris has voiced.  I won't repeat those. Same goes for the
> example Christian paraphrased previously, and the the fear of an implicit
> requirement for having to support a Cartesian product of the advertised
> machine-types and cpu-models (for each qemu binary).

I will try to come up with a patch description that explains the open issues
and it will tell that additional might or might not be necessary depending
on followup discussions.
I will schedule this patch for 2.11 then.

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

Re: [libvirt] [[RFC] 0/8] Implement async QEMU event handling in libvirtd.

[Peter Krempa]

On Thu, Oct 26, 2017 at 10:21:17 +0530, Prerna wrote:
> On Wed, Oct 25, 2017 at 4:12 PM, Jiri Denemark  wrote:
> > On Tue, Oct 24, 2017 at 10:34:53 -0700, Prerna Saxena wrote:

[...]

> > > Patch Series status:
> > > Strictly RFC only. No compilation issues. I have not had a chance to
> > > (stress) test it after rebase to latest master.
> > > Note that documentation and test coverage is TBD, since a few open
> > > points remain.
> > >
> > > Known issues/ caveats:
> > > - RPC handling time will become non-deterministic.
> > > - An event will only be "notified" to a client once the RPC for same VM
> > completes.
> > > - Needs careful consideration in all cases where a QMP event is used to
> > >   "signal" an RPC thread, else will deadlock.
> >
> > This last issue is actually a show stopper here. We need to make sure
> > QMP events are processed while a job is still active on the same domain.
> > Otherwise thinks kile block jobs and migration, which are long running
> > jobs driven by events, will break.
> >
> > Jirka
> >
> Completely agree, which is why I have explicitly mentioned this.
> However, I do not completely follow why it needs to be this way. Can the
> block job APIs between QEMU <--> libvirt be fixed so that such behaviour is
> avoided ?

Not really. Events from qemu are a big improvement from the times when
we were polling for state of jobs.

Additionally migration with storage in libvirt uses blockjobs which
are asynchronous in qemu but libvirt needs to wait for them
synchronously. Since blockjobs in qemu need to be asynchronous (they
take a long time and libvirt needs to be able to use the monitor
meanwhile) requires us to do handling of incomming events while a
libvirt API is active.


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

Re: [libvirt] [PATCH/QEMU] s390x/kvm: use cpu_model_available for guarded storage on compat machines

[David Hildenbrand]

On 25.10.2017 18:45, Marc Hartmayer wrote:
> On Wed, Oct 25, 2017 at 05:50 PM +0200, David Hildenbrand  
> wrote:
>> On 25.10.2017 17:09, Boris Fiuczynski wrote:
>>> On 10/25/2017 12:23 PM, David Hildenbrand wrote:
 On 25.10.2017 12:18, Christian Borntraeger wrote:
> Ping, I plan to submit belows patch for 2.11. We can then still look into
> a libvirt<->qemu interface for limiting host-model depending on machine 
> versions
> (or not).

 I think this would be sufficient for now.

 Having different host models, depending on the the machine type sounds
 wrong. But maybe we'll need it in the future.

>>>
>>> David, I disagree if your proposal is to generally tolerate new cpu
>>> features in old machine types. This *might* work for gs but how do you
>>> guaranty that guests do not behave differently/wrong when suddenly new
>>> cpu features are made available to guest when (re-)starting them.
>>> That is my feedback for the qemu side of this mater.
>>
>> Just re-reading this section, so what you mean is:
>>
>> a) guest is started, host model is copied and used. guest works.
>> b) guest is stopped.
>> c) QEMU/KVM/HW is updated.
>> d) guest is started, new host model is copied. guest no longer works.
>>
>> d) could be because there are now some additional features with e.g.
>> broken guest implementation or now missing features.
>>
>>
>> What you propose (if I am not wrong) is a to bind features somehow to a
>> QEMU machine. I think that should never be done. You could not catch now
>> missing features.
> 
> What exactly do you mean by the last sentence?

In general, up/downgrading QEMU/KVM/HW can lead to the removal of features.

Another example is the "nested" flag for KVM. toggling that can lead to
the host feature looking differently (+/- SIE features).

So if you really want to make sure that a VM XML that once ran perfectly
fine on a host will still run after any QEMU/KVM/HW changes on that host:

a) specify an explicit CPU model, not the host model (e.g. "z13")
b) copy the host model to the XML persistently.

Linking any of that to the machine types is in my opinion the very wrong
approach.

> 
>>
>> What would you think about a persistent host-model copy option? So
>> instead of copying at every start, only copy and replace it once in the XML?
>>
>> Easy to specify by the user and no CPU feature changes will happend
>> "blindly".
>>
>>
>> --
>>
>> Thanks,
>>
>> David
>>
> --
> Beste Grüße / Kind regards
>Marc Hartmayer
> 
> IBM Deutschland Research & Development GmbH
> Vorsitzende des Aufsichtsrats: Martina Koederitz
> Geschäftsführung: Dirk Wittkopp
> Sitz der Gesellschaft: Böblingen
> Registergericht: Amtsgericht Stuttgart, HRB 243294
> 


-- 

Thanks,

David

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

[libvirt] Plans for next release

[Daniel Veillard]

  Hi all,

we are getting close to the end of the month, and I think we should
enter freeze over the week-end. Then I can try to push an RC2 on Tuesday
and make the GA release on the 2nd Nov (I'm travelling ATM and Nov 1st
is closed, not sure I could do this that day).

  Hope this works for everyone,

thanks,

Daniel

-- 
Daniel Veillard  | Red Hat Developers Tools http://developer.redhat.com/
veill...@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

Re: [libvirt] [PATCH v2 3/3] vbox: Read runtime RDP port and handle autoport

[John Ferlan]

On 10/25/2017 05:54 PM, Dawid Zamirski wrote:
> On Wed, 2017-10-25 at 17:35 -0400, John Ferlan wrote:
>>
>> On 10/24/2017 05:09 PM, Dawid Zamirski wrote:
>>> VirutalBox has a IVRDEServerInfo structure available that
>>> gives the effective runtime port that the VM is using when it's
>>> running. This is useful when the "TCP/Ports" VBox property was set
>>> to
>>> port range (e.g. via autoport = "yes" or via VBoxManage) in which
>>> case it would be impossible to get the "active" port otherwise.
>>> ---
>>>  src/vbox/vbox_common.c|   3 +-
>>>  src/vbox/vbox_tmpl.c  | 134
>>> +++---
>>>  src/vbox/vbox_uniformed_api.h |   2 +-
>>>  3 files changed, 104 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
>>> index 92ee37164..d542f2b76 100644
>>> --- a/src/vbox/vbox_common.c
>>> +++ b/src/vbox/vbox_common.c
>>> @@ -3326,7 +3326,8 @@ vboxDumpDisplay(virDomainDefPtr def,
>>> vboxDriverPtr data, IMachine *machine)
>>>  if (VIR_ALLOC(graphics) < 0)
>>>  goto cleanup;
>>>  
>>> -gVBoxAPI.UIVRDEServer.GetPorts(data, VRDEServer,
>>> graphics);
>>> +gVBoxAPI.UIVRDEServer.GetPorts(data, VRDEServer, machine,
>>> graphics);
>>> +gVBoxAPI.UISession.Close(data->vboxSession);
>>
>> But @data is used shortly after this and I don't see in the calling
>> tree
>> a corresponding UISession.Open* of some type or am I missing it in
>> some
>> called function?
>>
>>
>> The rest looks good - just need to know about this.  I can remove
>> before
>> pushing or make some other sort of simple adjustment.
> 
> Yep this should be removed - it's a left over from my old internal
> patch [1], that I forgot to remove when preparing for upstream
> submission. It was originally preceded with OpenExisting (aka
> LockMachine) to get the IConsole - the new patch does it internally in
> the vboxGetActiveVRDEServerPort function.
> 
> https://github.com/datto/libvirt/commit/a3cb830bfce10b1a614c18a6ac50783
> 45433d900#diff-747d3af65e7ac81a564b7cb4fcd01eb6R3516
> 
> Thank you,
> Dawid
> 
Reviewed-by: John Ferlan 

John

(pushed now too)

>>
>> John
>>
>> (I'm at KVM Forum in Prague - so normal work schedule is a bit off)
>>
>>>  
>>>
>>>

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

Re: [libvirt] [PATCH] qemu: logrotate: drop minsize directive

[Daniel P. Berrange]

On Wed, Oct 25, 2017 at 03:30:46PM -0600, Jim Fehlig wrote:
> On a cloud host it is possible to create 100's of unique instances
> per day, each leaving behind a /var/log/libvirt/qemu/instance-name.log
> file that is < 100k. With the current 'minsize 100k' directive, these
> files are never rotated and hence never removed. Over months of time,
> tens of thousands of these files can accumulate on the host.
> 
> Dropping 'minsize 100k' allows rotating small files, which will
> increase the number of log files, but 'rotate 4' ensures they will
> be removed after a month.
> 
> Signed-off-by: Jim Fehlig 
> ---
>  daemon/libvirtd.qemu.logrotate.in | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/daemon/libvirtd.qemu.logrotate.in 
> b/daemon/libvirtd.qemu.logrotate.in
> index 15cf019b2..cdb399ef2 100644
> --- a/daemon/libvirtd.qemu.logrotate.in
> +++ b/daemon/libvirtd.qemu.logrotate.in
> @@ -5,5 +5,4 @@
>  compress
>  delaycompress
>  copytruncate
> -minsize 100k
>  }

Reviewed-by: Daniel P. Berrange 

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