Re: [libvirt] [PATCH] security:selinux: Fix crash when tcon is NULL

[lhuang]

On 11/10/2014 03:57 PM, Martin Kletzander wrote:

On Sat, Nov 08, 2014 at 06:17:26PM +0800, Luyao Huang wrote:

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

Libvirtd will crash when parameter tcon = NULL in 
virSecuritySELinuxSetFileconHelper
function, because libvirt do not check the first parameter when use 
strcmp().
Add a check for tcon before use strcmp() and output a error in log 
when tcon is NULL.


Signed-off-by: Luyao Huang lhu...@redhat.com
---
src/security/security_selinux.c | 5 +
1 file changed, 5 insertions(+)



NACK, calling virSecuritySELinuxSetFileconHelper() with NULL doesn't
make sense at all.  If you are trying to just fix a possible future
errot, then the check for non-NULL parameters needs to be done when
the function is starting.  But I doubt it's useful.  It's another
story if you managed to cause such call to this function.

Thanks your reply and It's a real issue :) what i do :
# /usr/libexec/qemu-kvm -cdrom /tmp/test.iso  -monitor 
unix:/tmp/demo,server,nowait  -name foo -uuid 
cece4f9f-dff0-575d-0e8e-01fe380f12ea  

[1] 16636

# VNC server running on `::1:5900'

# virsh qemu-attach 16636
Domain foo attached to pid 16636

# virsh screenshot foo
error: could not take a screenshot of foo
error: Cannot recv data: Connection reset by peer
error: Failed to reconnect to the hypervisor


Maybe root cause is : vm doesn't have a image label
  seclabel type='static' model='selinux' relabel='yes'
labelsystem_u:system_r:virtd_t:s0-s0:c0.c1023/label
  /seclabel




diff --git a/src/security/security_selinux.c 
b/src/security/security_selinux.c

index f96be50..4fd09b8 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -887,6 +887,11 @@ virSecuritySELinuxSetFileconHelper(const char 
*path, char *tcon, bool optional)

int setfilecon_errno = errno;

if (getfilecon_raw(path, econ) = 0) {
+if (tcon == NULL) {
+virReportSystemError(errno,%s,
+ _(Invalid security context : NULL));
+return -1;
+}


Explaining how did you reach this point with tcon == NULL, or if you
even managed to do that, would help others understanding the issue
from higher level.  Anyway, the problem here is that this function is
called with tcon == NULL and not that it doesn't check for that (if
you managed to see the segfault).  To explain what I mean; if you
managed to see the segfault when you performed some operation, that's
bad.  But when this patch is applied, you still won't be able to
perform that operation successfully and we need to find out the root
cause of that.

The backtrace would fit nicely in the commit message as well.

Martin


Backtrace is here:

(gdb) bt
#0  0x7ff38a0c9e76 in __strcmp_sse42 () from /lib64/libc.so.6
#1  0x7ff38d08d6a9 in virSecuritySELinuxSetFileconHelper 
(path=0x7ff36c2626c0 /var/cache/libvirt/qemu/qemu.screendump.xRL8fw, 
tcon=0x0, optional=optimized out)

at security/security_selinux.c:890
#2  0x7ff38d089c63 in virSecurityManagerSetSavedStateLabel 
(mgr=0x7ff36c0f6f40, vm=vm@entry=0x7ff3680011c0,
savefile=savefile@entry=0x7ff36c2626c0 
/var/cache/libvirt/qemu/qemu.screendump.xRL8fw) at 
security/security_manager.c:547
#3  0x7ff38d086bc6 in virSecurityStackSetSavedStateLabel 
(mgr=optimized out, vm=0x7ff3680011c0, savefile=0x7ff36c2626c0 
/var/cache/libvirt/qemu/qemu.screendump.xRL8fw)

at security/security_stack.c:351
#4  0x7ff38d089c63 in virSecurityManagerSetSavedStateLabel 
(mgr=0x7ff36c106fc0, vm=0x7ff3680011c0, savefile=0x7ff36c2626c0 
/var/cache/libvirt/qemu/qemu.screendump.xRL8fw)

at security/security_manager.c:547
#5  0x7ff375e7d8b4 in qemuDomainScreenshot (dom=0x7ff36c000910, 
st=0x7ff36c262010, screen=optimized out, flags=optimized out) at 
qemu/qemu_driver.c:3854
#6  0x7ff38cfa5d60 in virDomainScreenshot 
(domain=domain@entry=0x7ff36c000910, stream=stream@entry=0x7ff36c262010, 
screen=0, flags=0) at libvirt.c:3171
#7  0x7ff38da489d3 in remoteDispatchDomainScreenshot 
(server=optimized out, ret=0x7ff36c000a20, args=0x7ff36c23ef80, 
rerr=0x7ff37d6cdc80, msg=optimized out,

client=0x7ff38fb404e0) at remote_dispatch.h:7473
#8  remoteDispatchDomainScreenshotHelper (server=optimized out, 
client=0x7ff38fb404e0, msg=optimized out, rerr=0x7ff37d6cdc80, 
args=0x7ff36c23ef80, ret=0x7ff36c000a20)

at remote_dispatch.h:7440
#9  0x7ff38d019752 in virNetServerProgramDispatchCall 
(msg=0x7ff38fb40470, client=0x7ff38fb404e0, server=0x7ff38fb3f460, 
prog=0x7ff38fb4aea0) at rpc/virnetserverprogram.c:437
#10 virNetServerProgramDispatch (prog=0x7ff38fb4aea0, 
server=server@entry=0x7ff38fb3f460, client=0x7ff38fb404e0, 
msg=0x7ff38fb40470) at rpc/virnetserverprogram.c:307
#11 0x7ff38da6820d in virNetServerProcessMsg (msg=optimized out, 
prog=optimized out, client=optimized out, srv=0x7ff38fb3f460) at 
rpc/virnetserver.c:172
#12 virNetServerHandleJob (jobOpaque=optimized out, 

Re: [libvirt] [PATCH v2] phyp: Fix NULL dereference in phypConnectOpen

[Pavel Hrdina]

On 11/10/2014 07:41 AM, Martin Kletzander wrote:

Coverity found out that commit cd490086 caused a possible NULL pointer
dereference.  This is due to the fact, that phyp_driver is NULL at the
time of closing the socket, instead of connection_data, which kept the
socket before the mentioned commit, could not be NULL.

However, internal_socket is still the local socket that can be
closed, even unconditionally, if we initialize it to -1.

Signed-off-by: Martin Kletzander mklet...@redhat.com
---
  src/phyp/phyp_driver.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
index 7c8bc5c..386d25f 100644
--- a/src/phyp/phyp_driver.c
+++ b/src/phyp/phyp_driver.c
@@ -1128,7 +1128,7 @@ phypConnectOpen(virConnectPtr conn,
  virConnectAuthPtr auth, unsigned int flags)
  {
  LIBSSH2_SESSION *session = NULL;
-int internal_socket;
+int internal_socket = -1;
  uuid_tablePtr uuid_table = NULL;
  phyp_driverPtr phyp_driver = NULL;
  char *char_ptr;
@@ -1232,7 +1232,7 @@ phypConnectOpen(virConnectPtr conn,
  libssh2_session_free(session);
  }

-VIR_FORCE_CLOSE(phyp_driver-sock);
+VIR_FORCE_CLOSE(internal_socket);

  return VIR_DRV_OPEN_ERROR;
  }



ACK,

Pavel

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

Re: [libvirt] [PATCH v2] phyp: Fix NULL dereference in phypConnectOpen

[John Ferlan]

On 11/10/2014 01:41 AM, Martin Kletzander wrote:
 Coverity found out that commit cd490086 caused a possible NULL pointer
 dereference.  This is due to the fact, that phyp_driver is NULL at the
 time of closing the socket, instead of connection_data, which kept the
 socket before the mentioned commit, could not be NULL.
 
 However, internal_socket is still the local socket that can be
 closed, even unconditionally, if we initialize it to -1.
 

This works too...

ACK,

John

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

[libvirt] [PATCH v3 0/2] Numa : Allow specification of memory units

[Prerna Saxena]

Present XML specification of NUMA node accepts memory in 'KiB' only.
This series adds support for input of cell memory in units of choice.

Description:
===
Patch 1/2 : Export virDomainParseMemory for use outside domain_conf
Patch 2/2 : Allow specification of 'units' for NUMA nodes.

Reference:
==
v1: http://www.spinics.net/linux/fedora/libvir/msg105516.html
v2: http://www.mail-archive.com/libvir-list@redhat.com/msg104577.html

Changes Since v2:
===
* Patch 1 is newly introduced. It follows Michal's suggestion of reusing 
virDomainParseMemory, exposed by 01b4de2b9f5ca82
* Patch 2 : Now uses virDomainParseMemory()
* Also, documentation in patch 2 is now fixed to link to memory unit 
specification

-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India

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

[libvirt] [PATCH v3 1/2] Extend virDomainParseMemory for use outside domain_conf

[Prerna Saxena]

From 4978c8c2df19bdf738695d6cc64864f11071a08e Mon Sep 17 00:00:00 2001
From: Prerna Saxena pre...@linux.vnet.ibm.com
Date: Mon, 10 Nov 2014 14:48:03 +0530


Commit 01b4de2b9f5ca82 abstracts virDomainParseMemory()
for use by other functions in domain_conf.c
Extend the same for use, for functions outside of this file.

