[libvirt] [PATCH] qemu: add tun devices to cgroup ACL
From: Roy Li
When systemd is used as init, CGroup will be enabled by default, and
lead to virsh unable to start VM(which is using the tun/tap), since
virsh can not access /dev/net/tun device
Signed-off-by: Roy Li
---
src/qemu/qemu.conf | 3 ++-
src/qemu/qemu_cgroup.c | 2 +-
src/qemu/test_libvirtd_qemu.aug.in | 1 +
3 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index c6db568..225cd0f 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -272,7 +272,8 @@
#"/dev/null", "/dev/full", "/dev/zero",
#"/dev/random", "/dev/urandom",
#"/dev/ptmx", "/dev/kvm", "/dev/kqemu",
-#"/dev/rtc","/dev/hpet", "/dev/vfio/vfio"
+#"/dev/rtc","/dev/hpet", "/dev/vfio/vfio",
+#"/dev/net/tun"
#]
#
# RDMA migration requires the following extra files to be added to the list:
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 0e94cae..7c15c07 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -45,7 +45,7 @@ static const char *const defaultDeviceACL[] = {
"/dev/random", "/dev/urandom",
"/dev/ptmx", "/dev/kvm", "/dev/kqemu",
"/dev/rtc", "/dev/hpet", "/dev/vfio/vfio",
-NULL,
+"/dev/net/tun", NULL,
};
#define DEVICE_PTY_MAJOR 136
#define DEVICE_SND_MAJOR 116
diff --git a/src/qemu/test_libvirtd_qemu.aug.in
b/src/qemu/test_libvirtd_qemu.aug.in
index 30fd27e..2ab7a8d 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -48,6 +48,7 @@ module Test_libvirtd_qemu =
{ "9" = "/dev/rtc" }
{ "10" = "/dev/hpet" }
{ "11" = "/dev/vfio/vfio" }
+{ "12" = "/dev/net/tun" }
}
{ "save_image_format" = "raw" }
{ "dump_image_format" = "raw" }
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] systemd-cgroups-agent not working in containers
Hi! I run a Linux container setup with openSUSE 13.1/2 as guest distro. After some time containers slow down. An investigation showed that the containers slow down because a lot of stale user sessions slow down almost all systemd tools, mostly systemctl. loginctl reports many thousand sessions. All in state "closing". The vast majority of these sessions are from crond an ssh logins. It turned out that sessions are never closed and stay around. The control group of a said session contains zero tasks. So I started to explore why systemd keeps it. After another few hours of debugging I realized that systemd never issues the release signal from cgroups. Also calling the release agent by hand did not help. i.e. /usr/lib/systemd/systemd-cgroups-agent /user.slice/user-0.slice/session-c324.scope Therefore systemd never recognizes that a server/session has no more tasks and will close it. First I thought it is an issue in libvirt combined with user namespaces. But I can trigger this also without user namespaces and also with systemd-nspawn. Tested with systemd 208 and 210 from openSUSE, their packages have all known bugfixes. Any idea where to look further? How do you run the most current systemd on your distro? Thanks, //richard -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] nwfilter: Add support for icmpv6 filtering
Make use of the ebtables functionality to be able to filter certain
parameters of icmpv6 packets. Extend the XML parser for icmpv6 types,
type ranges, codes, and code ranges. Extend the nwfilter documentation,
schema, and test cases.
Being able to filter icmpv6 types and codes helps extending the DHCP
snooper for IPv6 and filtering at least some parameters of IPv6's NDP
(Neighbor Discovery Protocol) packets. However, the filtering will not
be as good as the filtering of ARP packets since we cannot check on IP
addresses in the payload of the NDP packets.
Signed-off-by: Stefan Berger stef...@linux.vnet.ibm.com
---
docs/formatnwfilter.html.in| 20 +++
docs/schemas/nwfilter.rng | 26 +
src/conf/nwfilter_conf.c | 26 +
src/conf/nwfilter_conf.h | 4 ++
src/nwfilter/nwfilter_ebiptables_driver.c | 80 ++
tests/nwfilterxml2firewalldata/ipv6-linux.args | 16 ++
tests/nwfilterxml2firewalldata/ipv6.xml| 38
tests/nwfilterxml2xmlin/ipv6-test.xml | 38
tests/nwfilterxml2xmlout/ipv6-test.xml | 12
9 files changed, 260 insertions(+)
diff --git a/docs/formatnwfilter.html.in b/docs/formatnwfilter.html.in
index 073b852..7c0dd5b 100644
--- a/docs/formatnwfilter.html.in
+++ b/docs/formatnwfilter.html.in
@@ -1197,6 +1197,26 @@
End of range of valid destination ports; requires
protocol
+ type(Since 1.x.y)
+ UINT8
+ ICMPv6 type; requires protocol to be set to
icmpv6
+
+
+ typeend(Since 1.x.y)
+ UINT8
+ ICMPv6 type end of range; requires protocol to be
set to icmpv6
+
+
+ code(Since 1.x.y)
+ UINT8
+ ICMPv6 code; requires protocol to be set to
icmpv6
+
+
+ code(Since 1.x.y)
+ UINT8
+ ICMPv6 code end of range; requires protocol to be
set to icmpv6
+
+
comment (Since 0.8.5)
STRING
text with max. 256 characters
diff --git a/docs/schemas/nwfilter.rng b/docs/schemas/nwfilter.rng
index 2b54fd5..9df39c0 100644
--- a/docs/schemas/nwfilter.rng
+++ b/docs/schemas/nwfilter.rng
@@ -90,6 +90,7 @@
+
@@ -588,6 +589,31 @@
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
index 074d745..0108dbe 100644
--- a/src/conf/nwfilter_conf.c
+++ b/src/conf/nwfilter_conf.c
@@ -1445,6 +1445,26 @@ static const virXMLAttr2Struct ipv6Attributes[] = {
.datatype = DATATYPE_UINT16 | DATATYPE_UINT16_HEX,
.dataIdx = offsetof(virNWFilterRuleDef,
p.ipv6HdrFilter.portData.dataDstPortEnd),
},
+{
+.name = "type",
+.datatype = DATATYPE_UINT8 | DATATYPE_UINT8_HEX,
+.dataIdx = offsetof(virNWFilterRuleDef,
p.ipv6HdrFilter.dataICMPTypeStart),
+},
+{
+.name = "typeend",
+.datatype = DATATYPE_UINT8 | DATATYPE_UINT8_HEX,
+.dataIdx = offsetof(virNWFilterRuleDef,
p.ipv6HdrFilter.dataICMPTypeEnd),
+},
+{
+.name = "code",
+.datatype = DATATYPE_UINT8 | DATATYPE_UINT8_HEX,
+.dataIdx = offsetof(virNWFilterRuleDef,
p.ipv6HdrFilter.dataICMPCodeStart),
+},
+{
+.name = "codeend",
+.datatype = DATATYPE_UINT8 | DATATYPE_UINT8_HEX,
+.dataIdx = offsetof(virNWFilterRuleDef,
p.ipv6HdrFilter.dataICMPCodeEnd),
+},
COMMENT_PROP_IPHDR(ipv6HdrFilter),
{
.name = NULL,
@@ -2219,6 +2239,12 @@ virNWFilterRuleDefFixup(virNWFilterRuleDefPtr rule)
rule->p.ipv6HdrFilter.ipHdr.dataSrcIPAddr);
COPY_NEG_SIGN(rule->p.ipv6HdrFilter.ipHdr.dataDstIPMask,
rule->p.ipv6HdrFilter.ipHdr.dataDstIPAddr);
+COPY_NEG_SIGN(rule->p.icmpHdrFilter.dataICMPTypeend,
+ rule->p.icmpHdrFilter.dataICMPType);
+COPY_NEG_SIGN(rule->p.icmpHdrFilter.dataICMPCode,
+ rule->p.icmpHdrFilter.dataICMPType);
+COPY_NEG_SIGN(rule->p.icmpHdrFilter.dataICMPCodeend,
+ rule->p.icmpHdrFilter.dataICMPType);
virNWFilterRuleDefFixupIPSet(&rule->p.ipv6HdrFilter.ipHdr);
break;
diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h
index f81df60..6e68ecc 100644
--- a/src/conf/nwfilter_conf.h
+++ b/src/conf/nwfilter_conf.h
@@ -265,6 +265,10 @@ struct _ipv6HdrFilterDef {
ethHdrDataDef ethHdr;
ipHdrDataDef ipHdr;
portDataDefportData;
+nwItemDesc dataICMPTypeStart;
+nwItemDesc dataICMPTypeEnd;
+nwI
Re: [libvirt] Cloning libvirt
On 11/26/2014 11:51 AM, Kevin Wilson wrote: > Hi, > According to the web page you can clone libvirt by: > git clone git://libvirt.org/libvirt.git > There are places which block access via git connection, but permit git > clone by http/https. > So my question is: > is there also an http/https server from which you can clone it ? Sure; libvirt.org is not maintaining its own http git service, but there are plenty of mirrors out there doing that. Here's one: http://repo.or.cz/libvirt.git -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Cloning libvirt
Hi, According to the web page you can clone libvirt by: git clone git://libvirt.org/libvirt.git There are places which block access via git connection, but permit git clone by http/https. So my question is: is there also an http/https server from which you can clone it ? Regards, Kevin -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] test: fix nwfilter tests following changes in virfirewall.c
On 11/26/2014 11:53 AM, Eric Blake wrote: On 11/26/2014 09:26 AM, Stefan Berger wrote: Some of the nwfilter tests are now failing since --concurrent shows up in the ebtables command. To avoid this, implement a function preventing the probing for lock support in the eb/iptables tools and use it in the tests. Now that I've read Martin and Prerna's exchange, I'm wondering if we should instead make this override force the locking flags ON, and adjust the expected test output to expect the -w/--concurrent. Not sure... this patch is definitely 'simpler' and that one call we add to virsocket.h would only to be called by test cases. What we would test if we considered the --concurrent and -w flag is basically not much, just that some probing happened. Regards, Stefan -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] tests: Fix sharable typo
Signed-off-by: John Ferlan
---
tests/qemuargv2xmltest.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c
index 9c00cd9..cf4a6a8 100644
--- a/tests/qemuargv2xmltest.c
+++ b/tests/qemuargv2xmltest.c
@@ -30,7 +30,7 @@ static int blankProblemElements(char *data)
virtTestClearLineRegex("[[:digit:]]+",
data) < 0 ||
virtTestClearLineRegex("", data) < 0 ||
-virtTestClearLineRegex("", data) < 0)
+virtTestClearLineRegex("", data) < 0)
return -1;
return 0;
}
--
1.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] Manage SELinux labels on shared/readonly hostdev's
https://bugzilla.redhat.com/show_bug.cgi?id=1082521 Patch 1 is innocuous and perhaps could have been pushed as trivial... For Patch 2 I wasn't sure if I should use the virSecuritySELinuxSetFilecon or virSecuritySELinuxSetFileconOptional, so I went with the latter since it follows what virSecuritySELinuxSetSecurityImageLabelInternal does. Beyond the check for shared/readonly, the other difference would be for the else condition which uses the Optional now as opposed to the previous code which would call virSecuritySELinuxSetSecurityHostdevLabelHelper and use the non optional call to set the label. John Ferlan (2): tests: Fix sharable typo security: Manage SELinux labels on shared/readonly hostdev's src/security/security_selinux.c | 58 ++--- tests/qemuargv2xmltest.c| 2 +- 2 files changed, 50 insertions(+), 10 deletions(-) -- 1.9.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] security: Manage SELinux labels on shared/readonly hostdev's
https://bugzilla.redhat.com/show_bug.cgi?id=1082521
Support for shared hostdev's was added in a number of commits, initially
starting with 'f2c1d9a80' and most recently commit id 'fd243fc4' to fix
issues with the initial implementation. Missed in all those changes was
the need to mimic the virSELinux{Set|Restore}SecurityDiskLabel code to
handle the "shared" (or shareable) and readonly options when Setting
or Restoring the SELinux labels.
This patch will adjust the virSecuritySELinuxSetSecuritySCSILabel to not
use the virSecuritySELinuxSetSecurityHostdevLabelHelper in order to set
the label. Rather follow what the Disk code does by setting the label
differently based on whether shareable/readonly is set. This patch will
also modify the virSecuritySELinuxRestoreSecuritySCSILabel to follow
the same logic as virSecuritySELinuxRestoreSecurityImageLabelInt and not
restore the label if shared/readonly
Signed-off-by: John Ferlan
---
src/security/security_selinux.c | 58 ++---
1 file changed, 49 insertions(+), 9 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index f96be50..3a794b8 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -71,6 +71,12 @@ struct _virSecuritySELinuxData {
#define SECURITY_SELINUX_VOID_DOI "0"
#define SECURITY_SELINUX_NAME "selinux"
+/* Data structure to pass to *FileIterate so we have everything we need */
+struct SELinuxSCSICallbackData {
+virSecurityManagerPtr mgr;
+virDomainDefPtr def;
+};
+
static int
virSecuritySELinuxRestoreSecurityTPMFileLabelInt(virSecurityManagerPtr mgr,
virDomainDefPtr def,
@@ -1143,7 +1149,7 @@
virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr,
!src->backingStore)
return 0;
-/* Don't restore labels on readoly/shared disks, because other VMs may
+/* Don't restore labels on readonly/shared disks, because other VMs may
* still be accessing these. Alternatively we could iterate over all
* running domains and try to figure out if it is in use, but this would
* not work for clustered filesystems, since we can't see running VMs using
@@ -1309,14 +1315,31 @@ virSecuritySELinuxSetSecurityUSBLabel(virUSBDevicePtr
dev ATTRIBUTE_UNUSED,
}
static int
-virSecuritySELinuxSetSecuritySCSILabel(virSCSIDevicePtr dev ATTRIBUTE_UNUSED,
+virSecuritySELinuxSetSecuritySCSILabel(virSCSIDevicePtr dev,
const char *file, void *opaque)
{
-return virSecuritySELinuxSetSecurityHostdevLabelHelper(file, opaque);
+virSecurityLabelDefPtr secdef;
+struct SELinuxSCSICallbackData *ptr = opaque;
+virSecurityManagerPtr mgr = ptr->mgr;
+virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr);
+
+secdef = virDomainDefGetSecurityLabelDef(ptr->def, SECURITY_SELINUX_NAME);
+if (secdef == NULL)
+return 0;
+
+if (virSCSIDeviceGetShareable(dev))
+return virSecuritySELinuxSetFileconOptional(file,
+data->file_context);
+else if (virSCSIDeviceGetReadonly(dev))
+return virSecuritySELinuxSetFileconOptional(file,
+data->content_context);
+else
+return virSecuritySELinuxSetFileconOptional(file, secdef->imagelabel);
}
static int
-virSecuritySELinuxSetSecurityHostdevSubsysLabel(virDomainDefPtr def,
+virSecuritySELinuxSetSecurityHostdevSubsysLabel(virSecurityManagerPtr mgr,
+virDomainDefPtr def,
virDomainHostdevDefPtr dev,
const char *vroot)
@@ -1377,17 +1400,27 @@
virSecuritySELinuxSetSecurityHostdevSubsysLabel(virDomainDefPtr def,
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: {
virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host;
+struct SELinuxSCSICallbackData *ptr;
+
+if (VIR_ALLOC(ptr) < 0)
+goto done;
+ptr->mgr = mgr;
+ptr->def = def;
+
virSCSIDevicePtr scsi =
virSCSIDeviceNew(NULL,
scsihostsrc->adapter, scsihostsrc->bus,
scsihostsrc->target, scsihostsrc->unit,
dev->readonly, dev->shareable);
-if (!scsi)
+if (!scsi) {
+VIR_FREE(ptr);
goto done;
+}
-ret = virSCSIDeviceFileIterate(scsi,
virSecuritySELinuxSetSecuritySCSILabel, def);
+ret = virSCSIDeviceFileIterate(scsi,
virSecuritySELinuxSetSecuritySCSILabel, ptr);
virSCSIDeviceFree(scsi);
+VIR_FREE(ptr);
break;
}
@@ -1454,7 +1487,7 @@
virSecuritySELinuxSetSecurityHostdevCapsLabel(virDomainDefPtr def,
static int
-virSecuritySELinuxSetSecurity
Re: [libvirt] [PATCH v2] test: fix nwfilter tests following changes in virfirewall.c
On 11/26/2014 09:26 AM, Stefan Berger wrote: > Some of the nwfilter tests are now failing since --concurrent shows > up in the ebtables command. To avoid this, implement a function > preventing the probing for lock support in the eb/iptables tools > and use it in the tests. Now that I've read Martin and Prerna's exchange, I'm wondering if we should instead make this override force the locking flags ON, and adjust the expected test output to expect the -w/--concurrent. -- 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 2/2] Tests : Make nwfilter testcases robust against optional locking flags.
On 11/26/2014 04:09 AM, Martin Kletzander wrote: > The problem is with expected results that are coded for these tests. On distros that support these flags, the issue would go away if the expected results take into account the locking flags. However, adding a permanent change to the expected args string would break older distros. >>> Actually, no, I wanted to unconditionally add the parameters there >>> only for tests. >>> >>> Looking at it more closely, this can fail only if you are building as >>> root, is that correct? >> >> Yes, that is correct. >> > > I'm testing a patch with different approach, I'll Cc you on that when > it is done. Let me know whether that works for you then. It looks like your work is duplicated with Stefan's patches: https://www.redhat.com/archives/libvir-list/2014-November/msg01022.html Hmm, Stefan's approach was to unconditionally DISABLE the locking parameter in the tests, but I'm now wondering if unconditionally ENABLING the parameter, and adjusting the expected test output, is the better course of action. At any rate, I agree that we want the tests to be independent of what the actual installation provides. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] test: fix nwfilter tests following changes in virfirewall.c
Some of the nwfilter tests are now failing since --concurrent shows
up in the ebtables command. To avoid this, implement a function
preventing the probing for lock support in the eb/iptables tools
and use it in the tests.
Signed-off-by: Stefan Berger
---
src/libvirt_private.syms | 1 +
src/util/virfirewall.c | 9 +
src/util/virfirewall.h | 2 ++
tests/nwfilterebiptablestest.c | 3 +++
tests/nwfilterxml2firewalltest.c | 2 ++
5 files changed, 17 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 2647d36..22d9116 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1362,6 +1362,7 @@ virFirewallRuleAddArgList;
virFirewallRuleAddArgSet;
virFirewallRuleGetArgCount;
virFirewallSetBackend;
+virFirewallSetLockOverride;
virFirewallStartRollback;
virFirewallStartTransaction;
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
index 8496062..b536912 100644
--- a/src/util/virfirewall.c
+++ b/src/util/virfirewall.c
@@ -107,6 +107,13 @@ VIR_ONCE_GLOBAL_INIT(virFirewall)
static bool iptablesUseLock;
static bool ip6tablesUseLock;
static bool ebtablesUseLock;
+static bool lockOverride; /* true to avoid lock probes */
+
+void
+virFirewallSetLockOverride(bool avoid)
+{
+lockOverride = avoid;
+}
static void
virFirewallCheckUpdateLock(bool *lockflag,
@@ -135,6 +142,8 @@ virFirewallCheckUpdateLocking(void)
const char *ebtablesArgs[] = {
EBTABLES_PATH, "--concurrent", "-L", NULL,
};
+if (lockOverride)
+return;
virFirewallCheckUpdateLock(&iptablesUseLock,
iptablesArgs);
virFirewallCheckUpdateLock(&ip6tablesUseLock,
diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h
index 1129219..dbf3975 100644
--- a/src/util/virfirewall.h
+++ b/src/util/virfirewall.h
@@ -106,4 +106,6 @@ void virFirewallStartRollback(virFirewallPtr firewall,
int virFirewallApply(virFirewallPtr firewall);
+void virFirewallSetLockOverride(bool avoid);
+
#endif /* __VIR_FIREWALL_H__ */
diff --git a/tests/nwfilterebiptablestest.c b/tests/nwfilterebiptablestest.c
index e04bc21..e1330ef 100644
--- a/tests/nwfilterebiptablestest.c
+++ b/tests/nwfilterebiptablestest.c
@@ -24,6 +24,7 @@
#include "testutils.h"
#include "nwfilter/nwfilter_ebiptables_driver.h"
#include "virbuffer.h"
+#include "virfirewall.h"
#define __VIR_FIREWALL_PRIV_H_ALLOW__
#include "virfirewallpriv.h"
@@ -522,6 +523,8 @@ mymain(void)
{
int ret = 0;
+virFirewallSetLockOverride(true);
+
if (virFirewallSetBackend(VIR_FIREWALL_BACKEND_DIRECT) < 0) {
ret = -1;
goto cleanup;
diff --git a/tests/nwfilterxml2firewalltest.c b/tests/nwfilterxml2firewalltest.c
index 01527f4..167ad42 100644
--- a/tests/nwfilterxml2firewalltest.c
+++ b/tests/nwfilterxml2firewalltest.c
@@ -474,6 +474,8 @@ mymain(void)
ret = -1; \
} while (0)
+virFirewallSetLockOverride(true);
+
if (virFirewallSetBackend(VIR_FIREWALL_BACKEND_DIRECT) < 0) {
ret = -1;
goto cleanup;
--
1.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] network: Fix upgrade from libvirt older than 1.2.4
On 11/26/2014 09:07 AM, Jiri Denemark wrote:
> Starting from libvirt-1.2.4, network state XML files moved to another
> directory (see commit b9e95491) and libvirt automatically migrates the
> network state files to a new location. However, the code used
> dirent.dt_type which is not supported by all filesystems. Thus, when
> libvirt is upgraded on a host which uses such filesystem, network state
> XMLs were not properly moved and running networks disappeared from
> libvirt.
>
> This patch falls back to stat() whenever dirent.dt_type is DT_UNKNOWN to
s/dt_type/d_type/
> fix this issue.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1167145
> Signed-off-by: Jiri Denemark
> ---
> src/network/bridge_driver.c | 23 +--
> 1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 6cb421c..7066dfe 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -499,15 +499,34 @@ networkMigrateStateFiles(void)
> }
>
> while ((direrr = virDirRead(dir, &entry, oldStateDir)) > 0) {
> +if (entry->d_type != DT_UNKNOWN &&
> +entry->d_type != DT_REG)
Even the existence of a d_type member is an extension that is not
guaranteed to compile. If someone complains, we may have to introduce a
helper macro (gnulib fts.c uses a macro D_TYPE(d) that resolves to
(d)->d_type where supported, and to DT_UNKNOWN elsewhere; although that
particular file is not LGPLv2 so we don't use it in libvirt). But as
your patch isn't changing whether something would compile, you don't
need to make that change now.
> +continue;
>
> -if (entry->d_type != DT_REG ||
> -STREQ(entry->d_name, ".") ||
> +if (STREQ(entry->d_name, ".") ||
> STREQ(entry->d_name, ".."))
> continue;
>
> if (virAsprintf(&oldPath, "%s/%s",
> oldStateDir, entry->d_name) < 0)
> goto cleanup;
> +
> +if (entry->d_type == DT_UNKNOWN) {
> +struct stat st;
> +
> +if (stat(oldPath, &st) < 0) {
Shouldn't this be lstat() instead of stat(), so we aren't chasing symlinks?
ACK with that answered.
--
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] test: fix nwfilter tests following changes in virfirewall.c
On 11/26/2014 08:58 AM, Stefan Berger wrote:
> Some of the nwfilter tests are now failing since --concurrent shows
> up in the ebtables command. To avoid this, implement a few functions
> that allow to set the booleans indicating how the locking is to be
> done to false, overriding the probing.
>
> Signed-off-by: Stefan Berger
> ---
> src/libvirt_private.syms | 3 +++
> src/util/virfirewall.c | 18 ++
> src/util/virfirewall.h | 4
> tests/nwfilterebiptablestest.c | 5 +
> tests/nwfilterxml2firewalltest.c | 4
> 5 files changed, 34 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 2647d36..376826a 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1353,7 +1353,10 @@ virFindFileInPath;
> virFirewallAddRule;
> virFirewallAddRuleFull;
> virFirewallApply;
> +virFirewallEbtablesNotUseLock;
Awkward naming scheme. If we keep it, I'd suggest
virFirewallEbtablesAvoidLock (and similar for the other functions). But
see below for an alternative suggestion with fewer lines of code required:
> +++ b/src/util/virfirewall.c
> @@ -108,6 +108,24 @@ static bool iptablesUseLock;
> static bool ip6tablesUseLock;
> static bool ebtablesUseLock;
>
> +void
> +virFirewallIptablesNotUseLock(void)
> +{
> +iptablesUseLock = false;
> +}
> +
> +void
> +virFirewallIp6tablesNotUseLock(void)
> +{
> +ip6tablesUseLock = false;
> +}
Rather than implementing 3 functions, why not just have one? Something
like:
static bool lockOverride; /* true to avoid lock probes */
void
virFirewallSetLockOverride(bool avoid)
{
lockOverride = avoid;
}
static void
virFirewallCheckUpdateLock(...)
{
if (lockOverride)
return;
... existing body
}
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] network: Fix upgrade from libvirt older than 1.2.4
Starting from libvirt-1.2.4, network state XML files moved to another
directory (see commit b9e95491) and libvirt automatically migrates the
network state files to a new location. However, the code used
dirent.dt_type which is not supported by all filesystems. Thus, when
libvirt is upgraded on a host which uses such filesystem, network state
XMLs were not properly moved and running networks disappeared from
libvirt.
This patch falls back to stat() whenever dirent.dt_type is DT_UNKNOWN to
fix this issue.
https://bugzilla.redhat.com/show_bug.cgi?id=1167145
Signed-off-by: Jiri Denemark
---
src/network/bridge_driver.c | 23 +--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 6cb421c..7066dfe 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -499,15 +499,34 @@ networkMigrateStateFiles(void)
}
while ((direrr = virDirRead(dir, &entry, oldStateDir)) > 0) {
+if (entry->d_type != DT_UNKNOWN &&
+entry->d_type != DT_REG)
+continue;
-if (entry->d_type != DT_REG ||
-STREQ(entry->d_name, ".") ||
+if (STREQ(entry->d_name, ".") ||
STREQ(entry->d_name, ".."))
continue;
if (virAsprintf(&oldPath, "%s/%s",
oldStateDir, entry->d_name) < 0)
goto cleanup;
+
+if (entry->d_type == DT_UNKNOWN) {
+struct stat st;
+
+if (stat(oldPath, &st) < 0) {
+virReportSystemError(errno,
+ _("failed to stat network status file
'%s'"),
+ oldPath);
+goto cleanup;
+}
+
+if (!S_ISREG(st.st_mode)) {
+VIR_FREE(oldPath);
+continue;
+}
+}
+
if (virFileReadAll(oldPath, 1024*1024, &contents) < 0)
goto cleanup;
--
2.1.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] test: fix nwfilter tests following changes in virfirewall.c
Some of the nwfilter tests are now failing since --concurrent shows
up in the ebtables command. To avoid this, implement a few functions
that allow to set the booleans indicating how the locking is to be
done to false, overriding the probing.
Signed-off-by: Stefan Berger
---
src/libvirt_private.syms | 3 +++
src/util/virfirewall.c | 18 ++
src/util/virfirewall.h | 4
tests/nwfilterebiptablestest.c | 5 +
tests/nwfilterxml2firewalltest.c | 4
5 files changed, 34 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 2647d36..376826a 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1353,7 +1353,10 @@ virFindFileInPath;
virFirewallAddRule;
virFirewallAddRuleFull;
virFirewallApply;
+virFirewallEbtablesNotUseLock;
virFirewallFree;
+virFirewallIp6tablesNotUseLock;
+virFirewallIptablesNotUseLock;
virFirewallNew;
virFirewallRemoveRule;
virFirewallRuleAddArg;
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
index 8496062..5df2a5f 100644
--- a/src/util/virfirewall.c
+++ b/src/util/virfirewall.c
@@ -108,6 +108,24 @@ static bool iptablesUseLock;
static bool ip6tablesUseLock;
static bool ebtablesUseLock;
+void
+virFirewallIptablesNotUseLock(void)
+{
+iptablesUseLock = false;
+}
+
+void
+virFirewallIp6tablesNotUseLock(void)
+{
+ip6tablesUseLock = false;
+}
+
+void
+virFirewallEbtablesNotUseLock(void)
+{
+ebtablesUseLock = false;
+}
+
static void
virFirewallCheckUpdateLock(bool *lockflag,
const char *const*args)
diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h
index 1129219..aa72a76 100644
--- a/src/util/virfirewall.h
+++ b/src/util/virfirewall.h
@@ -106,4 +106,8 @@ void virFirewallStartRollback(virFirewallPtr firewall,
int virFirewallApply(virFirewallPtr firewall);
+void virFirewallIptablesNotUseLock(void);
+void virFirewallIp6tablesNotUseLock(void);
+void virFirewallEbtablesNotUseLock(void);
+
#endif /* __VIR_FIREWALL_H__ */
diff --git a/tests/nwfilterebiptablestest.c b/tests/nwfilterebiptablestest.c
index e04bc21..e18a6c7 100644
--- a/tests/nwfilterebiptablestest.c
+++ b/tests/nwfilterebiptablestest.c
@@ -24,6 +24,7 @@
#include "testutils.h"
#include "nwfilter/nwfilter_ebiptables_driver.h"
#include "virbuffer.h"
+#include "virfirewall.h"
#define __VIR_FIREWALL_PRIV_H_ALLOW__
#include "virfirewallpriv.h"
@@ -527,6 +528,10 @@ mymain(void)
goto cleanup;
}
+virFirewallEbtablesNotUseLock();
+virFirewallIptablesNotUseLock();
+virFirewallIp6tablesNotUseLock();
+
if (virtTestRun("ebiptablesAllTeardown",
testNWFilterEBIPTablesAllTeardown,
NULL) < 0)
diff --git a/tests/nwfilterxml2firewalltest.c b/tests/nwfilterxml2firewalltest.c
index 01527f4..f6ef0e0 100644
--- a/tests/nwfilterxml2firewalltest.c
+++ b/tests/nwfilterxml2firewalltest.c
@@ -479,6 +479,10 @@ mymain(void)
goto cleanup;
}
+virFirewallEbtablesNotUseLock();
+virFirewallIptablesNotUseLock();
+virFirewallIp6tablesNotUseLock();
+
DO_TEST("ah");
DO_TEST("ah-ipv6");
DO_TEST("all");
--
1.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Revert "ip link needs 'name' in 3.16 to create the veth pair"
On 11/26/2014 08:21 AM, Eric Blake wrote: > On 11/26/2014 01:33 AM, Martin Kletzander wrote: >> This reverts commit 433b427ff853ab72d32573d415e6ec569b77c7cb. >> >> The patch was added in order to overcome a bug in iproute2 and since it >> was properly identified as a bug, particularly in openSUSE 13.2, and it >> is being worked on [1], the best solution for libvirt seems to be to >> keep the old behaviour. >> >> [1] https://bugzilla.novell.com/show_bug.cgi?id=907093 >> --- >> src/util/virnetdevveth.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) > > I'm 50:50 on this one. The workaround case is arguably more legible, > and is understood by all versions of iproute2 (including the buggy SUSE > release), so it's not like it is that ugly of a workaround. The revert > is clean if you want to do it, but I don't see any compelling reason > requiring us to revert. Maybe someone else can swing the vote. > > > Given the following - I think I'd be more in favor of the revert. Perhaps not exactly the same thing, but consider the posix_fallocate() issue with NFS for which I tried to submit workaround patches for, but was instead asked to submit a bug against glibc, see: http://www.redhat.com/archives/libvir-list/2014-August/msg00491.html So we didn't accept a workaround in this case and although this iproute2 issue has a much more expedient fix than posix_fallocate (still waiting, btw) - the decision was to leave the libvirt code 'buggy' until the root cause was fixed. So translate that here - if we revert this change then it seems for one version of iproute2 there's a bug. Seems this is easily document-able if we remove the patch (as ugly as the documentation could be). It seems the issue in keeping the code as is runs the risk that there is some iproute2 that doesn't understand or process the 'name' syntax, which is an unknown and could cause "other" failures... Hence, my vote for revert John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] ip link needs 'name' in 3.16 to create the veth pair
Am 26.11.2014 um 14:23 schrieb Eric Blake: > On 11/26/2014 02:25 AM, Richard Weinberger wrote: > > So I think we should keep that for those running the buggy 3.16. openSUSE has to fix their package and to serve a bugfix update, full stop. >>> >>> Thought that may not happen only to openSUSE... and that fix didn't harm >>> at all. >> >> Yes, but "fixing" this issue in libvirt is not correct. >> It needs to be fixed in the right place and iproute2 this is. >> >> libvirt is not the only user of this iproute2 feature. > > I agree that making all clients work around a bug is annoying; on the > other hand, once a client has worked around it, I see no reason to force > the client to have to revert, since the workaround is arguably more legible. I agree that we don't have to force the revert. But we have to be very sure that *every* iproute2 version understands the new command line. I'm not an iproute2 expert, so I can't tell for sure. I doubt I'd revert it. Distros which ship the broken iproute2 package have to fix it anyways as a lot of other tools/scripts break too. Thanks, //richard -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] ip link needs 'name' in 3.16 to create the veth pair
On 11/26/2014 02:25 AM, Richard Weinberger wrote: So I think we should keep that for those running the buggy 3.16. >>> >>> openSUSE has to fix their package and to serve a bugfix update, full stop. >> >> Thought that may not happen only to openSUSE... and that fix didn't harm >> at all. > > Yes, but "fixing" this issue in libvirt is not correct. > It needs to be fixed in the right place and iproute2 this is. > > libvirt is not the only user of this iproute2 feature. I agree that making all clients work around a bug is annoying; on the other hand, once a client has worked around it, I see no reason to force the client to have to revert, since the workaround is arguably more legible. -- 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] Revert "ip link needs 'name' in 3.16 to create the veth pair"
On 11/26/2014 01:33 AM, Martin Kletzander wrote: > This reverts commit 433b427ff853ab72d32573d415e6ec569b77c7cb. > > The patch was added in order to overcome a bug in iproute2 and since it > was properly identified as a bug, particularly in openSUSE 13.2, and it > is being worked on [1], the best solution for libvirt seems to be to > keep the old behaviour. > > [1] https://bugzilla.novell.com/show_bug.cgi?id=907093 > --- > src/util/virnetdevveth.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) I'm 50:50 on this one. The workaround case is arguably more legible, and is understood by all versions of iproute2 (including the buggy SUSE release), so it's not like it is that ugly of a workaround. The revert is clean if you want to do it, but I don't see any compelling reason requiring us to revert. Maybe someone else can swing the vote. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [libvirt-glib PATCHv3] Add gvir_domain_open_graphics_fd()
Add binding for virDomainOpenGraphicsFD. If virDomainOpenGraphicsFD is
not available, it means we are dealing with older libvirt so we create
the socket pair ourselves if that is the case.
---
configure.ac | 4 ++
libvirt-gobject/libvirt-gobject-domain.c | 72
libvirt-gobject/libvirt-gobject-domain.h | 4 ++
libvirt-gobject/libvirt-gobject.sym | 5 +++
4 files changed, 85 insertions(+)
diff --git a/configure.ac b/configure.ac
index 8cc3fca..bcb5cda 100644
--- a/configure.ac
+++ b/configure.ac
@@ -93,6 +93,10 @@ m4_if(m4_version_compare([2.61a.100],
LIBVIRT_GLIB_COMPILE_WARNINGS
PKG_CHECK_MODULES(LIBVIRT, libvirt >= $LIBVIRT_REQUIRED)
+# virDomainOpenGraphicsFD was introduced in libvirt 1.2.8
+AC_CHECK_LIB([virt],
+ [virDomainOpenGraphicsFD],
+ [AC_DEFINE([HAVE_VIR_DOMAIN_OPEN_GRAPHICS_FD], 1, [Have
virDomainOpenGraphicsFD?])])
enable_tests=no
PKG_CHECK_MODULES(GLIB2, glib-2.0 >= $GLIB2_TEST_REQUIRED,
[enable_tests=yes],
diff --git a/libvirt-gobject/libvirt-gobject-domain.c
b/libvirt-gobject/libvirt-gobject-domain.c
index 8df30d7..5a5189f 100644
--- a/libvirt-gobject/libvirt-gobject-domain.c
+++ b/libvirt-gobject/libvirt-gobject-domain.c
@@ -25,6 +25,9 @@
#include
#include
+#ifndef HAVE_VIR_DOMAIN_OPEN_GRAPHICS_FD
+#include
+#endif
#include "libvirt-glib/libvirt-glib.h"
#include "libvirt-gobject/libvirt-gobject.h"
@@ -1222,6 +1225,75 @@ cleanup:
}
/**
+ * gvir_domain_open_graphics_fd:
+ * @dom: the domain
+ * @idx: the graphics index
+ * @flags: extra flags, currently unused
+ *
+ * This will create a socket pair connected to the graphics backend of @dom.
One
+ * end of the socket will be returned on success, and the other end is handed
to
+ * the hypervisor. If @dom has multiple graphics backends configured, then @idx
+ * will determine which one is opened, starting from @idx 0.
+ *
+ * Returns: An fd on success, -1 on failure.
+ *
+ * Since: 0.2.0
+ */
+int gvir_domain_open_graphics_fd(GVirDomain *dom,
+ guint idx,
+ unsigned int flags,
+ GError **err)
+{
+GVirDomainPrivate *priv;
+int ret = -1;
+#ifndef HAVE_VIR_DOMAIN_OPEN_GRAPHICS_FD
+int pair[2];
+#endif
+
+g_return_val_if_fail(GVIR_IS_DOMAIN(dom), -1);
+g_return_val_if_fail(err == NULL || *err == NULL, -1);
+
+priv = dom->priv;
+
+#ifdef HAVE_VIR_DOMAIN_OPEN_GRAPHICS_FD
+ret = virDomainOpenGraphicsFD(priv->handle, idx, flags);
+if (ret <= 0) {
+gvir_set_error_literal(err, GVIR_DOMAIN_ERROR,
+ 0,
+ "Unable to open graphics");
+goto end;
+}
+
+#else
+if (socketpair(PF_UNIX, SOCK_STREAM, 0, pair) < 0) {
+g_set_error_literal(err, GVIR_DOMAIN_ERROR,
+0,
+"Failed to create socket pair");
+goto end;
+}
+
+if (virDomainOpenGraphics(priv->handle, idx, pair[0], flags) < 0) {
+virErrorPtr vir_err;
+
+vir_err = virGetLastError();
+gvir_set_error_literal(err, GVIR_DOMAIN_ERROR,
+ 0,
+ "Unable to open graphics");
+close(pair[0]);
+close(pair[1]);
+
+goto end;
+}
+close(pair[0]);
+ret = pair[1];
+
+#endif
+
+end:
+return ret;
+}
+
+/**
* gvir_domain_suspend:
* @dom: the domain to suspend
* @err: Place-holder for possible errors
diff --git a/libvirt-gobject/libvirt-gobject-domain.h
b/libvirt-gobject/libvirt-gobject-domain.h
index 47ed784..4fe381e 100644
--- a/libvirt-gobject/libvirt-gobject-domain.h
+++ b/libvirt-gobject/libvirt-gobject-domain.h
@@ -332,6 +332,10 @@ gboolean gvir_domain_open_graphics(GVirDomain *dom,
int fd,
unsigned int flags,
GError **err);
+int gvir_domain_open_graphics_fd(GVirDomain *dom,
+ guint idx,
+ unsigned int flags,
+ GError **err);
gboolean gvir_domain_suspend (GVirDomain *dom,
GError **err);
diff --git a/libvirt-gobject/libvirt-gobject.sym
b/libvirt-gobject/libvirt-gobject.sym
index d18769b..927cad9 100644
--- a/libvirt-gobject/libvirt-gobject.sym
+++ b/libvirt-gobject/libvirt-gobject.sym
@@ -260,4 +260,9 @@ LIBVIRT_GOBJECT_0.1.9 {
gvir_stream_io_condition_get_type;
} LIBVIRT_GOBJECT_0.1.5;
+LIBVIRT_GOBJECT_0.2.0 {
+ global:
+ gvir_domain_open_graphics_fd;
+} LIBVIRT_GOBJECT_0.1.9;
+
# define new API here using predicted next version number
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] Tests : Make nwfilter testcases robust against optional locking flags.
On Wed, Nov 26, 2014 at 04:11:16PM +0530, Prerna Saxena wrote: On Wednesday 26 November 2014 04:06 PM, Martin Kletzander wrote: On Wed, Nov 26, 2014 at 03:28:21PM +0530, Prerna Saxena wrote: Hi Martin, Thanks for the feedback. On Tuesday 25 November 2014 05:57 PM, Martin Kletzander wrote: On Tue, Nov 25, 2014 at 05:12:48PM +0530, Prerna Saxena wrote: Commit dc33e6e4a5a5d42 introduces iptables/ebtables to adding locking flags if/when these are available. However, the nwfilter testcases list outputs without taking into account whether locking flags have been passed. This shows up false testcase failures such as : 2) ebiptablesTearOldRules... Offset 1035 Expect [t nat -D PREROUTING -i vnet0 -j libvirt-I-vnet0 ebtables -t nat -D POSTROUTING -o vnet0 -j libvirt-O-vnet0 ebtables -t nat -L libvirt-I-vnet0 ebtables -t nat -L libvirt-O-vnet0 ...snip...] Actual [-concurrent -t nat -D PREROUTING -i vnet0 -j libvirt-I-vnet0 ebtables --concurrent -t nat -D POSTROUTING -o vnet0 -j libvirt-O-vnet0 ebtables --concurrent -t nat -L libvirt-I-vnet0 ebtables --concurrent -t nat -L libvirt-O-vnet0 ...snip...] This scrubs all reference to locking flags from test results buffer, so that achieved output matches the expected results. Instead of parsing and re-creating the string (which also doesn't check whether we use the locking flag properly), The function virtTestClearLockingArgs() merely replaces instances of 'ebtables --concurrent' with 'ebtables'. (likewise for iptables and ip6tables), if at all found. I didnt find the need for sanity checking in this approach :-) it would be way better if we could unify the result. Having said so, I agree with this. From the top of my head, we can either expose the virFirewallCheckUpdateLock() as non-static and mock it in tests to always set the lock flags to true or we can create new functions that will override setting of the flags. The problem is with expected results that are coded for these tests. On distros that support these flags, the issue would go away if the expected results take into account the locking flags. However, adding a permanent change to the expected args string would break older distros. Actually, no, I wanted to unconditionally add the parameters there only for tests. Looking at it more closely, this can fail only if you are building as root, is that correct? Yes, that is correct. I'm testing a patch with different approach, I'll Cc you on that when it is done. Let me know whether that works for you then. So I thought of tweaking the actual results. Approach #2: We could change the expected results to look somewhat like this : ebtables $FLAGS -t nat -D PREROUTING -i vnet0 -j libvirt-I-vnet0 And have a script that dynamically replaces $FLAGS with : * "--concurrent", if locking is supported at compile-time. * OR, with " ", if locking is not available. Ofcourse, not all tests have their expected results in a separate file. Some such as tests/nwfilterebiptablestest.c have their expected args in the form of char* encoded in the same program. This complicates this approach.. I am looking forward to community suggestions on how this can best be implemented. Will be happy to rework this patch if needed. Regards, -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: remove a redundant NULL assignment
On Wed, Nov 26, 2014 at 15:51:17 +0800, Chen Hanxiao wrote: > We already did this in virSecretDefFree. > > Signed-off-by: Chen Hanxiao > --- > src/storage/storage_backend.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c > index 98720f6..440f8b1 100644 > --- a/src/storage/storage_backend.c > +++ b/src/storage/storage_backend.c > @@ -558,7 +558,6 @@ virStorageGenerateQcowEncryption(virConnectPtr conn, > goto cleanup; > xml = virSecretDefFormat(def); > virSecretDefFree(def); > -def = NULL; > if (xml == NULL) > goto cleanup; > NACK, this patch could result in double-free. We do set def = NULL in virSecretDefFree() but that only touches def internally visible to this function. To make def from virStorageGenerateQcowEncryption be NULL after virSecretDefFree, we'd have to pass def by reference to it, i.e., to call virSecretDefFree(&def) and change virSecretDefFree to count with that. However, this is not a common practise so we shouldn't do it. Unless we change all *Free functions to do it this way. Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] Tests : Make nwfilter testcases robust against optional locking flags.
On Wednesday 26 November 2014 04:06 PM, Martin Kletzander wrote: > On Wed, Nov 26, 2014 at 03:28:21PM +0530, Prerna Saxena wrote: >> Hi Martin, >> Thanks for the feedback. >> >> On Tuesday 25 November 2014 05:57 PM, Martin Kletzander wrote: >>> On Tue, Nov 25, 2014 at 05:12:48PM +0530, Prerna Saxena wrote: Commit dc33e6e4a5a5d42 introduces iptables/ebtables to adding locking flags if/when these are available. However, the nwfilter testcases list outputs without taking into account whether locking flags have been passed. This shows up false testcase failures such as : 2) ebiptablesTearOldRules... Offset 1035 Expect [t nat -D PREROUTING -i vnet0 -j libvirt-I-vnet0 ebtables -t nat -D POSTROUTING -o vnet0 -j libvirt-O-vnet0 ebtables -t nat -L libvirt-I-vnet0 ebtables -t nat -L libvirt-O-vnet0 ...snip...] Actual [-concurrent -t nat -D PREROUTING -i vnet0 -j libvirt-I-vnet0 ebtables --concurrent -t nat -D POSTROUTING -o vnet0 -j libvirt-O-vnet0 ebtables --concurrent -t nat -L libvirt-I-vnet0 ebtables --concurrent -t nat -L libvirt-O-vnet0 ...snip...] This scrubs all reference to locking flags from test results buffer, so that achieved output matches the expected results. >>> Instead of parsing and re-creating the string (which also doesn't >>> check whether we use the locking flag properly), >> The function virtTestClearLockingArgs() merely replaces instances of >> 'ebtables --concurrent' with 'ebtables'. >> (likewise for iptables and ip6tables), if at all found. I didnt find the >> need for sanity checking in this approach :-) >>> it would be way >>> better if we could unify the result. >> Having said so, I agree with this. >> >>> From the top of my head, we can either expose the >>> virFirewallCheckUpdateLock() as non-static and mock it in tests to >>> always set the lock flags to true or we can create new functions that >>> will override setting of the flags. >>> >> The problem is with expected results that are coded for these tests. >> On distros that support these flags, the issue would go away if the expected >> results take into account the locking flags. However, adding a permanent >> change to the expected args string would break >> older distros. > Actually, no, I wanted to unconditionally add the parameters there > only for tests. > > Looking at it more closely, this can fail only if you are building as > root, is that correct? Yes, that is correct. >> So I thought of tweaking the actual results. >> >> Approach #2: >> We could change the expected results to look somewhat like this : >> ebtables $FLAGS -t nat -D PREROUTING -i vnet0 -j libvirt-I-vnet0 >> >> And have a script that dynamically replaces $FLAGS with : >> * "--concurrent", if locking is supported at compile-time. >> * OR, with " ", if locking is not available. >> >> Ofcourse, not all tests have their expected results in a separate file. Some >> such as >> tests/nwfilterebiptablestest.c have their expected args in the form of char* >> encoded in the same program. This complicates this approach.. >> >> I am looking forward to community suggestions on how this can best be >> implemented. Will be happy to rework this patch if needed. >> >> Regards, >> >> -- >> Prerna Saxena >> >> Linux Technology Centre, >> IBM Systems and Technology Lab, >> Bangalore, India >> -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] Tests : Make nwfilter testcases robust against optional locking flags.
On Wed, Nov 26, 2014 at 03:28:21PM +0530, Prerna Saxena wrote: Hi Martin, Thanks for the feedback. On Tuesday 25 November 2014 05:57 PM, Martin Kletzander wrote: On Tue, Nov 25, 2014 at 05:12:48PM +0530, Prerna Saxena wrote: Commit dc33e6e4a5a5d42 introduces iptables/ebtables to adding locking flags if/when these are available. However, the nwfilter testcases list outputs without taking into account whether locking flags have been passed. This shows up false testcase failures such as : 2) ebiptablesTearOldRules... Offset 1035 Expect [t nat -D PREROUTING -i vnet0 -j libvirt-I-vnet0 ebtables -t nat -D POSTROUTING -o vnet0 -j libvirt-O-vnet0 ebtables -t nat -L libvirt-I-vnet0 ebtables -t nat -L libvirt-O-vnet0 ...snip...] Actual [-concurrent -t nat -D PREROUTING -i vnet0 -j libvirt-I-vnet0 ebtables --concurrent -t nat -D POSTROUTING -o vnet0 -j libvirt-O-vnet0 ebtables --concurrent -t nat -L libvirt-I-vnet0 ebtables --concurrent -t nat -L libvirt-O-vnet0 ...snip...] This scrubs all reference to locking flags from test results buffer, so that achieved output matches the expected results. Instead of parsing and re-creating the string (which also doesn't check whether we use the locking flag properly), The function virtTestClearLockingArgs() merely replaces instances of 'ebtables --concurrent' with 'ebtables'. (likewise for iptables and ip6tables), if at all found. I didnt find the need for sanity checking in this approach :-) it would be way better if we could unify the result. Having said so, I agree with this. From the top of my head, we can either expose the virFirewallCheckUpdateLock() as non-static and mock it in tests to always set the lock flags to true or we can create new functions that will override setting of the flags. The problem is with expected results that are coded for these tests. On distros that support these flags, the issue would go away if the expected results take into account the locking flags. However, adding a permanent change to the expected args string would break older distros. Actually, no, I wanted to unconditionally add the parameters there only for tests. Looking at it more closely, this can fail only if you are building as root, is that correct? So I thought of tweaking the actual results. Approach #2: We could change the expected results to look somewhat like this : ebtables $FLAGS -t nat -D PREROUTING -i vnet0 -j libvirt-I-vnet0 And have a script that dynamically replaces $FLAGS with : * "--concurrent", if locking is supported at compile-time. * OR, with " ", if locking is not available. Ofcourse, not all tests have their expected results in a separate file. Some such as tests/nwfilterebiptablestest.c have their expected args in the form of char* encoded in the same program. This complicates this approach.. I am looking forward to community suggestions on how this can best be implemented. Will be happy to rework this patch if needed. Regards, -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC] storage: perform btrfs clone if possible
> -Original Message-
> From: Eric Blake [mailto:ebl...@redhat.com]
> Sent: Tuesday, November 25, 2014 11:59 PM
> To: Chen, Hanxiao/陈 晗霄; Martin Kletzander
> Cc: libvir-list@redhat.com
> Subject: Re: [libvirt] [PATCH RFC] storage: perform btrfs clone if possible
>
> On 11/25/2014 03:19 AM, Chen, Hanxiao wrote:
> >> I think it makes sense to expose this functionality; although I suspect
> >> it is better if we do so by having the user pass an explicit new flag
> >> value to existing API instead of doing it automatically.
> >>
> > Thanks for your clarification.
> >
> > But we've already had nocow in virStorageSource and tags.
> > So I think if we do not specify tags in XML,
> > we should try it according to 'nocow' in codes.
> >
> > Or do we need a new flags such as --reflink
> > for tools like virt-clone?
>
> It's a design trade-off - do we make the default operation efficient
> when possible (but with possible surprises due to overcommit of
> storage), or safe (but slow)? Regardless of the default, is it possible
> to explicitly request the opposite action? To me, it seems backwards
> compatible to make an operation faster by using underlying clone hooks
> as long as the end result is the same, and as long as a user NOT
> specifying a specific use of clone or avoidance of clone does not get an
> error message when clone is attempted but not supported. So at this
> point, I could go either way for the default as long as I can still have
> full control over the behavior, and we don't break existing users.
>
FYI, in my limited test,
the suggestion of this patch did not break existing users.
If not supported, it just print some debug logs
and try the original way then:
+if (!vol->target.nocow) {
+if (btrfs_clone_file(fd, inputfd) == -1) {
+if (errno == ENOTSUP)
+VIR_DEBUG("btrfs clone not supported, try another way.");
+} else {
+VIR_DEBUG("btrfs clone findished.");
+goto cleanup;
+}
+}
+
If I didn't miss something, I think this kind of operation
is acceptable.
Thanks,
- Chen
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: output error when try to hotplug unsupport console
On 11/26/2014 12:17 AM, Pavel Hrdina wrote:
On 11/17/2014 03:31 PM, Luyao Huang wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1164627
When try to use attach-device to hotplug a qemu
unsupport sonsole, command will return success,
and add a console in vm's xml.But this doesn't work
in qemu, qemu doesn't support these console.Add
a error output when try to hotplug a unsupport console
in qemuBuildConsoleChrDeviceStr.
Signed-off-by: Luyao Huang
---
src/qemu/qemu_command.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 1399ce4..2bf4a83 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -10069,13 +10069,17 @@ qemuBuildConsoleChrDeviceStr(char **deviceStr,
break;
case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL:
+break;
case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE:
case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN:
case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_UML:
case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LXC:
case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_OPENVZ:
case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LAST:
-break;
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unsupported console target type %s"),
+ NULLSTR(virDomainChrConsoleTargetTypeToString(chr->targetType)));
Wrong indentation
Thanks for pointing out
+return ret;
}
ret = 0;
Otherwise it seems good, bud what about the case that you will attach
unsupported device to offline domain? It is still possible but the
domain cannot be started afterwards. You should probably take care of
that too.
Eww, i thought that before, and i have a question about attach unsupported
device to a offline domain.
If we forbid attaching a unsupported device, is it means we should also
forbid
editing or defining a guest with a unsupported device?(because domain
cannot
start with these settings). I thinks it will be a big work. Actually, I
just don't
know when we try to set a invalid things to a offline domain, which way
we should
forbid and which way we can allow it.
Thanks advance for your answer.
Pavel
Thanks,
Luyao Huang
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] Tests : Make nwfilter testcases robust against optional locking flags.
Hi Martin, Thanks for the feedback. On Tuesday 25 November 2014 05:57 PM, Martin Kletzander wrote: > On Tue, Nov 25, 2014 at 05:12:48PM +0530, Prerna Saxena wrote: >> Commit dc33e6e4a5a5d42 introduces iptables/ebtables to adding locking >> flags if/when these are available. However, the nwfilter testcases >> list outputs without taking into account whether locking flags have been >> passed. >> >> This shows up false testcase failures such as : >> 2) ebiptablesTearOldRules... >> Offset 1035 >> Expect [t nat -D PREROUTING -i vnet0 -j libvirt-I-vnet0 >> ebtables -t nat -D POSTROUTING -o vnet0 -j libvirt-O-vnet0 >> ebtables -t nat -L libvirt-I-vnet0 >> ebtables -t nat -L libvirt-O-vnet0 >> ...snip...] >> Actual [-concurrent -t nat -D PREROUTING -i vnet0 -j libvirt-I-vnet0 >> ebtables --concurrent -t nat -D POSTROUTING -o vnet0 -j libvirt-O-vnet0 >> ebtables --concurrent -t nat -L libvirt-I-vnet0 >> ebtables --concurrent -t nat -L libvirt-O-vnet0 >> ...snip...] >> >> This scrubs all reference to locking flags from test results buffer, >> so that achieved output matches the expected results. >> > Instead of parsing and re-creating the string (which also doesn't > check whether we use the locking flag properly), The function virtTestClearLockingArgs() merely replaces instances of 'ebtables --concurrent' with 'ebtables'. (likewise for iptables and ip6tables), if at all found. I didnt find the need for sanity checking in this approach :-) > it would be way > better if we could unify the result. Having said so, I agree with this. > From the top of my head, we can either expose the > virFirewallCheckUpdateLock() as non-static and mock it in tests to > always set the lock flags to true or we can create new functions that > will override setting of the flags. > The problem is with expected results that are coded for these tests. On distros that support these flags, the issue would go away if the expected results take into account the locking flags. However, adding a permanent change to the expected args string would break older distros. So I thought of tweaking the actual results. Approach #2: We could change the expected results to look somewhat like this : ebtables $FLAGS -t nat -D PREROUTING -i vnet0 -j libvirt-I-vnet0 And have a script that dynamically replaces $FLAGS with : * "--concurrent", if locking is supported at compile-time. * OR, with " ", if locking is not available. Ofcourse, not all tests have their expected results in a separate file. Some such as tests/nwfilterebiptablestest.c have their expected args in the form of char* encoded in the same program. This complicates this approach.. I am looking forward to community suggestions on how this can best be implemented. Will be happy to rework this patch if needed. Regards, -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] ip link needs 'name' in 3.16 to create the veth pair
Am 26.11.2014 um 10:16 schrieb Cedric Bosdonnat: > On Wed, 2014-11-26 at 09:34 +0100, Richard Weinberger wrote: >> Am 26.11.2014 um 09:25 schrieb Cedric Bosdonnat: >>> Hi Martin, >>> >>> On Wed, 2014-11-26 at 05:51 +0100, Martin Kletzander wrote: > Instead of papering over the issue in libvirt better ship a non-broken > iproute2 > in openSUSE 13.2. > real fix: > https://git.kernel.org/cgit/linux/kernel/git/shemminger/iproute2.git/commit/?id=f1b66ff > Oh, thank you for finding that, I should've done my homework! Since it really is just a bug on iproute2 side in openSUSE, I'd rather keep it in its original state. And since the patch is already pushed, I'm inclining to reverting it. Other opinions? >>> >>> Quoting a colleague of mine working on the network stack: >>> >>> [this is] a regression in (upstream) iproute2 3.16, fixed in 3.17. >>> bnc#907093 has been created for it. (Factory is also affected but a >>> submit request is already on its way.) >>> >>> So I think we should keep that for those running the buggy 3.16. >> >> openSUSE has to fix their package and to serve a bugfix update, full stop. > > Thought that may not happen only to openSUSE... and that fix didn't harm > at all. Yes, but "fixing" this issue in libvirt is not correct. It needs to be fixed in the right place and iproute2 this is. libvirt is not the only user of this iproute2 feature. Thanks, //richard -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] ip link needs 'name' in 3.16 to create the veth pair
On Wed, 2014-11-26 at 09:34 +0100, Richard Weinberger wrote: > Am 26.11.2014 um 09:25 schrieb Cedric Bosdonnat: > > Hi Martin, > > > > On Wed, 2014-11-26 at 05:51 +0100, Martin Kletzander wrote: > >>> Instead of papering over the issue in libvirt better ship a non-broken > >>> iproute2 > >>> in openSUSE 13.2. > >>> real fix: > >>> https://git.kernel.org/cgit/linux/kernel/git/shemminger/iproute2.git/commit/?id=f1b66ff > >>> > >> > >> Oh, thank you for finding that, I should've done my homework! Since > >> it really is just a bug on iproute2 side in openSUSE, I'd rather keep > >> it in its original state. And since the patch is already pushed, I'm > >> inclining to reverting it. > >> > >> Other opinions? > > > > Quoting a colleague of mine working on the network stack: > > > > [this is] a regression in (upstream) iproute2 3.16, fixed in 3.17. > > bnc#907093 has been created for it. (Factory is also affected but a > > submit request is already on its way.) > > > > So I think we should keep that for those running the buggy 3.16. > > openSUSE has to fix their package and to serve a bugfix update, full stop. Thought that may not happen only to openSUSE... and that fix didn't harm at all. -- Cedric -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/5] Allow time sync on domain resume
On 06.11.2014 14:01, Michal Privoznik wrote: Diff to v1: -Fixed issues raised by Peter Michal Privoznik (5): Introduce virDomainResumeFlags API Implement virDomainResumeFlags in all drivers qemu: Split qemuDomainSetTime into two functions qemuDomainResumeFlags: Introduce VIR_DOMAIN_RESUME_SYNC_TIME virsh: Implement virDomainResumeFlags include/libvirt/libvirt-domain.h | 9 +++ src/driver-hypervisor.h | 5 ++ src/esx/esx_driver.c | 14 - src/hyperv/hyperv_driver.c | 14 - src/libvirt-domain.c | 50 src/libvirt_public.syms | 5 ++ src/libxl/libxl_driver.c | 14 - src/lxc/lxc_driver.c | 15 - src/openvz/openvz_driver.c | 13 - src/parallels/parallels_driver.c | 11 +++- src/phyp/phyp_driver.c | 12 +++- src/qemu/qemu_driver.c | 119 --- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 13 - src/remote_protocol-structs | 5 ++ src/test/test_driver.c | 13 - src/vbox/vbox_common.c | 11 +++- src/vmware/vmware_driver.c | 12 +++- src/xen/xen_driver.c | 14 - src/xenapi/xenapi_driver.c | 21 ++- tools/virsh-domain.c | 13 - tools/virsh.pod | 11 ++-- 22 files changed, 339 insertions(+), 56 deletions(-) Ping? -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [sandbox][PATCH 2/2] virt-sandbox-service: mount /var after all other file systems
Hi Martin, On Wed, 2014-11-26 at 05:32 +0100, Martin Kletzander wrote: > >+ > >+# /var contains the mounted image if there is an image: > should be the > >+# last thing to mount > >+self.add_bind_mount(source, "/var") > > You have the source set from a random value from self.dirs, ACK if you > use "%s/var" % self.dist instead of source. Nice catch! Pushed them both with that change. -- Cedric -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] ip link needs 'name' in 3.16 to create the veth pair
Am 26.11.2014 um 09:25 schrieb Cedric Bosdonnat: > Hi Martin, > > On Wed, 2014-11-26 at 05:51 +0100, Martin Kletzander wrote: >>> Instead of papering over the issue in libvirt better ship a non-broken >>> iproute2 >>> in openSUSE 13.2. >>> real fix: >>> https://git.kernel.org/cgit/linux/kernel/git/shemminger/iproute2.git/commit/?id=f1b66ff >>> >> >> Oh, thank you for finding that, I should've done my homework! Since >> it really is just a bug on iproute2 side in openSUSE, I'd rather keep >> it in its original state. And since the patch is already pushed, I'm >> inclining to reverting it. >> >> Other opinions? > > Quoting a colleague of mine working on the network stack: > > [this is] a regression in (upstream) iproute2 3.16, fixed in 3.17. > bnc#907093 has been created for it. (Factory is also affected but a > submit request is already on its way.) > > So I think we should keep that for those running the buggy 3.16. openSUSE has to fix their package and to serve a bugfix update, full stop. Thanks, //richard -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Revert "ip link needs 'name' in 3.16 to create the veth pair"
This reverts commit 433b427ff853ab72d32573d415e6ec569b77c7cb.
The patch was added in order to overcome a bug in iproute2 and since it
was properly identified as a bug, particularly in openSUSE 13.2, and it
is being worked on [1], the best solution for libvirt seems to be to
keep the old behaviour.
[1] https://bugzilla.novell.com/show_bug.cgi?id=907093
---
src/util/virnetdevveth.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c
index ad30e1d..e9d6f9c 100644
--- a/src/util/virnetdevveth.c
+++ b/src/util/virnetdevveth.c
@@ -89,7 +89,7 @@ static int virNetDevVethGetFreeNum(int startDev)
* @veth2: pointer to return name for container end of veth pair
*
* Creates a veth device pair using the ip command:
- * ip link add name veth1 type veth peer name veth2
+ * ip link add veth1 type veth peer name veth2
* If veth1 points to NULL on entry, it will be a valid interface on
* return. veth2 should point to NULL on entry.
*
@@ -146,7 +146,7 @@ int virNetDevVethCreate(char** veth1, char** veth2)
}
cmd = virCommandNew("ip");
-virCommandAddArgList(cmd, "link", "add", "name",
+virCommandAddArgList(cmd, "link", "add",
*veth1 ? *veth1 : veth1auto,
"type", "veth", "peer", "name",
*veth2 ? *veth2 : veth2auto,
--
2.1.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] ip link needs 'name' in 3.16 to create the veth pair
Hi Martin, On Wed, 2014-11-26 at 05:51 +0100, Martin Kletzander wrote: > >Instead of papering over the issue in libvirt better ship a non-broken > >iproute2 > >in openSUSE 13.2. > >real fix: > >https://git.kernel.org/cgit/linux/kernel/git/shemminger/iproute2.git/commit/?id=f1b66ff > > > > Oh, thank you for finding that, I should've done my homework! Since > it really is just a bug on iproute2 side in openSUSE, I'd rather keep > it in its original state. And since the patch is already pushed, I'm > inclining to reverting it. > > Other opinions? Quoting a colleague of mine working on the network stack: [this is] a regression in (upstream) iproute2 3.16, fixed in 3.17. bnc#907093 has been created for it. (Factory is also affected but a submit request is already on its way.) So I think we should keep that for those running the buggy 3.16. -- Cedric -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] ip link needs 'name' in 3.16 to create the veth pair
Am 26.11.2014 um 05:51 schrieb Martin Kletzander:
> On Tue, Nov 25, 2014 at 04:19:48PM +0100, Richard Weinberger wrote:
>> On Tue, Nov 25, 2014 at 9:21 AM, Cedric Bosdonnat
>> wrote:
>>> On Tue, 2014-11-25 at 08:42 +0100, Martin Kletzander wrote:
On Mon, Nov 24, 2014 at 09:54:44PM +0100, Cédric Bosdonnat wrote:
>Due to a change (or bug?) in ip link implementation, the command
>'ip link add vnet0...'
>is forced into
>'ip link add name vnet0...'
>The changed command also works on older versions of iproute2, just the
>'name' parameter has been made mandatory.
>---
> src/util/virnetdevveth.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c
>index e9d6f9c..ad30e1d 100644
>--- a/src/util/virnetdevveth.c
>+++ b/src/util/virnetdevveth.c
>@@ -89,7 +89,7 @@ static int virNetDevVethGetFreeNum(int startDev)
> * @veth2: pointer to return name for container end of veth pair
> *
> * Creates a veth device pair using the ip command:
>- * ip link add veth1 type veth peer name veth2
>+ * ip link add name veth1 type veth peer name veth2
> * If veth1 points to NULL on entry, it will be a valid interface on
> * return. veth2 should point to NULL on entry.
> *
>@@ -146,7 +146,7 @@ int virNetDevVethCreate(char** veth1, char** veth2)
> }
>
> cmd = virCommandNew("ip");
>-virCommandAddArgList(cmd, "link", "add",
>+virCommandAddArgList(cmd, "link", "add", "name",
> *veth1 ? *veth1 : veth1auto,
> "type", "veth", "peer", "name",
> *veth2 ? *veth2 : veth2auto,
>--
>2.1.2
>
I agree, the 'name' was always there, just optional. But what version
of iproute2 do you have that requires it? I checked the current HEAD
and it's still optional. This must be a bug in that particular
implementation.
ACK if you can argue with the version or platform this is required
on.
>>>
>>> At least the 3.16 shipped on openSUSE 13.2 has that problem... though I
>>> think it's just a side effect of another change in iproute2. It worked
>>> fine with version 3.12.
>>
>> Instead of papering over the issue in libvirt better ship a non-broken
>> iproute2
>> in openSUSE 13.2.
>> real fix:
>> https://git.kernel.org/cgit/linux/kernel/git/shemminger/iproute2.git/commit/?id=f1b66ff
>>
>
> Oh, thank you for finding that, I should've done my homework! Since
> it really is just a bug on iproute2 side in openSUSE, I'd rather keep
> it in its original state. And since the patch is already pushed, I'm
> inclining to reverting it.
Yes, please revert the libvirt change.
openSUSE is working on the issue:
https://bugzilla.novell.com/show_bug.cgi?id=907093
Thanks,
//richard
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
