Re: [libvirt] QEMU migration with non-shared storage
On Thu, Sep 11, 2014 at 02:45:41PM +1000, Blair Bethwaite wrote: Hi Michael! On 11 September 2014 14:13, Michael Chapman m...@very.puzzling.org wrote: Why is RBD is handled specially in this function? The current logic is that an RBD-backed disk is safe to be migrated even if it's got caching enabled, but I'm not sure how RBD is different from other backends in this regard. I recall this has was discussed before but am having trouble finding the thread. I think the gist of it was that the rbd integration was just lucky, but it wasn't using the appropriate interfaces defined by libvirt for flushing. And I think Debian patches away the qemuMigrationIsSafe pass for RBD, so it'll only pass for cache=none. Debian doesn't ship such a patch. Cheers, -- Guido -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V2 0/4] Xen-xl parser
Aah, just noticed the tests are not working On Thu, Sep 11, 2014 at 7:10 AM, Kiarie Kahurani davidkiar...@gmail.com wrote: Kiarie Kahurani (4): src/xenconfig: Export helper functions src/xenconfig: Xen-xl parser src/xenconfig: Introduce xen-xl on virsh command line tests: Tests for the xen-xl parser configure.ac | 7 + src/Makefile.am | 21 +- src/libvirt_xenconfig.syms | 4 + src/libxl/libxl_driver.c | 46 +++- src/xenconfig/libxlu_disk_i.h| 28 ++ src/xenconfig/libxlu_disk_l.l| 259 +++ src/xenconfig/xen_common.c | 147 +-- src/xenconfig/xen_common.h | 24 +- src/xenconfig/xen_xl.c | 479 +++ src/xenconfig/xen_xl.h | 29 +++ tests/Makefile.am| 9 +- tests/testutilsxen.c | 50 tests/testutilsxen.h | 9 +- tests/xlconfigdata/test-new-disk.cfg | 27 ++ tests/xlconfigdata/test-new-disk.xml | 45 tests/xlconfigdata/test-spice.cfg| 32 +++ tests/xlconfigdata/test-spice.xml| 45 tests/xlconfigtest.c | 224 18 files changed, 1389 insertions(+), 96 deletions(-) create mode 100644 src/xenconfig/libxlu_disk_i.h create mode 100644 src/xenconfig/libxlu_disk_l.l create mode 100644 src/xenconfig/xen_xl.c create mode 100644 src/xenconfig/xen_xl.h create mode 100644 tests/xlconfigdata/test-new-disk.cfg create mode 100644 tests/xlconfigdata/test-new-disk.xml create mode 100644 tests/xlconfigdata/test-spice.cfg create mode 100644 tests/xlconfigdata/test-spice.xml create mode 100644 tests/xlconfigtest.c Changes in V1 -introduced flex for xl disk format parsing Changes in V2 -changed ot use AC_PROG_LEX in configure.ac -got rid of unused data files Not done -compile the generated file using global compiler flags as it has unused parameters -- 1.8.4.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/26] Resolve more Coverity issues
On 09/10/14 23:56, John Ferlan wrote: Would it be easier if I made batches of 5 patches? or perhaps just batches of patches of the same error? It might help to overcome the psychological barrier of taking responsibility to review everything in the given series :) Anyhow, I'll have a look at the complete plile instead of having yoy to re-send. Just want to get this off the pile Tks, John Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] selinux: Properly check TAP FD label
After a4431931 the TAP FDs ale labeled with image label instead of the process label. On the other hand, the commit was incomplete as a few lines above, there's still old check for the process label presence while it should be check for the image label instead. Signed-off-by: Michal Privoznik mpriv...@redhat.com --- Pushed under trivial rule. After this commit, the function is completely the same as virSecuritySELinuxSetImageFDLabel(). However I'd like to keep them separate because there's an ongoing bug: https://bugzilla.redhat.com/show_bug.cgi?id=1095636 so with fair chance the TapFDLabel() function will be rewritten soon. src/security/security_selinux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 7064158..bf67fb5 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2347,7 +2347,7 @@ virSecuritySELinuxSetTapFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virSecurityLabelDefPtr secdef; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); -if (!secdef || !secdef-label) +if (!secdef || !secdef-imagelabel) return 0; return virSecuritySELinuxFSetFilecon(fd, secdef-imagelabel); -- 1.8.5.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 01/26] qemu_driver: Resolve Coverity COPY_PASTE_ERROR
On 09/05/14 00:26, John Ferlan wrote:
In qemuDomainSetBlkioParameters(), Coverity points out that the calls
to qemuDomainParseBlkioDeviceStr() are slightly different and points
out there may be a cut-n-paste error.
In the first call (AFFECT_LIVE), the second parameter is param-field;
however, for the second call (AFFECT_CONFIG), the second parameter is
params-field. It seems the param-field is correct especially since
each path as a setting of param to params[i]. Furthermore, there
were a few more instances of using params[i] instead of param-
which I cleaned up.
Signed-off-by: John Ferlan jfer...@redhat.com
---
src/qemu/qemu_driver.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5121f85..f52f00d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7825,7 +7825,7 @@ qemuDomainSetBlkioParameters(virDomainPtr dom,
virTypedParameterPtr param = params[i];
if (STREQ(param-field, VIR_DOMAIN_BLKIO_WEIGHT)) {
-if (virCgroupSetBlkioWeight(priv-cgroup,
params[i].value.ui) 0)
+if (virCgroupSetBlkioWeight(priv-cgroup, param-value.ui)
0)
ret = -1;
} else if (STREQ(param-field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT) ||
STREQ(param-field,
VIR_DOMAIN_BLKIO_DEVICE_READ_IOPS) ||
@@ -7836,7 +7836,7 @@ qemuDomainSetBlkioParameters(virDomainPtr dom,
virBlkioDevicePtr devices = NULL;
size_t j;
-if (qemuDomainParseBlkioDeviceStr(params[i].value.s,
+if (qemuDomainParseBlkioDeviceStr(param-value.s,
param-field,
devices,
ndevices) 0) {
@@ -7919,7 +7919,7 @@ qemuDomainSetBlkioParameters(virDomainPtr dom,
virTypedParameterPtr param = params[i];
if (STREQ(param-field, VIR_DOMAIN_BLKIO_WEIGHT)) {
-persistentDef-blkio.weight = params[i].value.ui;
+persistentDef-blkio.weight = param-value.ui;
} else if (STREQ(param-field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT) ||
STREQ(param-field,
VIR_DOMAIN_BLKIO_DEVICE_READ_IOPS) ||
STREQ(param-field,
VIR_DOMAIN_BLKIO_DEVICE_WRITE_IOPS) ||
@@ -7928,8 +7928,8 @@ qemuDomainSetBlkioParameters(virDomainPtr dom,
virBlkioDevicePtr devices = NULL;
size_t ndevices;
-if (qemuDomainParseBlkioDeviceStr(params[i].value.s,
- params-field,
+if (qemuDomainParseBlkioDeviceStr(param-value.s,
+ param-field,
wow, this was the real bug here. The original code would use the field
value of the first element in the params array. Very easy to overlook.
devices,
ndevices) 0) {
ret = -1;
ACK.
Peter
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 02/26] remote_driver: Resolve Coverity RESOURCE_LEAK
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 jfer...@redhat.com
---
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
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/26] storage: Resolve Coverity UNUSED_VALUE
On 09/05/14 00:26, John Ferlan wrote: Since cd4d547576a4f0371d1d4d4e0ca6db124c5ba257 Coverity notes that setting 'ret = -3' prior to the unconditional setting of 'ret = 0' will cause the value to be UNUSED. Since the comment indicates that it is expect to allow the code to continue, just remove the ret = -3 setting. Signed-off-by: John Ferlan jfer...@redhat.com --- src/storage/storage_backend_fs.c | 1 - 1 file changed, 1 deletion(-) ACK, Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/26] qemu: Resolve Coverity REVERSE_INULL
On 09/05/14 00:26, John Ferlan wrote: Coverity complains that checking for !domlist after setting doms = domlist and making a deref of doms just above It seems the call in question was intended to me made in the case that 'doms' was passed in and not when the virDomainObjListExport() call allocated domlist and already called virConnectGetAllDomainStatsCheckACL(). Thus rather than check for !domlist - check that doms != domlist in order to avoid the Coverity message. Signed-off-by: John Ferlan jfer...@redhat.com --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) False positive, but fair enough. ACK. Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/26] vbox: Resolve Coverity UNUSED_VALUE
On 09/05/14 00:26, John Ferlan wrote: Handle a few places where Coverity complains about the value being unused. For two of them (Close cases) - the comments above the close indicate there is no harm to ignore the error - so added an ignore_value. For the other condition, added an rc check like other callers. Signed-off-by: John Ferlan jfer...@redhat.com --- src/vbox/vbox_common.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) ACK signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/26] storage: Resolve Coverity OVERFLOW_BEFORE_WIDEN
On 09/05/14 00:26, John Ferlan wrote: Coverity complains that when multiplying to 32 bit values that eventually will be stored in a 64 bit value that it's possible the math could overflow unless one of the values being multiplied is type cast to the proper size. Signed-off-by: John Ferlan jfer...@redhat.com --- src/storage/storage_backend_disk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index cb6a8d5..abab1e1 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -560,7 +560,7 @@ virStorageBackendDiskPartBoundaries(virStoragePoolObjPtr pool, unsigned long long extraBytes = 0; unsigned long long alignedAllocation = allocation; virStoragePoolSourceDevicePtr dev = pool-def-source.devices[0]; -unsigned long long cylinderSize = dev-geometry.heads * +unsigned long long cylinderSize = (unsigned long long)dev-geometry.heads * dev-geometry.sectors * SECTOR_SIZE; VIR_DEBUG(find free area: allocation %llu, cyl size %llu, allocation, ACK signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 14/26] lxc: Resolve Coverity FORWARD_NULL
On 09/05/14 00:26, John Ferlan wrote: If we jump to cleanup before allocating 'result', then the call to virBlkioDeviceArrayClear() could dereference result Signed-off-by: John Ferlan jfer...@redhat.com --- src/lxc/lxc_driver.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) ACK signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 15/26] qemu: Resolve Coverity FORWARD_NULL
On 09/05/14 00:26, John Ferlan wrote: If we jump to cleanup before allocating the 'result', then the call to virBlkioDeviceArrayClear will deref result causing a problem. Signed-off-by: John Ferlan jfer...@redhat.com --- src/qemu/qemu_driver.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) ACK, Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 16/26] network: Resolve Coverity FORWARD_NULL
On 09/05/14 00:26, John Ferlan wrote: If the VIR_STRDUP(exptime,...) fails, then we will jump to cleanup, no need to check if exptime is set which causes Coverity to issue a complaint in the virStrToLong_ll call because there wasn't a check for a NULL value while there was one for the reference right after the VIR_STRDUP(). Signed-off-by: John Ferlan jfer...@redhat.com --- src/network/leaseshelper.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) ACK signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 17/26] virstring: Resolve Coverity FORWARD_NULL
On 09/05/14 00:26, John Ferlan wrote: Perhaps a false positive, but since Coverity doesn't understand the relationship between the 'count' and the 'strings', rather than leave the chance the on input 'strings' is NULL and causes a deref - just check for it and return Signed-off-by: John Ferlan jfer...@redhat.com --- src/util/virstring.c | 3 +++ 1 file changed, 3 insertions(+) Yep, false positive. All callers shall pass 0 as count if strings is NULL. ACK, doesn't hurt. Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 18/26] qemu: Resolve Coverity FORWARD_NULL
On 09/05/14 00:26, John Ferlan wrote: If the qemuMigrationEatCookie() fails to set mig, we jump to cleanup: which will call qemuMigrationCancelDriveMirror() without first checking if mig == NULL Signed-off-by: John Ferlan jfer...@redhat.com --- src/qemu/qemu_migration.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) ACK, genuine bug signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 19/26] network_conf: Resolve Coverity FORWARD_NULL
On 09/05/14 00:26, John Ferlan wrote: The code compares def-forwarders when deciding to return 0 at a couple of points, then uses def-nfwds as a way to index into the def-forwarders array. That reference results in Coverity complaining that def-forwarders being NULL was checked as part of an arithmetic OR operation where failure could be any one 5 conditions, but that is not checked when entering the loop to dereference the array. Changing the comparisons to use nfwds will clear the warnings Signed-off-by: John Ferlan jfer...@redhat.com --- src/conf/network_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) ACK signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 20/26] qemu: Resolve Coverity NEGATIVE_RETURNS
On 09/05/14 00:26, John Ferlan wrote: In qemuProcessInitPCIAddresses() if qemuMonitorGetAllPCIAddresses() returns a negative (or zero) value, then no need to call the qemuProcessDetectPCIAddresses(). Signed-off-by: John Ferlan jfer...@redhat.com --- src/qemu/qemu_process.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) ACK signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 21/26] nodeinfo: Resolve Coverity NEGATIVE_RETURNS
On 09/05/14 00:26, John Ferlan wrote: If the virNumaGetNodeCPUs() call fails with -1, then jumping to cleanup with 'cpus == NULL' and calling virCapabilitiesClearHostNUMACellCPUTopology will cause issues. Signed-off-by: John Ferlan jfer...@redhat.com --- src/nodeinfo.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 92a3718..2008ade 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -1933,7 +1933,7 @@ nodeCapsInitNUMA(virCapsPtr caps) ret = 0; cleanup: -if (topology_failed || ret 0) +if ((topology_failed || ret 0) cpus) virCapabilitiesClearHostNUMACellCPUTopology(cpus, ncpus); False positive. This function checks for NULL. virBitmapFree(cpumap); Also the cleanup section frees cpus twice. That might be worth fixing too. ACK Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 22/26] virsh: Resolve Coverity NEGATIVE_RETURNS
On 09/05/14 00:26, John Ferlan wrote: Coverity notes that if virDomainGetCPUStats returns a negative value into 'nparams' then when we end up at cleanup, the call to virTypedParams will have issues Signed-off-by: John Ferlan jfer...@redhat.com --- tools/virsh-domain.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f732a6e..6fe637b 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6650,6 +6650,7 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd) return ret; failed_stats: +nparams = 0; vshError(ctl, _(Failed to retrieve CPU statistics for domain '%s'), virDomainGetName(dom)); goto cleanup; This would cause a memleak if the second call to virDomainGetCPUStats fails as it will jump to the same label, while params was already allocated. You need to clear nparams explicitly on the first call that determines the count of the returned stats field. Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 23/26] xen: Resolve Coverity NEGATIVE_RETURNS
On 09/05/14 00:26, John Ferlan wrote: Coverity notes that if the call to virBitmapParse() returns a negative value, then when we jump to the error label, the call to virCapabilitiesClearHostNUMACellCPUTopology() will have issues with the negative nb_cpus Signed-off-by: John Ferlan jfer...@redhat.com --- src/xen/xend_internal.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) ACK signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 24/26] qemu: Resolve Coverity NEGATIVE_RETURNS
On 09/05/14 00:26, John Ferlan wrote: Coverity notes that if qemuMonitorGetMachines() returns a negative nmachines value, then the code at the cleanup label will have issues. Signed-off-by: John Ferlan jfer...@redhat.com --- 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 854a9b8..a652f29 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2285,7 +2285,7 @@ virQEMUCapsProbeQMPMachineTypes(virQEMUCapsPtr qemuCaps, size_t defIdx = 0; if ((nmachines = qemuMonitorGetMachines(mon, machines)) 0) -goto cleanup; +return -1; Also nmachines doesn't need to be initialized. if (VIR_ALLOC_N(qemuCaps-machineTypes, nmachines) 0) goto cleanup; ACK signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 25/26] qemu: Resolve Coverity NEGATIVE_RETURNS
On 09/05/14 00:26, John Ferlan wrote: Coverity notes that if the virConnectListAllDomains returns a negative value then the loop at the cleanup label that ends on numDomains will have issues. Signed-off-by: John Ferlan jfer...@redhat.com --- src/qemu/qemu_driver.c | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) ACK signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 26/26] libxl: Resolve Coverity NULL_RETURNS
On 09/05/14 00:26, John Ferlan wrote: With all the changes in my previous foray into this code, I forgot to remove the libxlDomainEventQueue(driver, event); call inside the dom == NULL condition. Signed-off-by: John Ferlan jfer...@redhat.com --- src/libxl/libxl_migration.c | 1 - 1 file changed, 1 deletion(-) ACK signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 13/26] qemu: Resolve Coverity FORWARD_NULL
On 09/05/14 00:26, John Ferlan wrote: If the virJSONValueNewObject() fails, then rather than going to error and getting a Coverity false positive since it doesn't seem to understand the relationship between nkeywords, keywords, and values and seems to believe calling qemuFreeKeywords will cause a NULL deref - just return NULL Signed-off-by: John Ferlan jfer...@redhat.com --- src/qemu/qemu_monitor_json.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) ACK signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 12/26] virsh: Resolve Coverity DEADCODE
On 09/05/14 00:26, John Ferlan wrote: Coverity points out that if 'dom' isn't returned from virDomainQemuAttach, then the code already jumps to cleanup, so there was no need for the subsequent if (dom != NULL) check. I moved the error message about failure into the goto cleanup on failure and then removed the if (dom != NULL) Signed-off-by: John Ferlan jfer...@redhat.com --- tools/virsh-domain.c | 16 +++- 1 file changed, 7 insertions(+), 9 deletions(-) ACK signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 11/26] tests: Resolve Coverity DEADCODE
On 09/05/14 00:26, John Ferlan wrote: Coverity complains that the various checks for autoincrement and changed variables are DEADCODE - seems to me to be a false positive - so mark it. They are not false positive. Currently the code doesn't allow that to happen. The tests are designed to catch if somebody broke it though they need to stay. Signed-off-by: John Ferlan jfer...@redhat.com --- tests/virstringtest.c | 5 + 1 file changed, 5 insertions(+) ACK signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/26] qemu: Resolve Coverity DEADCODE
On 09/05/14 00:26, John Ferlan wrote: Add another 'dead_code_begin' - victims of our own coding practices Signed-off-by: John Ferlan jfer...@redhat.com --- src/qemu/qemu_command.c | 1 + 1 file changed, 1 insertion(+) ACK signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 09/26] virsh: Resolve Coverity DEADCODE
On 09/05/14 00:26, John Ferlan wrote:
Coverity points out that by using EMPTYSTR(type) we are guarding against
the possibility that it could be NULL; however, based on how 'type' was
initialized to NULL, then either ipv4, ipv6, or - there is no way
it could be NULL. Since - is supposed to mean something empty in a
field - remove the initialization to NULL and use it as the ending else
rather than using .
Also changed the name from 'type' to 'typestr'.
Signed-off-by: John Ferlan jfer...@redhat.com
---
tools/virsh-network.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/tools/virsh-network.c b/tools/virsh-network.c
index 5fe4b32..f505c14 100644
--- a/tools/virsh-network.c
+++ b/tools/virsh-network.c
@@ -1360,7 +1360,7 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd)
-);
for (i = 0; i nleases; i++) {
-const char *type = NULL;
+const char *typestr;
char *cidr_format = NULL;
virNetworkDHCPLeasePtr lease = leases[i];
time_t expirytime_tmp = lease-expirytime;
@@ -1369,14 +1369,15 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd
*cmd)
ts = *localtime_r(expirytime_tmp, ts);
strftime(expirytime, sizeof(expirytime), %Y-%m-%d %H:%M:%S, ts);
-type = (lease-type == VIR_IP_ADDR_TYPE_IPV4) ? ipv4 :
-(lease-type == VIR_IP_ADDR_TYPE_IPV6) ? ipv6 : ;
+typestr = (lease-type == VIR_IP_ADDR_TYPE_IPV4) ? ipv4 :
+(lease-type == VIR_IP_ADDR_TYPE_IPV6) ? ipv6 : NULL;
Yuck, nested ternaries. Would you mind refactoring it to if/else or
switch() while touching this?
ignore_value(virAsprintf(cidr_format, %s/%d,
lease-ipaddr, lease-prefix));
vshPrintExtra(ctl, %-20s %-18s %-9s %-25s %-15s %s\n,
- expirytime, EMPTYSTR(lease-mac), EMPTYSTR(type),
cidr_format,
+ expirytime, EMPTYSTR(lease-mac),
+ EMPTYSTR(typestr), cidr_format,
EMPTYSTR(lease-hostname), EMPTYSTR(lease-clientid));
}
ACK
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 08/26] virfile: Resolve Coverity DEADCODE
Well, the bug here is a bit more serious than mere DEADCODE ...
On 09/05/14 00:26, John Ferlan wrote:
Adjust the parentheses in/for the waitpid loops; otherwise, Coverity
points out:
(1) Event assignment: Assigning: waitret = waitpid(pid, status, 0) ==
-1
(2) Event between: At condition waitret == -1, the value of waitret
must be between 0 and 1.
(3) Event dead_error_condition: The condition waitret == -1 cannot
be true.
(4) Event dead_error_begin: Execution cannot reach this statement:
ret = -*__errno_location();.
Signed-off-by: John Ferlan jfer...@redhat.com
---
src/util/virfile.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index cfb6cc1..b602144 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -2072,8 +2072,7 @@ virFileOpenForked(const char *path, int openflags,
mode_t mode,
}
/* wait for child to complete, and retrieve its exit code */
-while ((waitret = waitpid(pid, status, 0) == -1)
-(errno == EINTR));
This existing code worked by chance as the return value of waitpid was
compared to -1 and then assigned to waitret. ..also that part was used
in the comparison. If the return value was -1 then it would correctly loop.
+while ((waitret = waitpid(pid, status, 0)) == -1 (errno == EINTR));
the errno == EINTR doesn't need parentheses
if (waitret == -1) {
But this condition couldn't be reached.
ret = -errno;
virReportSystemError(errno,
@@ -2290,7 +2289,7 @@ virDirCreate(const char *path,
if (pid) { /* parent */
/* wait for child to complete, and retrieve its exit code */
VIR_FREE(groups);
-while ((waitret = waitpid(pid, status, 0) == -1) (errno ==
EINTR));
+while ((waitret = waitpid(pid, status, 0)) == -1 (errno ==
EINTR));
Same issue here.
if (waitret == -1) {
ret = -errno;
virReportSystemError(errno,
ACK
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V3 1/5] util: Introduce virJSONStringCompare for JSON doc comparisons
On Wed, Sep 03, 2014 at 09:07:35PM -0600, Jim Fehlig wrote: From: Daniel P. Berrange berra...@redhat.com 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 berra...@redhat.com Signed-off-by: Jim Fehlig jfeh...@suse.com --- 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 Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V3 3/5] tests: Add more test suite mock helpers
On Wed, Sep 03, 2014 at 09:07:37PM -0600, Jim Fehlig wrote: From: Daniel P. Berrange berra...@redhat.com Rename the VIR_MOCK_IMPL* macros to VIR_MOCK_WRAP* and add new VIR_MOCK_IMPL macros which let you directly implement overrides in the preloaded source. Signed-off-by: Daniel P. Berrange berra...@redhat.com Signed-off-by: Jim Fehlig jfeh...@suse.com --- tests/virfirewalltest.c | 4 ++-- tests/virmock.h | 54 + tests/virsystemdtest.c | 4 ++-- 3 files changed, 45 insertions(+), 17 deletions(-) ACK, might as well push this now too. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V3 2/5] util: Allow port allocator to skip bind() check
On Wed, Sep 03, 2014 at 09:07:36PM -0600, Jim Fehlig wrote: From: Daniel P. Berrange berra...@redhat.com Test suites using the port allocator don't want to have different behaviour depending on whether a port is in use on the host. Add a VIR_PORT_ALLOCATOR_SKIP_BIND_CHECK which test suites can use to skip the bind() test. The port allocator will thus only track ports in use by the test suite process itself. This is fine when using the port allocator to generate guest configs which won't actually be launched Signed-off-by: Daniel P. Berrange berra...@redhat.com Signed-off-by: Jim Fehlig jfeh...@suse.com --- src/libxl/libxl_driver.c | 5 +++-- src/qemu/qemu_driver.c | 9 ++--- src/util/virportallocator.c | 14 ++ src/util/virportallocator.h | 7 ++- tests/virportallocatortest.c | 4 ++-- 5 files changed, 27 insertions(+), 12 deletions(-) ACK, can push this independant of the rest Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V3 4/5] libxl: fix mapping of lifecycle actions
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 jfeh...@suse.com --- src/libxl/libxl_conf.c | 62 +++--- 1 file changed, 59 insertions(+), 3 deletions(-) ACK Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V3 5/5] libxl: Add a test suite for libxl option generator
On Wed, Sep 03, 2014 at 09:07:39PM -0600, Jim Fehlig wrote:
From: Daniel P. Berrange berra...@redhat.com
The libxl library allows a libxl_domain_config object to be
serialized to a JSON string. Use this to allow testing of
the XML - libxl_domain_config conversion process
Signed-off-by: Daniel P. Berrange berra...@redhat.com
Signed-off-by: Jim Fehlig jfeh...@suse.com
---
configure.ac | 2 +
tests/Makefile.am | 25 +++-
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 | 251
+
tests/virmocklibxl.c | 87
8 files changed, 808 insertions(+), 1 deletion(-)
diff --git a/tests/libxlxml2jsontest.c b/tests/libxlxml2jsontest.c
new file mode 100644
index 000..6ae654a
--- /dev/null
+++ b/tests/libxlxml2jsontest.c
+#if defined LIBXL_HAVE_BUILDINFO_HVM_VENDOR_DEVICE
+/*
+* Currently no paths ignored on Xen 4.4
+*/
+static const char **ignore_paths = NULL;
+#elif defined LIBXL_HAVE_DOMAIN_NODEAFFINITY
+/*
+* These paths must be ignored if running on Xen 4.3
+*/
+static const char *ignore_paths[] = {
+/c_info/pvh,
+/c_info/driver_domain,
+/b_info/device_model_version,
+/b_info/event_channels,
+/b_info/u/kernel,
+/b_info/u/cmdline,
+/b_info/u/ramdisk,
+/b_info/u/bios,
+/b_info/u/timer_mode,
+/b_info/u/vga/kind,
+/b_info/u/spice/vdagent,
+/b_info/u/spice/clipboard_sharing,
+/b_info/u/spice/usbredirection,
+/b_info/u/usbversion,
+/b_info/u/vendor_device,
+/on_watchdog,
+NULL
+};
+#else
+/*
+ * These paths must be ignored if running on Xen 4.2
+ */
+static const char *ignore_paths[] = {
+/c_info/pvh,
+/c_info/driver_domain,
+/b_info/nodemap,
+/b_info/exec_ssidref,
+/b_info/iomem,
+/b_info/claim_mode,
+/b_info/device_model_version,
+/b_info/event_channels,
+/b_info/u/usbdevice_list,
+/b_info/u/kernel,
+/b_info/u/cmdline,
+/b_info/u/ramdisk,
+/b_info/u/bios,
+/b_info/u/timer_mode,
+/b_info/u/vga/kind,
+/b_info/u/spice/vdagent,
+/b_info/u/spice/clipboard_sharing,
+/b_info/u/spice/usbredirection,
+/b_info/u/usbversion,
+/b_info/u/vendor_device,
+/disks/backend_domname,
+/nics/backend_domname,
+/nics/gatewaydev,
+/vfbs/backend_domname,
+/vkbs/backend_domname,
+/vtpms,
+/on_watchdog,
+NULL
+};
+#endif
As more more versions of Xen are released I think we might
end up with quite alot of duplication between these lists.
Could we structure it a little differently to avoid this
perhaps in this way:
#if defined LIBXL_HAVE_BUILDINFO_HVM_VENDOR_DEVICE
# define LIBXL_VERSION 4004
#elif define LIBXL_HAVE_DOMAIN_NODEAFFINITY
# define LIBXL_VERSION 4003
#else
# define LIBXL_VERSION 4002
#endif
static const char *ignore_paths[] = {
#if LIBXL_VERSION 4004
/c_info/pvh,
/c_info/driver_domain,
/b_info/device_model_version,
/b_info/event_channels,
/b_info/u/kernel,
/b_info/u/cmdline,
/b_info/u/ramdisk,
/b_info/u/bios,
/b_info/u/timer_mode,
/b_info/u/vga/kind,
/b_info/u/spice/vdagent,
/b_info/u/spice/clipboard_sharing,
/b_info/u/spice/usbredirection,
/b_info/u/usbversion,
/b_info/u/vendor_device,
/on_watchdog,
#endif
#if LIBXL_VERSION 4003
/b_info/nodemap,
/b_info/exec_ssidref,
/b_info/iomem,
/b_info/claim_mode,
/b_info/u/usbdevice_list,
/disks/backend_domname,
/nics/backend_domname,
/nics/gatewaydev,
/vfbs/backend_domname,
/vkbs/backend_domname,
/vtpms,
#endif
NULL
};
Regards,
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/2] parallels: build with parallels SDK
On Sat, Sep 06, 2014 at 08:28:09PM +0400, Dmitry Guryanov wrote: 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 dgurya...@parallels.com --- changes in v2: * run pkgconfig check only if --with-parallels set to yes or check configure.ac| 24 +--- src/Makefile.am | 4 +++- 2 files changed, 16 insertions(+), 12 deletions(-) ACK Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/2] parallels: login to parallels SDK
On Sat, Sep 06, 2014 at 08:28:10PM +0400, Dmitry Guryanov wrote:
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.
As a general rule, even if we count the open/close calls
it isn't safe to run deinit functions in libvirt. eg consider
if libvirt is linked against another program or library that
also uses the paralllels SDK. Libvirt does not know if the
other code has stopped using the SDK. So either the reference
counting must be done in PrlApi_{InitEx,Deinit} functions
directly, or libvirt should simply not call PrlApi_Deinit
at all. I'd probably just go with the latter, as I doubt there
is any real harm to skipping deinit.
diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c
new file mode 100644
index 000..22afd61
--- /dev/null
+++ b/src/parallels/parallels_sdk.c
@@ -0,0 +1,241 @@
+
+#define VIR_FROM_THIS VIR_FROM_PARALLELS
The use of this constant here will mean any error message printed
by libvirt will have a 'Parallels Cloud Server:' prefix on it
+static void
+logPrlErrorHelper(PRL_RESULT err, const char *filename,
+ const char *funcname, size_t linenr)
+{
+char *msg1 = NULL, *msg2 = NULL;
+PRL_UINT32 len = 0;
+
+/* Get required buffer length */
+PrlApi_GetResultDescription(err, PRL_TRUE, PRL_FALSE, NULL, len);
+
+if (VIR_ALLOC_N(msg1, len) 0)
+goto out;
+
+/* get short error description */
+PrlApi_GetResultDescription(err, PRL_TRUE, PRL_FALSE, msg1, len);
+
+PrlApi_GetResultDescription(err, PRL_FALSE, PRL_FALSE, NULL, len);
+
+if (VIR_ALLOC_N(msg2, len) 0)
+goto out;
+
+/* get long error description */
+PrlApi_GetResultDescription(err, PRL_FALSE, PRL_FALSE, msg2, len);
+
+virReportErrorHelper(VIR_FROM_THIS, VIR_ERR_INTERNAL_ERROR,
+ filename, funcname, linenr,
+ _(Parallels SDK: %s %s), msg1, msg2);
So adding 'Parallels SDK' is probably overkill here. I'd suggest
just us '%s %s' instead.
+
+ out:
nit-pick, we tend to use 'cleanup' as a standard label name
+VIR_FREE(msg1);
+VIR_FREE(msg2);
+}
+
+#define logPrlError(code) \
+logPrlErrorHelper(code, __FILE__, \
+ __FUNCTION__, __LINE__)
+
+static PRL_RESULT
+logPrlEventErrorHelper(PRL_HANDLE event, const char *filename,
+ const char *funcname, size_t linenr)
+{
+PRL_RESULT ret, retCode;
+char *msg1 = NULL, *msg2 = NULL;
+PRL_UINT32 len = 0;
+int err = -1;
+
+if ((ret = PrlEvent_GetErrCode(event, retCode))) {
+logPrlError(ret);
+return ret;
+}
+
+PrlEvent_GetErrString(event, PRL_TRUE, PRL_FALSE, NULL, len);
+
+if (VIR_ALLOC_N(msg1, len) 0)
+goto out;
+
+PrlEvent_GetErrString(event, PRL_TRUE, PRL_FALSE, msg1, len);
+
+PrlEvent_GetErrString(event, PRL_FALSE, PRL_FALSE, NULL, len);
+
+if (VIR_ALLOC_N(msg2, len) 0)
+goto out;
+
+PrlEvent_GetErrString(event, PRL_FALSE, PRL_FALSE, msg2, len);
+
+virReportErrorHelper(VIR_FROM_THIS, VIR_ERR_INTERNAL_ERROR,
+ filename, funcname, linenr,
+ _(Parallels SDK: %s %s), msg1, msg2);
Same note here.
+err = 0;
+
+ out:
And here.
+VIR_FREE(msg1);
+VIR_FREE(msg2);
+
+return err;
+}
+
+#define logPrlEventError(event)\
+logPrlEventErrorHelper(event, __FILE__,\
+ __FUNCTION__, __LINE__)
+
+static PRL_HANDLE
+getJobResultHelper(PRL_HANDLE job, unsigned int timeout,
+ const char *filename, const char *funcname,
+ size_t linenr)
+{
+PRL_RESULT ret, retCode;
+PRL_HANDLE result = NULL;
+
+if ((ret = PrlJob_Wait(job, timeout))) {
+logPrlErrorHelper(ret, filename, funcname, linenr);
+goto out;
+}
+
+if ((ret = PrlJob_GetRetCode(job, retCode))) {
+logPrlErrorHelper(ret, filename, funcname, linenr);
+goto out;
+}
+
+if (retCode) {
+PRL_HANDLE err_handle;
+
+/* Sometimes it's possible to get additional error info. */
+if ((ret = PrlJob_GetError(job, err_handle))) {
+logPrlErrorHelper(ret, filename, funcname, linenr);
+goto out;
+}
+
+if (logPrlEventErrorHelper(err_handle, filename, funcname, linenr))
+logPrlErrorHelper(retCode, filename, funcname, linenr);
+
+
Re: [libvirt] [PATCH v1 00/10] Keep original security label
On Wed, Sep 10, 2014 at 03:26:06PM +0200, Michal Privoznik wrote: I know I've sent several versions like ages ago, so this should not start with v1, but hey, this is completely new approach, so I'm gonna start from 1. Here, the virtlockd is misused to hold the original seclabels (although only DAC label is implemented so far). Even more, it does a reference counting, so that only the last label restore does the job, not the previous ones. Ah interesting approach. Do you have a pointer to your most recent posting of the previous approach for comparison. I remember seeing it before, but I'm being unlucky finding it in the archives right now. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 00/10] Keep original security label
On 11.09.2014 13:13, Daniel P. Berrange wrote: On Wed, Sep 10, 2014 at 03:26:06PM +0200, Michal Privoznik wrote: I know I've sent several versions like ages ago, so this should not start with v1, but hey, this is completely new approach, so I'm gonna start from 1. Here, the virtlockd is misused to hold the original seclabels (although only DAC label is implemented so far). Even more, it does a reference counting, so that only the last label restore does the job, not the previous ones. Ah interesting approach. Do you have a pointer to your most recent posting of the previous approach for comparison. I remember seeing it before, but I'm being unlucky finding it in the archives right now. I believe this was my last approach: http://www.redhat.com/archives/libvir-list/2014-March/msg00826.html The idea there was to have a file to keep original labels and use virtlockd to ensure mutual exclusion of multiple daemons. But I must say stripping the file and moving it into virtlockd (approach presented in this patch set) looks better to me. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/5] conf: remove redundant local variable
Use just one int variable for all the FromString calls.
---
src/conf/domain_conf.c | 27 +++
1 file changed, 11 insertions(+), 16 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index fcf7fb6..0fdfa6f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6904,7 +6904,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
virNWFilterHashTablePtr filterparams = NULL;
virDomainActualNetDefPtr actual = NULL;
xmlNodePtr oldnode = ctxt-node;
-int ret;
+int ret, val;
if (VIR_ALLOC(def) 0)
return NULL;
@@ -7248,13 +7248,12 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
}
if (mode != NULL) {
-int m;
-if ((m = virNetDevMacVLanModeTypeFromString(mode)) 0) {
+if ((val = virNetDevMacVLanModeTypeFromString(mode)) 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
_(Unknown mode has been specified));
goto error;
}
-def-data.direct.mode = m;
+def-data.direct.mode = val;
} else {
def-data.direct.mode = VIR_NETDEV_MACVLAN_MODE_VEPA;
}
@@ -7329,31 +7328,28 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
if (def-type != VIR_DOMAIN_NET_TYPE_HOSTDEV
STREQ_NULLABLE(def-model, virtio)) {
if (backend != NULL) {
-int name;
-if ((name = virDomainNetBackendTypeFromString(backend)) 0 ||
-name == VIR_DOMAIN_NET_BACKEND_TYPE_DEFAULT) {
+if ((val = virDomainNetBackendTypeFromString(backend)) 0 ||
+val == VIR_DOMAIN_NET_BACKEND_TYPE_DEFAULT) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_(Unknown interface driver name='%s'
has been specified),
backend);
goto error;
}
-def-driver.virtio.name = name;
+def-driver.virtio.name = val;
}
if (txmode != NULL) {
-int m;
-if ((m = virDomainNetVirtioTxModeTypeFromString(txmode)) 0 ||
-m == VIR_DOMAIN_NET_VIRTIO_TX_MODE_DEFAULT) {
+if ((val = virDomainNetVirtioTxModeTypeFromString(txmode)) 0 ||
+val == VIR_DOMAIN_NET_VIRTIO_TX_MODE_DEFAULT) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_(Unknown interface driver txmode='%s'
has been specified),
txmode);
goto error;
}
-def-driver.virtio.txmode = m;
+def-driver.virtio.txmode = val;
}
if (ioeventfd) {
-int val;
if ((val = virTristateSwitchTypeFromString(ioeventfd)) = 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_(unknown interface ioeventfd mode '%s'),
@@ -7363,14 +7359,13 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
def-driver.virtio.ioeventfd = val;
}
if (event_idx) {
-int idx;
-if ((idx = virTristateSwitchTypeFromString(event_idx)) = 0) {
+if ((val = virTristateSwitchTypeFromString(event_idx)) = 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_(unknown interface event_idx mode '%s'),
event_idx);
goto error;
}
-def-driver.virtio.event_idx = idx;
+def-driver.virtio.event_idx = val;
}
if (queues) {
unsigned int q;
--
1.8.5.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/5] Add virSwitch to data types
Just to make this series work until Martin pushes his more complete cleanup: https://www.redhat.com/archives/libvir-list/2014-September/msg00391.html --- docs/schemas/basictypes.rng | 6 ++ 1 file changed, 6 insertions(+) diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng index 75d5238..3e90262 100644 --- a/docs/schemas/basictypes.rng +++ b/docs/schemas/basictypes.rng @@ -446,4 +446,10 @@ /optional /define + define name=virSwitch +choice + valueon/value + valueoff/value +/choice + /define /grammar -- 1.8.5.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/5] qemu: wire up virtio-net segment offloading options
---
src/qemu/qemu_command.c | 20
.../qemuxml2argv-net-virtio-disable-offloads.args| 8
tests/qemuxml2argvtest.c | 2 ++
3 files changed, 30 insertions(+)
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-net-virtio-disable-offloads.args
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 665a590..0b88a48 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4430,6 +4430,26 @@ qemuBuildNicDevStr(virDomainDefPtr def,
virBufferAsprintf(buf, ,event_idx=%s,
virTristateSwitchTypeToString(net-driver.virtio.event_idx));
}
+if (net-driver.virtio.csum) {
+virBufferAsprintf(buf, ,csum=%s,
+
virTristateSwitchTypeToString(net-driver.virtio.csum));
+}
+if (net-driver.virtio.gso) {
+virBufferAsprintf(buf, ,gso=%s,
+
virTristateSwitchTypeToString(net-driver.virtio.gso));
+}
+if (net-driver.virtio.guest_tso4) {
+virBufferAsprintf(buf, ,guest_tso4=%s,
+
virTristateSwitchTypeToString(net-driver.virtio.guest_tso4));
+}
+if (net-driver.virtio.guest_tso6) {
+virBufferAsprintf(buf, ,guest_tso6=%s,
+
virTristateSwitchTypeToString(net-driver.virtio.guest_tso6));
+}
+if (net-driver.virtio.guest_ecn) {
+virBufferAsprintf(buf, ,guest_ecn=%s,
+
virTristateSwitchTypeToString(net-driver.virtio.guest_ecn));
+}
}
if (usingVirtio vhostfdSize 1) {
/* As advised at http://www.linux-kvm.org/page/Multiqueue
diff --git
a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-disable-offloads.args
b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-disable-offloads.args
new file mode 100644
index 000..49e8bf7
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-disable-offloads.args
@@ -0,0 +1,8 @@
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \
+/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults \
+-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \
+-hda /dev/HostVG/QEMUGuest7 \
+-device virtio-net-pci,csum=off,gso=off,guest_tso4=off,guest_tso6=off\
+,guest_ecn=off,vlan=0,id=net0,mac=00:22:44:66:88:aa,bus=pci.0,addr=0x3 \
+-net user,vlan=0,name=hostnet0 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 5c28253..9da1214 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -959,6 +959,8 @@ mymain(void)
DO_TEST(net-virtio, NONE);
DO_TEST(net-virtio-device,
QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_TX_ALG);
+DO_TEST(net-virtio-disable-offloads,
+QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
DO_TEST(net-virtio-netdev,
QEMU_CAPS_DEVICE, QEMU_CAPS_NETDEV, QEMU_CAPS_NODEFCONFIG);
DO_TEST(net-virtio-s390,
--
1.8.5.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/5] conf: add options for disabling segment offloading
Add the following attributes:
csum, gso, guest_tso4, guest_tso6, guest_ecn
to the driver element of network interface
which control the virtio-net device properties
of the same names.
---
docs/formatdomain.html.in | 27
docs/schemas/domaincommon.rng | 25 +++
src/conf/domain_conf.c | 81 ++
src/conf/domain_conf.h | 5 ++
.../qemuxml2argv-net-virtio-disable-offloads.xml | 32 +
tests/qemuxml2xmltest.c| 1 +
6 files changed, 171 insertions(+)
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-net-virtio-disable-offloads.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index a2ea758..5b2758a 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -3847,6 +3847,12 @@ qemu-kvm -net nic,model=? /dev/null
lt;model type='virtio'/gt;
blt;driver name='vhost' txmode='iothread' ioeventfd='on'
event_idx='off' queues='5'/gt;/b
lt;/interfacegt;
+lt;interface type='network'gt;
+ lt;source network='default'/gt;
+ lt;target dev='vnet2'/gt;
+ lt;model type='virtio'/gt;
+ blt;driver csum='off' gso='off' guest_tso4='off' guest_tso6='off'
guest_ecn='off'/gt;/b
+lt;/interfacegt;
lt;/devicesgt;
.../pre
@@ -3949,6 +3955,27 @@ qemu-kvm -net nic,model=? /dev/null
processor, resulting in much higher throughput.
span class=sinceSince 1.0.6 (QEMU and KVM only)/span
/dd
+ dtcodecsum/code/dt
+ dd
+The codecsum/code attribute with possible values codeon/code
+and codeoff/code controls host-side support for packets with
+partial checksum values.
+span class=sinceSince 1.2.9 (QEMU only)/spanbr/br/
+
+bIn general you should leave this option alone, unless you
+are very certain you know what you are doing./b
+ /dd
+ dtsegment offloading options/dt
+ dd
+The attributes codegso/code, codeguest_tso4/code,
+codeguest_tso6/code, codeguest_ecn/code with possible
+values of codeon/code and codeoff/code can be used
+to tune segment offloading.
+span class=sinceSince 1.2.9 (QEMU only)/spanbr/br/
+
+bIn general you should leave this option alone, unless you
+are very certain you know what you are doing./b
+ /dd
/dl
h5a name=elementsNICSTargetOverrideOverriding the target
element/a/h5
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 6ae940a..c5b092f 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2373,6 +2373,31 @@
optional
ref name=event_idx/
/optional
+ optional
+attribute name=csum
+ ref name=virSwitch/
+/attribute
+ /optional
+ optional
+attribute name=gso
+ ref name=virSwitch/
+/attribute
+ /optional
+ optional
+attribute name=guest_tso4
+ ref name=virSwitch/
+/attribute
+ /optional
+ optional
+attribute name=guest_tso6
+ ref name=virSwitch/
+/attribute
+ /optional
+ optional
+attribute name=guest_ecn
+ ref name=virSwitch/
+/attribute
+ /optional
/group
/choice
empty/
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0fdfa6f..cc67e35 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6892,6 +6892,11 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
char *ioeventfd = NULL;
char *event_idx = NULL;
char *queues = NULL;
+char *csum = NULL;
+char *gso = NULL;
+char *guest_tso4 = NULL;
+char *guest_tso6 = NULL;
+char *guest_ecn = NULL;
char *filter = NULL;
char *internal = NULL;
char *devaddr = NULL;
@@ -7015,6 +7020,11 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
ioeventfd = virXMLPropString(cur, ioeventfd);
event_idx = virXMLPropString(cur, event_idx);
queues = virXMLPropString(cur, queues);
+csum = virXMLPropString(cur, csum);
+gso = virXMLPropString(cur, gso);
+guest_tso4 = virXMLPropString(cur, guest_tso4);
+guest_tso6 = virXMLPropString(cur, guest_tso6);
+guest_ecn = virXMLPropString(cur, guest_ecn);
} else if (xmlStrEqual(cur-name, BAD_CAST filterref)) {
if (filter) {
virReportError(VIR_ERR_XML_ERROR, %s,
@@ -7377,6 +7387,51 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
}
[libvirt] [PATCH 1/5] conf: split out virtio net driver formatting
Instead of checking upfront if the driver element will be needed
in a big condition, just format all the attributes into a string
and output the driver element if the string is not empty.
---
src/conf/domain_conf.c | 68 --
1 file changed, 44 insertions(+), 24 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a2a7d92..fcf7fb6 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -16401,6 +16401,41 @@ virDomainActualNetDefFormat(virBufferPtr buf,
return 0;
}
+
+static int
+virDomainVirtioNetDriverFormat(char **outstr,
+ virDomainNetDefPtr def)
+{
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+if (def-driver.virtio.name) {
+virBufferAsprintf(buf, name='%s' ,
+
virDomainNetBackendTypeToString(def-driver.virtio.name));
+}
+if (def-driver.virtio.txmode) {
+virBufferAsprintf(buf, txmode='%s' ,
+
virDomainNetVirtioTxModeTypeToString(def-driver.virtio.txmode));
+}
+if (def-driver.virtio.ioeventfd) {
+virBufferAsprintf(buf, ioeventfd='%s' ,
+
virTristateSwitchTypeToString(def-driver.virtio.ioeventfd));
+}
+if (def-driver.virtio.event_idx) {
+virBufferAsprintf(buf, event_idx='%s' ,
+
virTristateSwitchTypeToString(def-driver.virtio.event_idx));
+}
+if (def-driver.virtio.queues)
+virBufferAsprintf(buf, queues='%u' , def-driver.virtio.queues);
+
+virBufferTrim(buf, , -1);
+
+if (virBufferCheckError(buf) 0)
+return -1;
+
+*outstr = virBufferContentAndReset(buf);
+return 0;
+}
+
+
int
virDomainNetDefFormat(virBufferPtr buf,
virDomainNetDefPtr def,
@@ -16576,30 +16611,15 @@ virDomainNetDefFormat(virBufferPtr buf,
if (def-model) {
virBufferEscapeString(buf, model type='%s'/\n,
def-model);
-if (STREQ(def-model, virtio)
-(def-driver.virtio.name || def-driver.virtio.txmode ||
- def-driver.virtio.ioeventfd || def-driver.virtio.event_idx ||
- def-driver.virtio.queues)) {
-virBufferAddLit(buf, driver);
-if (def-driver.virtio.name) {
-virBufferAsprintf(buf, name='%s',
-
virDomainNetBackendTypeToString(def-driver.virtio.name));
-}
-if (def-driver.virtio.txmode) {
-virBufferAsprintf(buf, txmode='%s',
-
virDomainNetVirtioTxModeTypeToString(def-driver.virtio.txmode));
-}
-if (def-driver.virtio.ioeventfd) {
-virBufferAsprintf(buf, ioeventfd='%s',
-
virTristateSwitchTypeToString(def-driver.virtio.ioeventfd));
-}
-if (def-driver.virtio.event_idx) {
-virBufferAsprintf(buf, event_idx='%s',
-
virTristateSwitchTypeToString(def-driver.virtio.event_idx));
-}
-if (def-driver.virtio.queues)
-virBufferAsprintf(buf, queues='%u',
def-driver.virtio.queues);
-virBufferAddLit(buf, /\n);
+if (STREQ(def-model, virtio)) {
+char *str;
+
+if (virDomainVirtioNetDriverFormat(str, def) 0)
+return -1;
+
+if (str)
+virBufferAsprintf(buf, driver %s/\n, str);
+VIR_FREE(str);
}
}
if (def-filter) {
--
1.8.5.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/5] Add support for turning off offloading for virtio-net
Ján Tomko (5): conf: split out virtio net driver formatting Add virSwitch to data types conf: remove redundant local variable conf: add options for disabling segment offloading qemu: wire up virtio-net segment offloading options docs/formatdomain.html.in | 27 docs/schemas/basictypes.rng| 6 + docs/schemas/domaincommon.rng | 25 +++ src/conf/domain_conf.c | 176 - src/conf/domain_conf.h | 5 + src/qemu/qemu_command.c| 20 +++ .../qemuxml2argv-net-virtio-disable-offloads.args | 8 + .../qemuxml2argv-net-virtio-disable-offloads.xml | 32 tests/qemuxml2argvtest.c | 2 + tests/qemuxml2xmltest.c| 1 + 10 files changed, 262 insertions(+), 40 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-disable-offloads.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-disable-offloads.xml -- 1.8.5.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 07/26] virsh: Resolve Coverity DEADCODE
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 jfer...@redhat.com --- 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
[libvirt] [PATCH 0/2] Couple of NVRAM fixes
*** BLURB HERE *** Michal Privoznik (2): nvram: Fix permissions virDomainUndefineFlags: Allow NVRAM unlinking include/libvirt/libvirt.h.in| 2 ++ libvirt.spec.in | 2 +- src/qemu/qemu_driver.c | 19 ++- src/security/security_selinux.c | 5 - tools/virsh-domain.c| 15 --- tools/virsh.pod | 6 +- 6 files changed, 42 insertions(+), 7 deletions(-) -- 1.8.5.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] nvram: Fix permissions
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 mpriv...@redhat.com
---
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
[libvirt] [PATCH 2/2] virDomainUndefineFlags: Allow NVRAM unlinking
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 mpriv...@redhat.com
---
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,
}
}
+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;
+}
+}
+
if (virDomainDeleteConfig(cfg-configDir, cfg-autostartDir, vm) 0)
goto cleanup;
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 (nvram) {
+flags |= VIR_DOMAIN_UNDEFINE_NVRAM;
+}
if (!(dom = vshCommandOptDomain(ctl, cmd, name)))
return false;
@@ -3504,10 +3512,11 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
* VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA flag was not present
* until 0.9.5; skip to piecewise emulation if we couldn't prove
* above that the new API is safe. */
-if (managed_save_safe snapshots_safe) {
+if ((managed_save_safe snapshots_safe) || nvram) {
rc = virDomainUndefineFlags(dom, flags);
-if (rc == 0 || (last_error-code != VIR_ERR_NO_SUPPORT
-last_error-code != VIR_ERR_INVALID_ARG))
+if (rc == 0 || nvram ||
+(last_error-code != VIR_ERR_NO_SUPPORT
+ last_error-code != VIR_ERR_INVALID_ARG))
goto out;
vshResetLibvirtError();
}
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 60ee515..ae396d2 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -2083,7 +2083,7 @@ Output the device used for the TTY console of the domain.
If the information
is not available the processes will provide an exit code of 1.
=item Bundefine Idomain [I--managed-save] [I--snapshots-metadata]
-[ {I--storage Bvolumes | I--remove-all-storage} I--wipe-storage]
+[I--nvram} [ {I--storage Bvolumes | I--remove-all-storage}
I--wipe-storage]
Re: [libvirt] [PATCH 00/26] Resolve more Coverity issues
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
[libvirt] [PATCH] util/virprocess.c: fix MinGW build
Signed-off-by: Pavel Hrdina phrd...@redhat.com
---
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 stdlib.h
#include sys/wait.h
#include unistd.h
-#include sys/syscall.h
#if HAVE_SETRLIMIT
# include sys/time.h
# include sys/resource.h
@@ -78,10 +77,21 @@ VIR_LOG_INIT(util.process);
#endif
#ifndef HAVE_SETNS
+# ifndef WIN32
+# include sys/syscall.h
+
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 */
/**
--
1.8.5.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util/virprocess.c: fix MinGW build
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.
Signed-off-by: Pavel Hrdina phrd...@redhat.com
---
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 stdlib.h
#include sys/wait.h
#include unistd.h
-#include sys/syscall.h
#if HAVE_SETRLIMIT
# include sys/time.h
# include sys/resource.h
@@ -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?
+# ifndef WIN32
+# include sys/syscall.h
+
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
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util/virprocess.c: fix MinGW build
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 phrd...@redhat.com
---
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 stdlib.h
#include sys/wait.h
#include unistd.h
-#include sys/syscall.h
#if HAVE_SETRLIMIT
# include sys/time.h
# include sys/resource.h
@@ -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 sys/syscall.h
+
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
Re: [libvirt] [PATCH] util/virprocess.c: fix MinGW build
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 phrd...@redhat.com --- 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 stdlib.h #include sys/wait.h #include unistd.h -#include sys/syscall.h #if HAVE_SETRLIMIT # include sys/time.h # include sys/resource.h @@ -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
Re: [libvirt] [PATCH] util/virprocess.c: fix MinGW build
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 phrd...@redhat.com --- 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 stdlib.h #include sys/wait.h #include unistd.h -#include sys/syscall.h #if HAVE_SETRLIMIT # include sys/time.h # include sys/resource.h @@ -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
Re: [libvirt] [PATCH 1/3] schemas: finish virTristate{Bool, Switch} transition
On 09/08/2014 07:40 AM, Martin Kletzander wrote: Signed-off-by: Martin Kletzander mklet...@redhat.com --- docs/schemas/basictypes.rng | 19 -- docs/schemas/capability.rng | 10 +-- docs/schemas/domaincaps.rng | 5 +- docs/schemas/domaincommon.rng | 155 +- docs/schemas/interface.rng| 19 +- docs/schemas/network.rng | 29 ++-- docs/schemas/nwfilter.rng | 5 +- docs/schemas/secret.rng | 10 +-- 8 files changed, 61 insertions(+), 191 deletions(-) diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng index 75d5238..d26da57 100644 --- a/docs/schemas/basictypes.rng +++ b/docs/schemas/basictypes.rng @@ -77,10 +77,7 @@ /attribute optional attribute name=multifunction -choice - valueon/value - valueoff/value -/choice +ref name=virSwitch/ Purely cosmetic, but how about calling them virYesNo and virOnOff to avoid confusion? When I see virBool I think true/false, and when I see virSwitch I think Does this have something to do with a network device? :-) /attribute /optional /define @@ -446,4 +443,18 @@ /optional /define + define name=virBool +choice + valueyes/value + valueno/value +/choice + /define + + define name=virSwitch +choice + valueon/value + valueoff/value +/choice + /define + /grammar diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng index f954599..65a8a0d 100644 --- a/docs/schemas/capability.rng +++ b/docs/schemas/capability.rng @@ -405,16 +405,10 @@ define name='featuretoggle' attribute name='toggle' - choice -valueyes/value -valueno/value - /choice + ref name=virBool/ /attribute attribute name='default' - choice -valueon/value -valueoff/value - /choice + ref name=virSwitch/ /attribute /define diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng index 627b699..bc36a28 100644 --- a/docs/schemas/domaincaps.rng +++ b/docs/schemas/domaincaps.rng @@ -66,10 +66,7 @@ define name='supported' attribute name='supported' - choice -valueyes/value -valueno/value - /choice + ref name=virBool/ /attribute /define diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index cedceae..25ff386 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -118,10 +118,7 @@ /attribute optional attribute name='relabel' - choice -valueyes/value -valueno/value - /choice + ref name=virBool/ /attribute /optional interleave @@ -254,10 +251,7 @@ optional element name=bootmenu attribute name=enable - choice -valueyes/value -valueno/value - /choice + ref name=virBool/ /attribute optional attribute name=timeout @@ -556,10 +550,7 @@ ref name='scaledInteger'/ optional attribute name=dumpCore -choice - valueon/value - valueoff/value -/choice +ref name=virSwitch/ /attribute /optional /element @@ -972,10 +963,7 @@ /choice optional attribute name=present - choice -valueyes/value -valueno/value - /choice + ref name=virBool/ /attribute /optional empty/ @@ -1225,10 +1213,7 @@ /attribute optional attribute name=rawio - choice -valueyes/value -valueno/value - /choice + ref name=virBool/ /attribute /optional optional @@ -1496,10 +1481,7 @@ /optional optional attribute name=removable - choice -valueon/value -valueoff/value - /choice + ref name=virSwitch/ /attribute /optional /element @@ -1632,26 +1614,17 @@ /define define name=ioeventfd attribute name=ioeventfd - choice -valueon/value -valueoff/value - /choice + ref name=virSwitch/ /attribute /define define name=event_idx attribute name=event_idx - choice -valueon/value -valueoff/value - /choice + ref name=virSwitch/ /attribute /define define name=copy_on_read attribute name='copy_on_read' - choice -valueon/value -valueoff/value -
Re: [libvirt] [PATCH 1/2] nvram: Fix permissions
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 mpriv...@redhat.com
---
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 ler...@redhat.com
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 2/2] parallels: login to parallels SDK
On Thursday 11 September 2014 12:09:20 Daniel P. Berrange wrote:
On Sat, Sep 06, 2014 at 08:28:10PM +0400, Dmitry Guryanov wrote:
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.
As a general rule, even if we count the open/close calls
it isn't safe to run deinit functions in libvirt. eg consider
if libvirt is linked against another program or library that
also uses the paralllels SDK. Libvirt does not know if the
other code has stopped using the SDK. So either the reference
counting must be done in PrlApi_{InitEx,Deinit} functions
directly, or libvirt should simply not call PrlApi_Deinit
at all. I'd probably just go with the latter, as I doubt there
is any real harm to skipping deinit.
I can move reference counting to parallels SDK,
diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c
new file mode 100644
index 000..22afd61
--- /dev/null
+++ b/src/parallels/parallels_sdk.c
@@ -0,0 +1,241 @@
+
+#define VIR_FROM_THIS VIR_FROM_PARALLELS
The use of this constant here will mean any error message printed
by libvirt will have a 'Parallels Cloud Server:' prefix on it
+static void
+logPrlErrorHelper(PRL_RESULT err, const char *filename,
+ const char *funcname, size_t linenr)
+{
+char *msg1 = NULL, *msg2 = NULL;
+PRL_UINT32 len = 0;
+
+/* Get required buffer length */
+PrlApi_GetResultDescription(err, PRL_TRUE, PRL_FALSE, NULL, len);
+
+if (VIR_ALLOC_N(msg1, len) 0)
+goto out;
+
+/* get short error description */
+PrlApi_GetResultDescription(err, PRL_TRUE, PRL_FALSE, msg1, len);
+
+PrlApi_GetResultDescription(err, PRL_FALSE, PRL_FALSE, NULL, len);
+
+if (VIR_ALLOC_N(msg2, len) 0)
+goto out;
+
+/* get long error description */
+PrlApi_GetResultDescription(err, PRL_FALSE, PRL_FALSE, msg2, len);
+
+virReportErrorHelper(VIR_FROM_THIS, VIR_ERR_INTERNAL_ERROR,
+ filename, funcname, linenr,
+ _(Parallels SDK: %s %s), msg1, msg2);
So adding 'Parallels SDK' is probably overkill here. I'd suggest
just us '%s %s' instead.
OK, thanks, I'll fix it.
+
+ out:
nit-pick, we tend to use 'cleanup' as a standard label name
+VIR_FREE(msg1);
+VIR_FREE(msg2);
+}
+
+#define logPrlError(code) \
+logPrlErrorHelper(code, __FILE__, \
+ __FUNCTION__, __LINE__)
+
+static PRL_RESULT
+logPrlEventErrorHelper(PRL_HANDLE event, const char *filename,
+ const char *funcname, size_t linenr)
+{
+PRL_RESULT ret, retCode;
+char *msg1 = NULL, *msg2 = NULL;
+PRL_UINT32 len = 0;
+int err = -1;
+
+if ((ret = PrlEvent_GetErrCode(event, retCode))) {
+logPrlError(ret);
+return ret;
+}
+
+PrlEvent_GetErrString(event, PRL_TRUE, PRL_FALSE, NULL, len);
+
+if (VIR_ALLOC_N(msg1, len) 0)
+goto out;
+
+PrlEvent_GetErrString(event, PRL_TRUE, PRL_FALSE, msg1, len);
+
+PrlEvent_GetErrString(event, PRL_FALSE, PRL_FALSE, NULL, len);
+
+if (VIR_ALLOC_N(msg2, len) 0)
+goto out;
+
+PrlEvent_GetErrString(event, PRL_FALSE, PRL_FALSE, msg2, len);
+
+virReportErrorHelper(VIR_FROM_THIS, VIR_ERR_INTERNAL_ERROR,
+ filename, funcname, linenr,
+ _(Parallels SDK: %s %s), msg1, msg2);
Same note here.
+err = 0;
+
+ out:
And here.
+VIR_FREE(msg1);
+VIR_FREE(msg2);
+
+return err;
+}
+
+#define logPrlEventError(event)\
+logPrlEventErrorHelper(event, __FILE__,\
+ __FUNCTION__, __LINE__)
+
+static PRL_HANDLE
+getJobResultHelper(PRL_HANDLE job, unsigned int timeout,
+ const char *filename, const char *funcname,
+ size_t linenr)
+{
+PRL_RESULT ret, retCode;
+PRL_HANDLE result = NULL;
+
+if ((ret = PrlJob_Wait(job, timeout))) {
+logPrlErrorHelper(ret, filename, funcname, linenr);
+goto out;
+}
+
+if ((ret = PrlJob_GetRetCode(job, retCode))) {
+logPrlErrorHelper(ret, filename, funcname, linenr);
+goto out;
+}
+
+if (retCode) {
+PRL_HANDLE err_handle;
+
+/* Sometimes it's possible to get additional error info. */
+if ((ret =
Re: [libvirt] [PATCH 1/2] nvram: Fix permissions
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 mpriv...@redhat.com
---
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
Re: [libvirt] [PATCH v3] leaseshelper: improvements to support all events
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 (network-name.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 weird reason the env
var
+ * DNSMASQ_SERVER_DUID wasn't set*/
+
[libvirt] [PATCH 2/2] Wire up the interface backend options
Pass the user-specified tun path down when creating tap device
when called from the qemu driver.
Also honor the vhost device path specified by user.
---
src/bhyve/bhyve_command.c | 2 +-
src/bhyve/bhyve_process.c | 2 +-
src/network/bridge_driver.c | 6 +++---
src/qemu/qemu_command.c | 22 +++---
src/qemu/qemu_process.c | 2 +-
src/uml/uml_conf.c | 2 +-
src/uml/uml_driver.c| 3 ++-
src/util/virnetdevtap.c | 37 +++--
src/util/virnetdevtap.h | 5 -
9 files changed, 55 insertions(+), 26 deletions(-)
diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index 94829e7..bea4a59 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -72,7 +72,7 @@ bhyveBuildNetArgStr(const virDomainDef *def,
if (!dryRun) {
if (virNetDevTapCreateInBridgePort(brname, net-ifname, net-mac,
- def-uuid, NULL, 0,
+ def-uuid, NULL, NULL, 0,
virDomainNetGetActualVirtPortProfile(net),
virDomainNetGetActualVlan(net),
VIR_NETDEV_TAP_CREATE_IFUP |
VIR_NETDEV_TAP_CREATE_PERSIST) 0) {
diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c
index 6b5403d..0bbe388 100644
--- a/src/bhyve/bhyve_process.c
+++ b/src/bhyve/bhyve_process.c
@@ -82,7 +82,7 @@ bhyveNetCleanup(virDomainObjPtr vm)
ignore_value(virNetDevBridgeRemovePort(
virDomainNetGetActualBridgeName(net),
net-ifname));
-ignore_value(virNetDevTapDelete(net-ifname));
+ignore_value(virNetDevTapDelete(net-ifname, NULL));
}
}
}
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 0bc4a4d..979fb13 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1991,7 +1991,7 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr
driver,
/* Keep tun fd open and interface up to allow for IPv6 DAD to happen */
if (virNetDevTapCreateInBridgePort(network-def-bridge,
macTapIfName, network-def-mac,
- NULL, tapfd, 1, NULL, NULL,
+ NULL, NULL, tapfd, 1, NULL, NULL,
VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE |
VIR_NETDEV_TAP_CREATE_IFUP |
VIR_NETDEV_TAP_CREATE_PERSIST) 0)
{
@@ -2117,7 +2117,7 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr
driver,
if (macTapIfName) {
VIR_FORCE_CLOSE(tapfd);
-ignore_value(virNetDevTapDelete(macTapIfName));
+ignore_value(virNetDevTapDelete(macTapIfName, NULL));
VIR_FREE(macTapIfName);
}
@@ -2156,7 +2156,7 @@ static int
networkShutdownNetworkVirtual(virNetworkDriverStatePtr driver ATTRIBU
if (network-def-mac_specified) {
char *macTapIfName = networkBridgeDummyNicName(network-def-bridge);
if (macTapIfName) {
-ignore_value(virNetDevTapDelete(macTapIfName));
+ignore_value(virNetDevTapDelete(macTapIfName, NULL));
VIR_FREE(macTapIfName);
}
}
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 665a590..fc17aab 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -290,6 +290,10 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
bool template_ifname = false;
int actualType = virDomainNetGetActualType(net);
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+const char *tunpath = /dev/net/tun;
+
+if (net-backend.tap)
+tunpath = net-backend.tap;
if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK) {
bool fail = false;
@@ -338,18 +342,18 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
if (cfg-privileged) {
if (virNetDevTapCreateInBridgePort(brname, net-ifname, net-mac,
- def-uuid, tapfd, *tapfdSize,
+ def-uuid, tunpath, tapfd,
*tapfdSize,
virDomainNetGetActualVirtPortProfile(net),
virDomainNetGetActualVlan(net),
tap_create_flags) 0) {
-virDomainAuditNetDevice(def, net, /dev/net/tun, false);
+virDomainAuditNetDevice(def, net, tunpath, false);
goto cleanup;
}
} else {
if (qemuCreateInBridgePortWithHelper(cfg, brname,
net-ifname,
tapfd, tap_create_flags) 0) {
-
[libvirt] [PATCH] conf: snapshot: Don't default-snapshot empty floppy drives
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 phrd...@redhat.com --- 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])) +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
Re: [libvirt] [PATCH 02/26] remote_driver: Resolve Coverity RESOURCE_LEAK
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
Re: [libvirt] [PATCHv3 4/8] qemu: bulk stats: implement VCPU group
- Original Message - From: Peter Krempa pkre...@redhat.com To: Francesco Romani from...@redhat.com, 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.num.* 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.num.state - state of the virtual CPU num, as int + * from virVcpuState enum. + * vcpu.num.time - virtual cpu time spent by virtual CPU num + * as unsigned long long. + * vcpu.num.cpu - physical CPU pinned to virtual CPU num 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.num.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
Re: [libvirt] [PATCH 02/26] remote_driver: Resolve Coverity RESOURCE_LEAK
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 jfer...@redhat.com
---
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, sigh... 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
Re: [libvirt] [PATCHv3 4/8] qemu: bulk stats: implement VCPU group
On 09/11/14 17:54, Francesco Romani wrote: - Original Message - From: Peter Krempa pkre...@redhat.com To: Francesco Romani from...@redhat.com, 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.num.* 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.num.state - state of the virtual CPU num, as int + * from virVcpuState enum. + * vcpu.num.time - virtual cpu time spent by virtual CPU num + * as unsigned long long. + * vcpu.num.cpu - physical CPU pinned to virtual CPU num 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.num.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
Re: [libvirt] [PATCH] conf: snapshot: Don't default-snapshot empty floppy drives
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 phrd...@redhat.com --- 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
Re: [libvirt] [PATCH v3] leaseshelper: improvements to support all events
On Thu, Sep 11, 2014 at 8:38 PM, Peter Krempa pkre...@redhat.com 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 (network-name.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
+ = virJSONValueObjectGetString(lease_tmp,
server-duid))) {
+if (!server_duid) {
+/* Control reaches here when the 'action' is
not for an
+ * ipv6 lease
[libvirt] [PATCH v4 0/2] parallels: use parallels SDK instead of prlctl tool
This patchset begins reworking of parallels driver. We have published Opensource version of parallels SDK (under LGPL license), so libvirt can link with it. Changes in v4: * Remove reference counting for PrlApi_InitEx and PrlApi_Deinit * remove Parallels SDK prefix in log messages * rename 'out' labels to 'cleanup' Dmitry Guryanov (2): parallels: build with parallels SDK parallels: login to parallels SDK configure.ac | 24 ++-- po/POTFILES.in | 1 + src/Makefile.am | 8 +- src/parallels/parallels_driver.c | 16 ++- src/parallels/parallels_sdk.c| 241 +++ src/parallels/parallels_sdk.h| 30 + src/parallels/parallels_utils.h | 4 + 7 files changed, 310 insertions(+), 14 deletions(-) create mode 100644 src/parallels/parallels_sdk.c create mode 100644 src/parallels/parallels_sdk.h -- 1.9.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 1/2] parallels: build with parallels SDK
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 dgurya...@parallels.com --- 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
[libvirt] [PATCH v4 2/2] parallels: login to parallels SDK
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 dgurya...@parallels.com
---
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
+ * http://www.gnu.org/licenses/.
+ *
+ */
+
+#include config.h
+
+#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,
+
Re: [libvirt] [PATCH 02/26] remote_driver: Resolve Coverity RESOURCE_LEAK
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
Re: [libvirt] [PATCH] conf: snapshot: Don't default-snapshot empty floppy drives
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 phrd...@redhat.com --- 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
Re: [libvirt] [PATCH] conf: snapshot: Don't default-snapshot empty floppy drives
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 phrd...@redhat.com --- 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
Re: [libvirt] [PATCHv3 4/8] qemu: bulk stats: implement VCPU group
- Original Message - From: Peter Krempa pkre...@redhat.com To: Francesco Romani from...@redhat.com, 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.num.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
Re: [libvirt] [PATCHv3 5/8] qemu: bulk stats: implement interface group
- Original Message -
From: Peter Krempa pkre...@redhat.com
To: Francesco Romani from...@redhat.com, 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.num.name - name of the interface num as string.
+ * net.num.rx.bytes - bytes received as long long.
+ * net.num.rx.pkts - packets received as long long.
+ * net.num.rx.errs - receive errors as long long.
+ * net.num.rx.drop - receive packets dropped as long long.
+ * net.num.tx.bytes - bytes transmitted as long long.
+ * net.num.tx.pkts - packets transmitted as long long.
+ * net.num.tx.errs - transmission errors as long long.
+ * net.num.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' expects argument of type
'unsigned int', but argument 4 has type 'size_t' [-Werror=format=]
$ gcc --version
gcc
Re: [libvirt] [PATCHv3 5/8] qemu: bulk stats: implement interface group
On 09/11/14 19:11, Francesco Romani wrote:
- Original Message -
From: Peter Krempa pkre...@redhat.com
To: Francesco Romani from...@redhat.com, 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.num.name - name of the interface num as string.
+ * net.num.rx.bytes - bytes received as long long.
+ * net.num.rx.pkts - packets received as long long.
+ * net.num.rx.errs - receive errors as long long.
+ * net.num.rx.drop - receive packets dropped as long long.
+ * net.num.tx.bytes - bytes transmitted as long long.
+ * net.num.tx.pkts - packets transmitted as long long.
+ * net.num.tx.errs - transmission errors as long long.
+ * net.num.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_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=]
[libvirt] [PATCH 3/6] util: storage: Allow metadata crawler to report useful errors
Add a new parameter to virStorageFileGetMetadata that will break the
backing chain detection process and report useful error message rather
than having to use virStorageFileChainGetBroken.
This patch just introduces the option, usage will be provided
separately.
---
src/qemu/qemu_domain.c| 3 ++-
src/security/virt-aa-helper.c | 2 +-
src/storage/storage_driver.c | 22 +++---
src/storage/storage_driver.h | 3 ++-
tests/virstoragetest.c| 2 +-
5 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index bd7d511..88c5d1b 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2659,7 +2659,8 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
if (virStorageFileGetMetadata(disk-src,
uid, gid,
- cfg-allowDiskFormatProbing) 0)
+ cfg-allowDiskFormatProbing,
+ false) 0)
ret = -1;
cleanup:
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index a06ba44..9afc8db 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -932,7 +932,7 @@ get_files(vahControl * ctl)
*/
if (!disk-src-backingStore) {
bool probe = ctl-allowDiskFormatProbing;
-virStorageFileGetMetadata(disk-src, -1, -1, probe);
+virStorageFileGetMetadata(disk-src, -1, -1, probe, false);
}
/* XXX passing ignoreOpenFailure = true to get back to the behavior
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 433d7b7..1963bd6 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -2783,6 +2783,7 @@ static int
virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
uid_t uid, gid_t gid,
bool allow_probe,
+ bool fail,
virHashTablePtr cycle)
{
int ret = -1;
@@ -2847,9 +2848,12 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
else
backingStore-format = backingFormat;
-if (virStorageFileGetMetadataRecurse(backingStore,
- uid, gid, allow_probe,
- cycle) 0) {
+if ((ret = virStorageFileGetMetadataRecurse(backingStore,
+uid, gid, allow_probe, fail,
+cycle)) 0) {
+if (fail)
+goto cleanup;
+
/* if we fail somewhere midway, just accept and return a
* broken chain */
ret = 0;
@@ -2883,15 +2887,19 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr
src,
* format, since a malicious guest can turn a raw file into any
* other non-raw format at will.
*
+ * If @fail is true, the whole function fails with a possibly sane error
+ * instead of just returning a broken chain.
+ *
* Caller MUST free result after use via virStorageSourceFree.
*/
int
virStorageFileGetMetadata(virStorageSourcePtr src,
uid_t uid, gid_t gid,
- bool allow_probe)
+ bool allow_probe,
+ bool fail)
{
-VIR_DEBUG(path=%s format=%d uid=%d gid=%d probe=%d,
- src-path, src-format, (int)uid, (int)gid, allow_probe);
+VIR_DEBUG(path=%s format=%d uid=%d gid=%d probe=%d, fail=%d,
+ src-path, src-format, (int)uid, (int)gid, allow_probe, fail);
virHashTablePtr cycle = NULL;
int ret = -1;
@@ -2903,7 +2911,7 @@ virStorageFileGetMetadata(virStorageSourcePtr src,
src-format = allow_probe ? VIR_STORAGE_FILE_AUTO :
VIR_STORAGE_FILE_RAW;
ret = virStorageFileGetMetadataRecurse(src, uid, gid,
- allow_probe, cycle);
+ allow_probe, fail, cycle);
virHashFree(cycle);
return ret;
diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h
index e773928..54a17a3 100644
--- a/src/storage/storage_driver.h
+++ b/src/storage/storage_driver.h
@@ -50,7 +50,8 @@ bool virStorageFileSupportsSecurityDriver(virStorageSourcePtr
src);
int virStorageFileGetMetadata(virStorageSourcePtr src,
uid_t uid, gid_t gid,
- bool allow_probe)
+ bool allow_probe,
+ bool fail)
ATTRIBUTE_NONNULL(1);
int virStorageTranslateDiskSourcePool(virConnectPtr conn,
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index e2ee3ff..29f5c7a 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -119,7 +119,7 @@ testStorageFileGetMetadata(const char *path,
if (VIR_STRDUP(ret-path, path) 0)
goto
[libvirt] [PATCH 6/6] qemu: Improve check for local storage
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
[libvirt] [PATCH 0/6] Improve backing store error reporting
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(-) -- 2.1.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/6] qemu: Drop unused formatting of uuid
The formatted UUID isn't used anywhere else in
qemuDomainCheckDiskStartupPolicy. Drop it.
---
src/qemu/qemu_domain.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c0306d7..bd7d511 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2403,12 +2403,9 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver,
size_t diskIndex,
bool cold_boot)
{
-char uuid[VIR_UUID_STRING_BUFLEN];
int startupPolicy = vm-def-disks[diskIndex]-startupPolicy;
int device = vm-def-disks[diskIndex]-device;
-virUUIDFormat(vm-def-uuid, uuid);
-
switch ((virDomainStartupPolicy) startupPolicy) {
case VIR_DOMAIN_STARTUP_POLICY_OPTIONAL:
/* Once started with an optional disk, qemu saves its section
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/6] qemu: Report better errors from broken backing chains
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
[libvirt] [PATCH 1/6] util: Add function to check if a virStorageSource is empty
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(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index fdf4548..e819049 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1941,6 +1941,7 @@ virStorageSourceFree;
virStorageSourceGetActualType;
virStorageSourceGetSecurityLabelDef;
virStorageSourceInitChainElement;
+virStorageSourceIsEmpty;
virStorageSourceIsLocalStorage;
virStorageSourceNewFromBacking;
virStorageSourcePoolDefFree;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 299edcd..ca8be7f 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1976,6 +1976,26 @@ virStorageSourceIsLocalStorage(virStorageSourcePtr src)
/**
+ * virStorageSourceIsEmpty:
+ *
+ * @src: disk source to check
+ *
+ * Returns true if @src points to an empty storage source.
+ */
+bool
+virStorageSourceIsEmpty(virStorageSourcePtr src)
+{
+if (virStorageSourceIsLocalStorage(src) !src-path)
+return true;
+
+if (src-type == VIR_STORAGE_TYPE_NONE)
+return true;
+
+return false;
+}
+
+
+/**
* virStorageSourceBackingStoreClear:
*
* @src: disk source to clear
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index eccbf4e..2583e10 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -353,6 +353,7 @@ void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr
def);
void virStorageSourceClear(virStorageSourcePtr def);
int virStorageSourceGetActualType(virStorageSourcePtr def);
bool virStorageSourceIsLocalStorage(virStorageSourcePtr src);
+bool virStorageSourceIsEmpty(virStorageSourcePtr src);
void virStorageSourceFree(virStorageSourcePtr def);
void virStorageSourceBackingStoreClear(virStorageSourcePtr def);
virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent);
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/6] storage: Improve error message when traversing backing chains
Report also the name of the parent file and uid/gid used to access it to
help debugging broken storage configurations.
---
src/storage/storage_driver.c | 20 +++-
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 1963bd6..87f3c1c 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -2781,6 +2781,7 @@ virStorageFileChown(virStorageSourcePtr src,
/* Recursive workhorse for virStorageFileGetMetadata. */
static int
virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
+ virStorageSourcePtr parent,
uid_t uid, gid_t gid,
bool allow_probe,
bool fail,
@@ -2805,9 +2806,18 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
return -1;
if (virStorageFileAccess(src, F_OK) 0) {
-virReportSystemError(errno,
- _(Cannot access backing file %s),
- src-path);
+if (src == parent) {
+virReportSystemError(errno,
+ _(Cannot access storage file '%s'
+ (as uid:%d, gid:%d)),
+ src-path, (int)uid, (int)gid);
+} else {
+virReportSystemError(errno,
+ _(Cannot access backing file '%s'
+ of storage file '%s' (as uid:%d, gid:%d)),
+ src-path, parent-path, (int)uid, (int)gid);
+}
+
goto cleanup;
}
@@ -2848,7 +2858,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
else
backingStore-format = backingFormat;
-if ((ret = virStorageFileGetMetadataRecurse(backingStore,
+if ((ret = virStorageFileGetMetadataRecurse(backingStore, parent,
uid, gid, allow_probe, fail,
cycle)) 0) {
if (fail)
@@ -2910,7 +2920,7 @@ virStorageFileGetMetadata(virStorageSourcePtr src,
if (src-format = VIR_STORAGE_FILE_NONE)
src-format = allow_probe ? VIR_STORAGE_FILE_AUTO :
VIR_STORAGE_FILE_RAW;
-ret = virStorageFileGetMetadataRecurse(src, uid, gid,
+ret = virStorageFileGetMetadataRecurse(src, src, uid, gid,
allow_probe, fail, cycle);
virHashFree(cycle);
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 1/2] DUPLICATE: util: Add function to check if a virStorageSource is empty
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(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index fdf4548..e819049 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1941,6 +1941,7 @@ virStorageSourceFree;
virStorageSourceGetActualType;
virStorageSourceGetSecurityLabelDef;
virStorageSourceInitChainElement;
+virStorageSourceIsEmpty;
virStorageSourceIsLocalStorage;
virStorageSourceNewFromBacking;
virStorageSourcePoolDefFree;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 299edcd..ca8be7f 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1976,6 +1976,26 @@ virStorageSourceIsLocalStorage(virStorageSourcePtr src)
/**
+ * virStorageSourceIsEmpty:
+ *
+ * @src: disk source to check
+ *
+ * Returns true if @src points to an empty storage source.
+ */
+bool
+virStorageSourceIsEmpty(virStorageSourcePtr src)
+{
+if (virStorageSourceIsLocalStorage(src) !src-path)
+return true;
+
+if (src-type == VIR_STORAGE_TYPE_NONE)
+return true;
+
+return false;
+}
+
+
+/**
* virStorageSourceBackingStoreClear:
*
* @src: disk source to clear
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index eccbf4e..2583e10 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -353,6 +353,7 @@ void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr
def);
void virStorageSourceClear(virStorageSourcePtr def);
int virStorageSourceGetActualType(virStorageSourcePtr def);
bool virStorageSourceIsLocalStorage(virStorageSourcePtr src);
+bool virStorageSourceIsEmpty(virStorageSourcePtr src);
void virStorageSourceFree(virStorageSourcePtr def);
void virStorageSourceBackingStoreClear(virStorageSourcePtr def);
virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent);
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] add migration support for OpenVZ driver
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 a...@sigxcpu.org 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
Re: [libvirt] [PATCHv2 2/2] conf: snapshot: Don't default-snapshot empty drives
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 phrd...@redhat.com --- 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
Re: [libvirt] [PATCH RFC] network: try to eliminate default network conflict during package install
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,/name,/name\n uuid$UUID/uuid, \
+sed -e s/${orig_sub}/${sub}/g \
+-e s,/name,/name\n uuid$UUID/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
Re: [libvirt] [PATCH V3 4/5] libxl: fix mapping of lifecycle actions
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
Re: [libvirt] [PATCH 1/2] nvram: Fix permissions
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 mpriv...@redhat.com
---
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 ler...@redhat.com
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
[libvirt] [PATCH 5/9] virfile: Resolve Coverity RESOURCE_LEAK
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 jfer...@redhat.com
---
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
[libvirt] [PATCH 8/9] libxl: Resolve Coverity CHECKED_RETURN
Add a check of the return for virDomainHostdevInsert() like every other call. Signed-off-by: John Ferlan jfer...@redhat.com --- src/libxl/libxl_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 17d6257..2f2c590 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2891,7 +2891,8 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev) return -1; } -virDomainHostdevInsert(vmdef, hostdev); +if (virDomainHostdevInsert(vmdef, hostdev) 0) +return -1; break; default: -- 1.9.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/9] virutil: Resolve Coverity RESOURCE_LEAK
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 jfer...@redhat.com
---
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
[libvirt] [PATCH 6/9] virtime: Resolve Coverity DEADCODE
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 jfer...@redhat.com
---
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
[libvirt] [PATCH 9/9] virstoragefile: Resolve Coverity DEADCODE
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 jfer...@redhat.com --- 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
[libvirt] [PATCH 7/9] qemu: Resolve Coverity FORWARD_NULL
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 jfer...@redhat.com --- 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
[libvirt] [PATCH 2/9] virsh: Resolve Coverity NEGATIVE_RETURNS
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 jfer...@redhat.com
---
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
[libvirt] [PATCH 0/9] More Coverity fixes
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
[libvirt] [PATCH 1/9] remote_driver: Resolve Coverity RESOURCE_LEAK
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 jfer...@redhat.com
---
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
[libvirt] [PATCH 3/9] daemon: Resolve Coverity RESOURCE_LEAK
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 jfer...@redhat.com --- daemon/libvirtd.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 0503cd0..05ee50e 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -163,9 +163,9 @@ static int daemonForkIntoBackground(const char *argv0) VIR_FORCE_CLOSE(statuspipe[0]); -if ((stdinfd = open(/dev/null, O_RDONLY)) 0) +if ((stdinfd = open(/dev/null, O_RDONLY)) = STDERR_FILENO) goto cleanup; -if ((stdoutfd = open(/dev/null, O_WRONLY)) 0) +if ((stdoutfd = open(/dev/null, O_WRONLY)) = STDERR_FILENO) goto cleanup; if (dup2(stdinfd, STDIN_FILENO) != STDIN_FILENO) goto cleanup; @@ -173,9 +173,9 @@ static int daemonForkIntoBackground(const char *argv0) goto cleanup; if (dup2(stdoutfd, STDERR_FILENO) != STDERR_FILENO) goto cleanup; -if (stdinfd STDERR_FILENO VIR_CLOSE(stdinfd) 0) +if (VIR_CLOSE(stdinfd) 0) goto cleanup; -if (stdoutfd STDERR_FILENO VIR_CLOSE(stdoutfd) 0) +if (VIR_CLOSE(stdoutfd) 0) goto cleanup; if (setsid() 0) -- 1.9.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH V2 2/4] src/xenconfig: Xen-xl parser
Kiarie Kahurani wrote:
Introduce a xen xl parser
[...]
diff --git a/configure.ac b/configure.ac
index f93c6c2..0daf411 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2177,6 +2177,13 @@ if test -z $PERL; then
AC_MSG_ERROR([Failed to find perl.])
fi
+AC_PROG_LEX
+if test x$LEX != xflex; then
+LEX=$SHELL $missing_dir/missing flex
+AC_SUBST([LEX_OUPTUT_ROOT], [lex.yy])
+AC_SUBST([LEXLIB], [''])
+fi
+
As mentioned previously, I think you only need AM_PROG_FLEX here. I
peeked at the macro in aclocal.m4 and it looks to handle the missing
flex case.
AC_ARG_WITH([test-suite],
[AS_HELP_STRING([--with-test-suite],
[build test suite by default @:@default=check@:@])],
diff --git a/src/Makefile.am b/src/Makefile.am
index 46e411e..52c7f1a 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -35,7 +35,6 @@ INCLUDES = -I../gnulib/lib
\
$(GETTEXT_CPPFLAGS)
AM_CFLAGS = $(LIBXML_CFLAGS)\
- $(WARN_CFLAGS) \
$(LOCK_CHECKING_CFLAGS) \
$(WIN32_EXTRA_CFLAGS) \
$(COVERAGE_CFLAGS)
@@ -964,11 +963,25 @@ CPU_SOURCES =
\
VMX_SOURCES =\
vmx/vmx.c vmx/vmx.h
+XENCONFIG_GENERATED = \
+ xenconfig/libxlu_disk_l.c \
+xenconfig/libxlu_disk_l.h
+
+$(XENCONFIG_GENERATED): $(srcdir)/xenconfig/libxlu_disk_l.l \
+ $(srcdir)/xenconfig/libxlu_disk_i.h Makefile.am
+ $(AM_V_GEN)$(LEX) --outfile=$(srcdir)/xenconfig/libxlu_disk_l.c
\
+ --header-file=$(srcdir)/xenconfig/libxlu_disk_l.h \
+ $(srcdir)/xenconfig/libxlu_disk_l.l
+
+CLEANFILES += $(XENCONFIG_GENERATED)
+
XENCONFIG_SOURCES = \
xenconfig/xenxs_private.h \
- xenconfig/xen_common.c xenconfig/xen_common.h \
- xenconfig/xen_sxpr.c xenconfig/xen_sxpr.h \
- xenconfig/xen_xm.c xenconfig/xen_xm.h
+ xenconfig/xen_common.c xenconfig/xen_common.h \
+ xenconfig/xen_sxpr.c xenconfig/xen_sxpr.h\
+ xenconfig/xen_xm.c xenconfig/xen_xm.h \
+ xenconfig/xen_xl.c xenconfig/xen_xl.h \
+ $(XENCONFIG_GENERATED)
Same comment as before. According to the automake documentation this
should be done with
BUILT_SOURCES += xenconfig/libxlu_disk_l.h xenconfig/libxlu_disk_l.c
XENCONFIG_SOURCES = \
xenconfig/xenxs_private.h \
xenconfig/xen_common.c xenconfig/xen_common.h \
xenconfig/xen_sxpr.c xenconfig/xen_sxpr.h \
xenconfig/xen_xm.c xenconfig/xen_xm.h \
xenconfig/xen_xl.c xenconfig/xen_xl.h \
xenconfig/libxlu_disk_l.l
But also as mentioned before, I can't figure out how to convince
automake to tell flex to generate the header file as well as the .c file.
Eric, do you have any experience with automake and flex? The following
thread makes it sound as though automake supports flex's
'--header-file=' option, but I can't find any hint on how to make it work
http://lists.gnu.org/archive/html/bug-automake/2012-08/msg00069.html
Regards,
Jim
pkgdata_DATA = cpu/cpu_map.xml
diff --git a/src/libvirt_xenconfig.syms b/src/libvirt_xenconfig.syms
index 6541685..3e2e5d6 100644
--- a/src/libvirt_xenconfig.syms
+++ b/src/libvirt_xenconfig.syms
@@ -16,6 +16,10 @@ xenParseSxprChar;
xenParseSxprSound;
xenParseSxprString;
+#xenconfig/xen_xl.h
+xenFormatXL;
+xenParseXL;
+
# xenconfig/xen_xm.h
xenFormatXM;
xenParseXM;
diff --git a/src/xenconfig/libxlu_disk_i.h b/src/xenconfig/libxlu_disk_i.h
new file mode 100644
index 000..911ea42
--- /dev/null
+++ b/src/xenconfig/libxlu_disk_i.h
@@ -0,0 +1,28 @@
+#ifndef LIBXLU_DISK_I_H
+#define LIBXLU_DISK_I_H
+
+#include ../util/virconf.h
+
+typedef struct {
+virConfPtr conf;
+int err;
+void *scanner;
+YY_BUFFER_STATE buf;
+virDomainDiskDefPtr disk;
+int access_set, had_depr_prefix;
+const char *spec;
+} DiskParseContext;
+
+void xlu__disk_err(DiskParseContext *dpc, const char *erroneous,
+ const char *message);
+
+
+#endif /*LIBXLU_DISK_I_H*/
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/src/xenconfig/libxlu_disk_l.l b/src/xenconfig/libxlu_disk_l.l
new file mode 100644
index 000..8842318
--- /dev/null
+++ b/src/xenconfig/libxlu_disk_l.l
@@ -0,0 +1,259 @@
+/* -*- fundamental -*- */
+/*
+ * libxlu_disk_l.l - parser for disk specification strings
+ *
+ * Copyright (C) 2011 Citrix Ltd.
+ * Author