Signed-off-by: Prerna Saxena pre...@linux.vnet.ibm.com
---
 src/conf/domain_conf.c | 2 +-
 src/conf/domain_conf.h | 8 
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e8d8f7d..5909655 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6421,7 +6421,7 @@ virDomainParseScaledValue(const char *xpath,
  *
  * Return 0 on success, -1 on failure after issuing error.
  */
-static int
+int
 virDomainParseMemory(const char *xpath,
  const char *units_xpath,
  xmlXPathContextPtr ctxt,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index fbb3b2f..9fb05c8 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2847,6 +2847,14 @@ int virDomainObjSetMetadata(virDomainObjPtr vm,
 const char *configDir,
 unsigned int flags);
 
+int
+virDomainParseMemory(const char *xpath,
+ const char *units_xpath,
+ xmlXPathContextPtr ctxt,
+ unsigned long long *mem,
+ bool required,
+ bool capped);
+
 bool virDomainDefNeedsPlacementAdvice(virDomainDefPtr def)
 ATTRIBUTE_NONNULL(1);
 
-- 
1.9.3

-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India

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

[libvirt] [PATCH v3 2/2] Numa : Allow specification of 'units' for numa nodes.

[Prerna Saxena]

From 1ead736712eec3bd098daf222a872c67b67e94ce Mon Sep 17 00:00:00 2001
From: Prerna Saxena pre...@linux.vnet.ibm.com
Date: Mon, 3 Nov 2014 07:53:59 +0530


CPU numa topology implicitly allows memory specification in 'KiB'.

Enabling this to accept the 'unit' in which memory needs to be specified.
This now allows users to specify memory in units of choice, and
lists the same in 'KiB' -- just like other 'memory' elements in XML.

numa
  cell cpus='0-3' memory='1024' unit='MiB' /
  cell cpus='4-7' memory='1024' unit='MiB' /
/numa

Also augment test cases to correctly model NUMA memory specification.
This adds the tag 'unit=KiB' for memory attribute in NUMA cells.

Signed-off-by: Prerna Saxena pre...@linux.vnet.ibm.com
---
 docs/formatdomain.html.in  |  8 +++-
 docs/schemas/domaincommon.rng  |  5 +++
 src/conf/cpu_conf.c| 44 ++
 .../qemuxml2argv-cpu-numa-disjoint.xml |  4 +-
 .../qemuxml2argv-cpu-numa-memshared.xml|  4 +-
 tests/qemuxml2argvdata/qemuxml2argv-cpu-numa1.xml  |  4 +-
 tests/qemuxml2argvdata/qemuxml2argv-cpu-numa2.xml  |  4 +-
 tests/qemuxml2argvdata/qemuxml2argv-cpu-numa3.xml  |  4 +-
 .../qemuxml2argv-hugepages-pages.xml   |  8 ++--
 .../qemuxml2argv-hugepages-pages2.xml  |  4 +-
 .../qemuxml2argv-hugepages-pages3.xml  |  4 +-
 .../qemuxml2argv-hugepages-pages4.xml  |  8 ++--
 .../qemuxml2argv-hugepages-shared.xml  |  8 ++--
 .../qemuxml2argv-numatune-auto-prefer.xml  |  2 +-
 .../qemuxml2argv-numatune-memnode-no-memory.xml|  4 +-
 .../qemuxml2argv-numatune-memnode.xml  |  6 +--
 .../qemuxml2argv-numatune-memnodes-problematic.xml |  4 +-
 .../qemuxml2xmlout-cpu-numa1.xml   |  4 +-
 .../qemuxml2xmlout-cpu-numa2.xml   |  4 +-
 .../qemuxml2xmlout-numatune-auto-prefer.xml|  2 +-
 .../qemuxml2xmlout-numatune-memnode.xml|  6 +--
 21 files changed, 81 insertions(+), 60 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 7196e75..f103a13 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1153,8 +1153,8 @@
   lt;cpugt;
 ...
 lt;numagt;
-  lt;cell id='0' cpus='0-3' memory='512000'/gt;
-  lt;cell id='1' cpus='4-7' memory='512000' memAccess='shared'/gt;
+  lt;cell id='0' cpus='0-3' memory='512000' unit='KiB'/gt;
+  lt;cell id='1' cpus='4-7' memory='512000' unit='KiB' 
memAccess='shared'/gt;
 lt;/numagt;
 ...
   lt;/cpugt;
@@ -1165,6 +1165,10 @@
   codecpus/code specifies the CPU or range of CPUs that are
   part of the node. codememory/code specifies the node memory
   in kibibytes (i.e. blocks of 1024 bytes).
+  span class=sinceSince 1.2.11/span one can specify an additional
+  codeunit/code attribute to describe the node memory unit.
+  The detailed syntax for allocation of memory units follows:
+  a href=#elementsMemoryAllocationcodeunit/code/a
   span class=sinceSince 1.2.7/span all cells should
   have codeid/code attribute in case referring to some cell is
   necessary in the code, otherwise the cells are
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 20d81ae..44cabad 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4144,6 +4144,11 @@
 ref name=memoryKB/
   /attribute
   optional
+attribute name=unit
+  ref name=unit/
+/attribute
+  /optional
+  optional
 attribute name=memAccess
   choice
 valueshared/value
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
index 1c74c66..d0323b0 100644
--- a/src/conf/cpu_conf.c
+++ b/src/conf/cpu_conf.c
@@ -177,6 +177,30 @@ virCPUDefCopy(const virCPUDef *cpu)
 return NULL;
 }
 
+static int
+virCPUNumaCellMemoryParseXML(xmlNodePtr node,
+  xmlXPathContextPtr ctxt,
+  unsigned long long* cellMemory)
+{
+int ret = -1;
+xmlNodePtr oldnode = ctxt-node;
+
+ctxt-node = node;
+
+if (virDomainParseMemory(./@memory, ./@unit, ctxt,
+ cellMemory, true, false)  0) {
+virReportError(VIR_ERR_XML_DETAIL, %s,
+   _(Unable to parse NUMA memory size attribute));
+goto cleanup;
+}
+
+ret = 0;
+ cleanup:
+ctxt-node = oldnode;
+return ret;
+
+}
+
 virCPUDefPtr
 virCPUDefParseXML(xmlNodePtr node,
   xmlXPathContextPtr ctxt,
@@ -440,7 +464,7 @@ virCPUDefParseXML(xmlNodePtr node,
 def-ncells = n;
 
 for (i = 0; i  n; i++) {
-char *cpus, *memory, *memAccessStr;
+char *cpus, *memAccessStr;
 int ret, ncpus = 0;
 unsigned int cur_cell;
 char *tmp = NULL;
@@ -489,21 +513,8 @@ virCPUDefParseXML(xmlNodePtr node,
 goto 

Re: [libvirt] [PATCH v6 0/7] qemu: Introduce support for new the block_set_io_throttle parameters add in the version 1.7 of qemu.

[Matthias Gatto]

On Fri, Nov 7, 2014 at 4:56 PM, Michal Privoznik mpriv...@redhat.com wrote:
 On 29.10.2014 13:15, Matthias Gatto wrote:

 This series of patches add support for bps_max, bps_rd_max, bps_wr_max,
 bps_max, bps_rd_max, bps_wr_max, and iops_size in the functions
 qemuDomainSetBlockIoTune
 and qemuDomainGetBlockIoTune.
 The last patch add support for these parameters to the virsh blkdeviotune
 command.

 v2: -Spellfix

 v3: -Merge patch 1/9,2/9,5/9 together.
  -Change the capability detection.(patch 2/7 and 3/7).
  -Try to make the usage of QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX more
 explicit(patch 3/7).

 v4: -Rebase on HEAD.
  -Update qemu_driver to comply with Pavel's patchs.(patch 3/6)
  -Remove the qemu_monitor_text modification.(remove old patch 5/7)

 v5: -Split patch 1/6 in two.
  -Add documentation for the new xml options (patch 2/7)
  -Change (void) to ATTRIBUTE_UNUSED (patch 4/7)
  -Capability detection of supportMaxOptions move before usage of
 supportMaxOptions (patch 4/7)

 v6: -Spellfix
  -Add comment (patch 4/7, 5/7)
  -Undo the modification of the supportMaxOptions made
  in the v5 because it was creating bugs(patch 4/5)

 The 2 first patches have been reviewed by Eric Blake and sould be merge
 soon
 The 3rd patch have been reviewed by Michal Privoznik and ack

 Matthias Gatto (7):
qemu: Add define for the new throttle options
qemu: Modify the structure _virDomainBlockIoTuneInfo.
qemu: Add Qemu capability for bps_max and friends
qemu: Add bps_max and friends qemu driver
qemu: Add bps_max and friends QMP suport
qemu: Add bps_max and friends to qemu command generation
virsh: Add bps_max and friends to virsh

   docs/formatdomain.html.in|  25 
   docs/schemas/domaincommon.rng|  43 ++
   include/libvirt/libvirt-domain.h | 110 
   src/conf/domain_conf.c   | 109 +++-
   src/conf/domain_conf.h   |   7 +
   src/qemu/qemu_capabilities.c |   2 +
   src/qemu/qemu_capabilities.h |   1 +
   src/qemu/qemu_command.c  |  57 +++-
   src/qemu/qemu_driver.c   | 187
 ++-
   src/qemu/qemu_monitor.c  |  10 +-
   src/qemu/qemu_monitor.h  |   6 +-
   src/qemu/qemu_monitor_json.c |  66 --
   src/qemu/qemu_monitor_json.h |   6 +-
   tests/qemucapabilitiesdata/caps_2.1.1-1.caps |   1 +
   tests/qemumonitorjsontest.c  |   6 +-
   tools/virsh-domain.c | 119 +
   tools/virsh.pod  |  10 ++
   17 files changed, 732 insertions(+), 33 deletions(-)



 So, I've reviewed the patches. They seem okay except for a few minor things
 I've found. I don't want to force you to send another version, so can you
 send just a follow-up patch? Well, 1/7 and 6/7 is easily fixable so I'll do
 that myself. However, 4/7 requires a bit more of work. So I'm okay with you
 sending follow-up patch just for that one.

 Partial ACK for now.

 Michal

Thank for the review.
I send the patch as soon as I fix it.

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

[libvirt] [PATCH v7 4/7] qemu: Add bps_max and friends qemu driver

[Matthias Gatto]

Add support for bps_max and friends in the driver part.
In the part checking if a qemu is running, check if the running binary
support bps_max, if not print an error message, if yes add it to
info variable

This patch follow therse patchs: 
http://www.redhat.com/archives/libvir-list/2014-November/msg00271.html
I've fix the syntax error and the nparams detection problem

Signed-off-by: Matthias Gatto matthias.ga...@outscale.com
---
 include/libvirt/libvirt-domain.h |  56 +++
 src/qemu/qemu_driver.c   | 197 ---
 src/qemu/qemu_monitor.c  |  10 +-
 src/qemu/qemu_monitor.h  |   6 +-
 src/qemu/qemu_monitor_json.c |  11 ++-
 src/qemu/qemu_monitor_json.h |   6 +-
 tests/qemumonitorjsontest.c  |   4 +-
 7 files changed, 264 insertions(+), 26 deletions(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 966a9ae..1fac2a3 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -3251,6 +3251,62 @@ typedef void 
(*virConnectDomainEventDeviceRemovedCallback)(virConnectPtr conn,
 # define VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC blkdeviotune.write_iops_sec
 
 /**
+ * VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX:
+ *
+ * Marco represents the total throughput limit in maximum bytes per second,
+ * as VIR_TYPED_PARAM_ULLONG.
+ */
+# define VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX 
blkdeviotune.total_bytes_sec_max
+
+/**
+ * VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX:
+ *
+ * Marco represents the read throughput limit in maximum bytes per second,
+ * as VIR_TYPED_PARAM_ULLONG.
+ */
+# define VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX 
blkdeviotune.read_bytes_sec_max
+
+/**
+ * VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX:
+ *
+ * Macro represents the write throughput limit in maximum bytes per second,
+ * as VIR_TYPED_PARAM_ULLONG.
+ */
+# define VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX 
blkdeviotune.write_bytes_sec_max
+
+/**
+ * VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX:
+ *
+ * Macro represents the total maximum I/O operations per second,
+ * as VIR_TYPED_PARAM_ULLONG.
+ */
+# define VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX 
blkdeviotune.total_iops_sec_max
+
+/**
+ * VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX:
+ *
+ * Macro represents the read maximum I/O operations per second,
+ * as VIR_TYPED_PARAM_ULLONG.
+ */
+# define VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX 
blkdeviotune.read_iops_sec_max
+
+/**
+ * VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX:
+ *
+ * Macro represents the write maximum I/O operations per second,
+ * as VIR_TYPED_PARAM_ULLONG.
+ */
+# define VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX 
blkdeviotune.write_iops_sec_max
+
+/**
+ * VIR_DOMAIN_TUNABLE_BLKDEV_SIZE_IOPS_SEC:
+ *
+ * Macro represents the size maximum I/O operations per second,
+ * as VIR_TYPED_PARAM_ULLONG.
+ */
+# define VIR_DOMAIN_TUNABLE_BLKDEV_SIZE_IOPS_SEC blkdeviotune.size_iops_sec
+
+/**
  * virConnectDomainEventTunableCallback:
  * @conn: connection object
  * @dom: domain on which the event occurred
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5eccacc..242b30e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -105,6 +105,7 @@ VIR_LOG_INIT(qemu.qemu_driver);
 #define QEMU_NB_MEM_PARAM  3
 
 #define QEMU_NB_BLOCK_IO_TUNE_PARAM  6
+#define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX  13
 
 #define QEMU_NB_NUMA_PARAM 2
 
@@ -16520,6 +16521,10 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
 int conf_idx = -1;
 bool set_bytes = false;
 bool set_iops = false;
+bool set_bytes_max = false;
+bool set_iops_max = false;
+bool set_size_iops = false;
+bool supportMaxOptions = true;
 virQEMUDriverConfigPtr cfg = NULL;
 virCapsPtr caps = NULL;
 virObjectEventPtr event = NULL;
@@ -16542,6 +16547,20 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
VIR_TYPED_PARAM_ULLONG,
VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC,
VIR_TYPED_PARAM_ULLONG,
+   VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX,
+   VIR_TYPED_PARAM_ULLONG,
+   VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX,
+   VIR_TYPED_PARAM_ULLONG,
+   VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX,
+   VIR_TYPED_PARAM_ULLONG,
+   VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX,
+   VIR_TYPED_PARAM_ULLONG,
+   VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX,
+   VIR_TYPED_PARAM_ULLONG,
+   VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX,
+   VIR_TYPED_PARAM_ULLONG,
+   VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC,
+   

[libvirt] [PATCH 2/3] lxc: don't setup cpuset.mems if memory mode in numatune is not 'strict'

[Wang Rui]

If the memory mode in numatune is not 'strict', we should not setup
cpuset.mems. Before commit 1a7be8c600905aa07ac2d78293336ba8523ad48e
we have checked the memory mode in virDomainNumatuneGetNodeset. This
patch adds the check as before.

Signed-off-by: Wang Rui moon.wang...@huawei.com
---
 src/lxc/lxc_cgroup.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
index f9af31c..eb67191 100644
--- a/src/lxc/lxc_cgroup.c
+++ b/src/lxc/lxc_cgroup.c
@@ -79,6 +79,10 @@ static int virLXCCgroupSetupCpusetTune(virDomainDefPtr def,
 goto cleanup;
 }
 
+if (virDomainNumatuneGetMode(def-numatune, -1) !=
+VIR_DOMAIN_NUMATUNE_MEM_STRICT)
+goto cleanup;
+
 if (virDomainNumatuneMaybeFormatNodeset(def-numatune, nodemask,
 mask, -1)  0)
 goto cleanup;
-- 
1.7.12.4

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

[libvirt] [PATCH 3/3] qemu: fix domain startup failing with 'strict' mode in numatune

[Wang Rui]

If the memory mode is specified as 'strict' and with one node, we
get the following error when starting domain.

error: Unable to write to '$cgroup_path/cpuset.mems': Device or resource busy

XML is configured with numatune as follows:
  numatune
memory mode='strict' nodeset='0'/
  /numatune

It's broken by Commit 411cea638f6ec8503b7142a31e58b1cd85dbeaba
which moved qemuSetupCgroupForEmulator() before setting cpuset.mems
in qemuSetupCgroupPostInit.

Directory '$cgroup_path/emulator/' is created in qemuSetupCgroupForEmulator.
But '$cgroup_path/emulator/cpuset.mems' it not set and has a default value
(all nodes, such as 0-1). Then we setup '$cgroup_path/cpuset.mems' to the
nodemask (in this case it's '0') in qemuSetupCgroupPostInit. It must fail.

This patch makes '$cgroup_path/emulator/cpuset.mems' is set before
'$cgroup_path/cpuset.mems'. The action is similar with that in
qemuDomainSetNumaParamsLive.

Signed-off-by: Wang Rui moon.wang...@huawei.com
---
 src/qemu/qemu_cgroup.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index a87ef40..3b1d16d 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -611,6 +611,7 @@ static int
 qemuSetupCpusetMems(virDomainObjPtr vm,
 virBitmapPtr nodemask)
 {
+virCgroupPtr cgroup_temp = NULL;
 qemuDomainObjPrivatePtr priv = vm-privateData;
 char *mem_mask = NULL;
 int ret = -1;
@@ -627,13 +628,16 @@ qemuSetupCpusetMems(virDomainObjPtr vm,
 mem_mask, -1)  0)
 goto cleanup;
 
-if (mem_mask 
-virCgroupSetCpusetMems(priv-cgroup, mem_mask)  0)
-goto cleanup;
+if (mem_mask)
+if (virCgroupNewEmulator(priv-cgroup, false, cgroup_temp)  0 ||
+virCgroupSetCpusetMems(cgroup_temp, mem_mask)  0 ||
+virCgroupSetCpusetMems(priv-cgroup, mem_mask)  0)
+goto cleanup;
 
 ret = 0;
  cleanup:
 VIR_FREE(mem_mask);
+virCgroupFree(cgroup_temp);
 return ret;
 }
 
-- 
1.7.12.4

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

[libvirt] [PATCH 1/3] qemu: don't setup cpuset.mems if memory mode in numatune is not 'strict'

[Wang Rui]

If the memory mode in numatune is specified as 'preferred' with one node
(such as nodeset='0'), domain's memory is not all in node 0 absolutely.
Assumption that node 0 doesn't have enough memory, memory can be allocated
on node 1 when qemu process startup. Then if we set cpuset.mems to '0',
it may invoke OOM.

Commit 1a7be8c600905aa07ac2d78293336ba8523ad48e changed the former logic of
checking memory mode in virDomainNumatuneGetNodeset. This patch adds the
check as before.

Signed-off-by: Wang Rui moon.wang...@huawei.com
---
 src/qemu/qemu_cgroup.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index b5bdb36..a87ef40 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -618,6 +618,10 @@ qemuSetupCpusetMems(virDomainObjPtr vm,
 if (!virCgroupHasController(priv-cgroup, VIR_CGROUP_CONTROLLER_CPUSET))
 return 0;
 
+if (virDomainNumatuneGetMode(vm-def-numatune, -1) !=
+VIR_DOMAIN_NUMATUNE_MEM_STRICT)
+return 0;
+
 if (virDomainNumatuneMaybeFormatNodeset(vm-def-numatune,
 nodemask,
 mem_mask, -1)  0)
-- 
1.7.12.4

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

[libvirt] [PATCH V2 0/3] Fix some problems of numatune

[Wang Rui]

Fix startup failing with memory mode(strict, preferred or interleave)
in numatune

V1:
https://www.redhat.com/archives/libvir-list/2014-November/msg00057.html

V2 has been revised as Martin' suggestion.

Wang Rui (3):
  qemu: don't setup cpuset.mems if memory mode in numatune is not
'strict'
  lxc: don't setup cpuset.mems if memory mode in numatune is not
'strict'
  qemu: fix domain startup failing with 'strict' mode in numatune

 src/lxc/lxc_cgroup.c   |  4 
 src/qemu/qemu_cgroup.c | 14 +++---
 2 files changed, 15 insertions(+), 3 deletions(-)

-- 
1.7.12.4

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

Re: [libvirt] [PATCH v3 2/2] Numa : Allow specification of 'units' for numa nodes.

[Michal Privoznik]

On 10.11.2014 12:52, Prerna Saxena wrote:
 
  From 1ead736712eec3bd098daf222a872c67b67e94ce Mon Sep 17 00:00:00 2001
 From: Prerna Saxena pre...@linux.vnet.ibm.com
 Date: Mon, 3 Nov 2014 07:53:59 +0530
 
 
 CPU numa topology implicitly allows memory specification in 'KiB'.
 
 Enabling this to accept the 'unit' in which memory needs to be specified.
 This now allows users to specify memory in units of choice, and
 lists the same in 'KiB' -- just like other 'memory' elements in XML.
 
  numa
cell cpus='0-3' memory='1024' unit='MiB' /
cell cpus='4-7' memory='1024' unit='MiB' /
  /numa
 
 Also augment test cases to correctly model NUMA memory specification.
 This adds the tag 'unit=KiB' for memory attribute in NUMA cells.
 
 Signed-off-by: Prerna Saxena pre...@linux.vnet.ibm.com
 ---
   docs/formatdomain.html.in  |  8 +++-
   docs/schemas/domaincommon.rng  |  5 +++
   src/conf/cpu_conf.c| 44 
 ++
   .../qemuxml2argv-cpu-numa-disjoint.xml |  4 +-
   .../qemuxml2argv-cpu-numa-memshared.xml|  4 +-
   tests/qemuxml2argvdata/qemuxml2argv-cpu-numa1.xml  |  4 +-
   tests/qemuxml2argvdata/qemuxml2argv-cpu-numa2.xml  |  4 +-
   tests/qemuxml2argvdata/qemuxml2argv-cpu-numa3.xml  |  4 +-
   .../qemuxml2argv-hugepages-pages.xml   |  8 ++--
   .../qemuxml2argv-hugepages-pages2.xml  |  4 +-
   .../qemuxml2argv-hugepages-pages3.xml  |  4 +-
   .../qemuxml2argv-hugepages-pages4.xml  |  8 ++--
   .../qemuxml2argv-hugepages-shared.xml  |  8 ++--
   .../qemuxml2argv-numatune-auto-prefer.xml  |  2 +-
   .../qemuxml2argv-numatune-memnode-no-memory.xml|  4 +-
   .../qemuxml2argv-numatune-memnode.xml  |  6 +--
   .../qemuxml2argv-numatune-memnodes-problematic.xml |  4 +-
   .../qemuxml2xmlout-cpu-numa1.xml   |  4 +-
   .../qemuxml2xmlout-cpu-numa2.xml   |  4 +-
   .../qemuxml2xmlout-numatune-auto-prefer.xml|  2 +-
   .../qemuxml2xmlout-numatune-memnode.xml|  6 +--
   21 files changed, 81 insertions(+), 60 deletions(-)
 
 diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
 index 7196e75..f103a13 100644
 --- a/docs/formatdomain.html.in
 +++ b/docs/formatdomain.html.in
 @@ -1153,8 +1153,8 @@
 lt;cpugt;
   ...
   lt;numagt;
 -  lt;cell id='0' cpus='0-3' memory='512000'/gt;
 -  lt;cell id='1' cpus='4-7' memory='512000' memAccess='shared'/gt;
 +  lt;cell id='0' cpus='0-3' memory='512000' unit='KiB'/gt;
 +  lt;cell id='1' cpus='4-7' memory='512000' unit='KiB' 
 memAccess='shared'/gt;
   lt;/numagt;
   ...
 lt;/cpugt;
 @@ -1165,6 +1165,10 @@
 codecpus/code specifies the CPU or range of CPUs that are
 part of the node. codememory/code specifies the node memory
 in kibibytes (i.e. blocks of 1024 bytes).
 +  span class=sinceSince 1.2.11/span one can specify an additional
 +  codeunit/code attribute to describe the node memory unit.
 +  The detailed syntax for allocation of memory units follows:
 +  a href=#elementsMemoryAllocationcodeunit/code/a
 span class=sinceSince 1.2.7/span all cells should
 have codeid/code attribute in case referring to some cell is
 necessary in the code, otherwise the cells are
 diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
 index 20d81ae..44cabad 100644
 --- a/docs/schemas/domaincommon.rng
 +++ b/docs/schemas/domaincommon.rng
 @@ -4144,6 +4144,11 @@
   ref name=memoryKB/
 /attribute
 optional
 +attribute name=unit
 +  ref name=unit/
 +/attribute
 +  /optional
 +  optional
   attribute name=memAccess
 choice
   valueshared/value
 diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
 index 1c74c66..d0323b0 100644
 --- a/src/conf/cpu_conf.c
 +++ b/src/conf/cpu_conf.c
 @@ -177,6 +177,30 @@ virCPUDefCopy(const virCPUDef *cpu)
   return NULL;
   }
   
 +static int
 +virCPUNumaCellMemoryParseXML(xmlNodePtr node,
 +  xmlXPathContextPtr ctxt,
 +  unsigned long long* cellMemory)
 +{
 +int ret = -1;
 +xmlNodePtr oldnode = ctxt-node;
 +
 +ctxt-node = node;
 +
 +if (virDomainParseMemory(./@memory, ./@unit, ctxt,
 + cellMemory, true, false)  0) {
 +virReportError(VIR_ERR_XML_DETAIL, %s,
 +   _(Unable to parse NUMA memory size attribute));

There's no need for virReportError() here. The helper reported error already.

 +goto cleanup;
 +}
 +
 +ret = 0;
 + cleanup:
 +ctxt-node = oldnode;
 +return ret;
 +
 +}
 +

Huh, I don't think it's necessary to have this as a function, but compiler will 
surely optimize it. So I can live with this as-is.

   virCPUDefPtr
   virCPUDefParseXML(xmlNodePtr 

Re: [libvirt] [PATCH] nodeinfo: report error when failure in nodeSetMemoryParameters

[Pavel Hrdina]

On 11/07/2014 11:27 AM, Jincheng Miao wrote:

nodeSetMemoryParameters() will call nodeSetMemoryParameterValue()
to set parameters. But it just filter the return code '-2' as
failure. Indeed we should report error when rc is negative.

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

Signed-off-by: Jincheng Miao jm...@redhat.com
---
  src/nodeinfo.c |3 +--
  1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 2e2fffa..3c22ebc 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -1374,8 +1374,7 @@ nodeSetMemoryParameters(virTypedParameterPtr params 
ATTRIBUTE_UNUSED,
  for (i = 0; i  nparams; i++) {
  rc = nodeSetMemoryParameterValue(params[i]);

-/* Out of memory */
-if (rc == -2)
+if (rc  0)
  return -1;
  }




ACK and pushed

Pavel

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

Re: [libvirt] [PATCH v3 1/2] Extend virDomainParseMemory for use outside domain_conf

[Michal Privoznik]

On 10.11.2014 12:52, Prerna Saxena wrote:


 From 4978c8c2df19bdf738695d6cc64864f11071a08e Mon Sep 17 00:00:00 2001
From: Prerna Saxena pre...@linux.vnet.ibm.com
Date: Mon, 10 Nov 2014 14:48:03 +0530


Commit 01b4de2b9f5ca82 abstracts virDomainParseMemory()
for use by other functions in domain_conf.c
Extend the same for use, for functions outside of this file.

Signed-off-by: Prerna Saxena pre...@linux.vnet.ibm.com
---
  src/conf/domain_conf.c | 2 +-
  src/conf/domain_conf.h | 8 
  2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e8d8f7d..5909655 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6421,7 +6421,7 @@ virDomainParseScaledValue(const char *xpath,
   *
   * Return 0 on success, -1 on failure after issuing error.
   */
-static int
+int
  virDomainParseMemory(const char *xpath,
   const char *units_xpath,
   xmlXPathContextPtr ctxt,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index fbb3b2f..9fb05c8 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2847,6 +2847,14 @@ int virDomainObjSetMetadata(virDomainObjPtr vm,
  const char *configDir,
  unsigned int flags);

+int
+virDomainParseMemory(const char *xpath,
+ const char *units_xpath,
+ xmlXPathContextPtr ctxt,
+ unsigned long long *mem,
+ bool required,
+ bool capped);
+
  bool virDomainDefNeedsPlacementAdvice(virDomainDefPtr def)
  ATTRIBUTE_NONNULL(1);




Any symbol that is to be exported as an internal API must be in 
src/libvirt_private.syms. So ACK with this squashed in:


diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 7e38cc6..b8f35e8 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -364,6 +364,7 @@ virDomainObjSetDefTransient;
 virDomainObjSetMetadata;
 virDomainObjSetState;
 virDomainObjTaint;
+virDomainParseMemory;
 virDomainPausedReasonTypeFromString;
 virDomainPausedReasonTypeToString;
 virDomainPMSuspendedReasonTypeFromString;

Michal

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

Re: [libvirt] [PATCH v3 0/2] Numa : Allow specification of memory units

[Michal Privoznik]

On 10.11.2014 12:49, Prerna Saxena wrote:

Present XML specification of NUMA node accepts memory in 'KiB' only.
This series adds support for input of cell memory in units of choice.

Description:
===
Patch 1/2 : Export virDomainParseMemory for use outside domain_conf
Patch 2/2 : Allow specification of 'units' for NUMA nodes.

Reference:
==
v1: http://www.spinics.net/linux/fedora/libvir/msg105516.html
v2: http://www.mail-archive.com/libvir-list@redhat.com/msg104577.html

Changes Since v2:
===
* Patch 1 is newly introduced. It follows Michal's suggestion of reusing 
virDomainParseMemory, exposed by 01b4de2b9f5ca82
* Patch 2 : Now uses virDomainParseMemory()
* Also, documentation in patch 2 is now fixed to link to memory unit 
specification



I've fixed the issues and pushed this.

Michal

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

Re: [libvirt] [PATCHv2 1/2] Use UPDATE_CPU when saving domain status

[Pavel Hrdina]

On 11/07/2014 10:55 AM, Ján Tomko wrote:

We only format cpu model for MODE_CUSTOM in domain status XML,
but we always format features if they are present.

This is a problem if we have a domain using MODE_HOST_PASSTHROUGH
that has been managedsaved, then restored, since it now has
a feature list but no model in /var/run/libvirt/qemu.



Isn't the real issue that on virsh start after managedsave there are
features defined for the cpu? According to documentation we don't allow
features if domain is using MODE_HOST_PASSTHROUGH, but we allow to start
it from managedsave state with those features which I think is a core
issue.


Use UPDATE_CPU even for the status XML to prevent libvirt
from losing track of the domain.

https://bugzilla.redhat.com/show_bug.cgi?id=1030793
https://bugzilla.redhat.com/show_bug.cgi?id=1151885
---
  src/conf/domain_conf.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 21309b0..acfb04b 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -19625,6 +19625,7 @@ virDomainSaveStatus(virDomainXMLOptionPtr xmlopt,
  virDomainObjPtr obj)
  {
  unsigned int flags = (VIR_DOMAIN_XML_SECURE |
+  VIR_DOMAIN_XML_UPDATE_CPU |
VIR_DOMAIN_XML_INTERNAL_STATUS |
VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET |
VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES |



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

Re: [libvirt] [PATCHv2 0/2] Fix snapshot-revert and managedsave with host-passthrough CPU mode

[Pavel Hrdina]

On 11/07/2014 10:55 AM, Ján Tomko wrote:

Ján Tomko (2):
   Use UPDATE_CPU when saving domain status
   Ignore missing CPU model for HOST_PASSTHROUGH

  src/conf/cpu_conf.c|  4 +-
  src/conf/domain_conf.c |  1 +
  ...argv-cpu-host-passthrough-features-invalid.args | 22 +
  ...2argv-cpu-host-passthrough-features-invalid.xml | 55 ++
  tests/qemuxml2argvtest.c   |  1 +
  5 files changed, 81 insertions(+), 2 deletions(-)
  create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-cpu-host-passthrough-features-invalid.args
  create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-cpu-host-passthrough-features-invalid.xml



NACK, features should not be in the state XML after the domain is
successfully started from managedsave or restored from snapshot.

Pavel

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

[libvirt] Problem with virInterfaceCreate(), IFF_UP, and NetworkManager

[Laine Stump]

Due to a checkered past (a myriad of minor issues, changing over
time), libvirt's semi-official position on the virInterface*() APIs and
NetworkManager is that virInterface*() is only supported if NM is
disabled. We do still attempt to make it work as well as possible, but
normally I only test those APIs on systems that have NM disabled and use
the network service (RHEL/Fedora/CentOS systems here) instead.

On a seemingly unrelated note, a few months ago mprivozn pushed a patch
that makes it an error to call virInterfaceCreate() (i.e. ifup) for an
interface that is already active. (the active state of an interface is
determined by looking at an interface's IFF_UP flag (and also
IFF_RUNNING, if the interface isn't a bridge device). Previously, this
was allowed, as it is common practive to ifup an interface to make new
config take effect.

Last week, I happened to test the virsh iface-bridge command on a
system with NM enabled. That command gave an error about the interface
being already active, so I tried again, this time ifdowning the
interface in advance - I *still* got the error. Further investigation
and questioning of NM developers led me to the realization that when NM
is enabled, all interfaces *always* have IFF_UP and IFF_RUNNING set,
even if they are ifdowned. Further, if NM is active there is no way to
determine an interface's active status via iotctl() or netlink;
instead, must query to determine if NM is active, and if it is you must
call a NM API instead (I got this much information from NM developers
directly; haven't investigated yet exactly what the API is).

NM developers say that this pinning-up of the IFF_UP flag has been done
for a long time, and is necessary to do interface auto-config. I think
it is violating a long-standing assumption (if not a standard) about the
meaning of IFF_UP, and I'm not convinced that it really is a necessity
(certainly once a config file is present for an interface, it shouldn't
be needed), but then I haven't spent as much time in that problem space
as they have.

In the meantime, the virInterfaceCreate() API fails 100% of the time on
any system that has NM enabled. My dilemma now is whether to attempt to
affect change in NM's use of IFF_UP so that it once again can be used as
an indicator of whether or not an interface is active, or to just give
in and 1) officially declare that virInterface*() isn't supported if NM
is enabled until 2) we add code to netcf that detects when NM is active
and learns how to query interface status from NM instead of the standard
ioctl(SIOCGIFFLAGS).

And if the latter is preferred, should we in the meantime perhaps revert
the patch that made virInterfaceCreate() an error if the interface was
active? Or just leave it completely broken?

Any opinions?

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

[libvirt] [PATCH] qemu: Always set migration capabilities

[Jiri Denemark]

We used to set migration capabilities only when a user asked for them in
flags. This is fine when migration succeeds since the QEMU process is
killed in the end but in case migration fails or if it's cancelled, some
capabilities may remain turned on with no way to turn them off. To fix
that, migration capabilities have to be turned on if requested but
explicitly turned off in case they were not requested but QEMU supports
them.

https://bugzilla.redhat.com/show_bug.cgi?id=1160997
Signed-off-by: Jiri Denemark jdene...@redhat.com
---
 src/qemu/qemu_migration.c| 42 +-
 src/qemu/qemu_monitor.c  |  5 +++--
 src/qemu/qemu_monitor.h  |  3 ++-
 src/qemu/qemu_monitor_json.c |  5 +++--
 src/qemu/qemu_monitor_json.h |  3 ++-
 tests/qemumonitorjsontest.c  |  3 ++-
 6 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index c797206..ff692a5 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1804,6 +1804,7 @@ qemuMigrationSetOffline(virQEMUDriverPtr driver,
 static int
 qemuMigrationSetCompression(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
+bool state,
 qemuDomainAsyncJob job)
 {
 qemuDomainObjPrivatePtr priv = vm-privateData;
@@ -1818,6 +1819,9 @@ qemuMigrationSetCompression(virQEMUDriverPtr driver,
 
 if (ret  0) {
 goto cleanup;
+} else if (ret == 0  !state) {
+/* Unsupported but we want it off anyway */
+goto cleanup;
 } else if (ret == 0) {
 if (job == QEMU_ASYNC_JOB_MIGRATION_IN) {
 virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, %s,
@@ -1834,7 +1838,8 @@ qemuMigrationSetCompression(virQEMUDriverPtr driver,
 
 ret = qemuMonitorSetMigrationCapability(
 priv-mon,
-QEMU_MONITOR_MIGRATION_CAPS_XBZRLE);
+QEMU_MONITOR_MIGRATION_CAPS_XBZRLE,
+state);
 
  cleanup:
 qemuDomainObjExitMonitor(driver, vm);
@@ -1844,6 +1849,7 @@ qemuMigrationSetCompression(virQEMUDriverPtr driver,
 static int
 qemuMigrationSetAutoConverge(virQEMUDriverPtr driver,
  virDomainObjPtr vm,
+ bool state,
  qemuDomainAsyncJob job)
 {
 qemuDomainObjPrivatePtr priv = vm-privateData;
@@ -1858,6 +1864,9 @@ qemuMigrationSetAutoConverge(virQEMUDriverPtr driver,
 
 if (ret  0) {
 goto cleanup;
+} else if (ret == 0  !state) {
+/* Unsupported but we want it off anyway */
+goto cleanup;
 } else if (ret == 0) {
 virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, %s,
_(Auto-Converge is not supported by 
@@ -1868,7 +1877,8 @@ qemuMigrationSetAutoConverge(virQEMUDriverPtr driver,
 
 ret = qemuMonitorSetMigrationCapability(
 priv-mon,
-QEMU_MONITOR_MIGRATION_CAPS_AUTO_CONVERGE);
+QEMU_MONITOR_MIGRATION_CAPS_AUTO_CONVERGE,
+state);
 
  cleanup:
 qemuDomainObjExitMonitor(driver, vm);
@@ -1879,6 +1889,7 @@ qemuMigrationSetAutoConverge(virQEMUDriverPtr driver,
 static int
 qemuMigrationSetPinAll(virQEMUDriverPtr driver,
virDomainObjPtr vm,
+   bool state,
qemuDomainAsyncJob job)
 {
 qemuDomainObjPrivatePtr priv = vm-privateData;
@@ -1893,6 +1904,9 @@ qemuMigrationSetPinAll(virQEMUDriverPtr driver,
 
 if (ret  0) {
 goto cleanup;
+} else if (ret == 0  !state) {
+/* Unsupported but we want it off anyway */
+goto cleanup;
 } else if (ret == 0) {
 if (job == QEMU_ASYNC_JOB_MIGRATION_IN) {
 virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, %s,
@@ -1909,7 +1923,8 @@ qemuMigrationSetPinAll(virQEMUDriverPtr driver,
 
 ret = qemuMonitorSetMigrationCapability(
 priv-mon,
-QEMU_MONITOR_MIGRATION_CAPS_RDMA_PIN_ALL);
+QEMU_MONITOR_MIGRATION_CAPS_RDMA_PIN_ALL,
+state);
 
  cleanup:
 qemuDomainObjExitMonitor(driver, vm);
@@ -2734,8 +2749,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
 dataFD[1] = -1; /* 'st' owns the FD now  will close it */
 }
 
-if (flags  VIR_MIGRATE_COMPRESSED 
-qemuMigrationSetCompression(driver, vm,
+if (qemuMigrationSetCompression(driver, vm,
+flags  VIR_MIGRATE_COMPRESSED,
 QEMU_ASYNC_JOB_MIGRATION_IN)  0)
 goto stop;
 
@@ -2744,8 +2759,9 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
 goto stop;
 }
 
-if (flags  VIR_MIGRATE_RDMA_PIN_ALL 
-qemuMigrationSetPinAll(driver, vm, QEMU_ASYNC_JOB_MIGRATION_IN)  0)
+if (qemuMigrationSetPinAll(driver, vm,
+   flags  VIR_MIGRATE_RDMA_PIN_ALL,
+   

[libvirt] [PATCH 0/2] Fix errors on unsuccessful chardev hotplug

[Ján Tomko]

Ján Tomko (2):
  Fix crash after attempting to hotplug a spicevmc channel
  Display nicer error message for unsupported chardev hotplug

 src/conf/domain_conf.c   |  3 +--
 src/qemu/qemu_monitor_json.c | 12 +---
 2 files changed, 10 insertions(+), 5 deletions(-)

-- 
2.0.4

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

[libvirt] [PATCH 2/2] Display nicer error message for unsupported chardev hotplug

[Ján Tomko]

Use the device type name if we know it instead of its number,
even if we can't hotplug it:
qemuMonitorJSONAttachCharDevCommand:6094 : operation failed: Unsupported
char device type '10'
---
 src/qemu/qemu_monitor_json.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 7870664..2d082bc 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -5915,9 +5915,15 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID,
 case VIR_DOMAIN_CHR_TYPE_STDIO:
 case VIR_DOMAIN_CHR_TYPE_NMDM:
 case VIR_DOMAIN_CHR_TYPE_LAST:
-virReportError(VIR_ERR_OPERATION_FAILED,
-   _(Unsupported char device type '%d'),
-   chr-type);
+if (virDomainChrTypeToString(chr-type)) {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _(Hotplug unsupported char device type '%s'),
+   virDomainChrTypeToString(chr-type));
+} else {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _(Unsupported char device type '%d'),
+   chr-type);
+}
 goto error;
 }
 
-- 
2.0.4

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

Re: [libvirt] [PATCH] qemu: Always set migration capabilities

[Jiri Denemark]

On Mon, Nov 10, 2014 at 16:41:23 +0100, Jiri Denemark wrote:
 We used to set migration capabilities only when a user asked for them in
 flags. This is fine when migration succeeds since the QEMU process is
 killed in the end but in case migration fails or if it's cancelled, some
 capabilities may remain turned on with no way to turn them off. To fix
 that, migration capabilities have to be turned on if requested but
 explicitly turned off in case they were not requested but QEMU supports
 them.
 
 https://bugzilla.redhat.com/show_bug.cgi?id=1160997

Oops, wrong bug number, I'll provide a new one once I found it...

Jirka

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

Re: [libvirt] [PATCH v7 4/7] qemu: Add bps_max and friends qemu driver

[Michal Privoznik]

On 10.11.2014 16:19, Matthias Gatto wrote:

Add support for bps_max and friends in the driver part.
In the part checking if a qemu is running, check if the running binary
support bps_max, if not print an error message, if yes add it to
info variable

This patch follow therse patchs: 
http://www.redhat.com/archives/libvir-list/2014-November/msg00271.html
I've fix the syntax error and the nparams detection problem

Signed-off-by: Matthias Gatto matthias.ga...@outscale.com
---
  include/libvirt/libvirt-domain.h |  56 +++
  src/qemu/qemu_driver.c   | 197 ---
  src/qemu/qemu_monitor.c  |  10 +-
  src/qemu/qemu_monitor.h  |   6 +-
  src/qemu/qemu_monitor_json.c |  11 ++-
  src/qemu/qemu_monitor_json.h |   6 +-
  tests/qemumonitorjsontest.c  |   4 +-
  7 files changed, 264 insertions(+), 26 deletions(-)


I've capped the long lines and

ACK.

Moreover, I've merged the patches. Congratulations on your first libvirt 
contribution!


Michal

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

Re: [libvirt] [PATCH v7 4/7] qemu: Add bps_max and friends qemu driver

[Matthias Gatto]

On Mon, Nov 10, 2014 at 5:25 PM, Michal Privoznik mpriv...@redhat.com wrote:
 On 10.11.2014 16:19, Matthias Gatto wrote:

 Add support for bps_max and friends in the driver part.
 In the part checking if a qemu is running, check if the running binary
 support bps_max, if not print an error message, if yes add it to
 info variable

 This patch follow therse patchs:
 http://www.redhat.com/archives/libvir-list/2014-November/msg00271.html
 I've fix the syntax error and the nparams detection problem

 Signed-off-by: Matthias Gatto matthias.ga...@outscale.com
 ---
   include/libvirt/libvirt-domain.h |  56 +++
   src/qemu/qemu_driver.c   | 197
 ---
   src/qemu/qemu_monitor.c  |  10 +-
   src/qemu/qemu_monitor.h  |   6 +-
   src/qemu/qemu_monitor_json.c |  11 ++-
   src/qemu/qemu_monitor_json.h |   6 +-
   tests/qemumonitorjsontest.c  |   4 +-
   7 files changed, 264 insertions(+), 26 deletions(-)


 I've capped the long lines and

 ACK.

 Moreover, I've merged the patches. Congratulations on your first libvirt
 contribution!

 Michal

Thank you very much.

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

[libvirt] 回复：How do I get the progress of a long operation

[windy]

Hi All:
In recent days, I have studied the libvirt event model, but found nothing I 
wanted.
In my requirement, I want to get the progress of long operations,especially 
snapshot taking and revcovering.
Today, I studied how virsh migrate command realised, I found that the 
process of migration is getted by job infomation.
But when i taking snapshot the domain's control state is 
VIR_DOMAIN_CONTROL_OCCUPIED, not expected VIR_DOMAIN_CONTROL_JOB status,
So i can't get any job information(all values are zero).
Are there any other methods to get the progress of long operation?
Thank you !‍





-- 原始邮件 --
发件人: 380372671;380372...@qq.com;
发送时间: 2014年11月7日(星期五) 晚上9:24
收件人: libvir-listlibvir-list@redhat.com; 

主题: How do  I get  the  progress of a long operation



Dear Libvirt:  How do  I get  the  progress of a long operation (such as 
Snapshot take and recover )through libvirt API?
  Thankyou.‍--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Problem with virInterfaceCreate(), IFF_UP, and NetworkManager

[Michal Privoznik]

On 10.11.2014 16:41, Laine Stump wrote:

Due to a checkered past (a myriad of minor issues, changing over
time), libvirt's semi-official position on the virInterface*() APIs and
NetworkManager is that virInterface*() is only supported if NM is
disabled. We do still attempt to make it work as well as possible, but
normally I only test those APIs on systems that have NM disabled and use
the network service (RHEL/Fedora/CentOS systems here) instead.

On a seemingly unrelated note, a few months ago mprivozn pushed a patch
that makes it an error to call virInterfaceCreate() (i.e. ifup) for an
interface that is already active. (the active state of an interface is
determined by looking at an interface's IFF_UP flag (and also
IFF_RUNNING, if the interface isn't a bridge device). Previously, this
was allowed, as it is common practive to ifup an interface to make new
config take effect.

Last week, I happened to test the virsh iface-bridge command on a
system with NM enabled. That command gave an error about the interface
being already active, so I tried again, this time ifdowning the
interface in advance - I *still* got the error. Further investigation
and questioning of NM developers led me to the realization that when NM
is enabled, all interfaces *always* have IFF_UP and IFF_RUNNING set,
even if they are ifdowned. Further, if NM is active there is no way to
determine an interface's active status via iotctl() or netlink;
instead, must query to determine if NM is active, and if it is you must
call a NM API instead (I got this much information from NM developers
directly; haven't investigated yet exactly what the API is).

NM developers say that this pinning-up of the IFF_UP flag has been done
for a long time, and is necessary to do interface auto-config. I think
it is violating a long-standing assumption (if not a standard) about the
meaning of IFF_UP, and I'm not convinced that it really is a necessity
(certainly once a config file is present for an interface, it shouldn't
be needed), but then I haven't spent as much time in that problem space
as they have.


Looking into kernel code reveals that IFF_UP really is meant for that. I 
mean, from the kernel sources (include/uapi/linux/if.h):



 * @IFF_UP: interface is up. Can be toggled through sysfs.

I think NM shouldn't be misusing this. I'd suggest getting back to NM 
developers trying to persuade them to reconsider.


In the meantime, the virInterfaceCreate() API fails 100% of the time on
any system that has NM enabled. My dilemma now is whether to attempt to
affect change in NM's use of IFF_UP so that it once again can be used as
an indicator of whether or not an interface is active, or to just give
in and 1) officially declare that virInterface*() isn't supported if NM
is enabled until 2) we add code to netcf that detects when NM is active
and learns how to query interface status from NM instead of the standard
ioctl(SIOCGIFFLAGS).

And if the latter is preferred, should we in the meantime perhaps revert
the patch that made virInterfaceCreate() an error if the interface was
active? Or just leave it completely broken?


We can revert the patch meantime, I guess.



Any opinions?



Michal

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

Re: [libvirt] [PATCH 2/2] Display nicer error message for unsupported chardev hotplug

[Pavel Hrdina]

On 11/10/2014 04:57 PM, Ján Tomko wrote:

Use the device type name if we know it instead of its number,
even if we can't hotplug it:
qemuMonitorJSONAttachCharDevCommand:6094 : operation failed: Unsupported
char device type '10'
---
  src/qemu/qemu_monitor_json.c | 12 +---
  1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 7870664..2d082bc 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -5915,9 +5915,15 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID,
  case VIR_DOMAIN_CHR_TYPE_STDIO:
  case VIR_DOMAIN_CHR_TYPE_NMDM:
  case VIR_DOMAIN_CHR_TYPE_LAST:
-virReportError(VIR_ERR_OPERATION_FAILED,
-   _(Unsupported char device type '%d'),
-   chr-type);
+if (virDomainChrTypeToString(chr-type)) {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _(Hotplug unsupported char device type '%s'),
+   virDomainChrTypeToString(chr-type));
+} else {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _(Unsupported char device type '%d'),
+   chr-type);
+}
  goto error;
  }




Both error messages could be the same as they are reporting the same
issue. ACK with that change.

Pavel

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

Re: [libvirt] [PATCH 1/2] Fix crash after attempting to hotplug a spicevmc channel

[Pavel Hrdina]

On 11/10/2014 04:56 PM, Ján Tomko wrote:

In qemuDomainAttachChrDevice, we add the device to the domain XML,
then we fail to construct the JSON command, saying we don't support
it.

But we don't clean up the device from the domain XML, because
virDomainChrEquals returns false when comparing the device against
itself.

https://bugzilla.redhat.com/show_bug.cgi?id=1162097
---
  src/conf/domain_conf.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e8d8f7d..31378cd 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1596,8 +1596,7 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef 
*src,
  case VIR_DOMAIN_CHR_TYPE_STDIO:
  case VIR_DOMAIN_CHR_TYPE_SPICEVMC:
  case VIR_DOMAIN_CHR_TYPE_LAST:
-/* nada */
-break;
+return true;
  }

  return false;



This isn't true for VIR_DOMAIN_CHR_TYPE_SPICEVMC as we are setting
data.spicevmc to some value and it could have different value.
See enum virDomainChrSpicevmcName.

Pavel

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

Re: [libvirt] 回复：How do I get the progress of a long operation

[Eric Blake]

On 11/10/2014 09:37 AM, windy wrote:
 Hi All:

[please don't top-post on technical lists; also, please configure your
mailer to wrap long lines]

 In recent days, I have studied the libvirt event model, but found nothing 
 I wanted.
 In my requirement, I want to get the progress of long 
 operations,especially snapshot taking and revcovering.

I already told you that we don't have very good job support for some
long-running jobs, and that one such long-running job is that of taking
an internal snapshot.  Patches are welcome.

 Today, I studied how virsh migrate command realised, I found that the 
 process of migration is getted by job infomation.
 But when i taking snapshot the domain's control state is 
 VIR_DOMAIN_CONTROL_OCCUPIED, not expected VIR_DOMAIN_CONTROL_JOB status,

Yes, because we don't yet have the ability to interrupt a snapshot
creation task.  Job control can only be probed for jobs that have been
wired up to the asynchronous job framework, and taking an internal
snapshot does not yet fit that framework (and it is more than just
libvirt that needs patching; qemu itself does not have a way to
interrupt the HMP 'savevm' command, and does not have a QMP counterpart
command).

 So i can't get any job information(all values are zero).
 Are there any other methods to get the progress of long operation?

It depends on the job you want to interrupt.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

Re: [libvirt] Problem with virInterfaceCreate(), IFF_UP, and NetworkManager

[Daniel P. Berrange]

On Mon, Nov 10, 2014 at 10:41:26AM -0500, Laine Stump wrote:
 On a seemingly unrelated note, a few months ago mprivozn pushed a patch
 that makes it an error to call virInterfaceCreate() (i.e. ifup) for an
 interface that is already active. (the active state of an interface is
 determined by looking at an interface's IFF_UP flag (and also
 IFF_RUNNING, if the interface isn't a bridge device). Previously, this
 was allowed, as it is common practive to ifup an interface to make new
 config take effect.
 
 Last week, I happened to test the virsh iface-bridge command on a
 system with NM enabled. That command gave an error about the interface
 being already active, so I tried again, this time ifdowning the
 interface in advance - I *still* got the error. Further investigation
 and questioning of NM developers led me to the realization that when NM
 is enabled, all interfaces *always* have IFF_UP and IFF_RUNNING set,
 even if they are ifdowned. Further, if NM is active there is no way to
 determine an interface's active status via iotctl() or netlink;
 instead, must query to determine if NM is active, and if it is you must
 call a NM API instead (I got this much information from NM developers
 directly; haven't investigated yet exactly what the API is).
 
 NM developers say that this pinning-up of the IFF_UP flag has been done
 for a long time, and is necessary to do interface auto-config. I think
 it is violating a long-standing assumption (if not a standard) about the
 meaning of IFF_UP, and I'm not convinced that it really is a necessity
 (certainly once a config file is present for an interface, it shouldn't
 be needed), but then I haven't spent as much time in that problem space
 as they have.

Yep, I understand their motivation here - with IPv6 address auto-config,
you really want your NICs to be permanently up (or online), so that
if an IPv6 router advertizement arrives it just works without the user
needing to turn this on manually.

IIUC, they essentially have 3 states

 - offline - IFF_UP not set - don't think this is used unless you
   explicitly tell NM to disable the interface (ie airplane mode
   for the wifi NIC)

 - unconfigured - IFF_UP set and no addresses present

 - configured - IFF_UP set and addresses present.

Our API design is really looking at the transition between offline
and configured. We don't have the concept of unconfigured really.

 In the meantime, the virInterfaceCreate() API fails 100% of the time on
 any system that has NM enabled. My dilemma now is whether to attempt to
 affect change in NM's use of IFF_UP so that it once again can be used as
 an indicator of whether or not an interface is active, or to just give
 in and 1) officially declare that virInterface*() isn't supported if NM
 is enabled until 2) we add code to netcf that detects when NM is active
 and learns how to query interface status from NM instead of the standard
 ioctl(SIOCGIFFLAGS).
 
 And if the latter is preferred, should we in the meantime perhaps revert
 the patch that made virInterfaceCreate() an error if the interface was
 active? Or just leave it completely broken?
 
 Any opinions?

It feels like when NM is present on the system, libvirt should still
honour the IFF_UP flag. ie it should always report all the NICs managed
by network manager to be up if IFF_UP is set.

I think this implies we should not forbid the virInterfaceCreate API if
the state is IFF_UP.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

[libvirt] LSN-2014-0007: CVE-2014-7823 virDomainGetXMLDesc leaks VNC passwords

[Eric Blake]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

Libvirt Security Notice: LSN-2014-0007
==

   Summary: virDomainGetXMLDesc leaks VNC passwords
   Reported on: 20141031
  Published on: 20141105
  Fixed on: 20141106
   Reported by: Eric Blake ebl...@redhat.com
Patched by: Eric Blake ebl...@redhat.com
  See also: CVE-2014-7823

Description
- ---

At the time the VIR_DOMAIN_XML_MIGRATABLE flag was added to the
virDomainGetXMLDesc API, the qemu implementation chose to make the
flag always imply the VIR_DOMAIN_XML_SECURE flag. The secure flag
had been previously deemed unsafe to use from a read-only
connection; however, because the new migratable flag is not
restricted against use by read-only clients, a client can use the
new flag to bypass the restrictions placed on the use of the old
flag.

Impact
- --

A read-only client can trigger an information leak of data that
should normally require the use of VIR_DOMAIN_XML_SECURE to access.
Fortunately, the only data in this category is the value of an
optional VNC password.

Workaround
- --

VNC passwords are notoriously weak (they are capped at an 8 byte
maximum length; the VNC protocol sends them in plaintext over the
network; and FIPS mode execution prohibits the use of a VNC
password), so it is recommended that users not create domains with a
VNC password in the first place. Domains that do not use VNC
passwords do not suffer from information leaks; the use of SPICE
connections is recommended not only because it avoids the leak, but
also because SPICE provides better features than VNC for a guest
graphics device. It is also possible to prevent the leak by denying
access to read-only clients; for builds of libvirt that support
fine-grained ACLs, this course of action requires ensuring that no
user is granted the 'read' ACL privilege without also having the
'read_secure' privilege.

Affected product
- 

Name: libvirt
  Repository: git://libvirt.org/git/libvirt.git
  http://libvirt.org/git/?p=libvirt.git

  Branch: master
   Broken in: v1.0.0
   Broken in: v1.0.1
   Broken in: v1.0.2
   Broken in: v1.0.3
   Broken in: v1.0.4
   Broken in: v1.0.5
   Broken in: v1.0.6
   Broken in: v1.1.0
   Broken in: v1.1.1
   Broken in: v1.1.2
   Broken in: v1.1.3
   Broken in: v1.1.4
   Broken in: v1.2.0
   Broken in: v1.2.1
   Broken in: v1.2.2
   Broken in: v1.2.3
   Broken in: v1.2.4
   Broken in: v1.2.5
   Broken in: v1.2.6
   Broken in: v1.2.7
   Broken in: v1.2.8
   Broken in: v1.2.9
   Broken in: v1.2.10
Fixed in: v1.2.11
   Broken by: 28f8dfdcccd4c0f69063ef741545b37d8a7f7935
Fixed by: b1674ad5a97441b7e1bd5f5ebaff498ef2fbb11b

  Branch: v1.0.2-maint
   Broken by: 28f8dfdcccd4c0f69063ef741545b37d8a7f7935
Fixed by: 7b334c1660e926da7c0644c945263ce40a80443f

  Branch: v1.0.3-maint
   Broken by: 28f8dfdcccd4c0f69063ef741545b37d8a7f7935
Fixed by: 220c6b867ca81f9027a7da54d5bc44b43c742d2a

  Branch: v1.0.4-maint
   Broken by: 28f8dfdcccd4c0f69063ef741545b37d8a7f7935
Fixed by: 3b7ce055e37e92c34090fcfcc0b6eaa860aa94a9

  Branch: v1.0.5-maint
   Broken in: v1.0.5.1
   Broken in: v1.0.5.2
   Broken in: v1.0.5.3
   Broken in: v1.0.5.4
   Broken in: v1.0.5.5
   Broken in: v1.0.5.6
   Broken in: v1.0.5.7
   Broken in: v1.0.5.8
   Broken in: v1.0.5.9
   Broken by: 28f8dfdcccd4c0f69063ef741545b37d8a7f7935
Fixed by: 107f1ff20edc805433cade910a00328158b1c231

  Branch: v1.0.6-maint
   Broken by: 28f8dfdcccd4c0f69063ef741545b37d8a7f7935
Fixed by: 333c95c9f3fb1e3c42b37f79b7f186511e8f8264

  Branch: v1.1.0-maint
   Broken by: 28f8dfdcccd4c0f69063ef741545b37d8a7f7935
Fixed by: 3d751cdcdbfac95b4a39a7db1b6e12e20838cb65

  Branch: v1.1.1-maint
   Broken by: 28f8dfdcccd4c0f69063ef741545b37d8a7f7935
Fixed by: f8c771335998f4d7a91b03c11526d819ee470dfc

  Branch: v1.1.2-maint
   Broken by: 28f8dfdcccd4c0f69063ef741545b37d8a7f7935
Fixed by: 520ecab4ca09859d4de39cad7ae2e34272e0437e

  Branch: v1.1.3-maint
   Broken in: v1.1.3.1
   Broken in: v1.1.3.2
   Broken in: v1.1.3.3
   Broken in: v1.1.3.4
   Broken in: v1.1.3.5
   Broken in: v1.1.3.6
   Broken by: 28f8dfdcccd4c0f69063ef741545b37d8a7f7935
Fixed by: bdbcf66ae72f82d45faa889a1208444f83f5756b

  Branch: v1.1.4-maint
   Broken by: 28f8dfdcccd4c0f69063ef741545b37d8a7f7935
Fixed by: 4e3856c06a3362a17a5aff0b59c4bfffbd97d105

  Branch: v1.2.0-maint
   Broken by: 28f8dfdcccd4c0f69063ef741545b37d8a7f7935
Fixed by: 757292bfb33b610daff0936d2205a90d5d787a1a

  Branch: v1.2.1-maint
   Broken by: 28f8dfdcccd4c0f69063ef741545b37d8a7f7935
Fixed by: 3adae530f549448cecfb6212a2e48bf4b04931bd

  Branch: v1.2.2-maint
   Broken by: 28f8dfdcccd4c0f69063ef741545b37d8a7f7935
Fixed by: bd78e6f6362d2484b931f112506dfde9d053fcde

  Branch: v1.2.3-maint
   Broken by: 28f8dfdcccd4c0f69063ef741545b37d8a7f7935
Fixed by: 

Re: [libvirt] [PATCH] network: Add bandwidth support to ethernet interface

[Anirban Chakraborty]

On 11/3/14, 2:58 AM, Michal Privoznik mpriv...@redhat.com wrote:

On 30.10.2014 00:56, Anirban Chakraborty wrote:
snip

 +static inline bool virNetDevSupportBandwidth(virDomainNetType type)
 +{
 +switch (type) {
 +case VIR_DOMAIN_NET_TYPE_BRIDGE:
 +case VIR_DOMAIN_NET_TYPE_NETWORK:
 +case VIR_DOMAIN_NET_TYPE_DIRECT:
 +case VIR_DOMAIN_NET_TYPE_ETHERNET:
 +return true;
 +case VIR_DOMAIN_NET_TYPE_USER:
 +case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
 +case VIR_DOMAIN_NET_TYPE_SERVER:
 +case VIR_DOMAIN_NET_TYPE_CLIENT:
 +case VIR_DOMAIN_NET_TYPE_MCAST:
 +case VIR_DOMAIN_NET_TYPE_INTERNAL:
 +case VIR_DOMAIN_NET_TYPE_HOSTDEV:
 +case VIR_DOMAIN_NET_TYPE_LAST:
 +break;
 +}
 +return false;
 +}
 +

I've got a feeling that this should go to src/util/virnetdevbandwidth*
among with the rest of virNetDevBandwitdh functions.

I thought about moving this and the qemuDomainClearNetBandwidth() to
src/util/virnetdevbandwidth.h earlier, however, these functions need
reference to virDomainNetType and virDomainObjPtr which are defined in
conf/domain_conf.h and apparently src/util/*.h can not have any reference
to files from conf/.


   #endif /* __DOMAIN_CONF_H */
 diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
 index 979382b..8a21af4 100644
 --- a/src/lxc/lxc_driver.c
 +++ b/src/lxc/lxc_driver.c
 @@ -72,6 +72,7 @@
   #include viraccessapicheck.h
   #include viraccessapichecklxc.h
   #include virhostdev.h
 +#include qemu/qemu_command.h

This is not allowed. In case somebody is building with LXC but without
QEMU ..

Thanks for pointing it out.



   #define VIR_FROM_THIS VIR_FROM_LXC

 @@ -4634,6 +4635,8 @@ lxcDomainDetachDeviceNetLive(virDomainObjPtr vm,

   detach = vm-def-nets[detachidx];

 +qemuDomainClearNetBandwidth(vm);
 +

.. this is going to be an undefined reference.

   switch (virDomainNetGetActualType(detach)) {
   case VIR_DOMAIN_NET_TYPE_BRIDGE:
   case VIR_DOMAIN_NET_TYPE_NETWORK:
 diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
 index ed30c37..3192011 100644
 --- a/src/lxc/lxc_process.c
 +++ b/src/lxc/lxc_process.c
 @@ -274,11 +274,6 @@ char
*virLXCProcessSetupInterfaceBridged(virConnectPtr conn,
   if (virNetDevSetOnline(parentVeth, true)  0)
   goto cleanup;

 -if (virNetDevBandwidthSet(net-ifname,
 -  virDomainNetGetActualBandwidth(net),
 -  false)  0)
 -goto cleanup;
 -


No, this function is called from two places:
1) when domain is starting up
2) on NIC hotplug

you are covering 1) but removing already working 2). We can't lose that
functionality.

Got it. Thanks.


   if (net-filter 
   virDomainConfNWFilterInstantiate(conn, vm-uuid, net)  0)
   goto cleanup;
 @@ -300,6 +295,7 @@ char
*virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
   virNetDevBandwidthPtr bw;
   virNetDevVPortProfilePtr prof;
   virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver);
 +const char *linkdev = virDomainNetGetActualDirectDev(net);

   /* XXX how todo bandwidth controls ?
* Since the 'net-ifname' is about to be moved to a different
 @@ -329,14 +325,13 @@ char
*virLXCProcessSetupInterfaceDirect(virConnectPtr conn,

   if (virNetDevMacVLanCreateWithVPortProfile(
   net-ifname, net-mac,
 -virDomainNetGetActualDirectDev(net),
 +linkdev,
   virDomainNetGetActualDirectMode(net),
   false, def-uuid,
 -virDomainNetGetActualVirtPortProfile(net),
 +prof,
   res_ifname,
   VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
 -cfg-stateDir,
 -virDomainNetGetActualBandwidth(net), 0)  0)
 +cfg-stateDir, 0)  0)
   goto cleanup;


Same comment applies here.

Thanks.


   ret = res_ifname;
 @@ -450,6 +445,11 @@ static int
virLXCProcessSetupInterfaces(virConnectPtr conn,
   goto cleanup;
   }

 +/* set network bandwidth */
 +if (virNetDevBandwidthSet(def-nets[i]-ifname,
 +virDomainNetGetActualBandwidth(def-nets[i]), false) 
0)
 +   goto cleanup;
 +

Shouldn't this be guarded with virNetDevSupportBandwidth()? The problem
is, there's a switch() just before this where all the unsupported NIC
types are rejected (ETHERNET is rejected too btw). What will come
through is DIRECT type. I'm not saying that we should not set bandwidth
there, but this patch aims at ethernet.

Agreed. Will take care of it next version of the patch.


   (*veths)[(*nveths)-1] = veth;

   /* Make sure all net definitions will have a name in the
container */
 diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
 index 2e5af4f..7922672 100644
 --- a/src/qemu/qemu_command.c
 +++ b/src/qemu/qemu_command.c
 @@ -191,7 +191,6 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
   

[libvirt] [PATCH v2 5/5] storage: Introduce 'managed' for the fchost parent

[John Ferlan]

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

Introduce a 'managed' attribute to allow libvirt to decide whether to
delete a vHBA vport created via external means such as nodedev-create.
The code currently decides whether to delete the vHBA based solely on
whether the parent was provided at creation time. However, that may not
be the desired action, so rather than delete and force someone to create
another vHBA via an additional nodedev-create allow the configuration of
the storage pool to decide the desired action.

During createVport when libvirt does the VPORT_CREATE, set the managed
value to YES if not already set to indicate to the deleteVport code that
it should delete the vHBA when the pool is destroyed.

If libvirtd is restarted all the memory only state was lost, so for a
persistent storage pool, use the virStoragePoolSaveConfig in order to
write out the managed value.

Because we're now saving the current configuration, we need to be sure
to not save the parent in the output XML if it was undefined at start.
Saving the name would cause future starts to always use the same parent
which is not the expected result when not providing a parent. By not
providing a parent, libvirt is expected to find the best available
vHBA port for each subsequent (re)start.

At deleteVport, use the new managed value to decide whether to execute
the VPORT_DELETE.  Since we no longer save the parent in memory or in
XML when provided, if it was not provided, then we have to look it up.

Signed-off-by: John Ferlan jfer...@redhat.com
---
 docs/formatstorage.html.in | 13 +++
 docs/schemas/basictypes.rng|  5 ++
 src/conf/storage_conf.c| 17 
 src/conf/storage_conf.h|  1 +
 src/storage/storage_backend_scsi.c | 93 +-
 .../pool-scsi-type-fc-host-managed.xml | 15 
 .../pool-scsi-type-fc-host-managed.xml | 18 +
 tests/storagepoolxml2xmltest.c |  1 +
 8 files changed, 143 insertions(+), 20 deletions(-)
 create mode 100644 
tests/storagepoolxml2xmlin/pool-scsi-type-fc-host-managed.xml
 create mode 100644 
tests/storagepoolxml2xmlout/pool-scsi-type-fc-host-managed.xml

diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
index 9d77c45..29c1931 100644
--- a/docs/formatstorage.html.in
+++ b/docs/formatstorage.html.in
@@ -190,6 +190,19 @@
 defined for an existing scsi_host or by creating a vHBA.
 span class=sinceSince 1.0.4/span
   /dd
+  dtcodemanaged/code/dt
+  ddAn optional attribute to instruct the SCSI storage backend to
+manage destroying the vHBA when the pool is destroyed. For
+configurations that do not provide an already created vHBA
+from a 'virsh nodedev-create', libvirt will set this property
+to yes. For configurations that have already created a vHBA
+via 'virsh nodedev-create' and are using the wwnn/wwpn from
+that vHBA and optionally the scsi_host parent, setting this
+attribute to yes will allow libvirt to destroy the node device
+when the pool is destroyed. If this attribute is set to no or
+not defined in the XML, then libvirt will not destroy the vHBA.
+span class=sinceSince 1.2.11/span
+  /dd
 /dl
 dl
   dtcodeparentaddr/code/dt
diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
index 14245c9..9ddd92b 100644
--- a/docs/schemas/basictypes.rng
+++ b/docs/schemas/basictypes.rng
@@ -389,6 +389,11 @@
   text/
 /attribute
   /optional
+  optional
+attribute name='managed'
+  ref name=virYesNo/
+/attribute
+  /optional
   attribute name='wwnn'
 ref name='wwn'/
   /attribute
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 4126451..1251b47 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -475,6 +475,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
 char *name = NULL;
 char *port = NULL;
 char *adapter_type = NULL;
+char *managed = NULL;
 int n;
 
 relnode = ctxt-node;
@@ -578,6 +579,18 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
 VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
 source-adapter.data.fchost.parent =
 virXPathString(string(./adapter/@parent), ctxt);
+managed = virXPathString(string(./adapter/@managed), ctxt);
+if (managed) {
+source-adapter.data.fchost.managed =
+virTristateBoolTypeFromString(managed);
+if (source-adapter.data.fchost.managed  0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _(unknown fc_host 

[libvirt] [PATCH v2 3/5] storage: Don't use a stack copy of the adapter

[John Ferlan]

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

Passing a copy of the storage pool adapter to a function just changes the
copy of the fields in the particular function and then when returning to
the caller those changes are discarded.  While not yet biting us in the
storage clean-up case, it did cause an issue for the fchost storage pool
startup case, createVport.  The issue was at startup, if no parent is found
in the XML, the code will search for the 'best available' parent and then
store that in the in memory copy of the adapter.  Of course, in this case
it was a copy, so when returning to the virStorageBackendSCSIStartPool that
change was discarded (or lost) from the pool-def-source.adapter which
meant at shutdown (deleteVport), the code assumed no adapter was passed
and skipped the deletion, leaving the vHBA created by libvirt still defined
requiring an additional stop of a nodedev-destroy to remove.

Adjusted the createVport to take virStoragePoolDefPtr instead of the
adapter copy. Then use the virStoragePoolSourceAdapterPtr when processing.
A future patch will need the 'def' anyway, so this just sets up for that.

Signed-off-by: John Ferlan jfer...@redhat.com
---
 src/conf/storage_conf.c| 16 
 src/conf/storage_conf.h|  1 +
 src/storage/storage_backend_scsi.c | 32 
 3 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index afd6cd4..52656e5 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -343,15 +343,15 @@ virStorageVolDefFree(virStorageVolDefPtr def)
 }
 
 static void
-virStoragePoolSourceAdapterClear(virStoragePoolSourceAdapter adapter)
+virStoragePoolSourceAdapterClear(virStoragePoolSourceAdapterPtr adapter)
 {
-if (adapter.type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
-VIR_FREE(adapter.data.fchost.wwnn);
-VIR_FREE(adapter.data.fchost.wwpn);
-VIR_FREE(adapter.data.fchost.parent);
-} else if (adapter.type ==
+if (adapter-type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
+VIR_FREE(adapter-data.fchost.wwnn);
+VIR_FREE(adapter-data.fchost.wwpn);
+VIR_FREE(adapter-data.fchost.parent);
+} else if (adapter-type ==
VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
-VIR_FREE(adapter.data.scsi_host.name);
+VIR_FREE(adapter-data.scsi_host.name);
 }
 }
 
@@ -380,7 +380,7 @@ virStoragePoolSourceClear(virStoragePoolSourcePtr source)
 VIR_FREE(source-devices);
 VIR_FREE(source-dir);
 VIR_FREE(source-name);
-virStoragePoolSourceAdapterClear(source-adapter);
+virStoragePoolSourceAdapterClear(source-adapter);
 VIR_FREE(source-initiator.iqn);
 virStorageAuthDefFree(source-auth);
 VIR_FREE(source-vendor);
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
index 1276ac2..a9b5bdb 100644
--- a/src/conf/storage_conf.h
+++ b/src/conf/storage_conf.h
@@ -177,6 +177,7 @@ typedef enum {
 VIR_ENUM_DECL(virStoragePoolSourceAdapter)
 
 typedef struct _virStoragePoolSourceAdapter virStoragePoolSourceAdapter;
+typedef virStoragePoolSourceAdapter *virStoragePoolSourceAdapterPtr;
 struct _virStoragePoolSourceAdapter {
 int type; /* virStoragePoolSourceAdapterType */
 
diff --git a/src/storage/storage_backend_scsi.c 
b/src/storage/storage_backend_scsi.c
index 5220292..41ea67a 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -644,24 +644,25 @@ checkVhbaSCSIHostParent(virConnectPtr conn,
 
 static int
 createVport(virConnectPtr conn,
-virStoragePoolSourceAdapter adapter)
+virStoragePoolDefPtr def)
 {
+virStoragePoolSourceAdapterPtr adapter = def-source.adapter;
 unsigned int parent_host;
 char *name = NULL;
 
-if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST)
+if (adapter-type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST)
 return 0;
 
 /* If a parent was provided, then let's make sure it's vhost capable */
-if (adapter.data.fchost.parent) {
-if (virGetSCSIHostNumber(adapter.data.fchost.parent, parent_host)  0)
+if (adapter-data.fchost.parent) {
+if (virGetSCSIHostNumber(adapter-data.fchost.parent, parent_host)  
0)
 return -1;
 
 if (!virIsCapableFCHost(NULL, parent_host)) {
 virReportError(VIR_ERR_XML_ERROR,
_(parent '%s' specified for vHBA 
  is not vport capable),
-   adapter.data.fchost.parent);
+   adapter-data.fchost.parent);
 return -1;
 }
 }
@@ -670,34 +671,34 @@ createVport(virConnectPtr conn,
  * using the wwnn/wwpn, then a nodedev is already created for
  * this pool and we don't have to create the vHBA
  */
-if ((name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn,
-   

[libvirt] [PATCH v2 4/5] storage: Introduce virStoragePoolSaveConfig

[John Ferlan]

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

Introduce the ability to save a configuration of a persistent configuration
that may be changed by storage pool backend activity, such as start or stop

Signed-off-by: John Ferlan jfer...@redhat.com
---
 src/conf/storage_conf.c  | 37 ++---
 src/conf/storage_conf.h  |  2 ++
 src/libvirt_private.syms |  1 +
 3 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 52656e5..4126451 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1884,14 +1884,33 @@ virStoragePoolLoadAllConfigs(virStoragePoolObjListPtr 
pools,
 }
 
 int
-virStoragePoolObjSaveDef(virStorageDriverStatePtr driver,
- virStoragePoolObjPtr pool,
+virStoragePoolSaveConfig(const char *configFile,
  virStoragePoolDefPtr def)
 {
 char uuidstr[VIR_UUID_STRING_BUFLEN];
 char *xml;
 int ret = -1;
 
+if (!(xml = virStoragePoolDefFormat(def))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, %s,
+   _(failed to generate XML));
+return -1;
+}
+
+virUUIDFormat(def-uuid, uuidstr);
+ret = virXMLSaveFile(configFile,
+ virXMLPickShellSafeComment(def-name, uuidstr),
+ pool-edit, xml);
+VIR_FREE(xml);
+
+return ret;
+}
+
+int
+virStoragePoolObjSaveDef(virStorageDriverStatePtr driver,
+ virStoragePoolObjPtr pool,
+ virStoragePoolDefPtr def)
+{
 if (!pool-configFile) {
 if (virFileMakePath(driver-configDir)  0) {
 virReportSystemError(errno,
@@ -1912,19 +1931,7 @@ virStoragePoolObjSaveDef(virStorageDriverStatePtr driver,
 }
 }
 
-if (!(xml = virStoragePoolDefFormat(def))) {
-virReportError(VIR_ERR_INTERNAL_ERROR, %s,
-   _(failed to generate XML));
-return -1;
-}
-
-virUUIDFormat(def-uuid, uuidstr);
-ret = virXMLSaveFile(pool-configFile,
- virXMLPickShellSafeComment(def-name, uuidstr),
- pool-edit, xml);
-VIR_FREE(xml);
-
-return ret;
+return virStoragePoolSaveConfig(pool-configFile, def);
 }
 
 int
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
index a9b5bdb..67145a0 100644
--- a/src/conf/storage_conf.h
+++ b/src/conf/storage_conf.h
@@ -360,6 +360,8 @@ virStoragePoolObjPtr
 virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools,
virStoragePoolDefPtr def);
 
+int virStoragePoolSaveConfig(const char *configDir,
+ virStoragePoolDefPtr def);
 int virStoragePoolObjSaveDef(virStorageDriverStatePtr driver,
  virStoragePoolObjPtr pool,
  virStoragePoolDefPtr def);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b8f35e8..0864618 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -753,6 +753,7 @@ virStoragePoolObjLock;
 virStoragePoolObjRemove;
 virStoragePoolObjSaveDef;
 virStoragePoolObjUnlock;
+virStoragePoolSaveConfig;
 virStoragePoolSourceAdapterTypeFromString;
 virStoragePoolSourceAdapterTypeToString;
 virStoragePoolSourceClear;
-- 
1.9.3

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

[libvirt] [PATCH v2 2/5] storage: Ensure fc_host parent matches wwnn/wwpn

[John Ferlan]

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

The existing code assumed that the configuration of a 'parent' attribute
was correct for the createVport path. As it turns out, that may not be
the case which leads errors during the deleteVport path because the
wwnn/wwpn isn't associated with the parent.

With this change the following is reported:

error: Failed to start pool fc_pool_host3
error: XML error: Parent attribute 'scsi_host4' does not match parent 
'scsi_host3' determined for the 'scsi_host16' wwnn/wwpn lookup.

for XML as follows:

  pool type='scsi'
namefc_pool/name
source
  adapter type='fc_host' parent='scsi_host4' wwnn='5001a4aaf3ca174b' 
wwpn='5001a4a77192b864'/
/source

Where 'nodedev-dumpxml scsi_host16' provides:

  device
namescsi_host16/name

path/sys/devices/pci:00/:00:04.0/:10:00.0/host3/vport-3:0-11/host16/path
parentscsi_host3/parent
capability type='scsi_host'
  host16/host
  unique_id13/unique_id
  capability type='fc_host'
wwnn5001a4aaf3ca174b/wwnn
wwpn5001a4a77192b864/wwpn
...

The patch also adjusts the description of the storage pool to describe the
restrictions.

Signed-off-by: John Ferlan jfer...@redhat.com
---
 docs/formatstorage.html.in |  15 -
 src/storage/storage_backend_scsi.c | 118 +++--
 2 files changed, 126 insertions(+), 7 deletions(-)

diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
index d3e6f05..9d77c45 100644
--- a/docs/formatstorage.html.in
+++ b/docs/formatstorage.html.in
@@ -166,7 +166,13 @@
 to uniquely identify the device in the Fibre Channel storage fabric
 (the device can be either a HBA or vHBA). Both wwnn and wwpn should
 be specified. Use the command 'virsh nodedev-dumpxml' to determine
-how to set the values for the wwnn/wwpn of a (v)HBA.
+how to set the values for the wwnn/wwpn of a (v)HBA. The wwnn and
+wwpn have very specific numerical format requirements based on the
+hypervisor being used, thus care should be taken if you decide to
+generate your own to follow the standards; otherwise, the pool
+will fail to start with an opaque error message indicating failure
+to write to the vport_create file during vport create/delete due
+to No such file or directory.
 span class=sinceSince 1.0.4/span
   /dd
 /dl
@@ -176,7 +182,12 @@
 parent scsi_host device defined in the
 a href=formatnode.htmlNode Device/a database as the
 a href=http://wiki.libvirt.org/page/NPIV_in_libvirt;NPIV/a
-virtual Host Bus Adapter (vHBA).
+virtual Host Bus Adapter (vHBA). The value provided must be
+a vport capable scsi_host. The value is not the scsi_host of
+the vHBA created by 'virsh nodedev-create', rather it is
+the parent of that vHBA. If the value is not provided, libvirt
+will determine the parent based either finding the wwnn,wwpn
+defined for an existing scsi_host or by creating a vHBA.
 span class=sinceSince 1.0.4/span
   /dd
 /dl
diff --git a/src/storage/storage_backend_scsi.c 
b/src/storage/storage_backend_scsi.c
index 549d8db..5220292 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -34,6 +34,7 @@
 #include virlog.h
 #include virfile.h
 #include vircommand.h
+#include viraccessapicheck.h
 #include virstring.h
 
 #define VIR_FROM_THIS VIR_FROM_STORAGE
@@ -547,8 +548,103 @@ getAdapterName(virStoragePoolSourceAdapter adapter)
 return name;
 }
 
+/*
+ * Using the host# name found via wwnn/wwpn lookup in the fc_host
+ * sysfs tree to get the parent 'scsi_host#'. On entry we need 'conn'
+ * set. We won't get here from the autostart path since the caller
+ * will return true before calling this function. For the shutdown
+ * path we won't be able to delete the vport.
+ */
+static char * ATTRIBUTE_NONNULL(1)
+getVhbaSCSIHostParent(virConnectPtr conn,
+  const char *name)
+{
+char *nodedev_name = NULL;
+virNodeDevicePtr device = NULL;
+char *xml = NULL;
+virNodeDeviceDefPtr def = NULL;
+char *vhba_parent = NULL;
+virErrorPtr savedError = NULL;
+
+VIR_DEBUG(conn=%p, name=%s, conn, name);
+
+/* We get passed host# from the return from virGetFCHostNameByWWN,
+ * so we need to adjust that to what the nodedev lookup expects
+ */
+if (virAsprintf(nodedev_name, scsi_%s, name)  0)
+goto cleanup;
+
+/* Compare the scsi_host for the name with the provided parent
+ * if not the same, then fail
+ */
+if (!(device = virNodeDeviceLookupByName(conn, nodedev_name))) {
+virReportError(VIR_ERR_XML_ERROR,
+   _(Cannot find '%s' in node device database),
+   

[libvirt] [PATCH v2 1/5] storage: Check for valid fc_host parent at startup

[John Ferlan]

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

If a 'parent' attribute is provided for the fchost, then at startup
time check to ensure it is a vport capable scsi_host. If the parent
is not vport capable, then disallow the startup. The following is the
expected results:

error: Failed to start pool fc_pool
error: XML error: parent 'scsi_host2' specified for vHBA is not vport capable

where the XML for the fc_pool is:

pool type='scsi'
  namefc_pool/name
  source
adapter type='fc_host' parent='scsi_host2' wwnn='5001a4aaf3ca174b' 
wwpn='5001a4a77192b864'/
  /source
...

and 'scsi_host2' is not vport capable.

Providing an incorrect parent and a correct wwnn/wwpn could lead to
failures at shutdown (deleteVport) where the assumption is the parent
is for the fchost.

NOTE: If the provided wwnn/wwpn doesn't resolve to an existing scsi_host,
  then we will be creating one with code (virManageVport) which
  assumes the parent is vport capable.

Signed-off-by: John Ferlan jfer...@redhat.com
---
 src/storage/storage_backend_scsi.c | 22 ++
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/src/storage/storage_backend_scsi.c 
b/src/storage/storage_backend_scsi.c
index 02160bc..549d8db 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -556,6 +556,20 @@ createVport(virStoragePoolSourceAdapter adapter)
 if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST)
 return 0;
 
+/* If a parent was provided, then let's make sure it's vhost capable */
+if (adapter.data.fchost.parent) {
+if (virGetSCSIHostNumber(adapter.data.fchost.parent, parent_host)  0)
+return -1;
+
+if (!virIsCapableFCHost(NULL, parent_host)) {
+virReportError(VIR_ERR_XML_ERROR,
+   _(parent '%s' specified for vHBA 
+ is not vport capable),
+   adapter.data.fchost.parent);
+return -1;
+}
+}
+
 /* This filters either HBA or already created vHBA */
 if ((name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn,
   adapter.data.fchost.wwpn))) {
@@ -565,14 +579,14 @@ createVport(virStoragePoolSourceAdapter adapter)
 
 if (!adapter.data.fchost.parent 
 !(adapter.data.fchost.parent = virFindFCHostCapableVport(NULL))) {
- virReportError(VIR_ERR_XML_ERROR, %s,
+virReportError(VIR_ERR_XML_ERROR, %s,
_('parent' for vHBA not specified, and 
  cannot find one on this host));
  return -1;
-}
 
-if (virGetSCSIHostNumber(adapter.data.fchost.parent, parent_host)  0)
-return -1;
+if (virGetSCSIHostNumber(adapter.data.fchost.parent, parent_host)  0)
+return -1;
+}
 
 if (virManageVport(parent_host, adapter.data.fchost.wwpn,
adapter.data.fchost.wwnn, VPORT_CREATE)  0)
-- 
1.9.3

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

[libvirt] [PATCH v2 0/5] Resolve fc_host startup, shutdown issues

[John Ferlan]

This series will replace:

http://www.redhat.com/archives/libvir-list/2014-November/msg00197.html

There are two bugs being fixed here and having them reviewed together
makes things easier in the long run as the last 3 patches depended upon
the first 2 being present.

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

Patch 1 is as was in the previous set
Patch 2 is essentially the same, except the single function
checkVhbaSCSIHostParent was split out to generate a
getVhbaSCSIHostParent which gets used later on.

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

Patch 3 fixes an issue which took a bit of gdb time in order
to figure out exactly what was going wrong. Essentially,
the exising createVport code had a path which would find
the best available fc_host to use and save the parent on
return so that the deleteVport code would delete the parent.
However, the code used a copy of the adapter not a reference
to the adapter and thus the change was lost leaving the vHBA
on the system.

Patch 4 adds a new function to save the StoragePool XML given a
configFile. This will be used in the next patch because
while having the in memory fchost copy updated does work -
if libvirtd dies in between we're back at square 1 reading
storage pool config files and not knowing whence we started.

Patch 5 adds a new managed attribute to the fchost XML. It does this
mainly to let the deleteVport know when it should delete the
self created vport.  Prior to this code, the reliance was that
we didn't have a parent provided. However, this causes an issue
where if someone used nodedev-create in order to create a vHBA
and then tried to use that vHBA in a storage pool we would delete
that vHBA when we were done, which may not be expected. Using
the managed attribute at creation time will let libvirt know
what to do in this case. 

NOTE: There's still one more issue in the code, but it's a bit trickier
to resolve. A libvirt created vport doesn't seem to want to find the LUN's
on the initial PoolRefresh that occurs during startup. However, if one
does a refresh immediately after starting the pool, the luns show up.
It seems perhaps there's some sort of locking issue that won't allow the
udevEventHandleCallback to 'add' the new device.

John Ferlan (5):
  storage: Check for valid fc_host parent at startup
  storage: Ensure fc_host parent matches wwnn/wwpn
  storage: Don't use a stack copy of the adapter
  storage: Introduce virStoragePoolSaveConfig
  storage: Introduce 'managed' for the fchost parent

 docs/formatstorage.html.in |  28 ++-
 docs/schemas/basictypes.rng|   5 +
 src/conf/storage_conf.c|  70 --
 src/conf/storage_conf.h|   4 +
 src/libvirt_private.syms   |   1 +
 src/storage/storage_backend_scsi.c | 237 ++---
 .../pool-scsi-type-fc-host-managed.xml |  15 ++
 .../pool-scsi-type-fc-host-managed.xml |  18 ++
 tests/storagepoolxml2xmltest.c |   1 +
 9 files changed, 323 insertions(+), 56 deletions(-)
 create mode 100644 
tests/storagepoolxml2xmlin/pool-scsi-type-fc-host-managed.xml
 create mode 100644 
tests/storagepoolxml2xmlout/pool-scsi-type-fc-host-managed.xml

-- 
1.9.3

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

Re: [libvirt] [PATCH] network: Add bandwidth support to ethernet interface

[Eric Blake]

On 11/10/2014 03:41 PM, Anirban Chakraborty wrote:

 BTW: it would be nice if you can version you patches. I mean, this is
 what, 4th or 5th version? Say that in subject explicitly please. You
 know, in the prefix: [PATCH v5] network: ...

Using 'git send-email -v5' will do that for you.

 
 I was doing it earlier and then dropped it. I¹ll resin the patch
 addressing all your comments and send it out. However, please let me know
 if I should move the above functions (virNetDevBandwidthSet etc.) in
 src/util/virnetdevbandwidth.* and add #include conf/domain_conf.h in
 virnetdevbandwidth.h file.

If it needs to reference structs defined in conf/, then the logical
place for the functions is in conf/ (possibly a new file).  That way, it
can still be shared between lxc and qemu.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

[libvirt] [PATCH] Fix invalid log, misused option types and a typo

[Hao Liu]

This patch fixes the following issues.

1)  When an invalid wwn is introduced, libvirt reports
Malformed wwn: %s. The template won't be replaced.

2)  target option for dompmsuspend and xml option for
save-image-define are required options and should use
VSH_OT_DATA instead of VSH_OT_STRING as an option type.

3)  A typo.

Signed-off-by: Hao Liu h...@redhat.com
---
 src/util/virutil.c   | 4 ++--
 tools/virsh-domain.c | 4 ++--
 tools/virsh-host.c   | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/util/virutil.c b/src/util/virutil.c
index 1116fda..c178515 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -1549,8 +1549,8 @@ virValidateWWN(const char *wwn)
 }
 
 if (i != 16 || p[i]) {
-virReportError(VIR_ERR_INTERNAL_ERROR, %s,
-   _(Malformed wwn: %s));
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _(Malformed wwn: %s), wwn);
 return false;
 }
 
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 13a904f..8e743f1 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -3228,7 +3228,7 @@ static const vshCmdOptDef opts_dom_pm_suspend[] = {
  .help = N_(duration in seconds)
 },
 {.name = target,
- .type = VSH_OT_STRING,
+ .type = VSH_OT_DATA,
  .flags = VSH_OFLAG_REQ,
  .help = N_(mem(Suspend-to-RAM), 
 disk(Suspend-to-Disk), 
@@ -4268,7 +4268,7 @@ static const vshCmdOptDef opts_save_image_define[] = {
  .help = N_(saved state file to modify)
 },
 {.name = xml,
- .type = VSH_OT_STRING,
+ .type = VSH_OT_DATA,
  .flags = VSH_OFLAG_REQ,
  .help = N_(filename containing updated XML for the target)
 },
diff --git a/tools/virsh-host.c b/tools/virsh-host.c
index 4a3ff28..28b160d 100644
--- a/tools/virsh-host.c
+++ b/tools/virsh-host.c
@@ -497,7 +497,7 @@ cmdAllocpages(vshControl *ctl, const vshCmd *cmd)
 pageSizes[0] = VIR_DIV_UP(tmp, 1024);
 
 if (vshCommandOptULongLong(cmd, pagecount, pageCounts[0])  0) {
-vshError(ctl, %s, _(pagecount hat to be a number));
+vshError(ctl, %s, _(pagecount has to be a number));
 return false;
 }
 
-- 
1.8.3.1

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

Re: [libvirt] [PATCH v3 0/2] Numa : Allow specification of memory units

[Prerna Saxena]

On Monday 10 November 2014 07:36 PM, Michal Privoznik wrote:
 On 10.11.2014 12:49, Prerna Saxena wrote:
 Present XML specification of NUMA node accepts memory in 'KiB' only.
 This series adds support for input of cell memory in units of choice.

 Description:
 ===
 Patch 1/2 : Export virDomainParseMemory for use outside domain_conf
 Patch 2/2 : Allow specification of 'units' for NUMA nodes.

 Reference:
 ==
 v1: http://www.spinics.net/linux/fedora/libvir/msg105516.html
 v2: http://www.mail-archive.com/libvir-list@redhat.com/msg104577.html

 Changes Since v2:
 ===
 * Patch 1 is newly introduced. It follows Michal's suggestion of reusing 
 virDomainParseMemory, exposed by 01b4de2b9f5ca82
 * Patch 2 : Now uses virDomainParseMemory()
 * Also, documentation in patch 2 is now fixed to link to memory unit 
 specification


 I've fixed the issues and pushed this.


Thanks,
I'll make sure I remember these points in the subsequent libvirt patches I send 
:-)

-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India

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