[libvirt] [PATCH] Add rlimits to lxc

2014-12-06 Thread Ryan Cleere 
---
 src/conf/domain_conf.c   | 115 +++
 src/conf/domain_conf.h   |  12 +
 src/lxc/lxc_controller.c |  12 +
 src/util/virprocess.c|   4 +-
 src/util/virprocess.h|   3 ++
 5 files changed, 144 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 2d81c37..a673dc2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -1003,7 +1004,86 @@ virDomainBlkioDeviceParseXML(xmlNodePtr root,
 return -1;
 }
 
+static virDomainRLimitsPtr
+virDomainRLimitsNew(void)
+{
+virDomainRLimitsPtr def = NULL;
+
+if (VIR_ALLOC(def) < 0)
+return NULL;
+
+return def;
+}
+
+static virDomainRLimitsPtr
+virDomainRLimitParseXML(xmlNodePtr node)
+{
+char *c = NULL;
+long long val;
+virDomainRLimitsPtr def;
 
+if (!(def = virDomainRLimitsNew()))
+return NULL;
+
+if (node->type == XML_ELEMENT_NODE) {
+c = (char *)xmlNodeGetContent(node);
+if (virStrToLong_ll(c, NULL, 10, &val) < 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("could not parse rlimit value of %s"),
+   c);
+goto error;
+}
+VIR_FREE(c);
+
+def->limit = val;
+if (VIR_STRDUP(def->name, (char *)node->name) < 0)
+goto error;
+
+if (xmlStrEqual(node->name, BAD_CAST "as")) {
+def->resource = RLIMIT_AS;
+} else if (xmlStrEqual(node->name, BAD_CAST "core")) {
+def->resource = RLIMIT_CORE;
+} else if (xmlStrEqual(node->name, BAD_CAST "cpu")) {
+def->resource = RLIMIT_CPU;
+} else if (xmlStrEqual(node->name, BAD_CAST "data")) {
+def->resource = RLIMIT_DATA;
+} else if (xmlStrEqual(node->name, BAD_CAST "fsize")) {
+def->resource = RLIMIT_FSIZE;
+} else if (xmlStrEqual(node->name, BAD_CAST "locks")) {
+def->resource = RLIMIT_LOCKS;
+} else if (xmlStrEqual(node->name, BAD_CAST "memlock")) {
+def->resource = RLIMIT_MEMLOCK;
+} else if (xmlStrEqual(node->name, BAD_CAST "msgqueue")) {
+def->resource = RLIMIT_MSGQUEUE;
+} else if (xmlStrEqual(node->name, BAD_CAST "nice")) {
+def->resource = RLIMIT_NICE;
+} else if (xmlStrEqual(node->name, BAD_CAST "nofile")) {
+def->resource = RLIMIT_NOFILE;
+} else if (xmlStrEqual(node->name, BAD_CAST "nproc")) {
+def->resource = RLIMIT_NPROC;
+} else if (xmlStrEqual(node->name, BAD_CAST "rss")) {
+def->resource = RLIMIT_RSS;
+} else if (xmlStrEqual(node->name, BAD_CAST "rtprio")) {
+def->resource = RLIMIT_RTPRIO;
+} else if (xmlStrEqual(node->name, BAD_CAST "sigpending")) {
+def->resource = RLIMIT_SIGPENDING;
+} else if (xmlStrEqual(node->name, BAD_CAST "stack")) {
+def->resource = RLIMIT_STACK;
+} else {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("could not determine resource type of '%s'"),
+   node->name);
+goto error;
+}
+}
+
+return def;
+
+ error:
+VIR_FREE(c);
+VIR_FREE(def);
+return NULL;
+}
 
 static void
 virDomainObjListDataFree(void *payload, const void *name ATTRIBUTE_UNUSED)
@@ -14180,6 +14260,28 @@ virDomainDefParseXML(xmlDocPtr xml,
 
 virHashFree(bootHash);
 
+if ((node = virXPathNode("./rlimits[1]", ctxt)) != NULL && (n = 
virXMLChildElementCount(node)) > 0) {
+xmlNodePtr cur = node->children;
+if (n && VIR_ALLOC_N(def->rlimits, n) < 0)
+goto error;
+
+for (i = 0; i < n; i++) {
+if (!(def->rlimits[i] = virDomainRLimitParseXML(cur)))
+goto error;
+def->nrlimits++;
+for (j = 0; j < i; j++) {
+if (def->rlimits[j]->resource == def->rlimits[i]->resource) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("duplicate rlimit resources '%s'"),
+   def->rlimits[j]->name);
+goto error;
+}
+}
+cur = cur->next;
+}
+}
+VIR_FREE(node);
+
 return def;
 
  error:
@@ -19759,6 +19861,19 @@ virDomainDefFormatInternal(virDomainDefPtr def,
 goto error;
 }
 
+if (def->nrlimits > 0) {
+virBufferAddLit(buf, "\n");
+virBufferAdjustIndent(buf, 2);
+for (n = 0; n < def->nrlimits; n++) {
+virBufferAsprintf(buf, "<%s>%lld\n",
+  def->rlimits[n]->name,
+  def->rlimits[n]->limit,
+  def->rlimits[n]->name);
+}
+virBufferAdjustIndent(buf, -2);
+virBuffer

Re: [libvirt] snapshot-create-as doesn't always work across libvirtd restarts

2014-12-06 Thread Shanzhi Yu 


On 12/06/2014 03:33 PM, Eric Blake wrote:

I just found out the hard way that we have a bug in snapshot-create-as,
when I corrupted a guest.  I was testing with an offline domain:

# virsh dumpxml win64 |grep -C1 driver
 
   
   
# virsh snapshot-create-as win64 --disk-only --diskspec
hda,file=/var/lib/libvirt/images/win64.qcow2 --no-metadata
Domain snapshot 1417850911 created
# virsh dumpxml win64 |grep -C1 driver
 
   
   
# systemctl restart libvirtd
# virsh dumpxml win64 |grep -C1 driver
 
   
   

Even though we don't want to save the snapshot metadata, we DO need to
save the XML change.  Otherwise, the restarted libvirtd sees the wrong
disk as the source; worse, if the user started the guest before
restarting libvirtd, they run the risk of accidentally reverting to the
pre-snapshot state and losing all changes they made in the meantime,
perhaps even getting their guest filesystem into an inconsistent state.

I'm out of time to get to a root cause or fix before next week, but
wanted to report it now.  I'm not sure if --no-metadata is essential to
reproducing the bug, or if it happens in all cases.


Hi Eric,
I can reproduce it without "--no-metadata" option. Can you give the bug 
number if you have file it so that QE can track it
I post a patch 
http://www.redhat.com/archives/libvir-list/2014-December/msg00369.html. 
It works for me after a simple test






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


--
Regards
shyu

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

[libvirt] [PATCH] qemu: snapshot: inactive external snapshot can't work after libvirtd restart

2014-12-06 Thread Shanzhi Yu 
When create inactive external snapshot, after update disk definitions,
virDomainSaveConfig is needed, if not after restart libvirtd the new snapshot
file definitions in xml will be lost.

Reproduce steps:

1. prepare a shut off guest
$ virsh domstate rhel7 && virsh domblklist rhel7
shut off

Target Source

vda/var/lib/libvirt/images/rhel7.img

2. create external disk snapshot
$ virsh snapshot-create rhel7 --disk-only && virsh domblklist rhel7
Domain snapshot 1417882967 created
Target Source

vda/var/lib/libvirt/images/rhel7.1417882967

3. restart libvirtd then check guest source file
$ service  libvirtd restart && virsh domblklist rhel7
Redirecting to /bin/systemctl restart  libvirtd.service
Target Source

vda/var/lib/libvirt/images/rhel7.img

This was first reported by Eric Blake
http://www.redhat.com/archives/libvir-list/2014-December/msg00369.html

Signed-off-by: Shanzhi Yu 
---
 src/qemu/qemu_driver.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9152cf5..9f8ea0a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12847,6 +12847,9 @@ 
qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver,
 goto cleanup;
 }
 defdisk->src->format = snapdisk->src->format;
