Hello,
I've observed memory leak in long-running python program and
suspects a bug in libvirt-python.
libvirt-python contains auto-generated code like this:
libvirt_virDomainGetXMLDesc(...) {
...
LIBVIRT_BEGIN_ALLOW_THREADS;
c_retval = virDomainGetXMLDesc(domain, flags);
LIBVIRT_END_ALLOW_THREADS;
py_retval = libvirt_charPtrWrap((char *) c_retval);
return py_retval;
}
virDomainGetXMLDesc() expects the caller to free c_retval.
Though it used to be freed in libvirt_charPtrWrap(), commit bb3301ba
("Don't free passed in args in libvirt_charPtrWrap /
libvirt_charPtrSizeWrap") has moved the responsibility to the outside.
So, it seems either GetXMLDesc should not depend on auto-generation or
the generator should be fixed.
Any comments?
--
Jun'ichi Nomura, NEC Corporation
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Hi Michal,
'have a kernel+qemu+libvirt setup with all recent upstream bits on a given host
& was trying to configure a VM with backing 1G huge pages...spanning 2 NUMA
nodes.
The host had 3 1G huge pages on each of the 2 NUMA nodes :
# cat /sys/devices/system/node/node0/hugepages/hugepages-1048576kB/nr_hugepages
3
# cat /sys/devices/system/node/node1/hugepages/hugepages-1048576kB/nr_hugepages
3
And I had the following in the /etc/fstab
hugetlbfs /hugepages_1Ghugetlbfs pagesize=1GB 0 0
I added the following entries in the xml file for the 4G/4vcpu VM
4
The resulting qemu command looked like this :
/usr/local/bin/qemu-system-x86_64 -name vm1 -S -machine
pc-i440fx-2.2,accel=kvm,usb=off \
-m 4096 -realtime mlock=off -smp 4,sockets=4,cores=1,threads=1 \
-object
memory-backend-file,prealloc=yes,mem-path=/hugepages_1G/libvirt/qemu,size=2048M,id=ram-node0,host-nodes=0-1,policy=bind
-numa node,nodeid=0,cpus=0-1,memdev=ram-node0 \
-object
memory-backend-file,prealloc=yes,mem-path=/hugepages_1G/libvirt/qemu,size=2048M,id=ram-node1,host-nodes=0-1,policy=bind
-numa node,nodeid=1,cpus=2-3,memdev=ram-node1 \
There were 3 1G pages available on each NUMA node on the host as shown above...
and I noticed that the VM got backed by 3 1G pages from node0 and 1 1G page
from node1.
#cat /sys/devices/system/node/node0/hugepages/hugepages-1048576kB/free_hugepages
0
# cat
/sys/devices/system/node/node1/hugepages/hugepages-1048576kB/free_hugepages
2
Not sure if this was expected behavior given the options I specified in the xml
file ? If yes...Is there some additional option to specify (in the XML file)
such that only a given number of 1Gig huge pages per node are picked to back
the VM (i.e. in the above case just 2 1G from each node) ?
Thanks!
Vinod
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
When enabling the migration_address option, by default it is
set to "127.0.0.1", but it's not a valid address for migration.
so we should add verification and set the default migration_address
to "0.0.0.0".
Signed-off-by: Chen Fan
---
src/qemu/qemu.conf | 2 +-
src/qemu/qemu_conf.c | 10 ++
src/qemu/test_libvirtd_qemu.aug.in | 2 +-
3 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 79bba36..666c303 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -459,7 +459,7 @@
# Override the listen address for all incoming migrations. Defaults to
# 0.0.0.0, or :: if both host and qemu are capable of IPv6.
-#migration_address = "127.0.0.1"
+#migration_address = "0.0.0.0"
# The default hostname or IP address which will be used by a migration
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 013f3de..2cbf2a6 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -719,6 +719,16 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
}
GET_VALUE_STR("migration_address", cfg->migrationAddress);
+if (cfg->migrationAddress) {
+if (STRPREFIX(cfg->migrationAddress, "localhost") ||
+STREQ(cfg->migrationAddress, "127.0.0.1") ||
+STREQ(cfg->migrationAddress, "::1") ||
+STREQ(cfg->migrationAddress, "[::1]")) {
+virReportError(VIR_ERR_CONF_SYNTAX, "%s",
+ _("migration_address must be a valid address or
hostname"));
+goto cleanup;
+}
+}
GET_VALUE_BOOL("log_timestamp", cfg->logTimestamp);
diff --git a/src/qemu/test_libvirtd_qemu.aug.in
b/src/qemu/test_libvirtd_qemu.aug.in
index d2bc2c0..30fd27e 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -69,7 +69,7 @@ module Test_libvirtd_qemu =
{ "keepalive_interval" = "5" }
{ "keepalive_count" = "5" }
{ "seccomp_sandbox" = "1" }
-{ "migration_address" = "127.0.0.1" }
+{ "migration_address" = "0.0.0.0" }
{ "migration_host" = "host.example.com" }
{ "migration_port_min" = "49152" }
{ "migration_port_max" = "49215" }
--
1.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
This version differs from the patch set
"conf: Check migration_host is valid or not during libvirt restarts"
I posted 2 weeks ago, I droped checking the migration_host on target
host. and find an issue when setting migration_host.
Chen Fan (3):
migration: add migration_host support for Ipv6 address without
brackets
conf: Check migration_host is localhost or not during restart
conf: Check migration_address is valid or not during restart
src/qemu/qemu.conf | 2 +-
src/qemu/qemu_conf.c | 21 +
src/qemu/qemu_migration.c | 19 +++
src/qemu/test_libvirtd_qemu.aug.in | 2 +-
4 files changed, 38 insertions(+), 6 deletions(-)
--
1.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/11/2014 06:05 PM, John Ferlan wrote:
> There are two repeats from the last series (1 & 2).
>
> For patch 1, I went with my suggestion - I'm open to others
> For patch 2, Coverity was complaining more about the way nparams
> would be overwritten - fix that by adding a new variable
>
> New patches
> 3 & 4 -> eblake helped out with these - especially the mgetgroups oddity
> 5 -> Fallout from fixing 4
> 6 -> virTimeFieldsThen() and the "offset = 0". I'd be OK with deleting the
> code, but it just feels like someone had it on a todo list to come
> back to some day
> 7 & 8 -> Fairly straightforward
> 9 -> This was an interesting case - it seems from what was being done
> that I have the right "answer". I did go all the way back to the
> initial submission of the code and it did the same thing, except it
> was using an unsigned long instead of int and well thus wouldn't
> ever hit the condition since we're grabbing the big endian int value
Too late for me to give a competent review on 1 or 6, I may try again in
the morning when I'm not as tired. 3 and 4 are indeed tricky, but I
already helped you on it earlier in the day. I've got a question on 9,
but again, sleep may help me reason about it better.
And with that, I'm off to some much-needed sleep :)
--
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
On 09/11/2014 06:06 PM, John Ferlan wrote:
> Coverity complains that the condition "size + 1 == 0" cannot happen.
> Since 'size' is unsigned 32bit value set using virReadBufInt32BE.
> Thus rather than + 1, it seems the comparison should be is it at
> max now and if so, return the failure.
>
> Signed-off-by: John Ferlan
> ---
> src/util/virstoragefile.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 299edcd..0219ce8 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -393,7 +393,7 @@ qcowXGetBackingStore(char **res,
> }
> if (offset + size > buf_size || offset + size < offset)
> return BACKING_STORE_INVALID;
> -if (size + 1 == 0)
> +if (size == UINT_MAX)
Is this dead code? After all, we just checked that offset+size is not
larger than buf_size (and buf_size is smaller than UINT_MAX); and also
that offset+size didn't overflow.
> return BACKING_STORE_INVALID;
> if (VIR_ALLOC_N(*res, size + 1) < 0)
> return BACKING_STORE_ERROR;
>
--
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
On 09/11/2014 06:06 PM, John Ferlan wrote:
> With the virGetGroupList() change in place - Coverity further complains
> that if we fail to virFork(), the groups will be leaked - which aha seems
> to be the case. Adjust the logic to save off the -errno, free the groups,
> and then return the value we saved
>
> Signed-off-by: John Ferlan
> ---
> src/util/virfile.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
ACK
>
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 7c506c9..b3b8be2 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -2000,8 +2000,11 @@ virFileOpenForked(const char *path, int openflags,
> mode_t mode,
> }
>
> pid = virFork();
> -if (pid < 0)
> -return -errno;
> +if (pid < 0) {
> +ret = -errno;
> +VIR_FREE(groups);
> +return ret;
> +}
>
> if (pid == 0) {
>
>
--
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
[adding gnulib, in case anyone else runs into the same false positive]
On 09/11/2014 06:06 PM, John Ferlan wrote:
> This ends up being a very bizarre false positive. With an assist from
> eblake, the claim is that mgetgroups() could return a -1 value, but yet
> still have a groups buffer allocated, yet the example shown doesn't
> seem to prove that.
>
> Rather than fret about it, by adding a well placed sa_assert() on the
> returned *list value we can "assure" ourselves that the mgetgroups()
> failure path won't signal this condition.
>
> Signed-off-by: John Ferlan
> ---
> src/util/virutil.c | 1 +
> 1 file changed, 1 insertion(+)
ACK.
More details that we worked out on IRC: the mgetgroups code gets a list
that may contain duplicates, so it makes a final pass to reduce two
types of duplicates - any later member with the first, or any two
adjacent members.
if (1 < ng)
{
gid_t first = *g;
gid_t *next;
gid_t *groups_end = g + ng;
for (next = g + 1; next < groups_end; next++)
{
if (*next == first || *next == *g)
ng--;
else
*++g = *next;
}
}
Coverity was assuming that ng-- could be executed enough times to make
it go negative, but looking closer at the loop bounds, the loop cannot
run more than the original ng - 1 times, so ng >= 1. I'm not sure if
upstream gnulib can or should do anything to silence Coverity from
triggering the false positive. But the added assert in the caller
definitely lets Coverity fill in the gaps for what we know to be true.
>
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index 8d2f62a..5197969 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -1063,6 +1063,7 @@ virGetGroupList(uid_t uid, gid_t gid, gid_t **list)
>
> ret = mgetgroups(user, primary, list);
> if (ret < 0) {
> +sa_assert(!*list);
> virReportSystemError(errno,
> _("cannot get group list for '%s'"), user);
> goto cleanup;
>
--
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
In order to implement the new virDomainBlockCopy, the existing
block copy internal implementation needs to be adjusted. The
new function will parse XML into a storage source, and parse
typed parameters into integers, then call into the same common
backend. For now, it's easier to keep the same implementation
limits that only local file destinations are suported, but now
the check needs to be explicit. Similar to qemuDomainBlockJobImpl
consuming 'vm', this code also consumes the caller's 'mirror'
description of the destination.
* src/qemu/qemu_driver.c (qemuDomainBlockCopy): Rename...
(qemuDomainBlockCopyCommon): ...and adjust parameters.
(qemuDomainBlockRebase): Adjust caller.
Signed-off-by: Eric Blake
---
src/qemu/qemu_driver.c | 118 ++---
1 file changed, 63 insertions(+), 55 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 917b286..75c529b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14916,7 +14916,8 @@ qemuDomainBlockPivot(virConnectPtr conn,
}
-/* bandwidth in MiB/s per public API */
+/* bandwidth in MiB/s per public API. Caller must lock vm beforehand,
+ * and not access it afterwards. */
static int
qemuDomainBlockJobImpl(virDomainObjPtr vm,
virConnectPtr conn,
@@ -15277,13 +15278,16 @@ qemuDomainBlockJobSetSpeed(virDomainPtr dom, const
char *path,
}
-/* bandwidth in bytes/s */
+/* bandwidth in bytes/s. Caller must lock vm beforehand, and not
+ * access it afterwards; likewise, caller must not access mirror
+ * afterwards. */
static int
-qemuDomainBlockCopy(virDomainObjPtr vm,
-virConnectPtr conn,
-const char *path,
-const char *dest, const char *format,
-unsigned long long bandwidth, unsigned int flags)
+qemuDomainBlockCopyCommon(virDomainObjPtr vm,
+ virConnectPtr conn,
+ const char *path,
+ virStorageSourcePtr mirror,
+ unsigned long long bandwidth,
+ unsigned int flags)
{
virQEMUDriverPtr driver = conn->privateData;
qemuDomainObjPrivatePtr priv;
@@ -15293,13 +15297,13 @@ qemuDomainBlockCopy(virDomainObjPtr vm,
int idx;
struct stat st;
bool need_unlink = false;
-virStorageSourcePtr mirror = NULL;
virQEMUDriverConfigPtr cfg = NULL;
+const char *format = NULL;
+int desttype = virStorageSourceGetActualType(mirror);
/* Preliminaries: find the disk we are editing, sanity checks */
-virCheckFlags(VIR_DOMAIN_BLOCK_REBASE_SHALLOW |
- VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT |
- VIR_DOMAIN_BLOCK_REBASE_COPY_DEV, -1);
+virCheckFlags(VIR_DOMAIN_BLOCK_COPY_SHALLOW |
+ VIR_DOMAIN_BLOCK_COPY_REUSE_EXT, -1);
priv = vm->privateData;
cfg = virQEMUDriverGetConfig(driver);
@@ -15344,8 +15348,8 @@ qemuDomainBlockCopy(virDomainObjPtr vm,
if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0)
goto endjob;
-if ((flags & VIR_DOMAIN_BLOCK_REBASE_SHALLOW) &&
-STREQ_NULLABLE(format, "raw") &&
+if ((flags & VIR_DOMAIN_BLOCK_COPY_SHALLOW) &&
+mirror->format == VIR_STORAGE_FILE_RAW &&
disk->src->backingStore->path) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk '%s' has backing file, so raw shallow copy "
@@ -15355,72 +15359,65 @@ qemuDomainBlockCopy(virDomainObjPtr vm,
}
/* Prepare the destination file. */
-if (stat(dest, &st) < 0) {
+/* XXX Allow non-file mirror destinations */
+if (!virStorageSourceIsLocalStorage(mirror)) {
+virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("non-file destination not supported yet"));
+}
+if (stat(mirror->path, &st) < 0) {
if (errno != ENOENT) {
virReportSystemError(errno, _("unable to stat for disk %s: %s"),
- disk->dst, dest);
+ disk->dst, mirror->path);
goto endjob;
-} else if (flags & (VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT |
-VIR_DOMAIN_BLOCK_REBASE_COPY_DEV)) {
+} else if (flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT ||
+ desttype == VIR_STORAGE_TYPE_BLOCK) {
virReportSystemError(errno,
_("missing destination file for disk %s: %s"),
- disk->dst, dest);
+ disk->dst, mirror->path);
goto endjob;
}
} else if (!S_ISBLK(st.st_mode)) {
-if (st.st_size && !(flags & VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT)) {
+if (st.st_size && !(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("external destination file for di
We stupidly modeled block job bandwidth after migration
bandwidth, which in turn was an 'unsigned long' and therefore
subject to 32-bit vs. 64-bit interpretations. To work around
the fact that 10-gigabit interfaces are possible but don't fit
within 32 bits, the original interface took the number scaled
as MiB/sec. But this scaling is rather coarse, and it might
be nice to tune bandwidth finer than in megabyte chunks.
Several of the block job calls that can set speed are fed
through a common interface, so it was easier to adjust them all
at once. Note that there is intentionally no flag for the new
virDomainBlockCopy; there, since the API already uses a 64-bit
type always, instead of a possible 32-bit type, and is brand
new, it was easier to just avoid scaling issues. As with the
previous patch that adjusted the query side (commit db33cc24),
omitting the new flag preserves old behavior, and the
documentation now mentions limits of what happens when a 32-bit
machine is on either client or server side.
* include/libvirt/libvirt.h.in (virDomainBlockJobSetSpeedFlags)
(virDomainBlockPullFlags)
(VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES)
(VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES): New enums.
* src/libvirt.c (virDomainBlockJobSetSpeed, virDomainBlockPull)
(virDomainBlockRebase, virDomainBlockCommit): Document them.
* src/qemu/qemu_driver.c (qemuDomainBlockJobSetSpeed)
(qemuDomainBlockPull, qemuDomainBlockRebase)
(qemuDomainBlockCommit, qemuDomainBlockJobImpl): Support new flag.
Signed-off-by: Eric Blake
---
include/libvirt/libvirt.h.in | 31 +
src/libvirt.c| 83 +++-
src/qemu/qemu_driver.c | 61 +++-
3 files changed, 118 insertions(+), 57 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 94b942c..729bc82 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -2617,11 +2617,24 @@ int virDomainGetBlockJobInfo(virDomainPtr dom, const
char *disk,
virDomainBlockJobInfoPtr info,
unsigned int flags);
-intvirDomainBlockJobSetSpeed(virDomainPtr dom, const char *disk,
- unsigned long bandwidth, unsigned int flags);
+/* Flags for use with virDomainBlockJobSetSpeed */
+typedef enum {
+VIR_DOMAIN_BLOCK_JOB_SPEED_BANDWIDTH_BYTES = 1 << 0, /* bandwidth in
bytes/s
+instead of MiB/s */
+} virDomainBlockJobSetSpeedFlags;
-int virDomainBlockPull(virDomainPtr dom, const char *disk,
- unsigned long bandwidth, unsigned int flags);
+int virDomainBlockJobSetSpeed(virDomainPtr dom, const char *disk,
+ unsigned long bandwidth, unsigned int flags);
+
+/* Flags for use with virDomainBlockPull (values chosen to be a subset
+ * of the flags for virDomainBlockRebase) */
+typedef enum {
+VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES = 1 << 6, /* bandwidth in bytes/s
+ instead of MiB/s */
+} virDomainBlockPullFlags;
+
+int virDomainBlockPull(virDomainPtr dom, const char *disk,
+ unsigned long bandwidth, unsigned int flags);
/**
* virDomainBlockRebaseFlags:
@@ -2640,11 +2653,13 @@ typedef enum {
names */
VIR_DOMAIN_BLOCK_REBASE_COPY_DEV = 1 << 5, /* Treat destination as block
device instead of file */
+VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES = 1 << 6, /* bandwidth in bytes/s
+ instead of MiB/s */
} virDomainBlockRebaseFlags;
-int virDomainBlockRebase(virDomainPtr dom, const char *disk,
- const char *base, unsigned long bandwidth,
- unsigned int flags);
+int virDomainBlockRebase(virDomainPtr dom, const char *disk,
+ const char *base, unsigned long bandwidth,
+ unsigned int flags);
/**
* virDomainBlockCopyFlags:
@@ -2719,6 +2734,8 @@ typedef enum {
VIR_DOMAIN_BLOCK_COMMIT_RELATIVE = 1 << 3, /* keep the backing chain
referenced using relative
names */
+VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES = 1 << 4, /* bandwidth in bytes/s
+ instead of MiB/s */
} virDomainBlockCommitFlags;
int virDomainBlockCommit(virDomainPtr dom, const char *disk, const char *base,
diff --git a/src/libvirt.c b/src/libvirt.c
index 941c518..f7e5a37 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -19733,11 +19733,20 @@ virDomainGetBlockJobInfo(virDomainPtr dom, const char
*disk,
* virDomainBlockJobSetSpeed:
* @dom: pointer to domai
Treating -1 as the maximum bandwidth of block jobs is not very
practical, given the restrictions on overflow between 32-bit
vs. 64-bit long, as well as conversions between MiB/s and bytes/s.
We already document that 0 means unlimited, so the only reason to
keep negative support is for back-compat.
Meanwhile, it is a good idea to allow users to input in scales
other than MiB/s. This patch just rounds back up to MiB/s, but by
using a common helper, a later patch can then determine when a
particular input scale will be better for using flags with the
corresponding API call.
* tools/virsh-domain.c (blockJobBandwidth): New function.
(blockJobImpl, cmdBlockCopy): Use it.
* tools/virsh.pod (blockjob, blockcopy, blockpull, blockcommit):
Document this.
Signed-off-by: Eric Blake
---
tools/virsh-domain.c | 73 ++--
tools/virsh.pod | 42 ++
2 files changed, 84 insertions(+), 31 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index cc1e554..594647a 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1460,6 +1460,9 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
goto cleanup;
}
+
+/* Code common between blockpull, blockcommit, blockcopy, blockjob */
+
typedef enum {
VSH_CMD_BLOCK_JOB_ABORT,
VSH_CMD_BLOCK_JOB_SPEED,
@@ -1467,6 +1470,56 @@ typedef enum {
VSH_CMD_BLOCK_JOB_COMMIT,
} vshCmdBlockJobMode;
+
+/* Parse a block job bandwidth parameter. Returns -1 on error with
+ * message printed, 0 if no bandwidth needed, and 1 if *bandwidth is
+ * set to non-zero. */
+static int
+blockJobBandwidth(vshControl *ctl, const vshCmd *cmd,
+ unsigned long *bandwidth)
+{
+const char *value = NULL;
+int ret = vshCommandOptString(cmd, "bandwidth", &value);
+unsigned long long speed;
+
+*bandwidth = 0;
+if (ret <= 0)
+return ret;
+
+/* For back-compat, accept -1 as meaning unlimited, by converting
+ * negative values to 0 (and forbid wraparound back to positive).
+ * Alas, we have to parse the raw string ourselves, instead of
+ * using vshCommandOptUL. If only we had never allowed wraparound
+ * on bandwidth. */
+if (strchr(value, '-')) {
+unsigned long tmp;
+
+if (vshCommandOptULWrap(cmd, "bandwidth", &tmp) < 0 ||
+(long) tmp >= 0) {
+vshError(ctl, _("could not parse bandwidth '%s'"), value);
+return -1;
+}
+return 0;
+}
+
+/* Read a number in bytes/s, but with default unit of MiB/s if no
+ * unit was specified. */
+if (vshCommandOptScaledInt(cmd, "bandwidth", &speed, 1024 * 1024,
+ (sizeof(*bandwidth) < sizeof(speed) ?
+ULONG_MAX * 1024 * 1024 : ULLONG_MAX)) < 0) {
+vshError(ctl, _("could not parse bandwidth '%s'"), value);
+return -1;
+}
+
+/* Now scale it to MiB/s for the caller. What a pain. */
+speed = VIR_DIV_UP(speed, 1024 * 1024);
+
+/* We already checked that narrowing the type will fit. */
+*bandwidth = speed;
+return !!speed;
+}
+
+
static bool
blockJobImpl(vshControl *ctl, const vshCmd *cmd,
vshCmdBlockJobMode mode, virDomainPtr *pdom)
@@ -1485,10 +1538,8 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd,
if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0)
goto cleanup;
-if (vshCommandOptULWrap(cmd, "bandwidth", &bandwidth) < 0) {
-vshError(ctl, "%s", _("bandwidth must be a number"));
+if (blockJobBandwidth(ctl, cmd, &bandwidth) < 0)
goto cleanup;
-}
switch (mode) {
case VSH_CMD_BLOCK_JOB_ABORT:
@@ -1610,7 +1661,7 @@ static const vshCmdOptDef opts_block_commit[] = {
},
{.name = "bandwidth",
.type = VSH_OT_DATA,
- .help = N_("bandwidth limit in MiB/s")
+ .help = N_("bandwidth limit, as scaled integer (default MiB/s)")
},
{.name = "base",
.type = VSH_OT_DATA,
@@ -1826,7 +1877,7 @@ static const vshCmdOptDef opts_block_copy[] = {
},
{.name = "bandwidth",
.type = VSH_OT_DATA,
- .help = N_("bandwidth limit in MiB/s")
+ .help = N_("bandwidth limit, as scaled integer (default MiB/s)")
},
{.name = "shallow",
.type = VSH_OT_BOOL,
@@ -1959,14 +2010,8 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
goto cleanup;
-/* XXX: Parse bandwidth as scaled input, rather than forcing
- * MiB/s, and either reject negative input or treat it as 0 rather
- * than trying to guess which value will work well across both
- * APIs with their different sizes and scales. */
-if (vshCommandOptULWrap(cmd, "bandwidth", &bandwidth) < 0) {
-vshError(ctl, "%s", _("bandwidth must be a number"));
+if (blockJobBandwidth(ctl, cmd, &bandwidth) < 0)
goto cleanup;
-}
if (vshCommandOptUInt(cm
Trickier than I had hoped; but now that we can parse numbers with
arbitrary scale, it's time to use those numbers to call into the
new API if the old API would have rounded the input, while still
gracefully falling back to the old API if the new one fails.
* tools/virsh-domain.c (blockJobBandwidth): Add parameter, adjust
return value.
(blockJobImpl, cmdBlockCopy): Adjust callers.
Signed-off-by: Eric Blake
---
tools/virsh-domain.c | 154 +--
1 file changed, 113 insertions(+), 41 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 594647a..14d6921 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1471,18 +1471,29 @@ typedef enum {
} vshCmdBlockJobMode;
-/* Parse a block job bandwidth parameter. Returns -1 on error with
- * message printed, 0 if no bandwidth needed, and 1 if *bandwidth is
- * set to non-zero. */
+/* Parse a block job bandwidth parameter. Caller must provide exactly
+ * one of @bandwidth or @bandwidthBytes. Returns -1 on error with
+ * message printed, 0 if no bandwidth needed, 1 if *bandwidth is set
+ * to non-zero MiB/s, and 2 if *bandwidth or *bandwidthBytes is
+ * non-zero bytes/s. */
static int
blockJobBandwidth(vshControl *ctl, const vshCmd *cmd,
- unsigned long *bandwidth)
+ unsigned long *bandwidth, unsigned long long *bandwidthBytes)
{
const char *value = NULL;
int ret = vshCommandOptString(cmd, "bandwidth", &value);
unsigned long long speed;
+unsigned long long limit = ULLONG_MAX;
+
+if (bandwidth) {
+*bandwidth = 0;
+bandwidthBytes = &speed;
+if (sizeof(*bandwidth) < sizeof(speed))
+limit = ULONG_MAX << 20ULL;
+} else {
+*bandwidthBytes = 0;
+}
-*bandwidth = 0;
if (ret <= 0)
return ret;
@@ -1504,19 +1515,29 @@ blockJobBandwidth(vshControl *ctl, const vshCmd *cmd,
/* Read a number in bytes/s, but with default unit of MiB/s if no
* unit was specified. */
-if (vshCommandOptScaledInt(cmd, "bandwidth", &speed, 1024 * 1024,
- (sizeof(*bandwidth) < sizeof(speed) ?
-ULONG_MAX * 1024 * 1024 : ULLONG_MAX)) < 0) {
+if (vshCommandOptScaledInt(cmd, "bandwidth", bandwidthBytes, 1024 * 1024,
+ limit) < 0) {
vshError(ctl, _("could not parse bandwidth '%s'"), value);
return -1;
}
+if (!bandwidth) /* bandwidthBytes is already correct */
+return 2;
-/* Now scale it to MiB/s for the caller. What a pain. */
-speed = VIR_DIV_UP(speed, 1024 * 1024);
+/* If the number fits in unsigned long, is not a multiple of
+ * MiB/s, and we haven't proven the new API flag will fail, then
+ * let the caller try it directly. Otherwise, scale it to MiB/s
+ * for the caller. What a pain. */
+if ((unsigned long) speed == speed && speed & (1024 * 1024 - 1) &&
+!ctl->blockJobNoBytes) {
+ret = 2;
+} else {
+speed = VIR_DIV_UP(speed, 1024 * 1024);
+ret = 1;
+}
/* We already checked that narrowing the type will fit. */
*bandwidth = speed;
-return !!speed;
+return ret;
}
@@ -1527,10 +1548,12 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd,
virDomainPtr dom = NULL;
const char *path;
unsigned long bandwidth = 0;
+int bandwidthScale;
bool ret = false;
const char *base = NULL;
const char *top = NULL;
unsigned int flags = 0;
+int rc;
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
goto cleanup;
@@ -1538,7 +1561,7 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd,
if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0)
goto cleanup;
-if (blockJobBandwidth(ctl, cmd, &bandwidth) < 0)
+if ((bandwidthScale = blockJobBandwidth(ctl, cmd, &bandwidth, NULL)) < 0)
goto cleanup;
switch (mode) {
@@ -1551,7 +1574,19 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd,
goto cleanup;
break;
case VSH_CMD_BLOCK_JOB_SPEED:
-if (virDomainBlockJobSetSpeed(dom, path, bandwidth, 0) < 0)
+if (bandwidthScale > 1)
+flags |= VIR_DOMAIN_BLOCK_JOB_SPEED_BANDWIDTH_BYTES;
+rc = virDomainBlockJobSetSpeed(dom, path, bandwidth, flags);
+if (rc < 0 && bandwidthScale > 1 &&
+last_error->code == VIR_ERR_INVALID_ARG) {
+/* try again with MiB/s */
+ctl->blockJobNoBytes = true;
+flags &= ~VIR_DOMAIN_BLOCK_JOB_SPEED_BANDWIDTH_BYTES;
+bandwidth = VIR_DIV_UP(bandwidth, 1024 * 1024);
+vshResetLibvirtError();
+rc = virDomainBlockJobSetSpeed(dom, path, bandwidth, flags);
+}
+if (rc < 0)
goto cleanup;
break;
case VSH_CMD_BLOCK_JOB_PULL:
@@ -1559,15 +1594,28 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd,
Upstream qemu 1.4 added some drive-mirror tunables not present
when it was first introduced in 1.3. Management apps may want
to set these in some cases (for example, without tuning
granularity down to sector size, a copy may end up occupying
more bytes than the original because an entire cluster is
copied even when only a sector within the cluster is dirty,
although tuning it down results in more CPU time to do the
copy). I haven't personally needed to use the parameters, but
since they exist, and since the new API supports virTypedParams,
we might as well expose them.
Since the tuning parameters aren't often used, and omitted from
the QMP command when unspecified, I think it is safe to rely on
qemu 1.3 to issue an error about them being unsupported, rather
than trying to create a new capability bit in libvirt.
Meanwhile, all versions of qemu from 1.4 to 2.1 have a bug where
a bad granularity (such as non-power-of-2) gives a poor message:
error: internal error: unable to execute QEMU command 'drive-mirror': Invalid
parameter 'drive-virtio-disk0'
because of abuse of QERR_INVALID_PARAMETER (which is supposed to
name the parameter that was given a bad value, rather than the
value passed to some other parameter). I don't see that a
capability check will help, so we'll just live with it (and it
has since been improved in upstream qemu).
* src/qemu/qemu_monitor.h (qemuMonitorDriveMirror): Add
parameters.
* src/qemu/qemu_monitor.c (qemuMonitorDriveMirror): Likewise.
* src/qemu/qemu_monitor_json.h (qemuMonitorJSONDriveMirror):
Likewise.
* src/qemu/qemu_monitor_json.c (qemuMonitorJSONDriveMirror):
Likewise.
* src/qemu/qemu_driver.c (qemuDomainBlockCopyCommon): Likewise.
(qemuDomainBlockRebase, qemuDomainBlockCopy): Adjust callers.
* src/qemu/qemu_migration.c (qemuMigrationDriveMirror): Likewise.
* tests/qemumonitorjsontest.c (qemuMonitorJSONDriveMirror): Likewise.
Signed-off-by: Eric Blake
---
src/qemu/qemu_driver.c | 18 +-
src/qemu/qemu_migration.c| 2 +-
src/qemu/qemu_monitor.c | 8 +---
src/qemu/qemu_monitor.h | 2 ++
src/qemu/qemu_monitor_json.c | 4
src/qemu/qemu_monitor_json.h | 2 ++
tests/qemumonitorjsontest.c | 2 +-
7 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6895b23..1673c7b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15287,6 +15287,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
const char *path,
virStorageSourcePtr mirror,
unsigned long long bandwidth,
+ unsigned int granularity,
+ unsigned long long buf_size,
unsigned int flags)
{
virQEMUDriverPtr driver = conn->privateData;
@@ -15432,7 +15434,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
/* Actually start the mirroring */
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorDriveMirror(priv->mon, device, mirror->path, format,
- bandwidth, flags);
+ bandwidth, granularity, buf_size, flags);
virDomainAuditDisk(vm, NULL, mirror, "mirror", ret >= 0);
qemuDomainObjExitMonitor(driver, vm);
if (ret < 0) {
@@ -15528,7 +15530,7 @@ qemuDomainBlockRebase(virDomainPtr dom, const char
*path, const char *base,
flags &= (VIR_DOMAIN_BLOCK_REBASE_SHALLOW |
VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT);
ret = qemuDomainBlockCopyCommon(vm, dom->conn, path, dest,
-bandwidth, flags);
+bandwidth, 0, 0, flags);
vm = NULL;
dest = NULL;
@@ -15596,23 +15598,13 @@ qemuDomainBlockCopy(virDomainPtr dom, const char
*disk, const char *destxml,
buf_size = param->value.ul;
}
}
-if (granularity) {
-virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("granularity tuning not supported yet"));
-goto cleanup;
-}
-if (buf_size) {
-virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("buffer size tuning not supported yet"));
-goto cleanup;
-}
if (!(dest = virDomainDiskDefSourceParse(destxml, vm->def, driver->xmlopt,
VIR_DOMAIN_XML_INACTIVE)))
goto cleanup;
ret = qemuDomainBlockCopyCommon(vm, dom->conn, disk, dest,
-bandwidth, flags);
+bandwidth, granularity, buf_size, flags);
vm = NULL;
cleanup:
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 4819c04..ce1a5cd 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1461,7 +1461,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
diff to v3:
patch 1 now consumes the destination in the common helper instead
of copying it
patch 2 rebased accordingly
patch 3 didn't need much work
patch 4 finally tested
patches 5-7 are new
Also available at git://repo.or.cz/libvirt/ericb.git branch blockcopy,
http://repo.or.cz/w/libvirt/ericb.git/shortlog/refs/heads/blockcopy
Eric Blake (7):
blockcopy: tweak how rebase calls into copy
blockcopy: add qemu implementation of new API
blockcopy: add qemu implementation of new tunables
blockjob: allow finer bandwidth tuning for set speed
blockjob: improve handling of bandwidth in virsh
blockjob: automatically use bytes for bandwidth in virsh
blockjob: allow forcing of bytes scale in virsh
include/libvirt/libvirt.h.in | 31 --
src/libvirt.c| 83 +-
src/qemu/qemu_driver.c | 255 +--
src/qemu/qemu_migration.c| 2 +-
src/qemu/qemu_monitor.c | 8 +-
src/qemu/qemu_monitor.h | 2 +
src/qemu/qemu_monitor_json.c | 4 +
src/qemu/qemu_monitor_json.h | 2 +
tests/qemumonitorjsontest.c | 2 +-
tools/virsh-domain.c | 226 ++
tools/virsh.pod | 50 +
11 files changed, 481 insertions(+), 184 deletions(-)
--
1.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
The hard part of managing the disk copy is already coded; all
this had to do was convert the XML and virTypedParameters into
the internal representation.
With this patch, all blockcopy operations that used the old
API should also work via the new API. Additional extensions,
such as supporting the granularity tunable or a network rather
than file destination, will be added as later patches.
* src/qemu/qemu_driver.c (qemuDomainBlockCopy): New function.
Signed-off-by: Eric Blake
---
src/qemu/qemu_driver.c | 84 ++
1 file changed, 84 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 75c529b..6895b23 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15539,6 +15539,89 @@ qemuDomainBlockRebase(virDomainPtr dom, const char
*path, const char *base,
return ret;
}
+
+static int
+qemuDomainBlockCopy(virDomainPtr dom, const char *disk, const char *destxml,
+virTypedParameterPtr params, int nparams,
+unsigned int flags)
+{
+virQEMUDriverPtr driver = dom->conn->privateData;
+virDomainObjPtr vm;
+int ret = -1;
+unsigned long long bandwidth = 0;
+unsigned int granularity = 0;
+unsigned long long buf_size = 0;
+virStorageSourcePtr dest = NULL;
+size_t i;
+
+virCheckFlags(VIR_DOMAIN_BLOCK_COPY_SHALLOW |
+ VIR_DOMAIN_BLOCK_COPY_REUSE_EXT, -1);
+if (virTypedParamsValidate(params, nparams,
+ VIR_DOMAIN_BLOCK_COPY_BANDWIDTH,
+ VIR_TYPED_PARAM_ULLONG,
+ VIR_DOMAIN_BLOCK_COPY_GRANULARITY,
+ VIR_TYPED_PARAM_UINT,
+ VIR_DOMAIN_BLOCK_COPY_BUF_SIZE,
+ VIR_TYPED_PARAM_ULLONG,
+ NULL) < 0)
+return -1;
+
+if (!(vm = qemuDomObjFromDomain(dom)))
+return -1;
+
+if (virDomainBlockCopyEnsureACL(dom->conn, vm->def) < 0)
+goto cleanup;
+
+for (i = 0; i < nparams; i++) {
+virTypedParameterPtr param = ¶ms[i];
+
+/* Typed params (wisely) refused to expose unsigned long, but
+ * back-compat demands that we stick with a maximum of
+ * unsigned long bandwidth in MiB/s, while our value is
+ * unsigned long long in bytes/s. Hence, we have to do
+ * overflow detection if this is a 32-bit server handling a
+ * 64-bit client. */
+if (STREQ(param->field, VIR_DOMAIN_BLOCK_COPY_BANDWIDTH)) {
+if (sizeof(unsigned long) < sizeof(bandwidth) &&
+param->value.ul > ULONG_MAX * (1ULL << 20)) {
+virReportError(VIR_ERR_OVERFLOW,
+ _("bandwidth must be less than %llu bytes"),
+ ULONG_MAX * (1ULL << 20));
+goto cleanup;
+}
+bandwidth = param->value.ul;
+} else if (STREQ(param->field, VIR_DOMAIN_BLOCK_COPY_GRANULARITY)) {
+granularity = param->value.ui;
+} else if (STREQ(param->field, VIR_DOMAIN_BLOCK_COPY_BUF_SIZE)) {
+buf_size = param->value.ul;
+}
+}
+if (granularity) {
+virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("granularity tuning not supported yet"));
+goto cleanup;
+}
+if (buf_size) {
+virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("buffer size tuning not supported yet"));
+goto cleanup;
+}
+
+if (!(dest = virDomainDiskDefSourceParse(destxml, vm->def, driver->xmlopt,
+ VIR_DOMAIN_XML_INACTIVE)))
+goto cleanup;
+
+ret = qemuDomainBlockCopyCommon(vm, dom->conn, disk, dest,
+bandwidth, flags);
+vm = NULL;
+
+ cleanup:
+if (vm)
+virObjectUnlock(vm);
+return ret;
+}
+
+
static int
qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long
bandwidth,
unsigned int flags)
@@ -17611,6 +17694,7 @@ static virDriver qemuDriver = {
.domainBlockJobSetSpeed = qemuDomainBlockJobSetSpeed, /* 0.9.4 */
.domainBlockPull = qemuDomainBlockPull, /* 0.9.4 */
.domainBlockRebase = qemuDomainBlockRebase, /* 0.9.10 */
+.domainBlockCopy = qemuDomainBlockCopy, /* 1.2.9 */
.domainBlockCommit = qemuDomainBlockCommit, /* 1.0.0 */
.connectIsAlive = qemuConnectIsAlive, /* 0.9.8 */
.nodeSuspendForDuration = qemuNodeSuspendForDuration, /* 0.9.8 */
--
1.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Take the previous patch one step further. In addition to
automatically selecting byte mode with fallback if the
user's result would be rounded, we also want to give the
user a way to guarantee byte mode even if rounding is not
required, with no fallback if the remote side doesn't
support byte mode. This way, virsh can be used to more
fully test the impact of setting the bytes flag.
* tools/virsh-domain.c (blockJobBandwidth): Adjust return type and
check for --bytes flag.
(cmdBlockCommit, cmdBlockPull, cmdBlockJob): Add --bytes flag.
(cmdBlockCopy): Likewise, and adjust caller.
(blockJobImpl): Adjust caller.
* tools/virsh.pod (blockcommit, blockcopy, blockpull, blockjob):
Document this.
Signed-off-by: Eric Blake
---
tools/virsh-domain.c | 57 ++--
tools/virsh.pod | 48 +--
2 files changed, 61 insertions(+), 44 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 14d6921..c6ed90e 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1474,14 +1474,16 @@ typedef enum {
/* Parse a block job bandwidth parameter. Caller must provide exactly
* one of @bandwidth or @bandwidthBytes. Returns -1 on error with
* message printed, 0 if no bandwidth needed, 1 if *bandwidth is set
- * to non-zero MiB/s, and 2 if *bandwidth or *bandwidthBytes is
- * non-zero bytes/s. */
+ * to non-zero MiB/s, 2 if *bandwidth or *bandwidthBytes is non-zero
+ * bytes/s but fallback to MiB/s is okay, and 3 if *bandwidth or
+ * *bandwidthBytes is non-zero bytes/s and fallback is not okay. */
static int
blockJobBandwidth(vshControl *ctl, const vshCmd *cmd,
unsigned long *bandwidth, unsigned long long *bandwidthBytes)
{
const char *value = NULL;
int ret = vshCommandOptString(cmd, "bandwidth", &value);
+bool bytes = vshCommandOptBool(cmd, "bytes");
unsigned long long speed;
unsigned long long limit = ULLONG_MAX;
@@ -1489,7 +1491,7 @@ blockJobBandwidth(vshControl *ctl, const vshCmd *cmd,
*bandwidth = 0;
bandwidthBytes = &speed;
if (sizeof(*bandwidth) < sizeof(speed))
-limit = ULONG_MAX << 20ULL;
+limit = ULONG_MAX << (bytes ? 0 : 20ULL);
} else {
*bandwidthBytes = 0;
}
@@ -1515,20 +1517,23 @@ blockJobBandwidth(vshControl *ctl, const vshCmd *cmd,
/* Read a number in bytes/s, but with default unit of MiB/s if no
* unit was specified. */
-if (vshCommandOptScaledInt(cmd, "bandwidth", bandwidthBytes, 1024 * 1024,
- limit) < 0) {
+if (vshCommandOptScaledInt(cmd, "bandwidth", bandwidthBytes,
+ bytes ? 1 : 1024 * 1024, limit) < 0) {
vshError(ctl, _("could not parse bandwidth '%s'"), value);
return -1;
}
if (!bandwidth) /* bandwidthBytes is already correct */
-return 2;
+return 3;
-/* If the number fits in unsigned long, is not a multiple of
- * MiB/s, and we haven't proven the new API flag will fail, then
- * let the caller try it directly. Otherwise, scale it to MiB/s
- * for the caller. What a pain. */
-if ((unsigned long) speed == speed && speed & (1024 * 1024 - 1) &&
-!ctl->blockJobNoBytes) {
+/* If the caller did not force bytes, the number fits in unsigned
+ * long, the number is not a multiple of MiB/s, and we haven't
+ * proven the new API flag will fail, then let the caller try it
+ * directly. Otherwise, scale it to MiB/s for the caller. What a
+ * pain. */
+if (bytes) {
+ret = 3;
+} else if ((unsigned long) speed == speed && speed & (1024 * 1024 - 1) &&
+ !ctl->blockJobNoBytes) {
ret = 2;
} else {
speed = VIR_DIV_UP(speed, 1024 * 1024);
@@ -1577,7 +1582,7 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd,
if (bandwidthScale > 1)
flags |= VIR_DOMAIN_BLOCK_JOB_SPEED_BANDWIDTH_BYTES;
rc = virDomainBlockJobSetSpeed(dom, path, bandwidth, flags);
-if (rc < 0 && bandwidthScale > 1 &&
+if (rc < 0 && bandwidthScale == 2 &&
last_error->code == VIR_ERR_INVALID_ARG) {
/* try again with MiB/s */
ctl->blockJobNoBytes = true;
@@ -1602,7 +1607,7 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd,
else
rc = virDomainBlockPull(dom, path, bandwidth, flags);
-if (rc < 0 && bandwidthScale > 1 &&
+if (rc < 0 && bandwidthScale == 2 &&
last_error->code == VIR_ERR_INVALID_ARG) {
/* try again with MiB/s */
ctl->blockJobNoBytes = true;
@@ -1634,7 +1639,7 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd,
if (bandwidthScale > 1)
flags |= VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES;
rc = virDomainBlockCommit(dom, path, base, top, bandwidth, flags);
-if (rc < 0 && bandwidthScale > 1 &&
+if
On 09/11/2014 06:05 PM, John Ferlan wrote:
> With eblake's help - adjust the checks for stdinfd/stdoutfd to ensure the
> values are within the range we expect; otherwise the dup2()'s and subsequent
> VIR_CLOSE() calls cause Coverity to believe there's a resource leak.
>
> Signed-off-by: John Ferlan
> ---
> daemon/libvirtd.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
ACK. This means that if anyone tries to do something crazy like
'libvirtd 0<&-' with stdin closed, and the OS doesn't auto-reopen an fd,
then spawning into a daemon will now fail where it previously worked.
But this is a _good_ thing, as it is much easier to reason about
programs that prohibit being started without something open at all three
standard descriptors, and since POSIX states that it is undefined
behavior if you exec a process without open fds.
--
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
On 09/11/2014 06:05 PM, John Ferlan wrote:
> Coverity notes that after we VIR_ALLOC_N(params, nparams) a failed call to
> virDomainGetCPUStats could result in nparams being set to -1. In that case,
> the subsequent virTypedParamsFree in cleanup will pass -1 which isn't good.
>
> Use the returned value as the number of stats to display in the loop as
> it will be the value reported from the hypervisor and may be less than
> nparams which is OK
>
> Signed-off-by: John Ferlan
> ---
> tools/virsh-domain.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
ACK
--
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
On Thu, 11 Sep 2014, Blair Bethwaite wrote:
A related problem arises mixing RBD and local disks when
drive-mirroring / block-migrating - the RBDs get migrated too, volume
round trips out of ceph -> into source hypervisor -> into dest
hypervisor -> back into ceph. Not great!
This patch fixes that issue (but thus far I've only tested against
1.1.1 on Precise - will do with 1.2 on Trusty next week)
=
qemuMigrationDriveMirror(virQEMUDriverPt
virDomainBlockJobInfo info;
/* skip shared, RO and source-less disks */
-if (disk->shared || disk->readonly || !disk->src)
+if (disk->shared || disk->readonly || !disk->src ||
+ (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK &&
+disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD))
continue;
VIR_FREE(diskAlias);
=
So interested in feedback on this and whether it should be pushed up...
I think you would need a corresponding change in
qemuMigrationStartNBDServer as well.
This change would largely solve the problems I'm encountering, but it's
still annoying having to use VIR_MIGRATE_UNSAFE just so that libvirt
doesn't complain about the non-shared disks, even though they're going to
be explicitly copied from the source to destination VM.
I'm going to see if I can find out why RBD is being treated differently
from other disk backends. As I said before, I can't see anything in QEMU
that indicates it behaves differently from the others, and indeed it
appears that *all* backends should be safe since (as far as I can tell)
they're explicitly flushed when the VM is paused. Maybe at the time that
RBD was added to QEMU and libvirt, QEMU only did this explicit flushing
for RBD.
It seems to me that libvirt is also probably going to need some more
detailed logic on deciding when a disk device should be copied or not
(with the understanding that when NBD is *not* being used, that decision
is ultimately up to QEMU only). If we have exceptional cases like RBD,
then the decision can't be made purely according to the shared flag.
- Michael
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Since 98b9acf5aa02551dd37d0209339aba2e22e4004a
This ends up being a false positive for two reasons...
expected to be already allocated and thus is passed by value; whereas,
the call into remoteDomainGetJobStats() 'params' is passed by reference.
Thus if the VIR_ALLOC is done there is no way for it to be leaked for
callers that passed by value.
path that handles 'nparams == 0 && params == NULL' on entry. Thus all
other callers have guaranteed that 'params' is non NULL. Of course
Coverity isn't wise enough to pick up on this, but nonetheless is
does point out something "future callers" for which future callers
need to be aware.
Even though it is a false positive, it's probably a good idea to at
least make some sort of check (and to "trick" Coverity into believing
we know what we're doing). The easiest/cheapest way was to check
the input 'limit' value. For the remoteDomainGetJobStats() it is
passed as 0 indicating (perhaps) that the caller has done the
limits length checking already and that its caller can handle
allocating something that can be passed back to the caller.
Doing this means an adjustment to remoteConnectGetAllDomainStats()
in order to do the prerequisite maximum checking per set of stats
returned and passing 0 to remoteDeserializeTypedParameters() in order
to allocate memory into &elem->params.
Signed-off-by: John Ferlan
---
src/remote/remote_driver.c | 20 +++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 8221683..1594c89 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -1761,6 +1761,18 @@ remoteDeserializeTypedParameters(remote_typed_param
*ret_params_val,
goto cleanup;
}
} else {
+/* For callers that can return this allocated buffer back to their
+ * caller, pass a 0 in the 'limit' field indicating that the
+ * ret_params_len MAX checking has already occurred *and* that
+ * the caller has passed 'params' by reference; otherwise, a
+ * caller that receives the 'params' by value will potentially
+ * leak memory we're allocating here
+ */
+if (limit != 0) {
+virReportError(VIR_ERR_RPC, "%s",
+ _("invalid call - params is NULL on input"));
+goto cleanup;
+}
if (VIR_ALLOC_N(*params, ret_params_len) < 0)
goto cleanup;
}
@@ -7771,6 +7783,12 @@ remoteConnectGetAllDomainStats(virConnectPtr conn,
for (i = 0; i < ret.retStats.retStats_len; i++) {
remote_domain_stats_record *rec = ret.retStats.retStats_val + i;
+if (rec->params.params_len > REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX) {
+virReportError(VIR_ERR_RPC, "%s",
+ _("returned number of parameters exceeds limit"));
+goto cleanup;
+}
+
if (VIR_ALLOC(elem) < 0)
goto cleanup;
@@ -7779,7 +7797,7 @@ remoteConnectGetAllDomainStats(virConnectPtr conn,
if (remoteDeserializeTypedParameters(rec->params.params_val,
rec->params.params_len,
-
REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX,
+ 0,
&elem->params,
&elem->nparams))
goto cleanup;
--
1.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
If we end up at the cleanup lable before we've VIR_EXPAND_N the list,
then calling virQEMUCapsFreeStringList() with a NULL proplist could
theoretically deref proplist if nproplist was set. Coverity doesn't
Signed-off-by: John Ferlan
---
src/qemu/qemu_capabilities.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index a652f29..81ada48 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1728,7 +1728,7 @@ virQEMUCapsParseDeviceStrObjectProps(const char *str,
ret = nproplist;
cleanup:
-if (ret < 0)
+if (ret < 0 && proplist)
virQEMUCapsFreeStringList(nproplist, proplist);
return ret;
}
--
1.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Coverity notes that after we VIR_ALLOC_N(params, nparams) a failed call to
virDomainGetCPUStats could result in nparams being set to -1. In that case,
the subsequent virTypedParamsFree in cleanup will pass -1 which isn't good.
Use the returned value as the number of stats to display in the loop as
it will be the value reported from the hypervisor and may be less than
nparams which is OK
Signed-off-by: John Ferlan
---
tools/virsh-domain.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index cc1e554..e951b1b 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6734,7 +6734,7 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd)
{
virDomainPtr dom;
virTypedParameterPtr params = NULL;
-int pos, max_id, cpu = 0, show_count = -1, nparams = 0;
+int pos, max_id, cpu = 0, show_count = -1, nparams = 0, stats_per_cpu;
size_t i, j;
bool show_total = false, show_per_cpu = false;
unsigned int flags = 0;
@@ -6853,11 +6853,12 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd)
goto cleanup;
/* passing start_cpu == -1 gives us domain's total status */
-if ((nparams = virDomainGetCPUStats(dom, params, nparams, -1, 1, flags)) <
0)
+if ((stats_per_cpu = virDomainGetCPUStats(dom, params, nparams,
+ -1, 1, flags)) < 0)
goto failed_stats;
vshPrint(ctl, _("Total:\n"));
-for (i = 0; i < nparams; i++) {
+for (i = 0; i < stats_per_cpu; i++) {
vshPrint(ctl, "\t%-12s ", params[i].field);
if ((STREQ(params[i].field, VIR_DOMAIN_CPU_STATS_CPUTIME) ||
STREQ(params[i].field, VIR_DOMAIN_CPU_STATS_USERTIME) ||
--
1.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
There are two repeats from the last series (1 & 2).
For patch 1, I went with my suggestion - I'm open to others
For patch 2, Coverity was complaining more about the way nparams
would be overwritten - fix that by adding a new variable
New patches
3 & 4 -> eblake helped out with these - especially the mgetgroups oddity
5 -> Fallout from fixing 4
6 -> virTimeFieldsThen() and the "offset = 0". I'd be OK with deleting the
code, but it just feels like someone had it on a todo list to come
back to some day
7 & 8 -> Fairly straightforward
9 -> This was an interesting case - it seems from what was being done
that I have the right "answer". I did go all the way back to the
initial submission of the code and it did the same thing, except it
was using an unsigned long instead of int and well thus wouldn't
ever hit the condition since we're grabbing the big endian int value
This gets me down to 5 issues
John Ferlan (9):
remote_driver: Resolve Coverity RESOURCE_LEAK
virsh: Resolve Coverity NEGATIVE_RETURNS
daemon: Resolve Coverity RESOURCE_LEAK
virutil: Resolve Coverity RESOURCE_LEAK
virfile: Resolve Coverity RESOURCE_LEAK
virtime: Resolve Coverity DEADCODE
qemu: Resolve Coverity FORWARD_NULL
libxl: Resolve Coverity CHECKED_RETURN
virstoragefile: Resolve Coverity DEADCODE
daemon/libvirtd.c| 8
src/libxl/libxl_driver.c | 3 ++-
src/qemu/qemu_capabilities.c | 2 +-
src/remote/remote_driver.c | 20 +++-
src/util/virfile.c | 7 +--
src/util/virstoragefile.c| 2 +-
src/util/virtime.c | 2 ++
src/util/virutil.c | 1 +
tools/virsh-domain.c | 7 ---
9 files changed, 39 insertions(+), 13 deletions(-)
--
1.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Coverity complains that because of how 'offset' is initialized to
0 (zero), the resulting math and comparison on rem is pointless.
For the "while (rem < 0)", the value of 'rem' must be between
0 and 86399 (SECS_PER_DAY = 86400ULL). Thus, the addition of
offset (0) does nothing and the while (rem < 0) is pointless.
For the "while (rem > SECS_PER_DAY)", we have the same issue.
Signed-off-by: John Ferlan
---
src/util/virtime.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/util/virtime.c b/src/util/virtime.c
index 9fefb67..99c7cf6 100644
--- a/src/util/virtime.c
+++ b/src/util/virtime.c
@@ -135,10 +135,12 @@ void virTimeFieldsThen(unsigned long long when, struct tm
*fields)
days = whenSecs / SECS_PER_DAY;
rem = whenSecs % SECS_PER_DAY;
rem += offset;
+/* coverity[dead_error_condition] - when offset is calculated remove this
*/
while (rem < 0) {
rem += SECS_PER_DAY;
--days;
}
+/* coverity[dead_error_condition] - when offset is calculated remove this
*/
while (rem >= SECS_PER_DAY) {
rem -= SECS_PER_DAY;
++days;
--
1.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Coverity complains that the condition "size + 1 == 0" cannot happen.
Since 'size' is unsigned 32bit value set using virReadBufInt32BE.
Thus rather than + 1, it seems the comparison should be is it at
max now and if so, return the failure.
Signed-off-by: John Ferlan
---
src/util/virstoragefile.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 299edcd..0219ce8 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -393,7 +393,7 @@ qcowXGetBackingStore(char **res,
}
if (offset + size > buf_size || offset + size < offset)
return BACKING_STORE_INVALID;
-if (size + 1 == 0)
+if (size == UINT_MAX)
return BACKING_STORE_INVALID;
if (VIR_ALLOC_N(*res, size + 1) < 0)
return BACKING_STORE_ERROR;
--
1.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
This ends up being a very bizarre false positive. With an assist from
eblake, the claim is that mgetgroups() could return a -1 value, but yet
still have a groups buffer allocated, yet the example shown doesn't
seem to prove that.
Rather than fret about it, by adding a well placed sa_assert() on the
returned *list value we can "assure" ourselves that the mgetgroups()
failure path won't signal this condition.
Signed-off-by: John Ferlan
---
src/util/virutil.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 8d2f62a..5197969 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -1063,6 +1063,7 @@ virGetGroupList(uid_t uid, gid_t gid, gid_t **list)
ret = mgetgroups(user, primary, list);
if (ret < 0) {
+sa_assert(!*list);
virReportSystemError(errno,
_("cannot get group list for '%s'"), user);
goto cleanup;
--
1.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
With the virGetGroupList() change in place - Coverity further complains
that if we fail to virFork(), the groups will be leaked - which aha seems
to be the case. Adjust the logic to save off the -errno, free the groups,
and then return the value we saved
Signed-off-by: John Ferlan
---
src/util/virfile.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 7c506c9..b3b8be2 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -2000,8 +2000,11 @@ virFileOpenForked(const char *path, int openflags,
mode_t mode,
}
pid = virFork();
-if (pid < 0)
-return -errno;
+if (pid < 0) {
+ret = -errno;
+VIR_FREE(groups);
+return ret;
+}
if (pid == 0) {
--
1.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
This is version 4 of the work started by danpb:
https://www.redhat.com/archives/libvir-list/2014-May/msg01102.html
This series tests the conversion of libvirt XML to libxl_domain_config
objects by the libvirt libxl driver.
Changed in V4:
- V3 patches 2-4 have been pushed
- Patch 1 is unchanged from V3
- Patch 2 is new and adds tests for virJSONStringCompare
- Cleanup of ignored context paths definition in patch 3
(was #5 in V3)
Daniel P. Berrange (2):
util: Introduce virJSONStringCompare for JSON doc comparisons
libxl: Add a test suite for libxl option generator
Jim Fehlig (1):
tests: add tests for virJSONStringCompare
configure.ac | 2 +
src/libvirt_private.syms | 1 +
src/util/virjson.c | 242 +
src/util/virjson.h | 16 +++
tests/Makefile.am | 25 +++-
tests/jsontest.c | 63 -
tests/libxlxml2jsondata/basic-hvm.json | 217 +
tests/libxlxml2jsondata/basic-hvm.xml | 36 +
tests/libxlxml2jsondata/basic-pv.json | 163 ++
tests/libxlxml2jsondata/basic-pv.xml | 28
tests/libxlxml2jsontest.c | 228 +++
tests/virmocklibxl.c | 87
12 files changed, 1102 insertions(+), 6 deletions(-)
create mode 100644 tests/libxlxml2jsondata/basic-hvm.json
create mode 100644 tests/libxlxml2jsondata/basic-hvm.xml
create mode 100644 tests/libxlxml2jsondata/basic-pv.json
create mode 100644 tests/libxlxml2jsondata/basic-pv.xml
create mode 100644 tests/libxlxml2jsontest.c
create mode 100644 tests/virmocklibxl.c
--
1.8.4.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
From: "Daniel P. Berrange"
Comparing JSON docs using strcmp is simple, but is not flexible
as it is sensitive to whitespace used in the doc generation.
When comparing objects it may also be desirable to treat the
existance of keys in the actual object but not expected object
as non-fatal. Introduce a virJSONStringCompare function which
takes two strings representing expected and actual JSON docs
and then does a DOM comparison. Comparison is controled with
the ignore_contexts and flags parameters. No comparison is
done on context paths specified in ignore_contexts. The
VIR_JSON_COMPARE_IGNORE_EXPECTED_NULL flag can be used to
ignore actual values that have changed from an expected value
of null.
Signed-off-by: Daniel P. Berrange
Signed-off-by: Jim Fehlig
---
Beyond rebasing, unchanged from V3.
src/libvirt_private.syms | 1 +
src/util/virjson.c | 242 +++
src/util/virjson.h | 16
3 files changed, 259 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index fdf4548..b0c0625 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1446,6 +1446,7 @@ virISCSIScanTargets;
# util/virjson.h
+virJSONStringCompare;
virJSONValueArrayAppend;
virJSONValueArrayGet;
virJSONValueArraySize;
diff --git a/src/util/virjson.c b/src/util/virjson.c
index ec83b2f..73b71f4 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -47,6 +47,11 @@
VIR_LOG_INIT("util.json");
+VIR_ENUM_DECL(virJSONType)
+VIR_ENUM_IMPL(virJSONType, VIR_JSON_TYPE_LAST,
+ "object", "array", "string",
+ "number", "boolean", "null")
+
typedef struct _virJSONParserState virJSONParserState;
typedef virJSONParserState *virJSONParserStatePtr;
struct _virJSONParserState {
@@ -91,6 +96,7 @@ virJSONValueFree(virJSONValuePtr value)
break;
case VIR_JSON_TYPE_BOOLEAN:
case VIR_JSON_TYPE_NULL:
+case VIR_JSON_TYPE_LAST:
break;
}
@@ -1107,6 +1113,204 @@ virJSONParserHandleEndArray(void *ctx)
}
+static bool
+virJSONValueCompare(virJSONValuePtr expect,
+virJSONValuePtr actual,
+const char *context,
+const char **ignore_contexts,
+unsigned int flags)
+{
+size_t i, j;
+
+if (expect->type != actual->type) {
+if (expect->type == VIR_JSON_TYPE_NULL &&
+(flags & VIR_JSON_COMPARE_IGNORE_EXPECTED_NULL))
+return true;
+
+const char *expectType = virJSONTypeTypeToString(expect->type);
+const char *actualType = virJSONTypeTypeToString(actual->type);
+
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Expected value type '%s' but got value type '%s' at
'%s'"),
+ expectType, actualType, context);
+return false;
+}
+
+switch (expect->type) {
+case VIR_JSON_TYPE_OBJECT:
+/* Ensure actual data contains all expected data */
+for (i = 0; i < expect->data.object.npairs; i++) {
+bool found = false;
+char *childcontext;
+
+if (virAsprintf(&childcontext, "%s%s%s",
+context,
+STREQ(context, "/") ? "" : "/",
+expect->data.object.pairs[i].key) < 0)
+return false;
+
+/* Bypass paths we've been asked to ignore */
+if (ignore_contexts) {
+bool ignored = false;
+
+j = 0;
+while (ignore_contexts[j]) {
+if (STREQ(childcontext, ignore_contexts[j])) {
+ignored = true;
+break;
+}
+j++;
+}
+
+if (ignored)
+continue;
+}
+
+ for (j = 0; j < actual->data.object.npairs; j++) {
+if (STREQ(expect->data.object.pairs[i].key,
+ actual->data.object.pairs[j].key)) {
+found = true;
+break;
+}
+}
+
+if (!found) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Expected object key '%s' not found in actual
object at '%s'"),
+ expect->data.object.pairs[i].key, context);
+VIR_FREE(childcontext);
+return false;
+}
+
+if (!virJSONValueCompare(expect->data.object.pairs[i].value,
+ actual->data.object.pairs[j].value,
+ childcontext,
+ ignore_contexts,
+ flags)) {
+VIR_FREE(childcontext);
+return false;
+}
+VIR_FREE(childcontext);
+}
+
+/* Ensure expected data contain
On 09/11/14 16:25, Michal Privoznik wrote:
> On 11.09.2014 16:07, Laszlo Ersek wrote:
>> On 09/11/14 14:09, Michal Privoznik wrote:
>>> I've noticed two problem with the automatically created NVRAM varstore
>>> file. The first, even though I run qemu as root:root for some reason I
>>> get Permission denied when trying to open the _VARS.fd file. The
>>> problem is, the upper directory misses execute permissions, which in
>>> combination with us dropping some capabilities result in EPERM.
>>>
>>> The next thing is, that if I switch SELinux to enforcing mode, I get
>>> another EPERM because the vars file is not labeled correctly. It is
>>> passed to qemu as disk and hence should be labelled as disk. QEMU may
>>> write to it eventually, so this is different to kernel or initrd.
>>>
>>> Signed-off-by: Michal Privoznik
>>> ---
>>> libvirt.spec.in | 2 +-
>>> src/security/security_selinux.c | 5 -
>>> 2 files changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libvirt.spec.in b/libvirt.spec.in
>>> index a6a58cf..ecf160b 100644
>>> --- a/libvirt.spec.in
>>> +++ b/libvirt.spec.in
>>> @@ -1938,7 +1938,7 @@ exit 0
>>> %dir %attr(0750, %{qemu_user}, %{qemu_group})
>>> %{_localstatedir}/lib/libvirt/qemu/
>>> %dir %attr(0750, %{qemu_user}, %{qemu_group})
>>> %{_localstatedir}/lib/libvirt/qemu/channel/
>>> %dir %attr(0750, %{qemu_user}, %{qemu_group})
>>> %{_localstatedir}/lib/libvirt/qemu/channel/target/
>>> -%dir %attr(0750, %{qemu_user}, %{qemu_group})
>>> %{_localstatedir}/lib/libvirt/qemu/nvram/
>>> +%dir %attr(0711, %{qemu_user}, %{qemu_group})
>>> %{_localstatedir}/lib/libvirt/qemu/nvram/
>>> %dir %attr(0750, %{qemu_user}, %{qemu_group})
>>> %{_localstatedir}/cache/libvirt/qemu/
>>> %{_datadir}/augeas/lenses/libvirtd_qemu.aug
>>> %{_datadir}/augeas/lenses/tests/test_libvirtd_qemu.aug
>>> diff --git a/src/security/security_selinux.c
>>> b/src/security/security_selinux.c
>>> index bf67fb5..3db2b27 100644
>>> --- a/src/security/security_selinux.c
>>> +++ b/src/security/security_selinux.c
>>> @@ -2300,8 +2300,11 @@
>>> virSecuritySELinuxSetSecurityAllLabel(virSecurityManagerPtr mgr,
>>>mgr) < 0)
>>> return -1;
>>>
>>> +/* This is different than kernel or initrd. The nvram store
>>> + * is really a disk, qemu can read and write to it. */
>>> if (def->os.loader && def->os.loader->nvram &&
>>> -virSecuritySELinuxSetFilecon(def->os.loader->nvram,
>>> data->content_context) < 0)
>>> +secdef && secdef->imagelabel &&
>>> +virSecuritySELinuxSetFilecon(def->os.loader->nvram,
>>> secdef->imagelabel) < 0)
>>> return -1;
>>>
>>> if (def->os.kernel &&
>>>
>>
>> Good detective work!
>>
>> Regarding the g+x,o+x change on
>> %{_localstatedir}/lib/libvirt/qemu/nvram. This change theoretically
>> allows a qemu instance to "probe" for the presence of "foreign" varstore
>> files (it won't be able to open any, but eg. error codes for open()
>> would differ between ENOENT vs. EACCES, and stat() would fail vs.
>> succeed). However I think we can live with this, and anyway, it's simply
>> impossible to open a file in directory D if directory D doesn't provide
>> the user with search permission. So that looks like a must.
>
> Indeed. Moreover, I was first surprised that even if was running qemu
> under root:root I got EACCES. Problem was, libvirt drops nearly all caps
> (even CAP_SYS_ADMIN) after fork and prior to executing qemu binary.
>
>>
>> Regarding the seclabel / context, I agree that it should have a label
>> consistent with other disk image files; for qemu it's just a -drive
>> after all. The hunk in question looks consistent with the rest of
>> "src/security/security_selinux.c".
>>
>> Acked-by: Laszlo Ersek
>>
>
> Thanks, applied.
... Unfortunately the patch is insufficient. Commit 742b08e3 added directory
%{_localstatedir}/lib/libvirt/qemu/nvram/
to two sub-packages:
%files daemon
%files daemon-driver-qemu
I assume that was a correct thing to do (it is consistent with the rest
of the directories being listed for both subpackages).
However, this recent patch (commit 37d8c75f) changes the 0750 permission
bits to 0711 only in the "daemon" subpackage, and the directory in the
daemon-driver-qemu subpackage remains with 0750. In my testing, the
daemon package's modification doesn't take effect at all. In fact the
daemon RPM doesn't even seem to contain the nvram directory.
Thanks,
Laszlo
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Jim Fehlig wrote:
> The libxl driver was blindly assigning libvirt's
> virDomainLifecycleAction to libxl's libxl_action_on_shutdown, when
> in fact the various actions take on different values in these enums.
>
> Introduce helpers to properly map the enum values.
>
BTW, forgot to mention that this is the first bug found by the proposed
unit tests :-).
Regards,
Jim
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Daniel P. Berrange wrote:
> On Wed, Sep 03, 2014 at 09:07:35PM -0600, Jim Fehlig wrote:
>
>> From: "Daniel P. Berrange"
>>
>> Comparing JSON docs using strcmp is simple, but is not flexible
>> as it is sensitive to whitespace used in the doc generation.
>> When comparing objects it may also be desirable to treat the
>> existance of keys in the actual object but not expected object
>> as non-fatal. Introduce a virJSONStringCompare function which
>> takes two strings representing expected and actual JSON docs
>> and then does a DOM comparison. Comparison is controled with
>> the ignore_contexts and flags parameters. No comparison is
>> done on context paths specified in ignore_contexts. The
>> VIR_JSON_COMPARE_IGNORE_EXPECTED_NULL flag can be used to
>> ignore actual values that have changed from an expected value
>> of null.
>>
>> Signed-off-by: Daniel P. Berrange
>> Signed-off-by: Jim Fehlig
>> ---
>> src/libvirt_private.syms | 1 +
>> src/util/virjson.c | 242
>> +++
>> src/util/virjson.h | 16
>> 3 files changed, 259 insertions(+)
>>
>
> Looks good, but perhaps we should also add to tests/virjsontest.c
> to verify the ignore context support is working as intended
>
Yes, good idea. I'll include that in V4.
Regards,
Jim
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Daniel P. Berrange wrote:
> On Wed, Sep 03, 2014 at 09:07:38PM -0600, Jim Fehlig wrote:
>
>> The libxl driver was blindly assigning libvirt's
>> virDomainLifecycleAction to libxl's libxl_action_on_shutdown, when
>> in fact the various actions take on different values in these enums.
>>
>> Introduce helpers to properly map the enum values.
>>
>> Signed-off-by: Jim Fehlig
>> ---
>> src/libxl/libxl_conf.c | 62
>> +++---
>> 1 file changed, 59 insertions(+), 3 deletions(-)
>>
>
> ACK
>
Thanks. I've pushed 2-4.
Regards,
Jim
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/10/2014 11:54 AM, Laine Stump wrote:
> Sometimes libvirt is installed on a host that is already using the
> network 192.168.122.0/24. If the libvirt-daemon-config-network package
> is installed, this creates a conflict, since that package has been
> hard-coded to create a virtual network that also uses
> 192.168.122.0/24. In the past libvirt has attempted to warn of /
> remediate this situation by checking for conflicting routes when the
> network is started, but it turns out that isn't always useful (for
> example in the case that the *other* interface/network creating the
> conflict hasn't yet been started at the time libvirtd start its owm
s/owm/own/
> networks).
>
> This patch attempts to catch the problem earlier - at install
> time. During the %post install for libvirt-daemon-config-network, we
> look through the output of "ip route show" for a route that exactly
> matches 192.1 68.122.0/24, and if found we search for a similar route
> that *doesn't* match (e.g. 192.168.123.0/24). When we find an
> available route, we just replace all occurences of "122" in the
s/occurences/occurrences/
> default.xml that is being created with ${new_sub}. This could
> obviously be made more complicated - automatically determine the
> existing network address and mask from examining the template
> default.xml, etc, but this scripting is simpler and gets the job done
> as long as we continue to use 192.168.122.0/24 in the template. (If
> anyone with mad bash skillz wants to suggest something to do that, by
> all means please do).
Is it worth adding comments into the template that the string "122" is
magic and must not be altered without also considering distro packaging?
>
> This is intended to at least "further reduce" the problems detailed in:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=811967
>
> I acknowledge that it doesn't help for cases of pre-built cloud images
> (or live images that are created on real hardware and then run in a
> virtual machine), but it should at least eliminate the troubles
> encountered by individuals doing one-off installs (and could be used
> to stifle complaints for live images, as long as libvirtd was running
> on the system where the live image compose took place (or the compose
> was itself done in a virtual machine that had a 192.168.122.0/24
> interface address).
No good suggestions on how to help those situations.
> ---
>
> The question here is: "Will this help some people's situation without
> causing new problems for anyone else?" I wouldn't mind pushing this
> patch, but also wouldn't mind if it was just the catalyst for
> discussion that leads to a better solution. We do need *some kind* of
> solution though, as more and more people are installing OSes that
> include the libvirt package in virtual machines, and are running into
> this problem with increasing frequency.
>
> libvirt.spec.in | 26 +-
> 1 file changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index a6a58cf..539d9ef 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -1728,8 +1728,32 @@ fi
> %if %{with_network}
> %post daemon-config-network
> if test $1 -eq 1 && test ! -f
> %{_sysconfdir}/libvirt/qemu/networks/default.xml ; then
> +# see if the network used by default network creates a conflict,
> +# and try to resolve it
> +orig_sub=122
> +sub=${orig_sub}
> +net=192.168.${sub}.0/24
> +routes=$(ip route show | cut -d' ' -f1)
How do we know that 'ip' is installed and available? Do we need any
Requires:, and/or making the script robust to 'ip' failing?
> +for route in $routes; do
> + if [ "${net}" = "${route}" ]; then
> +# there was a match, so we need to look for an unused subnet
Rather than using cut and a shell for loop, why not just use grep? [1]
if ip route show | grep -q "^192\\.168\\.$sub\\.0/24 "; then
> +for new_sub in $(seq 123 254); do
seq is a GNU coreutils extension that can't be used in shell code
designed to be portable everywhere; but this is a spec file for use by
rpms where we know it will be installed. So I'm fine with using it.
(If this were a configure script, I would have suggested using:
new_sub=123
while [ $new_sub -lt 255 ]; do
...
new_sub=$((new_sub + 1))
done
but that's overkill for this scenario.)
> + new_net="192.168.${new_sub}.0/24"
> + usable=yes
> + for route in ${routes}; do
[1] Oh, I see. You captured the ip output once, and are now scanning it
multiple times. In _that_ case, piping ip to grep on each loop is not
as efficient.
But you could still do the lookup in shell instead of spawning child
processes, and without needing a shell for loop:
routes=$(ip route show)
nl='
'
case $nl$routes in
*"${nl}192.168.$new_sub.0/24 "*) # code if found
*) # code if not found
esac
> +[ "${new_net}" = "${route}" ] && usable=no
Might be slightly faster if you skip the tail end o
On 09/10/2014 01:54 PM, Laine Stump wrote:
> Sometimes libvirt is installed on a host that is already using the
> network 192.168.122.0/24. If the libvirt-daemon-config-network package
> is installed, this creates a conflict, since that package has been
> hard-coded to create a virtual network that also uses
> 192.168.122.0/24. In the past libvirt has attempted to warn of /
> remediate this situation by checking for conflicting routes when the
> network is started, but it turns out that isn't always useful (for
> example in the case that the *other* interface/network creating the
> conflict hasn't yet been started at the time libvirtd start its owm
> networks).
>
> This patch attempts to catch the problem earlier - at install
> time. During the %post install for libvirt-daemon-config-network, we
> look through the output of "ip route show" for a route that exactly
> matches 192.1 68.122.0/24, and if found we search for a similar route
> that *doesn't* match (e.g. 192.168.123.0/24). When we find an
> available route, we just replace all occurences of "122" in the
> default.xml that is being created with ${new_sub}. This could
> obviously be made more complicated - automatically determine the
> existing network address and mask from examining the template
> default.xml, etc, but this scripting is simpler and gets the job done
> as long as we continue to use 192.168.122.0/24 in the template. (If
> anyone with mad bash skillz wants to suggest something to do that, by
> all means please do).
>
> This is intended to at least "further reduce" the problems detailed in:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=811967
>
> I acknowledge that it doesn't help for cases of pre-built cloud images
> (or live images that are created on real hardware and then run in a
> virtual machine), but it should at least eliminate the troubles
> encountered by individuals doing one-off installs (and could be used
> to stifle complaints for live images, as long as libvirtd was running
> on the system where the live image compose took place (or the compose
> was itself done in a virtual machine that had a 192.168.122.0/24
> interface address).
> ---
>
> The question here is: "Will this help some people's situation without
> causing new problems for anyone else?" I wouldn't mind pushing this
> patch, but also wouldn't mind if it was just the catalyst for
> discussion that leads to a better solution. We do need *some kind* of
> solution though, as more and more people are installing OSes that
> include the libvirt package in virtual machines, and are running into
> this problem with increasing frequency.
>
Agree with all the above FWIW. This isn't the perfect 'fix' but no one has
come up with one yet. So I say we give this a spin in rawhide/f21 and see how
it works out.
> libvirt.spec.in | 26 +-
> 1 file changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index a6a58cf..539d9ef 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -1728,8 +1728,32 @@ fi
> %if %{with_network}
> %post daemon-config-network
> if test $1 -eq 1 && test ! -f
> %{_sysconfdir}/libvirt/qemu/networks/default.xml ; then
> +# see if the network used by default network creates a conflict,
> +# and try to resolve it
> +orig_sub=122
> +sub=${orig_sub}
> +net=192.168.${sub}.0/24
> +routes=$(ip route show | cut -d' ' -f1)
> +for route in $routes; do
> + if [ "${net}" = "${route}" ]; then
> +# there was a match, so we need to look for an unused subnet
> +for new_sub in $(seq 123 254); do
> + new_net="192.168.${new_sub}.0/24"
> + usable=yes
> + for route in ${routes}; do
> +[ "${new_net}" = "${route}" ] && usable=no
> + done
> + if [ "${usable}" = "yes" ]; then
> +sub=${new_sub}
> +break;
> + fi
> +done
> + fi
> +done
> +
> UUID=`/usr/bin/uuidgen`
> -sed -e "s,,\n $UUID," \
> +sed -e "s/${orig_sub}/${sub}/g" \
> +-e "s,,\n $UUID," \
> < %{_datadir}/libvirt/networks/default.xml \
> > %{_sysconfdir}/libvirt/qemu/networks/default.xml
> ln -s ../default.xml
> %{_sysconfdir}/libvirt/qemu/networks/autostart/default.xml
>
My shell is pretty weak, but that looks okay to me. ACK. But I'd feel more
comfortable if Eric checked it out :)
Thanks,
Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Sep 11, 2014 at 07:47:46PM +0200, Peter Krempa wrote:
> Peter Krempa (6):
> util: Add function to check if a virStorageSource is "empty"
> qemu: Drop unused formatting of uuid
> util: storage: Allow metadata crawler to report useful errors
> qemu: Report better errors from broken backing chains
> storage: Improve error message when traversing backing chains
> qemu: Improve check for local storage
>
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_domain.c| 38 ++
> src/qemu/qemu_process.c | 5 ++---
> src/security/virt-aa-helper.c | 2 +-
> src/storage/storage_driver.c | 40 +---
> src/storage/storage_driver.h | 3 ++-
> src/util/virstoragefile.c | 20
> src/util/virstoragefile.h | 1 +
> tests/virstoragetest.c| 2 +-
> 9 files changed, 63 insertions(+), 49 deletions(-)
I don't suppose you found a way to test this? I'd like to, but it's
difficult (especially without qemu:///session working as root).
I'll have to fire up a VM with a patched libvirtd at some point, but
not today.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/11/2014 11:54 AM, Peter Krempa wrote:
> If a (floppy) drive isn't selected for snapshot explicitly and is empty
> don't try to snapshot it. For external snapshots this would fail as we
> can't generate a name for the snapshot from an empty drive.
>
> Reported-by: Pavel Hrdina
> ---
ACK.
--
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
On 09/11/2014 11:54 AM, Peter Krempa wrote:
> To express empty drive we historically use storage source with empty
> path. Unfortunately NBD disks may be declared without a path.
>
> Add a helper to wrap this logic.
> ---
> src/libvirt_private.syms | 1 +
> src/util/virstoragefile.c | 20
> src/util/virstoragefile.h | 1 +
> 3 files changed, 22 insertions(+)
ACK.
If I remember right from my virDomainBlockCopy code (commit 37588b25),
empty paths are allowed for device types of cdrom, floppy, and lun, but
forbidden at parse time for type='block' and but notionally forbidden for .
> + * Returns true if @src points to an empty storage source.
> + */
> +bool
> +virStorageSourceIsEmpty(virStorageSourcePtr src)
Maybe the comment is better as:
Returns true if the guest disk has no associated host storage source
(such as an empty cdrom drive).
--
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
On 09/11/2014 11:16 AM, Peter Krempa wrote:
>> struct _virDomainInterfaceStats {
>> long long rx_bytes;
>> long long rx_packets;
>> long long rx_errs;
>> long long rx_drop;
>> long long tx_bytes;
>> long long tx_packets;
>> long long tx_errs;
>> long long tx_drop;
>> };
>>
>> But I don't have any problem to cast them as unsigned, with something like:
>
> That will be fine with me as long as:
>>
>> #define QEMU_ADD_NET_PARAM(record, maxparams, num, name, value) \
>> do { \
>> char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
>> snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
>> "net.%u.%s", num, name); \
>> if (virTypedParamsAddULLong(&(record)->params, \
>
> ... you don't add the param if value is < 0. Thus rather than expressing
> the missing information via -1 just omit the parameter.
>
>> &(record)->nparams, \
>> maxparams, \
>> param_name, \
>> (unsigned long long)value) < 0) \
The cast is spurious; the C compiler does the right thing if you already
filter on value < 0 before calling this function to add the positive
values as unsigned long long.
--
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
On Thu, Sep 04, 2014 at 10:27:12PM -0400, Hongbin Lu wrote:
> Thanks Guido,
>
> Your comment is addressed:
> https://www.redhat.com/archives/libvir-list/2014-September/msg00284.html.
Great. The migration code looks good to me from a OpenVZ view but I
don't feel qualified enough to ACK this from a libvirt point of view
since I've lost a bit track over the different migration protocols.
Cheers,
-- Guido
>
> Best regards,
> Hongbin
>
>
> On Thu, Sep 4, 2014 at 1:42 AM, Guido Günther wrote:
>
> > Hi,
> > On Wed, Sep 03, 2014 at 11:07:20PM -0400, Hongbin Lu wrote:
> > [..snip..]
> > > +
> > > +if (virAsprintf(uri_out, "tcp://%s", hostname) < 0)
> > > +goto error;
> >
> > Since OpenVZ tunnels over ssh shouldn't this be something like ssh://
> > ?
> >
> > Cheers,
> > -- Guido
> >
> >
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/11/2014 10:28 AM, Peter Krempa wrote:
> On 09/11/14 18:25, Peter Krempa wrote:
>> On 09/11/14 18:16, Eric Blake wrote:
>>> On 09/11/2014 09:47 AM, Peter Krempa wrote:
If a floppy drive isn't selected for snapshot explicitly and is empty
don't try to snapshot it. For external snapshots this would fail as we
can't generate a name for the snapshot from an empty drive.
>>>
>>> Do we need the same for cdrom drives?
>>
>> CDROMs are automatically read only and thus get the _NONE target right
>> when parsing the configuration.
>>
+
+/* Don't snapshot empty floppy drives */
+if (def->dom->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY &&
+!virDomainDiskGetSource(def->dom->disks[i]))
>>>
>>> If we are worried about ALL empty drives, it would be simpler to just
>>> drop the left side of the &&, making it solely a test of whether there
>>> is currently a defined host source.
>>>
>>
>> Since only CDROMs and floppies can be empty and cdroms are already
>> exempted from here it should be functionally equivalent to do that. The
>> only limitation is that the check for the empty source probably needs to
>> be stronger (NBD disks may have the disk->src->path NULL even if they
>> have backing.)
also allows for a NULL src, if I
remember correctly.
>
> Which reminds me that snapshots of NBD disks are forbidden, so it should
> be fine even without tweaking the emptiness check.
Still feels fragile.
>
>>
>> I'll post a v2.
>>
>
> Your call whether I should try to improve the check or leave it as-is.
I'd feel more comfortable with the generic check that all source-less
disks are explicitly tweaked to not have a snapshot taken, rather than
relying on side checks like readonly saving the day.
--
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
If a (floppy) drive isn't selected for snapshot explicitly and is empty
don't try to snapshot it. For external snapshots this would fail as we
can't generate a name for the snapshot from an empty drive.
Reported-by: Pavel Hrdina
---
Version 2 now doesn't discriminate according to drive type
src/conf/snapshot_conf.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index c53a66b..4f99139 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -561,7 +561,13 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
if (VIR_STRDUP(disk->name, def->dom->disks[i]->dst) < 0)
goto cleanup;
disk->index = i;
-disk->snapshot = def->dom->disks[i]->snapshot;
+
+/* Don't snapshot empty drives */
+if (virStorageSourceIsEmpty(def->dom->disks[i]->src))
+disk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
+else
+disk->snapshot = def->dom->disks[i]->snapshot;
+
disk->src->type = VIR_STORAGE_TYPE_FILE;
if (!disk->snapshot)
disk->snapshot = default_snapshot;
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Request erroring out from the backing chain traveller and drop qemu's
internal backing chain integrity tester.
The backin chain traveller reports errors by itself with possibly more
detail than qemuDiskChainCheckBroken ever could.
We also need to make sure that we reconnect to existing qemu instances
even at the cost of losing the backing chain info (this really should be
stored in the XML rather than reloaded from disk, but that needs some
work).
---
src/qemu/qemu_domain.c | 26 ++
src/qemu/qemu_process.c | 5 ++---
2 files changed, 4 insertions(+), 27 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 88c5d1b..72638cf 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2440,27 +2440,6 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver,
return -1;
}
-static int
-qemuDiskChainCheckBroken(virDomainDiskDefPtr disk)
-{
-char *brokenFile = NULL;
-
-if (!virDomainDiskGetSource(disk))
-return 0;
-
-if (virStorageFileChainGetBroken(disk->src, &brokenFile) < 0)
-return -1;
-
-if (brokenFile) {
-virReportError(VIR_ERR_INVALID_ARG,
- _("Backing file '%s' of image '%s' is missing."),
- brokenFile, virDomainDiskGetSource(disk));
-VIR_FREE(brokenFile);
-return -1;
-}
-
-return 0;
-}
int
qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
@@ -2490,8 +2469,7 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
virFileExists(path))
continue;
-if (qemuDomainDetermineDiskChain(driver, vm, disk, false) >= 0 &&
-qemuDiskChainCheckBroken(disk) >= 0)
+if (qemuDomainDetermineDiskChain(driver, vm, disk, false) >= 0)
continue;
if (disk->startupPolicy &&
@@ -2660,7 +2638,7 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
if (virStorageFileGetMetadata(disk->src,
uid, gid,
cfg->allowDiskFormatProbing,
- false) < 0)
+ true) < 0)
ret = -1;
cleanup:
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 07335a0..a807f50 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3342,9 +3342,8 @@ qemuProcessReconnect(void *opaque)
goto error;
/* XXX we should be able to restore all data from XML in the future */
-if (qemuDomainDetermineDiskChain(driver, obj,
- obj->def->disks[i], true) < 0)
-goto error;
+ignore_value(qemuDomainDetermineDiskChain(driver, obj,
+ obj->def->disks[i], true));
dev.type = VIR_DOMAIN_DEVICE_DISK;
dev.data.disk = obj->def->disks[i];
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Now that we have a simple function to check locality of storage, reuse
it in qemuDomainCheckDiskPresence().
Also reuse check for empty storage source.
---
src/qemu/qemu_domain.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 72638cf..c9e8f89 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2453,20 +2453,18 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
for (i = vm->def->ndisks; i > 0; i--) {
size_t idx = i - 1;
virDomainDiskDefPtr disk = vm->def->disks[idx];
-const char *path = virDomainDiskGetSource(disk);
virStorageFileFormat format = virDomainDiskGetFormat(disk);
-virStorageType type = virStorageSourceGetActualType(disk->src);
-if (!path)
+if (virStorageSourceIsEmpty(disk->src))
continue;
/* There is no need to check the backing chain for disks
* without backing support, the fact that the file exists is
* more than enough */
-if (type != VIR_STORAGE_TYPE_NETWORK &&
+if (virStorageSourceIsLocalStorage(disk->src) &&
format >= VIR_STORAGE_FILE_NONE &&
format < VIR_STORAGE_FILE_BACKING &&
-virFileExists(path))
+virFileExists(virDomainDiskGetSource(disk)))
continue;
if (qemuDomainDetermineDiskChain(driver, vm, disk, false) >= 0)
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/11/14 19:11, Francesco Romani wrote:
> - Original Message -
>> From: "Peter Krempa"
>> To: "Francesco Romani" , libvir-list@redhat.com
>> Sent: Tuesday, September 9, 2014 1:42:15 PM
>> Subject: Re: [libvirt] [PATCHv3 5/8] qemu: bulk stats: implement interface
>> group
>
>>> + * VIR_DOMAIN_STATS_INTERFACE: Return network interface statistics.
>>> + * The typed parameter keys are in this format:
>>> + * "net.count" - number of network interfaces on this domain
>>> + * as unsigned int.
>>> + * "net..name" - name of the interface as string.
>>> + * "net..rx.bytes" - bytes received as long long.
>>> + * "net..rx.pkts" - packets received as long long.
>>> + * "net..rx.errs" - receive errors as long long.
>>> + * "net..rx.drop" - receive packets dropped as long long.
>>> + * "net..tx.bytes" - bytes transmitted as long long.
>>> + * "net..tx.pkts" - packets transmitted as long long.
>>> + * "net..tx.errs" - transmission errors as long long.
>>> + * "net..tx.drop" - transmit packets dropped as long long.
>>
>> Why are all of those represented as long long instead of unsigned long
>> long? I don't see how these could be negative. If we need to express
>> that the value is unsupported we can just drop it from here and not
>> waste half of the range here.
>>
>> Any other opinions on this?
>
> I used long long because of this:
>
> struct _virDomainInterfaceStats {
> long long rx_bytes;
> long long rx_packets;
> long long rx_errs;
> long long rx_drop;
> long long tx_bytes;
> long long tx_packets;
> long long tx_errs;
> long long tx_drop;
> };
>
> But I don't have any problem to cast them as unsigned, with something like:
That will be fine with me as long as:
>
> #define QEMU_ADD_NET_PARAM(record, maxparams, num, name, value) \
> do { \
> char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
> snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
> "net.%u.%s", num, name); \
> if (virTypedParamsAddULLong(&(record)->params, \
... you don't add the param if value is < 0. Thus rather than expressing
the missing information via -1 just omit the parameter.
> &(record)->nparams, \
> maxparams, \
> param_name, \
> (unsigned long long)value) < 0) \
> return -1; \
> } while (0)
>
>
>>
>>> + *
>>> * Using 0 for @stats returns all stats groups supported by the given
>>> * hypervisor.
>>> *
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index 6bcbfb5..989eb3e 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -17537,6 +17537,92 @@ qemuDomainGetStatsVcpu(virConnectPtr conn
>>> ATTRIBUTE_UNUSED,
>>> return ret;
>>> }
>>>
>>> +#define QEMU_ADD_COUNT_PARAM(record, maxparams, type, count) \
>>> +do { \
>>> +char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
>>> +snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, "%s.count", type);
>>> \
>>> +if (virTypedParamsAddUInt(&(record)->params, \
>>> + &(record)->nparams, \
>>> + maxparams, \
>>> + param_name, \
>>> + count) < 0) \
>>> +return -1; \
>>> +} while (0)
>>> +
>>> +#define QEMU_ADD_NAME_PARAM(record, maxparams, type, num, name) \
>>> +do { \
>>> +char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
>>> +snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
>>> + "%s.%lu.name", type, num); \
>>> +if (virTypedParamsAddString(&(record)->params, \
>>> +&(record)->nparams, \
>>> +maxparams, \
>>> +param_name, \
>>> +name) < 0) \
>>> +return -1; \
>>> +} while (0)
>>> +
>>> +#define QEMU_ADD_NET_PARAM(record, maxparams, num, name, value) \
>>> +do { \
>>> +char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
>>> +snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
>>> + "net.%lu.%s", num, name); \
>>
>> %lu? the count is unsigned int so you should be fine with %d
>
> Yep but the cycle counter is size_t and then...
>
> $ git diff
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 9d53883..e90a8c6 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -17487,7 +17487,7 @@ do { \
> do { \
> char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
> snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
> - "net.%lu.%s", num, name); \
> + "net.%u.%s", num, name); \
> if (virTypedParamsAddLLong(&(record)->params, \
> &(record)->nparams, \
> maxparams, \
> $ make
> [...]
> make[1]: Entering directory `/home/fromani/Projects/libvirt/src'
> CC qemu/libvirt_driver_qemu_im
- Original Message -
> From: "Peter Krempa"
> To: "Francesco Romani" , libvir-list@redhat.com
> Sent: Tuesday, September 9, 2014 1:42:15 PM
> Subject: Re: [libvirt] [PATCHv3 5/8] qemu: bulk stats: implement interface
> group
> > + * VIR_DOMAIN_STATS_INTERFACE: Return network interface statistics.
> > + * The typed parameter keys are in this format:
> > + * "net.count" - number of network interfaces on this domain
> > + * as unsigned int.
> > + * "net..name" - name of the interface as string.
> > + * "net..rx.bytes" - bytes received as long long.
> > + * "net..rx.pkts" - packets received as long long.
> > + * "net..rx.errs" - receive errors as long long.
> > + * "net..rx.drop" - receive packets dropped as long long.
> > + * "net..tx.bytes" - bytes transmitted as long long.
> > + * "net..tx.pkts" - packets transmitted as long long.
> > + * "net..tx.errs" - transmission errors as long long.
> > + * "net..tx.drop" - transmit packets dropped as long long.
>
> Why are all of those represented as long long instead of unsigned long
> long? I don't see how these could be negative. If we need to express
> that the value is unsupported we can just drop it from here and not
> waste half of the range here.
>
> Any other opinions on this?
I used long long because of this:
struct _virDomainInterfaceStats {
long long rx_bytes;
long long rx_packets;
long long rx_errs;
long long rx_drop;
long long tx_bytes;
long long tx_packets;
long long tx_errs;
long long tx_drop;
};
But I don't have any problem to cast them as unsigned, with something like:
#define QEMU_ADD_NET_PARAM(record, maxparams, num, name, value) \
do { \
char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
"net.%u.%s", num, name); \
if (virTypedParamsAddULLong(&(record)->params, \
&(record)->nparams, \
maxparams, \
param_name, \
(unsigned long long)value) < 0) \
return -1; \
} while (0)
>
> > + *
> > * Using 0 for @stats returns all stats groups supported by the given
> > * hypervisor.
> > *
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 6bcbfb5..989eb3e 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -17537,6 +17537,92 @@ qemuDomainGetStatsVcpu(virConnectPtr conn
> > ATTRIBUTE_UNUSED,
> > return ret;
> > }
> >
> > +#define QEMU_ADD_COUNT_PARAM(record, maxparams, type, count) \
> > +do { \
> > +char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
> > +snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, "%s.count", type);
> > \
> > +if (virTypedParamsAddUInt(&(record)->params, \
> > + &(record)->nparams, \
> > + maxparams, \
> > + param_name, \
> > + count) < 0) \
> > +return -1; \
> > +} while (0)
> > +
> > +#define QEMU_ADD_NAME_PARAM(record, maxparams, type, num, name) \
> > +do { \
> > +char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
> > +snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
> > + "%s.%lu.name", type, num); \
> > +if (virTypedParamsAddString(&(record)->params, \
> > +&(record)->nparams, \
> > +maxparams, \
> > +param_name, \
> > +name) < 0) \
> > +return -1; \
> > +} while (0)
> > +
> > +#define QEMU_ADD_NET_PARAM(record, maxparams, num, name, value) \
> > +do { \
> > +char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
> > +snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
> > + "net.%lu.%s", num, name); \
>
> %lu? the count is unsigned int so you should be fine with %d
Yep but the cycle counter is size_t and then...
$ git diff
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9d53883..e90a8c6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17487,7 +17487,7 @@ do { \
do { \
char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
- "net.%lu.%s", num, name); \
+ "net.%u.%s", num, name); \
if (virTypedParamsAddLLong(&(record)->params, \
&(record)->nparams, \
maxparams, \
$ make
[...]
make[1]: Entering directory `/home/fromani/Projects/libvirt/src'
CC qemu/libvirt_driver_qemu_impl_la-qemu_driver.lo
qemu/qemu_driver.c: In function 'qemuDomainGetStatsInterface':
qemu/qemu_driver.c:17521:9: error: format '%u' expects argument of type
'unsigned int', but argument 4 has type 'size_t' [-Werror=format=]
QEMU_ADD_NET_PARAM(record, maxparams, i,
^
qemu/qemu_driver.c:17521:9: error: format '%u
- Original Message -
> From: "Peter Krempa"
> To: "Francesco Romani" , libvir-list@redhat.com
> Sent: Thursday, September 11, 2014 6:07:48 PM
> Subject: Re: [libvirt] [PATCHv3 4/8] qemu: bulk stats: implement VCPU group
> > I like this, but I'd also need to do a cross-check on my our code in oVirt.
[...]
> > Will be acceptable to drop the misleading vcpu..cpu info and to add
> > the pin info in a new followup patch, in this stats group?
>
> You definitely can add that later on. But you should drop .cpu from this
> patch (not revert it later).
Very good, next submission (v4) will drop the misleading 'cpu' item,
Hopefully I'll also be able to add the pin information;
otherwise I'll save for a followup patch.
Bests,
--
Francesco Romani
RedHat Engineering Virtualization R & D
Phone: 8261328
IRC: fromani
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/11/14 18:25, Peter Krempa wrote:
> On 09/11/14 18:16, Eric Blake wrote:
>> On 09/11/2014 09:47 AM, Peter Krempa wrote:
>>> If a floppy drive isn't selected for snapshot explicitly and is empty
>>> don't try to snapshot it. For external snapshots this would fail as we
>>> can't generate a name for the snapshot from an empty drive.
>>
>> Do we need the same for cdrom drives?
>
> CDROMs are automatically read only and thus get the _NONE target right
> when parsing the configuration.
>
>>
>>>
>>> Reported-by: Pavel Hrdina
>>> ---
>>> src/conf/snapshot_conf.c | 9 -
>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
>>> index c53a66b..cbaff74 100644
>>> --- a/src/conf/snapshot_conf.c
>>> +++ b/src/conf/snapshot_conf.c
>>> @@ -561,7 +561,14 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr
>>> def,
>>> if (VIR_STRDUP(disk->name, def->dom->disks[i]->dst) < 0)
>>> goto cleanup;
>>> disk->index = i;
>>> -disk->snapshot = def->dom->disks[i]->snapshot;
>>> +
>>> +/* Don't snapshot empty floppy drives */
>>> +if (def->dom->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY &&
>>> +!virDomainDiskGetSource(def->dom->disks[i]))
>>
>> If we are worried about ALL empty drives, it would be simpler to just
>> drop the left side of the &&, making it solely a test of whether there
>> is currently a defined host source.
>>
>
> Since only CDROMs and floppies can be empty and cdroms are already
> exempted from here it should be functionally equivalent to do that. The
> only limitation is that the check for the empty source probably needs to
> be stronger (NBD disks may have the disk->src->path NULL even if they
> have backing.)
Which reminds me that snapshots of NBD disks are forbidden, so it should
be fine even without tweaking the emptiness check.
>
> I'll post a v2.
>
Your call whether I should try to improve the check or leave it as-is.
Peter
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/11/14 18:16, Eric Blake wrote:
> On 09/11/2014 09:47 AM, Peter Krempa wrote:
>> If a floppy drive isn't selected for snapshot explicitly and is empty
>> don't try to snapshot it. For external snapshots this would fail as we
>> can't generate a name for the snapshot from an empty drive.
>
> Do we need the same for cdrom drives?
CDROMs are automatically read only and thus get the _NONE target right
when parsing the configuration.
>
>>
>> Reported-by: Pavel Hrdina
>> ---
>> src/conf/snapshot_conf.c | 9 -
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
>> index c53a66b..cbaff74 100644
>> --- a/src/conf/snapshot_conf.c
>> +++ b/src/conf/snapshot_conf.c
>> @@ -561,7 +561,14 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
>> if (VIR_STRDUP(disk->name, def->dom->disks[i]->dst) < 0)
>> goto cleanup;
>> disk->index = i;
>> -disk->snapshot = def->dom->disks[i]->snapshot;
>> +
>> +/* Don't snapshot empty floppy drives */
>> +if (def->dom->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY &&
>> +!virDomainDiskGetSource(def->dom->disks[i]))
>
> If we are worried about ALL empty drives, it would be simpler to just
> drop the left side of the &&, making it solely a test of whether there
> is currently a defined host source.
>
Since only CDROMs and floppies can be empty and cdroms are already
exempted from here it should be functionally equivalent to do that. The
only limitation is that the check for the empty source probably needs to
be stronger (NBD disks may have the disk->src->path NULL even if they
have backing.)
I'll post a v2.
Peter
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/11/2014 11:55 AM, Eric Blake wrote:
> On 09/11/2014 02:38 AM, Peter Krempa wrote:
>> On 09/05/14 00:26, John Ferlan wrote:
>>> Since 98b9acf5aa02551dd37d0209339aba2e22e4004a
>>>
>>> This ends up being a false positive for two reasons...
>>>
>>> expected to be already allocated and thus is passed by value; whereas,
>>> the call into remoteDomainGetJobStats() 'params' is passed by reference.
>>> Thus if the VIR_ALLOC is done there is no way for it to be leaked for
>>> callers that passed by value.
>>>
>>> path that handles 'nparams == 0 && params == NULL' on entry. Thus all
>
> Careful, my virDomainBlockCopy API passes nparams==0 && params ==
> (non-NULL 0-length array) from the RPC daemon/remote.c receiver into the
> libvirtd side of the API call. It tripped me up the first time I tried
> to assert that nparams==0 implied params==NULL, and failed the test.
>
Well in general the newer Coverity has been squawking about this case -
that is assumptions where nparams/params type parameters are used. In
most cases, it's a "for (i = 0; i < nparams; i++)" do something with
"params[i]" that get flagged on the params[i] reference because there's
no nparams == 0 type check.
Particular to this query though the code in question is
remoteDeserializeTypedParameters(); whereas, the remoteDomainBlockCopy()
uses remoteSerializeTypedParameters().
And yes, it's all very tricky. While I do believe from reading the code
that this particular case is a false positive - I really was trying to
find a way around making too many changes. Open to suggestions naturally!
John
>>> other callers have guaranteed that 'params' is non NULL. Of course
>>> Coverity isn't wise enough to pick up on this, but nonetheless is
>>> does point out something "future callers" for which future callers
>>> need to be aware.
>>>
>>> Even though it is a false positive, it's probably a good idea to at
>>> least make some sort of check (and to "trick" Coverity into believing
>>> we know what we're doing). The easiest/cheapest way was to check
>>> the input 'limit' value. For the remoteDomainGetJobStats() it is
>>> passed as 0 indicating (perhaps) that the caller has done the
>>> limits length checking already and that its caller can handle
>>> allocating something that can be passed back to the caller.
>
>>
>> This unfortunately breaks the remote driver impl of GetAllDomainStats.
>> As it seems that the limit parameter isn't used for automatically
>> allocated arrays and I expected that it is I'll need either fix the
>> remote impl of the stats function or add support for checking the limit
>> here. And I probably prefer option 2, fixing
>> remoteDeserializeTypedParameters to use limit correctly even for
>> auto-alloced typed parameters.
>
> I haven't looked closely at the patch, but it is a tricky area of code.
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Executing prlctl command is not an optimal way to interact with
Parallels Cloud Server (PCS), it's better to use parallels SDK,
which is a remote API to paralles dispatcher service.
We prepared opensource version of this SDK and published it on
github, it's distributed under LGPL license. Here is a git repo:
https://github.com/Parallels/parallels-sdk.
To build with parallels SDK user should get compiler and linker
options from pkg-config 'parallels-sdk' file. So fix checks in
configure script and build with parallels SDK, if that pkg-config
file exists and add gcc options to makefile.
Signed-off-by: Dmitry Guryanov
---
configure.ac| 24 +---
src/Makefile.am | 4 +++-
2 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/configure.ac b/configure.ac
index b4fb99a..0062d5d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1046,19 +1046,21 @@ dnl
dnl Checks for the Parallels driver
dnl
-if test "$with_parallels" = "check"; then
-with_parallels=$with_linux
-if test ! $host_cpu = 'x86_64'; then
-with_parallels=no
-fi
-fi
-if test "$with_parallels" = "yes" && test "$with_linux" = "no"; then
-AC_MSG_ERROR([The Parallels driver can be enabled on Linux only.])
-fi
+if test "$with_parallels" = "yes" ||
+ test "$with_parallels" = "check"; then
+PKG_CHECK_MODULES([PARALLELS_SDK], [parallels-sdk],
+ [PARALLELS_SDK_FOUND=yes], [PARALLELS_SDK_FOUND=no])
-if test "$with_parallels" = "yes"; then
-AC_DEFINE_UNQUOTED([WITH_PARALLELS], 1, [whether Parallels driver is
enabled])
+if test "$with_parallels" = "yes" && test "$PARALLELS_SDK_FOUND" = "no";
then
+AC_MSG_ERROR([Parallels Virtualization SDK is needed to build the
Parallels driver.])
+fi
+
+with_parallels=$PARALLELS_SDK_FOUND
+if test "$with_parallels" = "yes"; then
+AC_DEFINE_UNQUOTED([WITH_PARALLELS], 1,
+ [whether Parallels driver is enabled])
+fi
fi
AM_CONDITIONAL([WITH_PARALLELS], [test "$with_parallels" = "yes"])
diff --git a/src/Makefile.am b/src/Makefile.am
index fa741a8..7fffc33 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1387,7 +1387,9 @@ if WITH_PARALLELS
noinst_LTLIBRARIES += libvirt_driver_parallels.la
libvirt_la_BUILT_LIBADD += libvirt_driver_parallels.la
libvirt_driver_parallels_la_CFLAGS = \
- -I$(top_srcdir)/src/conf $(AM_CFLAGS)
+ -I$(top_srcdir)/src/conf $(AM_CFLAGS) \
+ $(PARALLELS_SDK_CFLAGS)
+libvirt_driver_parallels_la_LIBADD = $(PARALLELS_SDK_LIBS)
libvirt_driver_parallels_la_SOURCES = $(PARALLELS_DRIVER_SOURCES)
endif WITH_PARALLELS
--
1.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Add files parallels_sdk.c and parallels_sdk.h for code
which works with SDK, so libvirt's code will not mix with
dealing with parallels SDK.
To use Parallels SDK you must first call PrlApi_InitEx function,
and then you will be able to connect to a server with
PrlSrv_LoginLocalEx function. When you've done you must call
PrlApi_Deinit. So let's call PrlApi_InitEx on first .connectOpen,
count number of connections and deinitialize, when this counter
becomes zero.
Signed-off-by: Dmitry Guryanov
---
po/POTFILES.in | 1 +
src/Makefile.am | 4 +-
src/parallels/parallels_driver.c | 16 ++-
src/parallels/parallels_sdk.c| 241 +++
src/parallels/parallels_sdk.h| 30 +
src/parallels/parallels_utils.h | 4 +
6 files changed, 294 insertions(+), 2 deletions(-)
create mode 100644 src/parallels/parallels_sdk.c
create mode 100644 src/parallels/parallels_sdk.h
diff --git a/po/POTFILES.in b/po/POTFILES.in
index f17b35f..4c1302d 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -96,6 +96,7 @@ src/openvz/openvz_driver.c
src/openvz/openvz_util.c
src/parallels/parallels_driver.c
src/parallels/parallels_network.c
+src/parallels/parallels_sdk.c
src/parallels/parallels_utils.c
src/parallels/parallels_utils.h
src/parallels/parallels_storage.c
diff --git a/src/Makefile.am b/src/Makefile.am
index 7fffc33..f2982d2 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -787,7 +787,9 @@ PARALLELS_DRIVER_SOURCES =
\
parallels/parallels_utils.c \
parallels/parallels_utils.h \
parallels/parallels_storage.c \
- parallels/parallels_network.c
+ parallels/parallels_network.c \
+ parallels/parallels_sdk.h \
+ parallels/parallels_sdk.c
BHYVE_DRIVER_SOURCES = \
bhyve/bhyve_capabilities.c \
diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c
index 13a7d95..516a296 100644
--- a/src/parallels/parallels_driver.c
+++ b/src/parallels/parallels_driver.c
@@ -55,6 +55,7 @@
#include "parallels_driver.h"
#include "parallels_utils.h"
+#include "parallels_sdk.h"
#define VIR_FROM_THIS VIR_FROM_PARALLELS
@@ -929,9 +930,17 @@ parallelsOpenDefault(virConnectPtr conn)
if (virMutexInit(&privconn->lock) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot initialize mutex"));
-goto error;
+goto err_free;
+}
+
+if (prlsdkInit(privconn)) {
+VIR_DEBUG("%s", _("Can't initialize Parallels SDK"));
+goto err_free;
}
+if (prlsdkConnect(privconn) < 0)
+goto err_free;
+
if (!(privconn->caps = parallelsBuildCapabilities()))
goto error;
@@ -953,6 +962,9 @@ parallelsOpenDefault(virConnectPtr conn)
virObjectUnref(privconn->domains);
virObjectUnref(privconn->caps);
virStoragePoolObjListFree(&privconn->pools);
+prlsdkDisconnect(privconn);
+prlsdkDeinit();
+ err_free:
VIR_FREE(privconn);
return VIR_DRV_OPEN_ERROR;
}
@@ -999,7 +1011,9 @@ parallelsConnectClose(virConnectPtr conn)
virObjectUnref(privconn->caps);
virObjectUnref(privconn->xmlopt);
virObjectUnref(privconn->domains);
+prlsdkDisconnect(privconn);
conn->privateData = NULL;
+prlsdkDeinit();
parallelsDriverUnlock(privconn);
virMutexDestroy(&privconn->lock);
diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c
new file mode 100644
index 000..1c77d27
--- /dev/null
+++ b/src/parallels/parallels_sdk.c
@@ -0,0 +1,241 @@
+/*
+ * parallels_sdk.c: core driver functions for managing
+ * Parallels Cloud Server hosts
+ *
+ * Copyright (C) 2014 Parallels, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * .
+ *
+ */
+
+#include
+
+#include "virerror.h"
+#include "viralloc.h"
+
+#include "parallels_sdk.h"
+
+#define VIR_FROM_THIS VIR_FROM_PARALLELS
+#define JOB_INFINIT_WAIT_TIMEOUT UINT_MAX
+
+PRL_UINT32 defaultJobTimeout = JOB_INFINIT_WAIT_TIMEOUT;
+
+/*
+ * Log error description
+ */
+static void
+logPrlErrorHelper(PRL_RESULT err, const char *filename,
+
On Thu, Sep 11, 2014 at 8:38 PM, Peter Krempa wrote:
> On 08/04/14 09:59, Nehal J Wani wrote:
>> This patch enables the helper program to detect event(s) triggered when there
>> is a change in lease length or expiry and client-id. This transfers complete
>> control of leases database to libvirt and obsoletes use of the lease database
>> file (.leases). That file will not be created, read, or
>> written.
>> This is achieved by adding the option --leasefile-ro to dnsmasq and passing a
>> custom env var to leaseshelper, which helps us map events related to leases
>> with their corresponding network bridges, no matter what the event be.
>>
>> Also, this requires the addition of a new non-lease entry in our custom lease
>> database: "server-duid". It is required to identify a DHCPv6 server.
>>
>> Now that dnsmasq doesn't maintain its own leases database, it relies on our
>> helper program to tell it about previous leases and server duid. Thus, this
>> patch makes our leases program honor an extra action: "init", in which it
>> sends
>> the known info in a particular format to dnsmasq by printing it to stdout.
>>
>> ---
>>
>> This is compatible with libvirt 1.2.6 as only additions have been
>> introduced, and the old leases file (*.status) will still be supported.
>>
>> v3: * Add server-duid as an entry in the lease object for every ipv6 lease.
>> * Remove unnecessary variables and double copies.
>> * Take value from DNSMASQ_OLD_HOSTNAME if hostname is not known.
>>
>> v2: http://www.redhat.com/archives/libvir-list/2014-July/msg01109.html
>>
>> v1: https://www.redhat.com/archives/libvir-list/2014-July/msg00568.html
>>
>> src/network/bridge_driver.c | 3 +
>> src/network/leaseshelper.c | 132
>> +++-
>> 2 files changed, 109 insertions(+), 26 deletions(-)
>>
>
>> diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c
>> index c8543a2..e984cbb 100644
>> --- a/src/network/leaseshelper.c
>> +++ b/src/network/leaseshelper.c
>
>
>> @@ -260,11 +275,13 @@ main(int argc, char **argv)
>> goto cleanup;
>> if (clientid && virJSONValueObjectAppendString(lease_new,
>> "client-id", clientid) < 0)
>> goto cleanup;
>> +if (server_duid &&
>> virJSONValueObjectAppendString(lease_new, "server-duid", server_duid) < 0)
>> +goto cleanup;
>> if (expirytime &&
>> virJSONValueObjectAppendNumberLong(lease_new, "expiry-time", expirytime) < 0)
>> goto cleanup;
>> }
>> }
>> -} else {
>> +} else if (action != VIR_LEASE_ACTION_INIT) {
>> fprintf(stderr, _("Unsupported action: %s\n"),
>> virLeaseActionTypeToString(action));
>> exit(EXIT_FAILURE);
>> @@ -294,7 +311,7 @@ main(int argc, char **argv)
>> i = 0;
>> while (i < virJSONValueArraySize(leases_array)) {
>> const char *ip_tmp = NULL;
>> -long long expirytime_tmp = -1;
>> +const char *server_duid_tmp = NULL;
>>
>> if (!(lease_tmp = virJSONValueArrayGet(leases_array, i))) {
>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> @@ -303,14 +320,13 @@ main(int argc, char **argv)
>> }
>>
>> if (!(ip_tmp = virJSONValueObjectGetString(lease_tmp,
>> "ip-address")) ||
>> -(virJSONValueObjectGetNumberLong(lease_tmp,
>> "expiry-time", &expirytime_tmp) < 0)) {
>> +(virJSONValueObjectGetNumberLong(lease_tmp,
>> "expiry-time", &expirytime) < 0)) {
>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> _("failed to parse json"));
>> goto cleanup;
>> }
>> -
>> /* Check whether lease has expired or not */
>> -if (expirytime_tmp < currtime) {
>> +if (expirytime < currtime) {
>> i++;
>> continue;
>> }
>> @@ -321,6 +337,30 @@ main(int argc, char **argv)
>> continue;
>> }
>>
>> +/* Store pointers to ipv4 and ipv6 leases */
>> +if (strchr(ip_tmp, ':')) {
>> +/* This is an ipv6 lease */
>
> Is there a need to separate them? Can't we just flush them out from the
> json array in the order they are stored?
>
>> +ignore_value(VIR_APPEND_ELEMENT_COPY(leases_ipv6,
>> count_ipv6, lease_tmp));
>
> It would save us the need to copy the leases separately even if dnsmasq
> isn't initializing.
I am only copying the pointer to the lease and not the lease itself.
>
> Also you should probably check the return value, otherwise you will drop
> leases silently on OOM.
Agreed.
>
>> +if ((server_duid_tmp
>> + = virJSONValueObje
On 09/11/2014 09:47 AM, Peter Krempa wrote:
> If a floppy drive isn't selected for snapshot explicitly and is empty
> don't try to snapshot it. For external snapshots this would fail as we
> can't generate a name for the snapshot from an empty drive.
Do we need the same for cdrom drives?
>
> Reported-by: Pavel Hrdina
> ---
> src/conf/snapshot_conf.c | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index c53a66b..cbaff74 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c
> @@ -561,7 +561,14 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
> if (VIR_STRDUP(disk->name, def->dom->disks[i]->dst) < 0)
> goto cleanup;
> disk->index = i;
> -disk->snapshot = def->dom->disks[i]->snapshot;
> +
> +/* Don't snapshot empty floppy drives */
> +if (def->dom->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY &&
> +!virDomainDiskGetSource(def->dom->disks[i]))
If we are worried about ALL empty drives, it would be simpler to just
drop the left side of the &&, making it solely a test of whether there
is currently a defined host source.
--
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
On 09/11/14 17:54, Francesco Romani wrote:
> - Original Message -
>> From: "Peter Krempa"
>> To: "Francesco Romani" , libvir-list@redhat.com
>> Sent: Tuesday, September 9, 2014 1:56:09 PM
>> Subject: Re: [libvirt] [PATCHv3 4/8] qemu: bulk stats: implement VCPU group
>
>>> + * VIR_DOMAIN_STATS_VCPU: Return virtual CPU statistics.
>>> + * Due to VCPU hotplug, the vcpu..* array could be sparse.
>>> + * The actual size of the array correspond to "vcpu.current".
>>> + * The array size will never exceed "vcpu.maximum".
>>> + * The typed parameter keys are in this format:
>>> + * "vcpu.current" - current number of online virtual CPUs as unsigned int.
>>> + * "vcpu.maximum" - maximum number of online virtual CPUs as unsigned int.
>>> + * "vcpu..state" - state of the virtual CPU , as int
>>> + * from virVcpuState enum.
>>> + * "vcpu..time" - virtual cpu time spent by virtual CPU
>>> + * as unsigned long long.
>>> + * "vcpu..cpu" - physical CPU pinned to virtual CPU as int.
>>
>> This is not the CPU number the vCPU is pinned to but rather the current
>> CPU number where the vCPU is actually running. If you pin it to multiple
>> CPUs this may change in the range of the host CPUs the vCPU is pinned
>> to. Said this I don't think this is an useful stat.
>
> Right, my bad, I overlooked the docs (started to suspect when saw it changing
> too often
> in my tests..).
>
> I agree this is not very useful, I'll drop it.
>
>> Rather than this I'd like to see the mask of the host CPUs where this
>> vCPU is pinned to. (returned as a human readable bitmask string).
>>
>> Any thoughts?
>
> Is that the data provided by
> http://libvirt.org/html/libvirt-libvirt.html#virDomainGetVcpuPinInfo
> it isn't? (I'm asking because docs aren't crystal clear for me).
Yes that's exactly it. You should return it as a bitmap formatted to a
string. (see virBitmapFormat).
>
> I like this, but I'd also need to do a cross-check on my our code in oVirt.
>
> Will be acceptable to drop the misleading vcpu..cpu info and to add
> the pin info in a new followup patch, in this stats group?
You definitely can add that later on. But you should drop .cpu from this
patch (not revert it later).
>
> Bests,
>
Peter
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/11/2014 04:38 AM, Peter Krempa wrote:
> On 09/05/14 00:26, John Ferlan wrote:
>> Since 98b9acf5aa02551dd37d0209339aba2e22e4004a
>>
>> This ends up being a false positive for two reasons...
>>
>> expected to be already allocated and thus is passed by value; whereas,
>> the call into remoteDomainGetJobStats() 'params' is passed by reference.
>> Thus if the VIR_ALLOC is done there is no way for it to be leaked for
>> callers that passed by value.
>>
>> path that handles 'nparams == 0 && params == NULL' on entry. Thus all
>> other callers have guaranteed that 'params' is non NULL. Of course
>> Coverity isn't wise enough to pick up on this, but nonetheless is
>> does point out something "future callers" for which future callers
>> need to be aware.
>>
>> Even though it is a false positive, it's probably a good idea to at
>> least make some sort of check (and to "trick" Coverity into believing
>> we know what we're doing). The easiest/cheapest way was to check
>> the input 'limit' value. For the remoteDomainGetJobStats() it is
>> passed as 0 indicating (perhaps) that the caller has done the
>> limits length checking already and that its caller can handle
>> allocating something that can be passed back to the caller.
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/remote/remote_driver.c | 12
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
>> index 915e8e5..4b4644d 100644
>> --- a/src/remote/remote_driver.c
>> +++ b/src/remote/remote_driver.c
>> @@ -1761,6 +1761,18 @@ remoteDeserializeTypedParameters(remote_typed_param
>> *ret_params_val,
>> goto cleanup;
>> }
>> } else {
>> +/* For callers that can return this allocated buffer back to their
>> + * caller, pass a 0 in the 'limit' field indicating that the
>> + * ret_params_len MAX checking has already occurred *and* that
>> + * the caller has passed 'params' by reference; otherwise, a
>> + * caller that receives the 'params' by value will potentially
>> + * leak memory we're allocating here
>> + */
>> +if (limit != 0) {
>> +virReportError(VIR_ERR_RPC, "%s",
>> + _("invalid call - params is NULL on input"));
>> +goto cleanup;
>> +}
>> if (VIR_ALLOC_N(*params, ret_params_len) < 0)
>> goto cleanup;
>> }
>>
>
> This unfortunately breaks the remote driver impl of GetAllDomainStats.
> As it seems that the limit parameter isn't used for automatically
> allocated arrays and I expected that it is I'll need either fix the
> remote impl of the stats function or add support for checking the limit
> here. And I probably prefer option 2, fixing
> remoteDeserializeTypedParameters to use limit correctly even for
> auto-alloced typed parameters.
>
> Peter
>
Hmm... yeah after a bit of digging I see - the '&ret->params' is a
virTypedParameterPtr which yes, will be NULL on input, ... Of
course 'limit' never being checked could have led to other problems down
the road, but I don't think we're there yet.
The "good" news is it seems a comparison
if (rec->params.params_len > REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX) {
virReportError(VIR_ERR_RPC, "%s",
_("returned number of parameters exceeds limit"));
goto cleanup;
}
Prior to the (VIR_ALLOC(elem) < 0) check would suffice as well as
passing the '0' in the 3rd (or limit) param.
If we don't do the limit != 0 check, then other callers of
remoteDeserializeTypedParameters() would probably need some sort of /*
coverity[resource_leak] */ comment or the remoteDomainGetJobStats would
have to change to not pass 0 if the decision was to adjust the logic in
remoteDeserializeTypedParameters
I was merely using that 0 passing as a "marker" since 'limit' wasn't
checked/used in the auto allocated case. It was a whole lot easier,
cheaper, simpler than the alternatives.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
- Original Message -
> From: "Peter Krempa"
> To: "Francesco Romani" , libvir-list@redhat.com
> Sent: Tuesday, September 9, 2014 1:56:09 PM
> Subject: Re: [libvirt] [PATCHv3 4/8] qemu: bulk stats: implement VCPU group
> > + * VIR_DOMAIN_STATS_VCPU: Return virtual CPU statistics.
> > + * Due to VCPU hotplug, the vcpu..* array could be sparse.
> > + * The actual size of the array correspond to "vcpu.current".
> > + * The array size will never exceed "vcpu.maximum".
> > + * The typed parameter keys are in this format:
> > + * "vcpu.current" - current number of online virtual CPUs as unsigned int.
> > + * "vcpu.maximum" - maximum number of online virtual CPUs as unsigned int.
> > + * "vcpu..state" - state of the virtual CPU , as int
> > + * from virVcpuState enum.
> > + * "vcpu..time" - virtual cpu time spent by virtual CPU
> > + * as unsigned long long.
> > + * "vcpu..cpu" - physical CPU pinned to virtual CPU as int.
>
> This is not the CPU number the vCPU is pinned to but rather the current
> CPU number where the vCPU is actually running. If you pin it to multiple
> CPUs this may change in the range of the host CPUs the vCPU is pinned
> to. Said this I don't think this is an useful stat.
Right, my bad, I overlooked the docs (started to suspect when saw it changing
too often
in my tests..).
I agree this is not very useful, I'll drop it.
> Rather than this I'd like to see the mask of the host CPUs where this
> vCPU is pinned to. (returned as a human readable bitmask string).
>
> Any thoughts?
Is that the data provided by
http://libvirt.org/html/libvirt-libvirt.html#virDomainGetVcpuPinInfo
it isn't? (I'm asking because docs aren't crystal clear for me).
I like this, but I'd also need to do a cross-check on my our code in oVirt.
Will be acceptable to drop the misleading vcpu..cpu info and to add
the pin info in a new followup patch, in this stats group?
Bests,
--
Francesco Romani
RedHat Engineering Virtualization R & D
Phone: 8261328
IRC: fromani
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/11/2014 02:38 AM, Peter Krempa wrote:
> On 09/05/14 00:26, John Ferlan wrote:
>> Since 98b9acf5aa02551dd37d0209339aba2e22e4004a
>>
>> This ends up being a false positive for two reasons...
>>
>> expected to be already allocated and thus is passed by value; whereas,
>> the call into remoteDomainGetJobStats() 'params' is passed by reference.
>> Thus if the VIR_ALLOC is done there is no way for it to be leaked for
>> callers that passed by value.
>>
>> path that handles 'nparams == 0 && params == NULL' on entry. Thus all
Careful, my virDomainBlockCopy API passes nparams==0 && params ==
(non-NULL 0-length array) from the RPC daemon/remote.c receiver into the
libvirtd side of the API call. It tripped me up the first time I tried
to assert that nparams==0 implied params==NULL, and failed the test.
>> other callers have guaranteed that 'params' is non NULL. Of course
>> Coverity isn't wise enough to pick up on this, but nonetheless is
>> does point out something "future callers" for which future callers
>> need to be aware.
>>
>> Even though it is a false positive, it's probably a good idea to at
>> least make some sort of check (and to "trick" Coverity into believing
>> we know what we're doing). The easiest/cheapest way was to check
>> the input 'limit' value. For the remoteDomainGetJobStats() it is
>> passed as 0 indicating (perhaps) that the caller has done the
>> limits length checking already and that its caller can handle
>> allocating something that can be passed back to the caller.
>
> This unfortunately breaks the remote driver impl of GetAllDomainStats.
> As it seems that the limit parameter isn't used for automatically
> allocated arrays and I expected that it is I'll need either fix the
> remote impl of the stats function or add support for checking the limit
> here. And I probably prefer option 2, fixing
> remoteDeserializeTypedParameters to use limit correctly even for
> auto-alloced typed parameters.
I haven't looked closely at the patch, but it is a tricky area of code.
--
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
On 08/04/14 09:59, Nehal J Wani wrote:
> This patch enables the helper program to detect event(s) triggered when there
> is a change in lease length or expiry and client-id. This transfers complete
> control of leases database to libvirt and obsoletes use of the lease database
> file (.leases). That file will not be created, read, or written.
> This is achieved by adding the option --leasefile-ro to dnsmasq and passing a
> custom env var to leaseshelper, which helps us map events related to leases
> with their corresponding network bridges, no matter what the event be.
>
> Also, this requires the addition of a new non-lease entry in our custom lease
> database: "server-duid". It is required to identify a DHCPv6 server.
>
> Now that dnsmasq doesn't maintain its own leases database, it relies on our
> helper program to tell it about previous leases and server duid. Thus, this
> patch makes our leases program honor an extra action: "init", in which it
> sends
> the known info in a particular format to dnsmasq by printing it to stdout.
>
> ---
>
> This is compatible with libvirt 1.2.6 as only additions have been
> introduced, and the old leases file (*.status) will still be supported.
>
> v3: * Add server-duid as an entry in the lease object for every ipv6 lease.
> * Remove unnecessary variables and double copies.
> * Take value from DNSMASQ_OLD_HOSTNAME if hostname is not known.
>
> v2: http://www.redhat.com/archives/libvir-list/2014-July/msg01109.html
>
> v1: https://www.redhat.com/archives/libvir-list/2014-July/msg00568.html
>
> src/network/bridge_driver.c | 3 +
> src/network/leaseshelper.c | 132
> +++-
> 2 files changed, 109 insertions(+), 26 deletions(-)
>
> diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c
> index c8543a2..e984cbb 100644
> --- a/src/network/leaseshelper.c
> +++ b/src/network/leaseshelper.c
> @@ -260,11 +275,13 @@ main(int argc, char **argv)
> goto cleanup;
> if (clientid && virJSONValueObjectAppendString(lease_new,
> "client-id", clientid) < 0)
> goto cleanup;
> +if (server_duid && virJSONValueObjectAppendString(lease_new,
> "server-duid", server_duid) < 0)
> +goto cleanup;
> if (expirytime &&
> virJSONValueObjectAppendNumberLong(lease_new, "expiry-time", expirytime) < 0)
> goto cleanup;
> }
> }
> -} else {
> +} else if (action != VIR_LEASE_ACTION_INIT) {
> fprintf(stderr, _("Unsupported action: %s\n"),
> virLeaseActionTypeToString(action));
> exit(EXIT_FAILURE);
> @@ -294,7 +311,7 @@ main(int argc, char **argv)
> i = 0;
> while (i < virJSONValueArraySize(leases_array)) {
> const char *ip_tmp = NULL;
> -long long expirytime_tmp = -1;
> +const char *server_duid_tmp = NULL;
>
> if (!(lease_tmp = virJSONValueArrayGet(leases_array, i))) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> @@ -303,14 +320,13 @@ main(int argc, char **argv)
> }
>
> if (!(ip_tmp = virJSONValueObjectGetString(lease_tmp,
> "ip-address")) ||
> -(virJSONValueObjectGetNumberLong(lease_tmp,
> "expiry-time", &expirytime_tmp) < 0)) {
> +(virJSONValueObjectGetNumberLong(lease_tmp,
> "expiry-time", &expirytime) < 0)) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("failed to parse json"));
> goto cleanup;
> }
> -
> /* Check whether lease has expired or not */
> -if (expirytime_tmp < currtime) {
> +if (expirytime < currtime) {
> i++;
> continue;
> }
> @@ -321,6 +337,30 @@ main(int argc, char **argv)
> continue;
> }
>
> +/* Store pointers to ipv4 and ipv6 leases */
> +if (strchr(ip_tmp, ':')) {
> +/* This is an ipv6 lease */
Is there a need to separate them? Can't we just flush them out from the
json array in the order they are stored?
> +ignore_value(VIR_APPEND_ELEMENT_COPY(leases_ipv6,
> count_ipv6, lease_tmp));
It would save us the need to copy the leases separately even if dnsmasq
isn't initializing.
Also you should probably check the return value, otherwise you will drop
leases silently on OOM.
> +if ((server_duid_tmp
> + = virJSONValueObjectGetString(lease_tmp,
> "server-duid"))) {
> +if (!server_duid) {
> +/* Control reaches here when the 'action' is not
> for an
> + * ipv6 lease or, for some w
some comments below
On 09/11/14 14:09, Michal Privoznik wrote:
> When a domain is undefined, there are options to remove it's
> managed save state or snapshots. However, there's another file
> that libvirt creates per domain: the NVRAM variable store file.
> Make sure that the file is not left behind if the domain is
> undefined.
>
> Signed-off-by: Michal Privoznik
> ---
> include/libvirt/libvirt.h.in | 2 ++
> src/qemu/qemu_driver.c | 19 ++-
> tools/virsh-domain.c | 15 ---
> tools/virsh.pod | 6 +-
> 4 files changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index 94b942c..3c2a51a 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -2257,6 +2257,8 @@ typedef enum {
> VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA = (1 << 1), /* If last use of
> domain,
>then also remove
> any
>snapshot metadata
> */
> +VIR_DOMAIN_UNDEFINE_NVRAM = (1 << 2), /* Also remove any
> + nvram file */
>
> /* Future undefine control flags should come here. */
> } virDomainUndefineFlagsValues;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index a8cda43..7f17fee 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6403,7 +6403,8 @@ qemuDomainUndefineFlags(virDomainPtr dom,
> virQEMUDriverConfigPtr cfg = NULL;
>
> virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE |
> - VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, -1);
> + VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA |
> + VIR_DOMAIN_UNDEFINE_NVRAM, -1);
>
> if (!(vm = qemuDomObjFromDomain(dom)))
> return -1;
> @@ -6452,6 +6453,22 @@ qemuDomainUndefineFlags(virDomainPtr dom,
> }
> }
>
Seems okay and consistent with the rest of the code thus far. OK.
> +if (!virDomainObjIsActive(vm) &&
> +vm->def->os.loader && vm->def->os.loader->nvram) {
> +if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
> +virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("cannot delete inactive domain with nvram"));
> +goto cleanup;
> +}
> +
> +if (unlink(vm->def->os.loader->nvram) < 0) {
> +virReportSystemError(errno,
> + _("failed to remove nvram: %s"),
> + vm->def->os.loader->nvram);
> +goto cleanup;
> +}
> +}
> +
Again, this seems consistent with the rest of the code, eg. with the
"cannot delete inactive domain with %d snapshots".
I slightly wonder what happens when you succeed in the above, bute then
the following fails:
> if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0)
> goto cleanup;
I guess the full undefine operation will fail in this case. And if the
user retries the undefine, then even the unlink() will fail (because the
nvram file won't exist any longer).
(1) After staring a bit longer at qemuDomainUndefineFlags(): can you
please *further* restrict this code with a check for virFileExists()?
Namely, the VIR_DOMAIN_UNDEFINE_MANAGED_SAVE flag does something very
similar (including an unlink()), and that one is protected with a
virFileExists().
I'll leave it up to you if you add the && to the top IF, or if you just
short-circuit the unlink() with it (as in, (!virFileExists() && unlink()
< 0)). I think it would be more useful to restrict the outer condition
with it -- there's no point in complaining about the lack of
VIR_DOMAIN_UNDEFINE_NVRAM of the nvram file doesn't exist anyway.
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 6aa8631..db52271 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -3248,6 +3248,10 @@ static const vshCmdOptDef opts_undefine[] = {
> .type = VSH_OT_BOOL,
> .help = N_("remove all domain snapshot metadata, if inactive")
> },
> +{.name = "nvram",
> + .type = VSH_OT_BOOL,
> + .help = N_("remove nvram file, if inactive")
> +},
> {.name = NULL}
> };
>
> @@ -3270,6 +3274,7 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
> bool snapshots_metadata = vshCommandOptBool(cmd, "snapshots-metadata");
> bool wipe_storage = vshCommandOptBool(cmd, "wipe-storage");
> bool remove_all_storage = vshCommandOptBool(cmd, "remove-all-storage");
> +bool nvram = vshCommandOptBool(cmd, "nvram");
> /* Positive if these items exist. */
> int has_managed_save = 0;
> int has_snapshots_metadata = 0;
> @@ -3313,6 +3318,9 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
> flags |= VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA;
> snapshots_safe = true;
> }
> +if
On 09/11/2014 04:25 PM, Michal Privoznik wrote:
> On 11.09.2014 16:07, Laszlo Ersek wrote:
>> On 09/11/14 14:09, Michal Privoznik wrote:
>>> I've noticed two problem with the automatically created NVRAM varstore
>>> file. The first, even though I run qemu as root:root for some reason I
>>> get Permission denied when trying to open the _VARS.fd file. The
>>> problem is, the upper directory misses execute permissions, which in
>>> combination with us dropping some capabilities result in EPERM.
>>>
>>> The next thing is, that if I switch SELinux to enforcing mode, I get
>>> another EPERM because the vars file is not labeled correctly. It is
>>> passed to qemu as disk and hence should be labelled as disk. QEMU may
>>> write to it eventually, so this is different to kernel or initrd.
>>>
>>> Signed-off-by: Michal Privoznik
>>> ---
>>> libvirt.spec.in | 2 +-
>>> src/security/security_selinux.c | 5 -
>>> 2 files changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libvirt.spec.in b/libvirt.spec.in
>>> index a6a58cf..ecf160b 100644
>>> --- a/libvirt.spec.in
>>> +++ b/libvirt.spec.in
>>> @@ -1938,7 +1938,7 @@ exit 0
>>> %dir %attr(0750, %{qemu_user}, %{qemu_group})
>>> %{_localstatedir}/lib/libvirt/qemu/
>>> %dir %attr(0750, %{qemu_user}, %{qemu_group})
>>> %{_localstatedir}/lib/libvirt/qemu/channel/
>>> %dir %attr(0750, %{qemu_user}, %{qemu_group})
>>> %{_localstatedir}/lib/libvirt/qemu/channel/target/
>>> -%dir %attr(0750, %{qemu_user}, %{qemu_group})
>>> %{_localstatedir}/lib/libvirt/qemu/nvram/
>>> +%dir %attr(0711, %{qemu_user}, %{qemu_group})
>>> %{_localstatedir}/lib/libvirt/qemu/nvram/
>>> %dir %attr(0750, %{qemu_user}, %{qemu_group})
>>> %{_localstatedir}/cache/libvirt/qemu/
>>> %{_datadir}/augeas/lenses/libvirtd_qemu.aug
>>> %{_datadir}/augeas/lenses/tests/test_libvirtd_qemu.aug
>>> diff --git a/src/security/security_selinux.c
>>> b/src/security/security_selinux.c
>>> index bf67fb5..3db2b27 100644
>>> --- a/src/security/security_selinux.c
>>> +++ b/src/security/security_selinux.c
>>> @@ -2300,8 +2300,11 @@
>>> virSecuritySELinuxSetSecurityAllLabel(virSecurityManagerPtr mgr,
>>>mgr) < 0)
>>> return -1;
>>>
>>> +/* This is different than kernel or initrd. The nvram store
>>> + * is really a disk, qemu can read and write to it. */
>>> if (def->os.loader && def->os.loader->nvram &&
>>> -virSecuritySELinuxSetFilecon(def->os.loader->nvram,
>>> data->content_context) < 0)
>>> +secdef && secdef->imagelabel &&
>>> +virSecuritySELinuxSetFilecon(def->os.loader->nvram,
>>> secdef->imagelabel) < 0)
>>> return -1;
>>>
>>> if (def->os.kernel &&
>>>
>>
>> Good detective work!
>>
>> Regarding the g+x,o+x change on
>> %{_localstatedir}/lib/libvirt/qemu/nvram. This change theoretically
>> allows a qemu instance to "probe" for the presence of "foreign" varstore
>> files (it won't be able to open any, but eg. error codes for open()
>> would differ between ENOENT vs. EACCES, and stat() would fail vs.
>> succeed). However I think we can live with this, and anyway, it's simply
>> impossible to open a file in directory D if directory D doesn't provide
>> the user with search permission. So that looks like a must.
>
> Indeed. Moreover, I was first surprised that even if was running qemu under
> root:root I got EACCES. Problem was, libvirt drops nearly all caps (even
> CAP_SYS_ADMIN) after fork and prior to executing qemu binary.
>
I believe CAP_DAC_OVERRIDE is the capability.
Jan
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11.09.2014 16:07, Laszlo Ersek wrote:
On 09/11/14 14:09, Michal Privoznik wrote:
I've noticed two problem with the automatically created NVRAM varstore
file. The first, even though I run qemu as root:root for some reason I
get Permission denied when trying to open the _VARS.fd file. The
problem is, the upper directory misses execute permissions, which in
combination with us dropping some capabilities result in EPERM.
The next thing is, that if I switch SELinux to enforcing mode, I get
another EPERM because the vars file is not labeled correctly. It is
passed to qemu as disk and hence should be labelled as disk. QEMU may
write to it eventually, so this is different to kernel or initrd.
Signed-off-by: Michal Privoznik
---
libvirt.spec.in | 2 +-
src/security/security_selinux.c | 5 -
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index a6a58cf..ecf160b 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1938,7 +1938,7 @@ exit 0
%dir %attr(0750, %{qemu_user}, %{qemu_group})
%{_localstatedir}/lib/libvirt/qemu/
%dir %attr(0750, %{qemu_user}, %{qemu_group})
%{_localstatedir}/lib/libvirt/qemu/channel/
%dir %attr(0750, %{qemu_user}, %{qemu_group})
%{_localstatedir}/lib/libvirt/qemu/channel/target/
-%dir %attr(0750, %{qemu_user}, %{qemu_group})
%{_localstatedir}/lib/libvirt/qemu/nvram/
+%dir %attr(0711, %{qemu_user}, %{qemu_group})
%{_localstatedir}/lib/libvirt/qemu/nvram/
%dir %attr(0750, %{qemu_user}, %{qemu_group})
%{_localstatedir}/cache/libvirt/qemu/
%{_datadir}/augeas/lenses/libvirtd_qemu.aug
%{_datadir}/augeas/lenses/tests/test_libvirtd_qemu.aug
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index bf67fb5..3db2b27 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -2300,8 +2300,11 @@
virSecuritySELinuxSetSecurityAllLabel(virSecurityManagerPtr mgr,
mgr) < 0)
return -1;
+/* This is different than kernel or initrd. The nvram store
+ * is really a disk, qemu can read and write to it. */
if (def->os.loader && def->os.loader->nvram &&
-virSecuritySELinuxSetFilecon(def->os.loader->nvram,
data->content_context) < 0)
+secdef && secdef->imagelabel &&
+virSecuritySELinuxSetFilecon(def->os.loader->nvram, secdef->imagelabel)
< 0)
return -1;
if (def->os.kernel &&
Good detective work!
Regarding the g+x,o+x change on
%{_localstatedir}/lib/libvirt/qemu/nvram. This change theoretically
allows a qemu instance to "probe" for the presence of "foreign" varstore
files (it won't be able to open any, but eg. error codes for open()
would differ between ENOENT vs. EACCES, and stat() would fail vs.
succeed). However I think we can live with this, and anyway, it's simply
impossible to open a file in directory D if directory D doesn't provide
the user with search permission. So that looks like a must.
Indeed. Moreover, I was first surprised that even if was running qemu
under root:root I got EACCES. Problem was, libvirt drops nearly all caps
(even CAP_SYS_ADMIN) after fork and prior to executing qemu binary.
Regarding the seclabel / context, I agree that it should have a label
consistent with other disk image files; for qemu it's just a -drive
after all. The hunk in question looks consistent with the rest of
"src/security/security_selinux.c".
Acked-by: Laszlo Ersek
Thanks, applied.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/11/14 14:09, Michal Privoznik wrote:
> I've noticed two problem with the automatically created NVRAM varstore
> file. The first, even though I run qemu as root:root for some reason I
> get Permission denied when trying to open the _VARS.fd file. The
> problem is, the upper directory misses execute permissions, which in
> combination with us dropping some capabilities result in EPERM.
>
> The next thing is, that if I switch SELinux to enforcing mode, I get
> another EPERM because the vars file is not labeled correctly. It is
> passed to qemu as disk and hence should be labelled as disk. QEMU may
> write to it eventually, so this is different to kernel or initrd.
>
> Signed-off-by: Michal Privoznik
> ---
> libvirt.spec.in | 2 +-
> src/security/security_selinux.c | 5 -
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index a6a58cf..ecf160b 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -1938,7 +1938,7 @@ exit 0
> %dir %attr(0750, %{qemu_user}, %{qemu_group})
> %{_localstatedir}/lib/libvirt/qemu/
> %dir %attr(0750, %{qemu_user}, %{qemu_group})
> %{_localstatedir}/lib/libvirt/qemu/channel/
> %dir %attr(0750, %{qemu_user}, %{qemu_group})
> %{_localstatedir}/lib/libvirt/qemu/channel/target/
> -%dir %attr(0750, %{qemu_user}, %{qemu_group})
> %{_localstatedir}/lib/libvirt/qemu/nvram/
> +%dir %attr(0711, %{qemu_user}, %{qemu_group})
> %{_localstatedir}/lib/libvirt/qemu/nvram/
> %dir %attr(0750, %{qemu_user}, %{qemu_group})
> %{_localstatedir}/cache/libvirt/qemu/
> %{_datadir}/augeas/lenses/libvirtd_qemu.aug
> %{_datadir}/augeas/lenses/tests/test_libvirtd_qemu.aug
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index bf67fb5..3db2b27 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -2300,8 +2300,11 @@
> virSecuritySELinuxSetSecurityAllLabel(virSecurityManagerPtr mgr,
> mgr) < 0)
> return -1;
>
> +/* This is different than kernel or initrd. The nvram store
> + * is really a disk, qemu can read and write to it. */
> if (def->os.loader && def->os.loader->nvram &&
> -virSecuritySELinuxSetFilecon(def->os.loader->nvram,
> data->content_context) < 0)
> +secdef && secdef->imagelabel &&
> +virSecuritySELinuxSetFilecon(def->os.loader->nvram,
> secdef->imagelabel) < 0)
> return -1;
>
> if (def->os.kernel &&
>
Good detective work!
Regarding the g+x,o+x change on
%{_localstatedir}/lib/libvirt/qemu/nvram. This change theoretically
allows a qemu instance to "probe" for the presence of "foreign" varstore
files (it won't be able to open any, but eg. error codes for open()
would differ between ENOENT vs. EACCES, and stat() would fail vs.
succeed). However I think we can live with this, and anyway, it's simply
impossible to open a file in directory D if directory D doesn't provide
the user with search permission. So that looks like a must.
Regarding the seclabel / context, I agree that it should have a label
consistent with other disk image files; for qemu it's just a -drive
after all. The hunk in question looks consistent with the rest of
"src/security/security_selinux.c".
Acked-by: Laszlo Ersek
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/11/2014 03:13 PM, Peter Krempa wrote:
On 09/11/14 15:12, Pavel Hrdina wrote:
On 09/11/2014 03:03 PM, Peter Krempa wrote:
Subject doesn't describe what caused the build to fail.
On 09/11/14 14:57, Pavel Hrdina wrote:
Neither the rest of the commit message.
The build failed because of missing "sys/syscall.h".
Will update the commit message, thanks.
Signed-off-by: Pavel Hrdina
---
src/util/virprocess.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 15d8309..3dae1bd 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -28,7 +28,6 @@
#include
#include
#include
-#include
#if HAVE_SETRLIMIT
# include
# include
@@ -78,10 +77,21 @@ VIR_LOG_INIT("util.process");
#endif
#ifndef HAVE_SETNS
Is this set on the windows build? That's strange. Shouldn't we fix the
make system to avoid it?
This is a workaround if the HAVE_SETNS is not defined because old glibc
may not have a wrapper for this syscall. And it obviously isn't defined
for windows.
Aaah, right. I overlooked the "n" in ifndef ...
ACK if you add the commit message then.
Peter
Pushed, thanks.
Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/11/14 15:12, Pavel Hrdina wrote:
> On 09/11/2014 03:03 PM, Peter Krempa wrote:
>> Subject doesn't describe what caused the build to fail.
>>
>> On 09/11/14 14:57, Pavel Hrdina wrote:
>>
>> Neither the rest of the commit message.
>
> The build failed because of missing "sys/syscall.h".
>
> Will update the commit message, thanks.
>
>>
>>> Signed-off-by: Pavel Hrdina
>>> ---
>>> src/util/virprocess.c | 12 +++-
>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
>>> index 15d8309..3dae1bd 100644
>>> --- a/src/util/virprocess.c
>>> +++ b/src/util/virprocess.c
>>> @@ -28,7 +28,6 @@
>>> #include
>>> #include
>>> #include
>>> -#include
>>> #if HAVE_SETRLIMIT
>>> # include
>>> # include
>>> @@ -78,10 +77,21 @@ VIR_LOG_INIT("util.process");
>>> #endif
>>>
>>> #ifndef HAVE_SETNS
>>
>> Is this set on the windows build? That's strange. Shouldn't we fix the
>> make system to avoid it?
>>
>
> This is a workaround if the HAVE_SETNS is not defined because old glibc
> may not have a wrapper for this syscall. And it obviously isn't defined
> for windows.
Aaah, right. I overlooked the "n" in ifndef ...
ACK if you add the commit message then.
Peter
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/11/2014 03:03 PM, Peter Krempa wrote:
Subject doesn't describe what caused the build to fail.
On 09/11/14 14:57, Pavel Hrdina wrote:
Neither the rest of the commit message.
The build failed because of missing "sys/syscall.h".
Will update the commit message, thanks.
Signed-off-by: Pavel Hrdina
---
src/util/virprocess.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 15d8309..3dae1bd 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -28,7 +28,6 @@
#include
#include
#include
-#include
#if HAVE_SETRLIMIT
# include
# include
@@ -78,10 +77,21 @@ VIR_LOG_INIT("util.process");
#endif
#ifndef HAVE_SETNS
Is this set on the windows build? That's strange. Shouldn't we fix the
make system to avoid it?
This is a workaround if the HAVE_SETNS is not defined because old glibc
may not have a wrapper for this syscall. And it obviously isn't defined
for windows.
Pavel
+# ifndef WIN32
+# include
+
static inline int setns(int fd, int nstype)
{
return syscall(__NR_setns, fd, nstype);
}
+# else
+static inline int setns(int fd ATTRIBUTE_UNUSED, int nstype ATTRIBUTE_UNUSED)
+{
+virReportSystemError(ENOSYS, "%s",
+ _("Namespaces are not supported on windows."));
+return -1;
+}
+# endif /* WIN32 */
#endif /* HAVE_SETNS */
/**
Peter
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/04/2014 06:26 PM, John Ferlan wrote:
> Sorry for the large dump, but before I got too involved in other things
> I figured I'd go through the list of the remaining 68 Coverity issues
> from the new version in order to reduce the pile. Many are benign, some
> seemingly false positives, and I think most are error paths. The one
> non error path that does stick out is the qemu_driver.c changes in the
> qemuDomainSetBlkioParameters() routine where 'param' and 'params' were
> used differently between LIVE and CONFIG. In particular, in CONFIG the
> use of 'params->field' instead of 'param->field'.
>
> One that does bear looking at more closely and if someone has a better
> idea is avoiding a false positive resource_leak in remote_driver.c. I
> left a healthy comment in the code - you'll know when you see it.
>
> These patches get the numbers down to 19 issues. Of the remaining
> issues - some are related to Coverity thinking that 'mgetgroups' could
> return a negative value with an allocated groups structure (which I'm
> still scratching my head over). There is also a few calls to
> virJSONValueObjectGetNumberUlong() in qemu_monitor_json.c that don't
> check status, but I'm not sure why - just didn't have the research
> cycles for that.
>
> John Ferlan (26):
> qemu_driver: Resolve Coverity COPY_PASTE_ERROR
> remote_driver: Resolve Coverity RESOURCE_LEAK
> storage: Resolve Coverity UNUSED_VALUE
> vbox: Resolve Coverity UNUSED_VALUE
> qemu: Resolve Coverity REVERSE_INULL
> storage: Resolve Coverity OVERFLOW_BEFORE_WIDEN
> virsh: Resolve Coverity DEADCODE
> virfile: Resolve Coverity DEADCODE
> virsh: Resolve Coverity DEADCODE
> qemu: Resolve Coverity DEADCODE
> tests: Resolve Coverity DEADCODE
> virsh: Resolve Coverity DEADCODE
> qemu: Resolve Coverity FORWARD_NULL
> lxc: Resolve Coverity FORWARD_NULL
> qemu: Resolve Coverity FORWARD_NULL
> network: Resolve Coverity FORWARD_NULL
> virstring: Resolve Coverity FORWARD_NULL
> qemu: Resolve Coverity FORWARD_NULL
> network_conf: Resolve Coverity FORWARD_NULL
> qemu: Resolve Coverity NEGATIVE_RETURNS
> nodeinfo: Resolve Coverity NEGATIVE_RETURNS
> virsh: Resolve Coverity NEGATIVE_RETURNS
> xen: Resolve Coverity NEGATIVE_RETURNS
> qemu: Resolve Coverity NEGATIVE_RETURNS
> qemu: Resolve Coverity NEGATIVE_RETURNS
> libxl: Resolve Coverity NULL_RETURNS
>
> src/conf/network_conf.c| 4 ++--
> src/libxl/libxl_migration.c| 1 -
> src/lxc/lxc_driver.c | 6 --
> src/network/leaseshelper.c | 3 +--
> src/nodeinfo.c | 2 +-
> src/qemu/qemu_capabilities.c | 2 +-
> src/qemu/qemu_command.c| 1 +
> src/qemu/qemu_driver.c | 26 +++---
> src/qemu/qemu_migration.c | 3 ++-
> src/qemu/qemu_monitor_json.c | 2 +-
> src/qemu/qemu_process.c| 5 +++--
> src/remote/remote_driver.c | 12
> src/storage/storage_backend_disk.c | 2 +-
> src/storage/storage_backend_fs.c | 1 -
> src/util/virfile.c | 5 ++---
> src/util/virstring.c | 3 +++
> src/vbox/vbox_common.c | 9 +++--
> src/xen/xend_internal.c| 3 ++-
> tests/virstringtest.c | 5 +
> tools/virsh-domain.c | 22 --
> tools/virsh-edit.c | 9 -
> tools/virsh-interface.c| 3 ---
> tools/virsh-network.c | 12 +---
> tools/virsh-nwfilter.c | 3 ---
> tools/virsh-pool.c | 3 ---
> tools/virsh-snapshot.c | 3 ---
> 26 files changed, 76 insertions(+), 74 deletions(-)
>
Thanks for taking the time to review...
I have pushed those that were ACK'd:
1, 3-21, 23-26
Modifying 8 to remove the parentheses around the (errno == EINTR)
Modifying 9 to change the ternary into an if/else, restoring the typestr
= NULL; initializer. Also changed the commit message to match the change
made...
Modifying 21 to remove the "if (cpus < 0) VIR_FREE(cpus)" since the code
above had already handled clearing cpus
Leaving
2, 22
To revisit in a v2
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
I've noticed two problem with the automatically created NVRAM varstore
file. The first, even though I run qemu as root:root for some reason I
get Permission denied when trying to open the _VARS.fd file. The
problem is, the upper directory misses execute permissions, which in
combination with us dropping some capabilities result in EPERM.
The next thing is, that if I switch SELinux to enforcing mode, I get
another EPERM because the vars file is not labeled correctly. It is
passed to qemu as disk and hence should be labelled as disk. QEMU may
write to it eventually, so this is different to kernel or initrd.
Signed-off-by: Michal Privoznik
---
libvirt.spec.in | 2 +-
src/security/security_selinux.c | 5 -
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index a6a58cf..ecf160b 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1938,7 +1938,7 @@ exit 0
%dir %attr(0750, %{qemu_user}, %{qemu_group})
%{_localstatedir}/lib/libvirt/qemu/
%dir %attr(0750, %{qemu_user}, %{qemu_group})
%{_localstatedir}/lib/libvirt/qemu/channel/
%dir %attr(0750, %{qemu_user}, %{qemu_group})
%{_localstatedir}/lib/libvirt/qemu/channel/target/
-%dir %attr(0750, %{qemu_user}, %{qemu_group})
%{_localstatedir}/lib/libvirt/qemu/nvram/
+%dir %attr(0711, %{qemu_user}, %{qemu_group})
%{_localstatedir}/lib/libvirt/qemu/nvram/
%dir %attr(0750, %{qemu_user}, %{qemu_group})
%{_localstatedir}/cache/libvirt/qemu/
%{_datadir}/augeas/lenses/libvirtd_qemu.aug
%{_datadir}/augeas/lenses/tests/test_libvirtd_qemu.aug
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index bf67fb5..3db2b27 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -2300,8 +2300,11 @@
virSecuritySELinuxSetSecurityAllLabel(virSecurityManagerPtr mgr,
mgr) < 0)
return -1;
+/* This is different than kernel or initrd. The nvram store
+ * is really a disk, qemu can read and write to it. */
if (def->os.loader && def->os.loader->nvram &&
-virSecuritySELinuxSetFilecon(def->os.loader->nvram,
data->content_context) < 0)
+secdef && secdef->imagelabel &&
+virSecuritySELinuxSetFilecon(def->os.loader->nvram,
secdef->imagelabel) < 0)
return -1;
if (def->os.kernel &&
--
1.8.5.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/05/14 00:26, John Ferlan wrote:
> Since 0766783abbe8bbc9ea686c2c3149f4c0ac139e19
>
> Coverity complains that the EDIT_FREE definition results in DEADCODE.
>
> As it turns out with the change to use the EDIT_FREE macro the call to
> vir*Free() wouldn't be necessary nor would it happen...
>
> Prior code to above commitid would :
>
> vir*Ptr foo = NULL;
> ...
> foo = vir*GetXMLDesc()
> ...
> vir*Free(foo);
> foo = vir*DefineXML()
> ...
>
> And thus the free was needed. With the change to use EDIT_FREE the
> same code changed to:
>
> vir*Ptr foo = NULL;
> vir*Ptr foo_edited = NULL;
> ...
> foo = vir*GetXMLDesc()
> ...
> if (foo_edited)
> vir*Free(foo_edited);
> foo_edited = vir*DefineXML()
> ...
>
> However, foo_edited could never be set in the code path - even with
> all the goto's since the only way for it to be set is if vir*DefineXML()
> succeeds in which case the code to allow a retry (and thus all the goto's)
> never leaves foo_edited set
>
> All error paths lead to "cleanup:" which causes both foo and foo_edited
> to call the respective vir*Free() routines if set.
>
> Signed-off-by: John Ferlan
> ---
> tools/virsh-domain.c| 5 -
> tools/virsh-edit.c | 9 -
> tools/virsh-interface.c | 3 ---
> tools/virsh-network.c | 3 ---
> tools/virsh-nwfilter.c | 3 ---
> tools/virsh-pool.c | 3 ---
> tools/virsh-snapshot.c | 3 ---
> 7 files changed, 29 deletions(-)
Yep, the free()s in the cleanup section of each command are redundant.
ACK
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