+
+if (virDomainSaveConfig(cfg->configDir, vm->def) < 0)
+goto cleanup;
 }
 }
 
-- 
2.1.0

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

Re: [libvirt] [PATCH v3 2/2] qemu: Automaticly create tap device for VIR_DOMAIN_NET_TYPE_ETHERNET

2014-12-06 Thread Vasiliy Tolstov 
2014-12-06 2:37 GMT+03:00 Anirban Chakraborty :
> In Open Contrail (www.opencontrail.org), we use this feature where tap
> interface is created first, so that we know the name of the tap device a
> priori, before creating the vm. So, this patch will break existing code.


May be we can add some flag like managed true/false ? and create tap
only if managed ?

-- 
Vasiliy Tolstov,
e-mail: v.tols...@selfip.ru
jabber: v...@selfip.ru

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

Re: [libvirt] [PATCH] docs: Correct invalid hyperlinks

2014-12-06 Thread Martin Kletzander 

On Fri, Dec 05, 2014 at 09:55:00AM -0700, Eric Blake wrote:

On 12/05/2014 03:20 AM, Martin Kletzander wrote:

On Fri, Dec 05, 2014 at 09:47:47AM +, Ian Campbell wrote:

On Tue, 2014-12-02 at 07:50 +0100, Martin Kletzander wrote:

That said, I found out that that XML-XPath is only needed when
creating the tarball, but (probably) not when building an RPM with it.


It's needed when building from the git tree to, Xen automated testing
picked up on this overnight:
http://www.chiark.greenend.org.uk/~xensrcts/logs/32083/build-amd64-libvirt/5.ts-libvirt-build.log


I'm not sure if things which are only needed to build the tarball are
supposed to be checked for by configure or not.



We didn't check for XHTML DTDs either, so I don't think so, but OTOH
I'm fine with adding that there if majority agrees.


Ideally, something that is required for building 'make dist' or 'make
rpm' should be required by bootstrap (autogen.sh uses bootstrap.conf to
require minimum versions of maintainer-only tools); but if they are
optional when building from a tarball, then they should not be required
by configure (but configure should still check for them, and we should
do a better job of cleanly skipping rather than erroring if an optional
build component is not present when building from tarball).



How can you add a perl module in "buildreq"?  I'm not familiar with how
bootstrap works.

Martin


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

[libvirt] [PATCH 11/12] getstats: split block stats reporting for easier recursion

2014-12-06 Thread Eric Blake 
In order to report stats on backing chains, we need to separate
the output of stats for one block from how we traverse blocks.

* src/qemu/qemu_driver.c (qemuDomainGetStatsBlock): Split...
(qemuDomainGetStatsOneBlock): ...into new helper.

Signed-off-by: Eric Blake 
---
 src/qemu/qemu_driver.c | 127 ++---
 1 file changed, 77 insertions(+), 50 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index cb80f49..feaa4a2 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18529,6 +18529,79 @@ do { \
 goto cleanup; \
 } while (0)

+
+static int
+qemuDomainGetStatsOneBlock(virQEMUDriverPtr driver,
+   virQEMUDriverConfigPtr cfg,
+   virDomainObjPtr dom,
+   virDomainStatsRecordPtr record,
+   int *maxparams,
+   virDomainDiskDefPtr disk,
+   virStorageSourcePtr src,
+   size_t block_idx,
+   bool abbreviated,
+   virHashTablePtr stats)
+{
+qemuBlockStats *entry;
+int ret = -1;
+
+QEMU_ADD_NAME_PARAM(record, maxparams, "block", "name", block_idx,
+disk->dst);
+if (virStorageSourceIsLocalStorage(src) && src->path)
+QEMU_ADD_NAME_PARAM(record, maxparams, "block", "path",
+block_idx, src->path);
+
+if (abbreviated || !disk->info.alias ||
+!(entry = virHashLookup(stats, disk->info.alias))) {
+if (qemuStorageLimitsRefresh(driver, cfg, dom,
+ disk, src, NULL, NULL) < 0)
+goto cleanup;
+if (src->allocation)
+QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx,
+ "allocation", src->allocation);
+if (src->capacity)
+QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx,
+ "capacity", src->capacity);
+if (src->physical)
+QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx,
+ "physical", src->physical);
+ret = 0;
+goto cleanup;
+}
+
+QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
+"rd.reqs", entry->rd_req);
+QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
+"rd.bytes", entry->rd_bytes);
+QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
+"rd.times", entry->rd_total_times);
+QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
+"wr.reqs", entry->wr_req);
+QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
+"wr.bytes", entry->wr_bytes);
+QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
+"wr.times", entry->wr_total_times);
+QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
+"fl.reqs", entry->flush_req);
+QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
+"fl.times", entry->flush_total_times);
+
+QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx,
+ "allocation", entry->wr_highest_offset);
+
+if (entry->capacity)
+QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx,
+ "capacity", entry->capacity);
+if (entry->physical)
+QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx,
+ "physical", entry->physical);
+
+ret = 0;
+ cleanup:
+return ret;
+}
+
+
 static int
 qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
 virDomainObjPtr dom,
@@ -18566,58 +18639,12 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
 QEMU_ADD_COUNT_PARAM(record, maxparams, "block", 0);

 for (i = 0; i < dom->def->ndisks; i++) {
-qemuBlockStats *entry;
 virDomainDiskDefPtr disk = dom->def->disks[i];

-QEMU_ADD_NAME_PARAM(record, maxparams, "block", "name", i, disk->dst);
-if (virStorageSourceIsLocalStorage(disk->src) && disk->src->path)
-QEMU_ADD_NAME_PARAM(record, maxparams, "block", "path",
-i, disk->src->path);
-
-if (abbreviated || !disk->info.alias ||
-!(entry = virHashLookup(stats, disk->info.alias))) {
-if (qemuStorageLimitsRefresh(driver, cfg, dom,
- disk, disk->src, NULL, NULL) < 0)
-goto cleanup;
-if (disk->src->allocation)
-QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, i,
- "allocation", disk->src->allocation);
-if (disk->src->capacity)
-QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, i,
-

[libvirt] [PATCH 12/12] getstats: start crawling backing chain for qemu

2014-12-06 Thread Eric Blake 
Wire up backing chain recursion.  Note that for now, we just use
the same allocation numbers for read-only backing files as what
offline domains would report.  It is not the correct allocation
number for qcow2 over block devices during block-commit, and it
misses out on the fact that qemu also reports read statistics on
backing files that are worth knowing about (seriously - for a
thin-provisioned setup, it would be nice to easily get at a count
of how many reads were serviced from the backing file in relation
to reads serviced by the active layer).  But it is at least
sufficient to prove that the algorithm is working, and to let
other people start coding to the interface while waiting for
later patches that get the correct information.

For a running domain, where one of the two images has a backing
file, I see the traditional output:

$ virsh domstats --block testvm2
Domain: 'testvm2'
  block.count=2
  block.0.name=vda
  block.0.path=/tmp/wrapper.qcow2
  block.0.rd.reqs=1
  block.0.rd.bytes=512
  block.0.rd.times=28858
  block.0.wr.reqs=0
  block.0.wr.bytes=0
  block.0.wr.times=0
  block.0.fl.reqs=0
  block.0.fl.times=0
  block.0.allocation=0
  block.0.capacity=131072
  block.0.physical=200704
  block.1.name=vdb
  block.1.path=/dev/sda7
  block.1.rd.reqs=0
  block.1.rd.bytes=0
  block.1.rd.times=0
  block.1.wr.reqs=0
  block.1.wr.bytes=0
  block.1.wr.times=0
  block.1.fl.reqs=0
  block.1.fl.times=0
  block.1.allocation=0
  block.1.capacity=131072

vs. the new output:

$ virsh domstats --block --backing testvm2
Domain: 'testvm2'
  block.count=3
  block.0.name=vda
  block.0.path=/tmp/wrapper.qcow2
  block.0.rd.reqs=1
  block.0.rd.bytes=512
  block.0.rd.times=28858
  block.0.wr.reqs=0
  block.0.wr.bytes=0
  block.0.wr.times=0
  block.0.fl.reqs=0
  block.0.fl.times=0
  block.0.allocation=0
  block.0.capacity=131072
  block.0.physical=200704
  block.1.name=vda
  block.1.path=/dev/sda6
  block.1.backingIndex=1
  block.1.allocation=1073741824
  block.1.capacity=131072
  block.1.physical=1073741824
  block.2.name=vdb
  block.2.path=/dev/sda7
  block.2.rd.reqs=0
  block.2.rd.bytes=0
  block.2.rd.times=0
  block.2.wr.reqs=0
  block.2.wr.bytes=0
  block.2.wr.times=0
  block.2.fl.reqs=0
  block.2.fl.times=0
  block.2.allocation=0
  block.2.capacity=131072

* src/qemu/qemu_driver.c (QEMU_DOMAIN_STATS_BACKING): New internal
enum bit.
(qemuConnectGetAllDomainStats): Recognize new user flag, and pass
details to...
(qemuDomainGetStatsBlock): ...here, where we can do longer recursion.
(qemuDomainGetStatsOneBlock): Output new field.

Signed-off-by: Eric Blake 
---
 src/qemu/qemu_driver.c | 55 +-
 1 file changed, 46 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index feaa4a2..b57beeb 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18256,8 +18256,10 @@ qemuDomainGetStatsState(virQEMUDriverPtr driver 
ATTRIBUTE_UNUSED,


 typedef enum {
-QEMU_DOMAIN_STATS_HAVE_JOB = (1 << 0), /* job is entered, monitor can be
-  accessed */
+QEMU_DOMAIN_STATS_HAVE_JOB = 1 << 0, /* job is entered, monitor can be
+accessed */
+QEMU_DOMAIN_STATS_BACKING  = 1 << 1, /* include backing chain in
+block stats */
 } qemuDomainStatsFlags;


@@ -18502,6 +18504,19 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver 
ATTRIBUTE_UNUSED,

 #undef QEMU_ADD_NET_PARAM

+#define QEMU_ADD_BLOCK_PARAM_UI(record, maxparams, num, name, value) \
+do { \
+char param_name[VIR_TYPED_PARAM_FIELD_LENGTH];   \
+snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,   \
+ "block.%zu.%s", num, name); \
+if (virTypedParamsAddUInt(&(record)->params, \
+  &(record)->nparams,\
+  maxparams, \
+  param_name,\
+  value) < 0)\
+goto cleanup;\
+} while (0)
+
 /* expects a LL, but typed parameter must be ULL */
 #define QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, num, name, value) \
 do { \
@@ -18539,6 +18554,7 @@ qemuDomainGetStatsOneBlock(virQEMUDriverPtr driver,
virDomainDiskDefPtr disk,
virStorageSourcePtr src,
size_t block_idx,
+   unsigned int backing_idx,
bool abbreviated,
virHashTablePtr stats)
 {
@@ -18550,8 +18566,16 @@ qemuDomainGetStatsOneBlock(virQEMUDriverPtr driver,
 if (virStorageSourceIsLocalStorage

[libvirt] [PATCH 07/12] getstats: report block sizes for offline domains

2014-12-06 Thread Eric Blake 
The prior refactoring can now be put to use. With the same domain
as the previous commit (one qcow2 disk and an empty cdrom drive):
$ virsh domstats --block foo
Domain: 'foo'
  block.count=2
  block.0.name=hda
  block.0.allocation=200704
  block.0.capacity=42949672960
  block.0.physical=200704
  block.1.name=hdc

* src/qemu/qemu_driver.c (qemuStorageLimitsRefresh): Tweak
semantics of helper function.
(qemuDomainGetStatsBlock): Use it to report offline statistics.

Signed-off-by: Eric Blake 
---
 src/qemu/qemu_driver.c | 60 +++---
 1 file changed, 42 insertions(+), 18 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 404decd..723391b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10983,7 +10983,9 @@ qemuDomainMemoryPeek(virDomainPtr dom,

 /* Refresh the capacity and allocation limits of a given storage
  * source.  Assumes that the caller has already obtained a domain job.
- * Set *activeFail to true if data cannot be obtained because a
+ * Pass NULL for path to skip any errors about an empty drive.
+ * Pass NULL for activeFail to skip any monitor attempt, otherwise,
+ * set *activeFail to true if data cannot be obtained because a
  * transient guest is no longer active.  */
 static int
 qemuStorageLimitsRefresh(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg,
@@ -11000,6 +11002,12 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver, 
virQEMUDriverConfigPtr cfg,
 char *buf = NULL;
 ssize_t len;

+if (!path) {
+if (virStorageSourceIsEmpty(src))
+return 0;
+path = src->path;
+}
+
 /* FIXME: For an offline domain, we always want to check current
  * on-disk statistics (as users have been known to change offline
  * images behind our backs).  For a running domain, however, it
@@ -6,23 +11124,27 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver, 
virQEMUDriverConfigPtr cfg,
 if (virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_BLOCK &&
 format != VIR_STORAGE_FILE_RAW &&
 S_ISBLK(sb.st_mode)) {
-qemuDomainObjPrivatePtr priv = vm->privateData;
-
-/* If the guest is not running, then success/failure return
- * depends on whether domain is persistent
- */
-if (!virDomainObjIsActive(vm)) {
-*activeFail = true;
+if (!activeFail) {
+disk->src->allocation = 0;
 ret = 0;
-goto cleanup;
+} else {
+qemuDomainObjPrivatePtr priv = vm->privateData;
+
+/* If the guest is not running, then success/failure return
+ * depends on whether domain is persistent
+ */
+if (!virDomainObjIsActive(vm)) {
+*activeFail = true;
+ret = 0;
+goto cleanup;
+}
+
+qemuDomainObjEnterMonitor(driver, vm);
+ret = qemuMonitorGetBlockExtent(priv->mon,
+disk->info.alias,
+&src->allocation);
+qemuDomainObjExitMonitor(driver, vm);
 }
-
-qemuDomainObjEnterMonitor(driver, vm);
-ret = qemuMonitorGetBlockExtent(priv->mon,
-disk->info.alias,
-&src->allocation);
-qemuDomainObjExitMonitor(driver, vm);
-
 } else {
 ret = 0;
 }
@@ -18530,6 +18542,7 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
 virHashTablePtr stats = NULL;
 qemuDomainObjPrivatePtr priv = dom->privateData;
 bool abbreviated = false;
+virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);

 if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) {
 abbreviated = true; /* it's ok, just go ahead silently */
@@ -18555,8 +18568,18 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,

 if (abbreviated || !disk->info.alias ||
 !(entry = virHashLookup(stats, disk->info.alias))) {
-/* FIXME: we could still look up sizing by sharing code
- * with qemuDomainGetBlockInfo */
+if (qemuStorageLimitsRefresh(driver, cfg, dom,
+ disk, disk->src, NULL, NULL) < 0)
+goto cleanup;
+if (disk->src->allocation)
+QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, i,
+ "allocation", disk->src->allocation);
+if (disk->src->capacity)
+QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, i,
+ "capacity", disk->src->capacity);
+if (disk->src->physical)
+QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, i,
+ "physical", disk->src->physical);
 continue;
 }

@@ -18593,6 +18616,7 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,

[libvirt] [PATCH 09/12] getstats: prepare for dynamic block.count stat

2014-12-06 Thread Eric Blake 
A coming patch will make it optionally possible to list backing
chain block stats; in this mode of operation, block.counts is no
longer the number of  in the domain, but the number of
blocks in the array being reported.  We still want block.count
listed first, but rather than iterate the tree twice (once to
count, and once to list stats), it's easier to just touch things
up after the fact.

* src/qemu/qemu_driver.c (qemuDomainGetStatsBlock): Compute count
after the fact.

Signed-off-by: Eric Blake 
---
 src/qemu/qemu_driver.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5334f8d..cb80f49 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18543,6 +18543,7 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
 qemuDomainObjPrivatePtr priv = dom->privateData;
 bool abbreviated = false;
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+int count_index = -1;

 if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) {
 abbreviated = true; /* it's ok, just go ahead silently */
@@ -18558,7 +18559,11 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
 }
 }

-QEMU_ADD_COUNT_PARAM(record, maxparams, "block", dom->def->ndisks);
+/* When listing backing chains, it's easier to fix up the count
+ * after the iteration than it is to iterate twice; but we still
+ * want count listed first.  */
+count_index = record->nparams;
+QEMU_ADD_COUNT_PARAM(record, maxparams, "block", 0);

 for (i = 0; i < dom->def->ndisks; i++) {
 qemuBlockStats *entry;
@@ -18618,6 +18623,8 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
 ret = 0;

  cleanup:
+if (count_index >= 0)
+record->params[count_index].value.ui = i;
 virHashFree(stats);
 virObjectUnref(cfg);
 return ret;
-- 
1.9.3

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

[libvirt] [PATCH 05/12] qemu: refactor blockinfo data gathering

2014-12-06 Thread Eric Blake 
Create a helper function that can be reused for gathering block
info from virDomainListGetStats.

* src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Split guts...
(qemuStorageLimitsRefresh): ...into new helper function.

Signed-off-by: Eric Blake 
---
 src/qemu/qemu_driver.c | 196 ++---
 1 file changed, 105 insertions(+), 91 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e873362..1e254bc 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10981,59 +10981,25 @@ qemuDomainMemoryPeek(virDomainPtr dom,
 }


+/* Refresh the capacity and allocation limits of a given storage
+ * source.  Assumes that the caller has already obtained a domain job.
+ * Set *activeFail to true if data cannot be obtained because a
+ * transient guest is no longer active.  */
 static int
-qemuDomainGetBlockInfo(virDomainPtr dom,
-   const char *path,
-   virDomainBlockInfoPtr info,
-   unsigned int flags)
+qemuStorageLimitsRefresh(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg,
+ virDomainObjPtr vm, virDomainDiskDefPtr disk,
+ virStorageSourcePtr src, const char *path,
+ bool *activeFail)
 {
-virQEMUDriverPtr driver = dom->conn->privateData;
-virDomainObjPtr vm;
 int ret = -1;
 int fd = -1;
 off_t end;
 virStorageSourcePtr meta = NULL;
-virDomainDiskDefPtr disk = NULL;
 struct stat sb;
-int idx;
-int format;
-bool activeFail = false;
-virQEMUDriverConfigPtr cfg = NULL;
+int format = src->format;
 char *buf = NULL;
 ssize_t len;

-virCheckFlags(0, -1);
-
-if (!(vm = qemuDomObjFromDomain(dom)))
-return -1;
-
-cfg = virQEMUDriverGetConfig(driver);
-
-if (virDomainGetBlockInfoEnsureACL(dom->conn, vm->def) < 0)
-goto cleanup;
-
-if (!path || path[0] == '\0') {
-virReportError(VIR_ERR_INVALID_ARG, "%s", _("NULL or empty path"));
-goto cleanup;
-}
-
-/* Technically, we only need a job if we are going to query the
- * monitor, which is only for active domains that are using
- * non-raw block devices.  But it is easier to share code if we
- * always grab a job; furthermore, grabbing the job ensures that
- * hot-plug won't change disk behind our backs.  */
-if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
-goto cleanup;
-
-/* Check the path belongs to this domain. */
-if ((idx = virDomainDiskIndexByName(vm->def, path, false)) < 0) {
-virReportError(VIR_ERR_INVALID_ARG,
-   _("invalid path %s not assigned to domain"), path);
-goto endjob;
-}
-
-disk = vm->def->disks[idx];
-
 /* FIXME: For an offline domain, we always want to check current
  * on-disk statistics (as users have been known to change offline
  * images behind our backs).  For a running domain, however, it
@@ -11054,76 +11020,74 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
  * punching holes), and physical size of a non-raw file can
  * change.
  */
-if (virStorageSourceIsLocalStorage(disk->src)) {
-if (!disk->src->path) {
+if (virStorageSourceIsLocalStorage(src)) {
+if (!src->path) {
 virReportError(VIR_ERR_INVALID_ARG,
_("disk '%s' does not currently have a source 
assigned"),
path);
-goto endjob;
+goto cleanup;
 }

-if ((fd = qemuOpenFile(driver, vm, disk->src->path, O_RDONLY,
+if ((fd = qemuOpenFile(driver, vm, src->path, O_RDONLY,
NULL, NULL)) == -1)
-goto endjob;
+goto cleanup;

 if (fstat(fd, &sb) < 0) {
 virReportSystemError(errno,
- _("cannot stat file '%s'"), disk->src->path);
-goto endjob;
+ _("cannot stat file '%s'"), src->path);
+goto cleanup;
 }

 if ((len = virFileReadHeaderFD(fd, VIR_STORAGE_MAX_HEADER, &buf)) < 0) 
{
 virReportSystemError(errno, _("cannot read header '%s'"),
- disk->src->path);
-goto endjob;
+ src->path);
+goto cleanup;
 }
 } else {
-if (virStorageFileInitAs(disk->src, cfg->user, cfg->group) < 0)
-goto endjob;
+if (virStorageFileInitAs(src, cfg->user, cfg->group) < 0)
+goto cleanup;

-if ((len = virStorageFileReadHeader(disk->src, VIR_STORAGE_MAX_HEADER,
+if ((len = virStorageFileReadHeader(src, VIR_STORAGE_MAX_HEADER,
 &buf)) < 0)
-goto endjob;
+goto cleanup;

-if (virStorageFileStat(disk->src, &sb) < 0) {
+if (virStorageFileStat(src

[libvirt] [PATCH 00/12] more progress towards backing chain allocation stats

2014-12-06 Thread Eric Blake 
This is in response to my earlier v2 patch attempt at exposing
qcow2 allocation watermarks during a block commit (although that
series attempted to expand virDomainGetXMLDesc, while this one
expands virDomainListGetStats instead):
https://www.redhat.com/archives/libvir-list/2014-November/msg00589.html

It is also a followup to my preliminiaries (where I had review
comments to split patch 2/2, now 3 separate patches in this series):
https://www.redhat.com/archives/libvir-list/2014-November/msg00932.html

There's enough new content here that I didn't know whether to call
this a v3 of the original approach, but as the original approach was
rejected on design without much review, it feels like this is a new
series, even if a couple of patches from the v2 series ended up in
this series in a slightly different form.

See patch 12 for a sample of the planned output; I still have a few
more patches to polish to properly get qemu JSON output wired into
the command for correct values, but this should at least be sufficient
to write up client code that interacts with the interface.

The patches can also be grabbed from:
git fetch git://repo.or.cz/libvirt/ericb.git getstats

or browsed at:
http://repo.or.cz/w/libvirt/ericb.git/tree/refs/heads/getstats

Eric Blake (12):
  getstats: avoid memory leak on OOM
  getstats: improve documentation
  qemu: refactor blockinfo job handling
  qemu: let blockinfo reuse virStorageSource
  qemu: refactor blockinfo data gathering
  getstats: start giving offline block stats
  getstats: report block sizes for offline domains
  getstats: add block.n.path stat
  getstats: prepare for dynamic block.count stat
  getstats: add new flag for block backing chain
  getstats: split block stats reporting for easier recursion
  getstats: start crawling backing chain for qemu

 include/libvirt/libvirt-domain.h |   1 +
 src/libvirt-domain.c |  24 ++-
 src/qemu/qemu_driver.c   | 415 ++-
 src/util/virstoragefile.h|   3 +-
 tools/virsh-domain-monitor.c |   7 +
 tools/virsh.pod  |  10 +-
 6 files changed, 315 insertions(+), 145 deletions(-)

-- 
1.9.3

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

[libvirt] [PATCH 08/12] getstats: add block.n.path stat

2014-12-06 Thread Eric Blake 
I'm about to make block stats optionally more complex to cover
backing chains, where block.count will no longer equal the number
of  for a domain.  For these reasons, it is nicer if the
statistics output includes the source path (for local files).
This patch doesn't add anything for network disks, although we
may decide to add that later.

With this patch, I now see the following for the same domain as
in earlier patches (one qcow2 file, and an empty cdrom drive):
$ virsh domstats --block foo
Domain: 'foo'
  block.count=2
  block.0.name=hda
  block.0.path=/var/lib/libvirt/images/foo.qcow2
  block.0.allocation=200704
  block.0.capacity=42949672960
  block.0.physical=200704
  block.1.name=hdc

* src/libvirt-domain.c (virConnectGetAllDomainStats): Document
new field.
* tools/virsh.pod (domstats): Document new field.
* src/qemu/qemu_driver.c (qemuDomainGetStatsBlock): Return the new
stat for local files/block devices.
(QEMU_ADD_NAME_PARAM): Add parameter.
(qemuDomainGetStatsInterface): Update caller.

Signed-off-by: Eric Blake 
---
 src/libvirt-domain.c   |  3 +++
 src/qemu/qemu_driver.c | 11 +++
 tools/virsh.pod|  2 ++
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 87123d1..cb76d8c 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -10910,6 +10910,9 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
  * "block..name" - name of the block device  as string.
  *  matches the target name (vda/sda/hda) of the
  *  block device.
+ * "block..path" - string describing the source of block device ,
+ *  if it is a file or block device (omitted for network
+ *  sources and drives with no media inserted).
  * "block..rd.reqs" - number of read requests as unsigned long long.
  * "block..rd.bytes" - number of read bytes as unsigned long long.
  * "block..rd.times" - total time (ns) spent on reads as
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 723391b..5334f8d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18422,11 +18422,11 @@ do { \
 goto cleanup; \
 } while (0)

-#define QEMU_ADD_NAME_PARAM(record, maxparams, type, num, name) \
+#define QEMU_ADD_NAME_PARAM(record, maxparams, type, subtype, num, name) \
 do { \
 char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
 snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
- "%s.%zu.name", type, num); \
+ "%s.%zu.%s", type, num, subtype); \
 if (virTypedParamsAddString(&(record)->params, \
 &(record)->nparams, \
 maxparams, \
@@ -18471,7 +18471,7 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver 
ATTRIBUTE_UNUSED,
 memset(&tmp, 0, sizeof(tmp));

 QEMU_ADD_NAME_PARAM(record, maxparams,
-"net", i, dom->def->nets[i]->ifname);
+"net", "name", i, dom->def->nets[i]->ifname);

 if (virNetInterfaceStats(dom->def->nets[i]->ifname, &tmp) < 0) {
 virResetLastError();
@@ -18564,7 +18564,10 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
 qemuBlockStats *entry;
 virDomainDiskDefPtr disk = dom->def->disks[i];

-QEMU_ADD_NAME_PARAM(record, maxparams, "block", i, disk->dst);
+QEMU_ADD_NAME_PARAM(record, maxparams, "block", "name", i, disk->dst);
+if (virStorageSourceIsLocalStorage(disk->src) && disk->src->path)
+QEMU_ADD_NAME_PARAM(record, maxparams, "block", "path",
+i, disk->src->path);

 if (abbreviated || !disk->info.alias ||
 !(entry = virHashLookup(stats, disk->info.alias))) {
diff --git a/tools/virsh.pod b/tools/virsh.pod
index c070261..cbd82275 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -883,6 +883,8 @@ I<--interface> returns:
 I<--block> returns:
 "block.count" - number of block devices on this domain,
 "block..name" - name of the target of the block device ,
+"block..path" - file source of block device , if it is a
+local file or block device,
 "block..rd.reqs" - number of read requests,
 "block..rd.bytes" - number of read bytes,
 "block..rd.times" - total time (ns) spent on reads,
-- 
1.9.3

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

[libvirt] [PATCH 10/12] getstats: add new flag for block backing chain

2014-12-06 Thread Eric Blake 
This patch introduces access to allocation information about
a backing chain of a live domain.  While querying storage
volumes for read-only disks could provide some of the details,
there is one case where we have to rely on qemu: when doing
a block commit into a backing file, where that file is stored
in qcow2 format on a host block device, we want to know the
current highest write offset into that image, in order to know
if the disk must be resized larger.  qemu-img does not
(currently) show this information, and none of the earlier
block APIs were extensible enough to expose it.  But
virDomainListGetStats is perfect for the job!

We don't need a new group of statistics, as the existing block
group is sufficient.  On the other hand, as existing libvirt
releases already report 1:1 mapping of block.count to 
devices, changing the array size could confuse older clients;
and even with newer clients, the time and memory taken to
report additional statistics is not always necessary (backing
files are generally read-only except for block-commit, so while
read statistics may change, sizing statistics will not).  So
the choice here is to add a new flag that only newer callers
will pass, when they are prepared for the additional information.

This patch introduces the new API, but it will take more
patches to get it implemented for qemu.

* include/libvirt/libvirt-domain.h
(VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING): New flag.
* src/libvirt-domain.c (virConnectGetAllDomainStats): Document it,
and add a new field when it is in use.
* tools/virsh-domain-monitor.c (cmdDomstats): Use new flag.
* tools/virsh.pod (domstats): Document it.

Signed-off-by: Eric Blake 
---
 include/libvirt/libvirt-domain.h | 1 +
 src/libvirt-domain.c | 7 ++-
 tools/virsh-domain-monitor.c | 7 +++
 tools/virsh.pod  | 8 ++--
 4 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index ae2c49c..e6185b9 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1698,6 +1698,7 @@ typedef enum {
 VIR_CONNECT_GET_ALL_DOMAINS_STATS_SHUTOFF = 
VIR_CONNECT_LIST_DOMAINS_SHUTOFF,
 VIR_CONNECT_GET_ALL_DOMAINS_STATS_OTHER = VIR_CONNECT_LIST_DOMAINS_OTHER,

+VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING = 1 << 30, /* include backing 
chain for block stats */
 VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS = 1 << 31, /* enforce 
requested stats */
 } virConnectGetAllDomainStatsFlags;

diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index cb76d8c..8c4ad7b 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -10903,13 +10903,18 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
  * "net..tx.errs" - transmission errors as unsigned long long.
  * "net..tx.drop" - transmit packets dropped as unsigned long long.
  *
- * VIR_DOMAIN_STATS_BLOCK: Return block devices statistics.
+ * VIR_DOMAIN_STATS_BLOCK: Return block devices statistics.  By default,
+ * this information is limited to the active layer of each  of the
+ * domain, but adding VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING to @flags
+ * will expand the array to cover backing chains.
  * The typed parameter keys are in this format:
  * "block.count" - number of block devices on this domain
  * as unsigned int.
  * "block..name" - name of the block device  as string.
  *  matches the target name (vda/sda/hda) of the
  *  block device.
+ * "block..backingIndex" - unsigned int giving the  index,
+ *  when backing images are listed.
  * "block..path" - string describing the source of block device ,
  *  if it is a file or block device (omitted for network
  *  sources and drives with no media inserted).
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index 259400f..214c0b2 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -2037,6 +2037,10 @@ static const vshCmdOptDef opts_domstats[] = {
  .type = VSH_OT_BOOL,
  .help = N_("enforce requested stats parameters"),
 },
+{.name = "backing",
+ .type = VSH_OT_BOOL,
+ .help = N_("add backing chain information to block stats"),
+},
 {.name = "domain",
  .type = VSH_OT_ARGV,
  .flags = VSH_OFLAG_NONE,
@@ -2130,6 +2134,9 @@ cmdDomstats(vshControl *ctl, const vshCmd *cmd)
 if (vshCommandOptBool(cmd, "enforce"))
 flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS;

+if (vshCommandOptBool(cmd, "backing"))
+flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING;
+
 if (vshCommandOptBool(cmd, "domain")) {
 if (VIR_ALLOC_N(domlist, 1) < 0)
 goto cleanup;
diff --git a/tools/virsh.pod b/tools/virsh.pod
index cbd82275..378f1c0 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -825,7 +825,7 @@ that require a block device name (suc

[libvirt] [PATCH 02/12] getstats: improve documentation

2014-12-06 Thread Eric Blake 
At least with 'virsh domstats --block' on an offline domain, we
currently output no stats even though we recognize the stat
category.  Although a later patch will improve this situation,
it is better to document that this is expected behavior.

Also, while the current implementation rejects filtering flags
for virDomainListGetStats, this limitation may be lifted in the
future and we do not enforce it at the API level.

* src/libvirt-domain.c (virConnectGetAllDomainStats): Document
that recognized stats might not be reported.
(virDomainListGetStats): Likewise, and tweak filtering documentation.

Signed-off-by: Eric Blake 
---
 src/libvirt-domain.c | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 8333183..87123d1 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -10940,7 +10940,11 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
  *
  * Specifying VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS as @flags makes
  * the function return error in case some of the stat types in @stats were
- * not recognized by the daemon.
+ * not recognized by the daemon.  However, even with this flag, a hypervisor
+ * may omit individual fields within a known group if the information is not
+ * available; as an extreme example, a supported group may produce zero
+ * fields for offline domains if the statistics are meaningful only for a
+ * running domain.
  *
  * Similarly to virConnectListAllDomains, @flags can contain various flags to
  * filter the list of domains to provide stats for.
@@ -11020,9 +11024,13 @@ virConnectGetAllDomainStats(virConnectPtr conn,
  *
  * Specifying VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS as @flags makes
  * the function return error in case some of the stat types in @stats were
- * not recognized by the daemon.
+ * not recognized by the daemon.  However, even with this flag, a hypervisor
+ * may omit individual fields within a known group if the information is not
+ * available; as an extreme example, a supported group may produce zero
+ * fields for offline domains if the statistics are meaningful only for a
+ * running domain.
  *
- * Note that any of the domain list filtering flags in @flags will be rejected
+ * Note that any of the domain list filtering flags in @flags may be rejected
  * by this function.
  *
  * Returns the count of returned statistics structures on success, -1 on error.
-- 
1.9.3

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

[libvirt] [PATCH 01/12] getstats: avoid memory leak on OOM

2014-12-06 Thread Eric Blake 
qemuDomainGetStatsBlock() could leak a stats hash table if it
encountered OOM while populating the virTypedParameters.
Oddly, the fix doesn't even touch qemuDomainGetStatsBlock :)

* src/qemu/qemu_driver.c (QEMU_ADD_COUNT_PARAM)
(QEMU_ADD_NAME_PARAM): Don't return early.
(qemuDomainGetStatsInterface): Adjust caller.

Signed-off-by: Eric Blake 
---
 src/qemu/qemu_driver.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9152cf5..a7b208f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18371,7 +18371,7 @@ do { \
   maxparams, \
   param_name, \
   count) < 0) \
-return -1; \
+goto cleanup; \
 } while (0)

 #define QEMU_ADD_NAME_PARAM(record, maxparams, type, num, name) \
@@ -18384,7 +18384,7 @@ do { \
 maxparams, \
 param_name, \
 name) < 0) \
-return -1; \
+goto cleanup; \
 } while (0)

 #define QEMU_ADD_NET_PARAM(record, maxparams, num, name, value) \
@@ -18448,6 +18448,7 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver 
ATTRIBUTE_UNUSED,
"tx.drop", tmp.tx_drop);
 }

+ cleanup:
 return 0;
 }

-- 
1.9.3

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

[libvirt] [PATCH 06/12] getstats: start giving offline block stats

2014-12-06 Thread Eric Blake 
I noticed that for an offline domain, 'virsh domstats --block $dom'
was producing just the domain name, with no stats.  But the older
'virsh domblkinfo' works just fine on offline domains.  This patch
starts to get us closer, by at least reporting the disk names for
an offline domain.

With this patch, I now see the following for an offline domain
with one qcow2 disk and an empty cdrom drive:
$ virsh domstats --block foo
Domain: 'foo'
  block.count=2
  block.0.name=hda
  block.1.name=hdc

* src/qemu/qemu_driver.c (qemuDomainGetStatsBlock): Don't short-circuit
output of block name.

Signed-off-by: Eric Blake 
---
 src/qemu/qemu_driver.c | 30 +-
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1e254bc..404decd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18529,19 +18529,20 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
 int rc;
 virHashTablePtr stats = NULL;
 qemuDomainObjPrivatePtr priv = dom->privateData;
+bool abbreviated = false;

-if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom))
-return 0; /* it's ok, just go ahead silently */
+if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) {
+abbreviated = true; /* it's ok, just go ahead silently */
+} else {
+qemuDomainObjEnterMonitor(driver, dom);
+rc = qemuMonitorGetAllBlockStatsInfo(priv->mon, &stats);
+ignore_value(qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats));
+qemuDomainObjExitMonitor(driver, dom);

-qemuDomainObjEnterMonitor(driver, dom);
-rc = qemuMonitorGetAllBlockStatsInfo(priv->mon, &stats);
-ignore_value(qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats));
-qemuDomainObjExitMonitor(driver, dom);
-
-if (rc < 0) {
-virResetLastError();
-ret = 0; /* still ok, again go ahead silently */
-goto cleanup;
+if (rc < 0) {
+virResetLastError();
+abbreviated = true; /* still ok, again go ahead silently */
+}
 }

 QEMU_ADD_COUNT_PARAM(record, maxparams, "block", dom->def->ndisks);
@@ -18552,9 +18553,12 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,

 QEMU_ADD_NAME_PARAM(record, maxparams, "block", i, disk->dst);

-if (!disk->info.alias ||
-!(entry = virHashLookup(stats, disk->info.alias)))
+if (abbreviated || !disk->info.alias ||
+!(entry = virHashLookup(stats, disk->info.alias))) {
+/* FIXME: we could still look up sizing by sharing code
+ * with qemuDomainGetBlockInfo */
 continue;
+}

 QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,
 "rd.reqs", entry->rd_req);
-- 
1.9.3

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

[libvirt] [PATCH 04/12] qemu: let blockinfo reuse virStorageSource

2014-12-06 Thread Eric Blake 
Right now, grabbing blockinfo always calls stat on the disk, then
opens the image to determine the capacity, using a throw-away
virStorageSourcePtr.  This has a couple of drawbacks:

1. We are calling stat and opening a file on every invocation of
the API.  However, there are cases where the stats should NOT be
changing between successive calls (if a domain is running, no
one should be changing the physical size of a block device or raw
image behind our backs; capacity of read-only files should not
be changing; and we are the gateway to the block-resize command
to know when the capacity of read-write files should be changing).
True, we still have to use stat in some cases (a sparse raw file
changes allocation if it is read-write and the amount of holes is
changing, and a read-write qcow2 image stored in a file changes
physical size if it was not fully pre-allocated).  But for
read-only images, even this should be something we can remember
from the previous time, rather than repeating every call.

2. We want to enhance the power of virDomainListGetStats, by
sharing code.  But we already have a virStorageSourcePtr for
each disk, and it would be easier to reuse the common structure
than to have to worry about the one-off virDomainBlockInfoPtr.

While this patch does not optimize reuse of information in point
1, it does get us closer to being able to do so; by updating a
structure that survives between consecutive calls.

* src/util/virstoragefile.h (_virStorageSource): Add physical, to
mirror virDomainBlockInfo.
* src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Store into
storage source, then copy to block info.

Signed-off-by: Eric Blake 
---
 src/qemu/qemu_driver.c| 42 ++
 src/util/virstoragefile.h |  3 ++-
 2 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ae4485a..e873362 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11034,6 +11034,26 @@ qemuDomainGetBlockInfo(virDomainPtr dom,

 disk = vm->def->disks[idx];

+/* FIXME: For an offline domain, we always want to check current
+ * on-disk statistics (as users have been known to change offline
+ * images behind our backs).  For a running domain, however, it
+ * would be nice to avoid opening a file (particularly since
+ * reading a file while qemu is writing it risks the reader seeing
+ * bogus data), or even avoid a stat, if the information
+ * remembered from the prevoius run is still viable.
+ *
+ * For read-only disks, nothing should be changing unless the user
+ * has requested a block-commit action.  For read-write disks, we
+ * know some special cases: capacity should not change without a
+ * block-resize (where capacity is the only stat that requires
+ * opening a file, and even then, only for non-raw files); and
+ * physical size of a raw image or of a block device should
+ * likewise not be changing without block-resize.  On the other
+ * hand, allocation of a raw file can change (if the file is
+ * sparse, but the amount of sparseness changes due to writes or
+ * punching holes), and physical size of a non-raw file can
+ * change.
+ */
 if (virStorageSourceIsLocalStorage(disk->src)) {
 if (!disk->src->path) {
 virReportError(VIR_ERR_INVALID_ARG,
@@ -11095,15 +5,15 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
 /* Get info for normal formats */
 if (S_ISREG(sb.st_mode) || fd == -1) {
 #ifndef WIN32
-info->physical = (unsigned long long)sb.st_blocks *
+disk->src->physical = (unsigned long long)sb.st_blocks *
 (unsigned long long)DEV_BSIZE;
 #else
-info->physical = sb.st_size;
+disk->src->physical = sb.st_size;
 #endif
 /* Regular files may be sparse, so logical size (capacity) is not same
  * as actual physical above
  */
-info->capacity = sb.st_size;
+disk->src->capacity = sb.st_size;
 } else {
 /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should
  * be 64 bits on all platforms.
@@ -4,17 +11134,17 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
  _("failed to seek to end of %s"), path);
 goto endjob;
 }
-info->physical = end;
-info->capacity = end;
+disk->src->physical = end;
+disk->src->capacity = end;
 }

 /* If the file we probed has a capacity set, then override
  * what we calculated from file/block extents */
 if (meta->capacity)
-info->capacity = meta->capacity;
+disk->src->capacity = meta->capacity;

 /* Set default value .. */
-info->allocation = info->physical;
+disk->src->allocation = disk->src->physical;

 /* ..but if guest is not using raw disk format and on a block device,
  * then query highest allocated extent from QEMU
@@ -11146,13 +1116

[libvirt] [PATCH 03/12] qemu: refactor blockinfo job handling

2014-12-06 Thread Eric Blake 
In order for a future patch to virDomainListGetStats to reuse
some code for determining disk usage of offline domains, we
need to make it easier to pull out part of the guts of grabbing
blockinfo.  The current implementation grabs a job fairly late
in the game, while getstats will already own a job; reordering
things so that the job is always grabbed up front in both
functions will make it easier to pull out the common code.
This patch results in grabbing a job in cases where one was not
previously needed, but as it is a query job, it should not be
noticeably slower.

This patch touches the same code as the fix for CVE-2014-6458
(commit b799259); in that patch, we avoided hotplug changing
a disk reference during the time of obtaining a monitor lock
by copying all data we needed and no longer referencing disk;
this patch goes the other way and ensures that by holding the
job, the disk cannot be changed so we no longer need to worry
about the disk being invalidated across the monitor lock.

* src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Rearrange job
control to be outside of disk information.

Signed-off-by: Eric Blake 
---
 src/qemu/qemu_driver.c | 62 +++---
 1 file changed, 29 insertions(+), 33 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a7b208f..ae4485a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10999,7 +10999,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
 int format;
 bool activeFail = false;
 virQEMUDriverConfigPtr cfg = NULL;
-char *alias = NULL;
 char *buf = NULL;
 ssize_t len;

@@ -11018,11 +11017,19 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
 goto cleanup;
 }

+/* Technically, we only need a job if we are going to query the
+ * monitor, which is only for active domains that are using
+ * non-raw block devices.  But it is easier to share code if we
+ * always grab a job; furthermore, grabbing the job ensures that
+ * hot-plug won't change disk behind our backs.  */
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
+goto cleanup;
+
 /* Check the path belongs to this domain. */
 if ((idx = virDomainDiskIndexByName(vm->def, path, false)) < 0) {
 virReportError(VIR_ERR_INVALID_ARG,
_("invalid path %s not assigned to domain"), path);
-goto cleanup;
+goto endjob;
 }

 disk = vm->def->disks[idx];
@@ -11032,36 +11039,36 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
 virReportError(VIR_ERR_INVALID_ARG,
_("disk '%s' does not currently have a source 
assigned"),
path);
-goto cleanup;
+goto endjob;
 }

 if ((fd = qemuOpenFile(driver, vm, disk->src->path, O_RDONLY,
NULL, NULL)) == -1)
-goto cleanup;
+goto endjob;

 if (fstat(fd, &sb) < 0) {
 virReportSystemError(errno,
  _("cannot stat file '%s'"), disk->src->path);
-goto cleanup;
+goto endjob;
 }

 if ((len = virFileReadHeaderFD(fd, VIR_STORAGE_MAX_HEADER, &buf)) < 0) 
{
 virReportSystemError(errno, _("cannot read header '%s'"),
  disk->src->path);
-goto cleanup;
+goto endjob;
 }
 } else {
 if (virStorageFileInitAs(disk->src, cfg->user, cfg->group) < 0)
-goto cleanup;
+goto endjob;

 if ((len = virStorageFileReadHeader(disk->src, VIR_STORAGE_MAX_HEADER,
 &buf)) < 0)
-goto cleanup;
+goto endjob;

 if (virStorageFileStat(disk->src, &sb) < 0) {
 virReportSystemError(errno, _("failed to stat remote file '%s'"),
  NULLSTR(disk->src->path));
-goto cleanup;
+goto endjob;
 }
 }

@@ -11073,17 +11080,17 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("no disk format for %s and probing is disabled"),
path);
-goto cleanup;
+goto endjob;
 }

 if ((format = virStorageFileProbeFormatFromBuf(disk->src->path,
buf, len)) < 0)
-goto cleanup;
+goto endjob;
 }

 if (!(meta = virStorageFileGetMetadataFromBuf(disk->src->path, buf, len,
   format, NULL)))
-goto cleanup;
+goto endjob;

 /* Get info for normal formats */
 if (S_ISREG(sb.st_mode) || fd == -1) {
@@ -11105,7 +2,7 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
 if (end == (off_t)-1) {
 virReportSystemError(errno,