Re: [libvirt] [PATCH 2/3] xenconfig: add support for openvswitch configuration
On 12/6/18 12:44 AM, Michal Privoznik wrote:
On 11/16/18 11:26 PM, Jim Fehlig wrote:
Add support for converting openvswitch interface configuration
to/from libvirt domXML and xl.cfg(5). The xl config syntax for
virtual interfaces is described in detail in the
xl-network-configuration(5) man page. The Xen Networking wiki
also contains information and examples for using openvswitch
in xl.cfg config format
https://wiki.xenproject.org/wiki/Xen_Networking#Open_vSwitch
Tests are added to check conversions of openvswitch tagged and
trunked VLAN configuration.
Signed-off-by: Jim Fehlig
---
src/xenconfig/xen_common.c| 113 +-
.../test-fullvirt-ovswitch-tagged.cfg | 25
.../test-fullvirt-ovswitch-tagged.xml | 50
.../test-fullvirt-ovswitch-trunked.cfg| 25
.../test-fullvirt-ovswitch-trunked.xml| 51
tests/xlconfigtest.c | 2 +
6 files changed, 262 insertions(+), 4 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index 0a9958711f..5390b933e0 100644
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -856,6 +856,84 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def,
const char *nativeFormat)
}
+static int
+xenParseVifBridge(virDomainNetDefPtr net, char *bridge)
+{
+char *vlanstr;
+unsigned int tag;
+
+/* 'bridge' string contains a bridge name and single vlan tag */
+vlanstr = strchr(bridge, '.');
+if (vlanstr) {
+if (VIR_STRNDUP(net->data.bridge.brname, bridge, vlanstr - bridge) < 0)
+return -1;
+
+vlanstr++;
+if (virStrToLong_ui(vlanstr, NULL, 10, &tag) < 0)
+return -1;
+
+if (VIR_ALLOC_N(net->vlan.tag, 1) < 0)
+return -1;
+
+net->vlan.tag[0] = tag;
+net->vlan.nTags = 1;
+
+if (VIR_ALLOC(net->virtPortProfile) < 0)
+return -1;
+
+net->virtPortProfile->virtPortType =
VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH;
+return 0;
+}
+
+/* 'bridge' string contains a bridge name and one or more vlan trunks */
+vlanstr = strchr(bridge, ':');
+if (vlanstr) {
+size_t i;
+size_t nvlans = 0;
+char **vlanstr_list = virStringSplit(bridge, ":", 0);
+
+if (!vlanstr_list)
+return -1;
+
+if (VIR_STRDUP(net->data.bridge.brname, vlanstr_list[0]) < 0) {
+virStringListFree(vlanstr_list);
+return -1;
+}
+
+for (i = 1; vlanstr_list[i]; i++)
+nvlans++;
+
+if (VIR_ALLOC_N(net->vlan.tag, nvlans) < 0) {
+virStringListFree(vlanstr_list);
+return -1;
+}
+
+for (i = 1; i <= nvlans; i++) {
+if (virStrToLong_ui(vlanstr_list[i], NULL, 10, &tag) < 0) {
+virStringListFree(vlanstr_list);
+return -1;
+}
+net->vlan.tag[i - 1] = tag;
+}
+net->vlan.nTags = nvlans;
+net->vlan.trunk = true;
+virStringListFree(vlanstr_list);
+
+if (VIR_ALLOC(net->virtPortProfile) < 0)
+return -1;
+
+net->virtPortProfile->virtPortType =
VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH;
+return 0;
+}
+
+/* 'bridge' string only contains the bridge name */
+if (VIR_STRDUP(net->data.bridge.brname, bridge) < 0)
+return -1;
+
+return 0;
Not a show stopper, I just wonder if we should perhaps use:
if ((vlanstr = strchr(bridge, '.'))) {
...
} else if ((vlanstr = strchr(bridge, ':'))) {
...
} else {
...
}
return 0;
To me that looks a bit cleaner, but I'll leave it up to you. Don't want
to be picky.
It definitely looks cleaner. I'll squash in the below diff before pushing.
Thanks a lot for reviewing the patches!
Regards,
Jim
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index 83eb28cdc9..60c8d7edc8 100644
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -862,9 +862,8 @@ xenParseVifBridge(virDomainNetDefPtr net, char *bridge)
char *vlanstr;
unsigned int tag;
-/* 'bridge' string contains a bridge name and single vlan tag */
-vlanstr = strchr(bridge, '.');
-if (vlanstr) {
+if ((vlanstr = strchr(bridge, '.'))) {
+/* 'bridge' string contains a bridge name and single vlan tag */
if (VIR_STRNDUP(net->data.bridge.brname, bridge, vlanstr - bridge) < 0)
return -1;
@@ -883,11 +882,8 @@ xenParseVifBridge(virDomainNetDefPtr net, char *bridge)
net->virtPortProfile->virtPortType =
VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH;
return 0;
-}
-
-/* 'bridge' string contains a bridge name and one or more vlan trunks */
-vlanstr = strchr(bridge, ':');
-if (vlanstr) {
+} else if ((vlanstr = strchr(bridge, ':'))) {
+/* 'bridge' string contains a bridge name and one or more vlan trunks
*/
size_t i;
size
Re: [libvirt] [PATCH 1/3] libxl: support openvswitch interfaces
On 12/6/18 12:44 AM, Michal Privoznik wrote:
On 11/16/18 11:26 PM, Jim Fehlig wrote:
It is currently possible to use s of type openvswitch
with the libxl driver in a non-standard way, e.g.
This patch adds support for openvswitch s specified
in typical libvirt config
VLAN tags and trunking are also supported using the extended
syntax for specifying an openvswitch bridge in libxl
BRIDGE_NAME[.VLAN][:TRUNK:TRUNK]
See Xen's networking wiki for more details on openvswitch support
https://wiki.xenproject.org/wiki/Xen_Networking#Open_vSwitch
Signed-off-by: Jim Fehlig
---
src/libxl/libxl_conf.c | 47 --
1 file changed, 45 insertions(+), 2 deletions(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index e2bfa2f5c3..39c969e482 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -51,6 +51,7 @@
#include "cpu/cpu.h"
#include "xen_common.h"
#include "xen_xl.h"
+#include "virnetdevvportprofile.h"
#define VIR_FROM_THIS VIR_FROM_LIBXL
@@ -1190,6 +1191,11 @@ libxlMakeNic(virDomainDefPtr def,
virNetworkPtr network = NULL;
virConnectPtr conn = NULL;
virNetDevBandwidthPtr actual_bw;
+virNetDevVPortProfilePtr port_profile;
+virNetDevVlanPtr virt_vlan;
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+size_t i;
+const char *script = NULL;
int ret = -1;
/* TODO: Where is mtu stored?
@@ -1247,14 +1253,50 @@ libxlMakeNic(virDomainDefPtr def,
if (VIR_STRDUP(x_nic->ifname, l_nic->ifname) < 0)
goto cleanup;
+port_profile = virDomainNetGetActualVirtPortProfile(l_nic);
+virt_vlan = virDomainNetGetActualVlan(l_nic);
+script = l_nic->script;
switch (actual_type) {
case VIR_DOMAIN_NET_TYPE_BRIDGE:
+virBufferAsprintf(&buf, "%s",
virDomainNetGetActualBridgeName(l_nic));
Or virBufferAddStr() or virBufferAdd(,,-1);
I recall liking virBufferAddStr() that last time it was recommended :-).
Regards,
Jim
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] version: Add ParseVersion and a Version struct
On 12/6/18 4:04 PM, John Ferlan wrote:
>
>
> On 10/11/18 7:58 PM, W. Trevor King wrote:
>> Make it easier to convert version integers to the more human-readable
>> major.minor.release format.
Oh, and I'll need you to "OK" me adding an Signed-off-by: or you need to
provide one. See:
https://libvirt.org/hacking.html
and point 6 of general tips:
Contributors to libvirt projects must assert that they are in compliance
with the Developer Certificate of Origin 1.1. This is achieved by adding
a "Signed-off-by" line containing the contributor's name and e-mail to
every commit message. The presence of this line attests that the
contributor has read the above lined DCO and agrees with its statements.
https://developercertificate.org/
John
>> ---
>> connect.go | 8
>> version.go | 52 ++
>> version_test.go | 64
>> +
>> 3 files changed, 124 insertions(+)
>> create mode 100644 version.go
>> create mode 100644 version_test.go
>>
>
> Well I'm far from the expert here, but since this has been "out there"
> for a while, I'll given it a go.
>
> [...]
>
>> diff --git a/version_test.go b/version_test.go
>
> [...]
>
>> +func TestParseVersion(t *testing.T) {
>> +for _, testCase := range []struct{
>> +inputuint32
>> +expected *Version
>> +}{
>> +{
>> +input: 3009000,
>> +expected: &Version{Major: 3, Minor: 9},
>
> NIT: Should the above have the "Release: 0" if nothing more than to
> prove your algorithm? I can add it or leave it as is.
>
> Reviewed-by: John Ferlan
>
> John
>
> [let me know either way on the above and I can push this]
>
> [...]
>
> --
> 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] [PATCH] version: Add ParseVersion and a Version struct
On 10/11/18 7:58 PM, W. Trevor King wrote:
> Make it easier to convert version integers to the more human-readable
> major.minor.release format.
> ---
> connect.go | 8
> version.go | 52 ++
> version_test.go | 64
> +
> 3 files changed, 124 insertions(+)
> create mode 100644 version.go
> create mode 100644 version_test.go
>
Well I'm far from the expert here, but since this has been "out there"
for a while, I'll given it a go.
[...]
> diff --git a/version_test.go b/version_test.go
[...]
> +func TestParseVersion(t *testing.T) {
> + for _, testCase := range []struct{
> + inputuint32
> + expected *Version
> + }{
> + {
> + input: 3009000,
> + expected: &Version{Major: 3, Minor: 9},
NIT: Should the above have the "Release: 0" if nothing more than to
prove your algorithm? I can add it or leave it as is.
Reviewed-by: John Ferlan
John
[let me know either way on the above and I can push this]
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: Save qemuDomainGetStats error
On 12/6/18 10:15 AM, Ján Tomko wrote:
> On Thu, Dec 06, 2018 at 02:49:59PM +0100, Ján Tomko wrote:
>> On Tue, Nov 27, 2018 at 11:23:22AM -0500, John Ferlan wrote:
>>> During qemuConnectGetAllDomainStats if qemuDomainGetStats causes
>>> a failure, then when collecting more than one domain's worth of
>>> statistics the loop in virDomainStatsRecordListFree would call
>>> virDomainFree which would call virResetLastError effectively wiping
>>> out the reason we failed leaving the caller with no idea why the
>>> collection failed.
>>>
>>> To fix this, let's save the error and restore it prior to return
>>> so that a caller such as 'virsh domstats' doesn't get the generic
>>> "error: An error occurred, but the cause is unknown".
>>>
>>> Signed-off-by: John Ferlan
>>> ---
>>> src/qemu/qemu_driver.c | 6 ++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index 7d9e17e72c..2fb8eef609 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -21092,6 +21092,7 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
>>> unsigned int flags)
>>> {
>>> virQEMUDriverPtr driver = conn->privateData;
>>> + virErrorPtr save_err = NULL;
>>
>> git grep virError src/qemu shows most files use orig_err as the variable
>> name
>>
OK - I'll modify... Of course the example I cut-n-paste'd from was
save_err - chances were slim, but possible.
>>> virDomainObjPtr *vms = NULL;
>>> virDomainObjPtr vm;
>>> size_t nvms;
>>> @@ -21160,6 +21161,7 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
>>> if (flags & VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING)
>>> domflags |= QEMU_DOMAIN_STATS_BACKING;
>>> if (qemuDomainGetStats(conn, vm, stats, &tmp, domflags) < 0) {
>>> + save_err = virSaveLastError();
>>
>> Since virDomainStatsRecordListFree is the one resetting the error,
>> I think we should only save it right before that call.
>>
>
>> This would cause a memory leak if qemuDomainGetStats would fail for more
>> than one domain.
>
> Ehm, disregard this sentence. (Thanks Erik for pointing that out)
> But I still consider the cleanup section a beter place for this.
>
It was a 50/50 coin flip for placement. I'm fine with and will move this
to just before the virDomainStatsRecordListFree. Perhaps the more common
model used throughout the code. I hedged my "bets" by putting it here
because I thought it was clearer as to why an error would be saved.
>>
>>> if (HAVE_JOB(domflags) && vm)
>>> qemuDomainObjEndJob(driver, vm);
>>>
>>> @@ -21184,6 +21186,10 @@ qemuConnectGetAllDomainStats(virConnectPtr
>>> conn,
>>> cleanup:
>>
>> Here. And we have a specific helper for that:
>> virErrorPreserveLast(&orig_err);
>>
We should be "more consistent" in the code regarding usage to avoid
future copypasta's, but going with Preserve and Restore is fine by me.
And by more consistent the underlying question/tone is - should the
virSaveLastError w/ paired virSetError/virFreeError all be replaced with
the Preserve/Restore... I see that could cause a lot of short term
churn, but perhaps the long term gain is worth it. The second question
then becomes all at one time or module my module...
Thanks for the review - the changes are made here at least.
John
>>> virDomainStatsRecordListFree(tmpstats);
>>> virObjectListFreeCount(vms, nvms);
>>> + if (save_err) {
>>> + virSetError(save_err);
>>> + virFreeError(save_err);
>>> + }
>>
>> virErrorRestore(&orig_err);
>>
>> With that addressed:
>> Reviewed-by: Ján Tomko
>>
>> Jano
>
>
>
>> --
>> 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
[libvirt] [PATCH] domain: conf: graphics: Fix picking DRI renderer automatically for SPICE
Commit 255e0732 introduced a few graphics-related helpers. The problem
is that virDomainGraphicsNeedsAutoRenderNode returns true if it gets
NULL as a response from virDomainGraphicsNeedsAutoRenderNode. That's
okay for egl-headless because that one always needs a DRM render node,
the same is not true for SPICE though, and unless the XML specifies
for SPICE, there's no need for any renderer.
https://bugzilla.redhat.com/show_bug.cgi?id=1656895
Signed-off-by: Erik Skultety
---
src/conf/domain_conf.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b70dca6c61..efa0a94f39 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -30982,8 +30982,7 @@ virDomainGraphicsGetRenderNode(const
virDomainGraphicsDef *graphics)
switch (graphics->type) {
case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
-if (graphics->data.spice.gl == VIR_TRISTATE_BOOL_YES)
-ret = graphics->data.spice.rendernode;
+ret = graphics->data.spice.rendernode;
break;
case VIR_DOMAIN_GRAPHICS_TYPE_EGL_HEADLESS:
ret = graphics->data.egl_headless.rendernode;
@@ -31006,6 +31005,10 @@ virDomainGraphicsNeedsAutoRenderNode(const
virDomainGraphicsDef *graphics)
if (!virDomainGraphicsSupportsRenderNode(graphics))
return false;
+if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
+graphics->data.spice.gl != VIR_TRISTATE_BOOL_YES)
+return false;
+
if (virDomainGraphicsGetRenderNode(graphics))
return false;
--
2.19.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 0/2] Fix 2 small LXC network interface bugs
danpb suggested that I shouldn't completely remove the validation of interface type I had removed in V1: https://www.redhat.com/archives/libvir-list/2018-December/msg00120.html This also re-orders the two patches, so the "check actual type" patch loses its previous ACK due to modifications. Laine Stump (2): lxc: check actual type of interface not config type lxc: don't forbid src/lxc/lxc_controller.c | 23 +++ 1 file changed, 19 insertions(+), 4 deletions(-) -- 2.19.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 2/2] lxc: don't forbid
Commit 017dfa27d changed a few switch statements in the LXC code to
have all possible enum values, and in the process changed the switch
statement in virLXCControllerGetNICIndexes() to return an error status
for unsupported interface types, but it erroneously put type='direct'
on the list of unsupported types.
type='direct' (implemented with a macvlan interface) is supported on
LXC, but it's interface shouldn't be placed on the list of interfaces
given to CreateMachineWithNetwork() because the interface is put
inside the container, while CreateMachineWithNetwork() only wants to
know about the parent veths of veth pairs (the parent veth remains on
the host side, while the child veth is put into the container).
Resolves: https://bugzilla.redhat.com/1656463
Signed-off-by: Laine Stump
---
src/lxc/lxc_controller.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 07342cbc77..cff004a034 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -364,6 +364,16 @@ static int
virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
size_t i;
int ret = -1;
+/* Gather the ifindexes of the "parent" veths for all interfaces
+ * implemented with a veth pair. These will be used when calling
+ * virCgroupNewMachine (and eventually the dbus method
+ * CreateMachineWithNetwork). ifindexes for the child veths, and
+ * for macvlan interfaces, *should not* be in this list, as they
+ * will be moved into the container. Only the interfaces that will
+ * remain outside the container, but are used for communication
+ * with the container, should be added to the list.
+ */
+
VIR_DEBUG("Getting nic indexes");
for (i = 0; i < ctrl->def->nnets; i++) {
int nicindex = -1;
@@ -388,6 +398,9 @@ static int
virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
ctrl->nicindexes[ctrl->nnicindexes-1] = nicindex;
break;
+case VIR_DOMAIN_NET_TYPE_DIRECT:
+ break;
+
case VIR_DOMAIN_NET_TYPE_USER:
case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
case VIR_DOMAIN_NET_TYPE_SERVER:
@@ -395,7 +408,6 @@ static int
virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
case VIR_DOMAIN_NET_TYPE_MCAST:
case VIR_DOMAIN_NET_TYPE_UDP:
case VIR_DOMAIN_NET_TYPE_INTERNAL:
-case VIR_DOMAIN_NET_TYPE_DIRECT:
case VIR_DOMAIN_NET_TYPE_HOSTDEV:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unsupported net type %s"),
--
2.19.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 1/2] lxc: check actual type of interface not config type
virLXCControllerGetNICIndexes() was deciding whether or not to add the
ifindex for an interface's ifname to the list of ifindexes sent to
CreateMachineWithNetwork based on the interface type stored in the
config. This would be incorrect in the case of where the network was giving out macvlan interfaces
tied to a physical device (i.e. when the actual interface type was
"direct").
Instead of checking the setting of "net->type", we should be checking
the setting of virDomainNetGetActualType(net).
I don't think this caused any actual misbehavior, it was just
technically wrong.
Signed-off-by: Laine Stump
---
src/lxc/lxc_controller.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index e853d02d65..07342cbc77 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -367,7 +367,10 @@ static int
virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
VIR_DEBUG("Getting nic indexes");
for (i = 0; i < ctrl->def->nnets; i++) {
int nicindex = -1;
-switch (ctrl->def->nets[i]->type) {
+virDomainNetType actualType
+ = virDomainNetGetActualType(ctrl->def->nets[i]);
+
+switch (actualType) {
case VIR_DOMAIN_NET_TYPE_BRIDGE:
case VIR_DOMAIN_NET_TYPE_NETWORK:
case VIR_DOMAIN_NET_TYPE_ETHERNET:
@@ -396,11 +399,11 @@ static int
virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
case VIR_DOMAIN_NET_TYPE_HOSTDEV:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unsupported net type %s"),
- virDomainNetTypeToString(ctrl->def->nets[i]->type));
+ virDomainNetTypeToString(actualType));
goto cleanup;
case VIR_DOMAIN_NET_TYPE_LAST:
default:
-virReportEnumRangeError(virDomainNetType,
ctrl->def->nets[i]->type);
+virReportEnumRangeError(virDomainNetType, actualType);
goto cleanup;
}
}
--
2.19.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 03/18] security: Include security_util
On Thu, Dec 06, 2018 at 04:12:45PM +0100, Michal Privoznik wrote:
> On 12/6/18 3:34 PM, Daniel P. Berrangé wrote:
> > On Thu, Dec 06, 2018 at 03:17:47PM +0100, Michal Privoznik wrote:
> >> On 12/6/18 12:38 PM, Daniel P. Berrangé wrote:
> >>> On Thu, Nov 29, 2018 at 02:52:18PM +0100, Michal Privoznik wrote:
> This file implements wrappers over XATTR getter/setter. It
> ensures the proper XATTR namespace is used.
>
> Signed-off-by: Michal Privoznik
> ---
> src/security/Makefile.inc.am | 2 +
> src/security/security_util.c | 226 +++
> src/security/security_util.h | 32 +
> 3 files changed, 260 insertions(+)
> create mode 100644 src/security/security_util.c
> create mode 100644 src/security/security_util.h
> >
> +int
> +virSecuritySetRememberedLabel(const char *name,
> + const char *path,
> + const char *label)
> +{
> +char *ref_name = NULL;
> +char *attr_name = NULL;
> +char *value = NULL;
> +unsigned int refcount = 0;
> +int ret = -1;
> +
> +if (!(ref_name = virSecurityGetRefCountAttrName(name)))
> +goto cleanup;
> +
> +if (virFileGetXAtrr(path, ref_name, &value) < 0) {
> +if (errno == ENOSYS || errno == ENOTSUP) {
> +ret = 0;
> +goto cleanup;
> +} else if (errno != ENODATA) {
> +virReportSystemError(errno,
> + _("Unable to get XATTR %s on %s"),
> + ref_name,
> + path);
> +goto cleanup;
> +}
> +}
> +
> +if (value &&
> +virStrToLong_ui(value, NULL, 10, &refcount) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("malformed refcount %s on %s"),
> + value, path);
> +goto cleanup;
> +}
> +
> +VIR_FREE(value);
> +
> +refcount++;
> +
> +if (refcount == 1) {
> +if (!(attr_name = virSecurityGetAttrName(name)))
> +goto cleanup;
> +
> +if (virFileSetXAtrr(path, attr_name, label) < 0)
> +goto cleanup;
> +}
> >>>
> >>> Do we need to have a
> >>>
> >>> } else {
> >>> check the currently remember label matches
> >>> what was passed into this method ?
> >>> }
> >>>
> >>> if not, can you add API docs for this method explainng the
> >>> intended semantics when label is already remembered.
> >>
> >> I don't think that's a good idea because that sets us off onto a path
> >> where we'd have to determine whether a file is accessible or not. I
> >> mean, if the current owner is UID_A:GID_A (and qemu has access through
> >> both) and this new label passed into here is UID_B:GID_B that doesn't
> >> necessarily mean that qemu loses the access (UID_A can be a member of
> >> GID_B).
> >
> > I wasn't actually suggesting checking accessibility.
> >
> > IMHO if you are going to have 2 guests both accessing the same file,
> > we should simply declare that they *MUST* both use the same label.
> >
> > Simply reject the idea that the 2nd guest can change the label, using
> > a GID_B that magically still allows guest A to access it. Such scenarios
> > are way more likely to be an admin screw up than an intentional decision.
> >
> > As an admin - the guest A might set svirt_image_t:s0:c12,c35. The guest B
> > then comes along and sets 'svirt_image_t:s0' which grants access to
> > all guests. This is a clearly a mistake because the first guests' label
> > was an exclusive access label, while the second guests label was a shared
> > access label. The lock manager should have blocked this, but I still
> > think we should also block it here, as the lock manager won't block it
> > until after you've already set the labels.
> >
> > IOW we're justified in requiring strict equality of labels for all
> > guests, even if they technically might prevent something the kernel would
> > otherwise allow.
>
> Okay then, but this is not the correct place for that check then. In the
> XATTRs there is stored the original owner, not the current one. If domA
> is already running, then domB doesn't need to check if its seclabel ==
> original owner rather than if its seclabel == current seclabel.
>
> What I can do is to have virSecuritySetRememberedLabel() return the
> updated value of the ref counter and if it is greater than 1 require the
> seclabel to be the one that @path currently has (in the caller). I mean,
> these virSecurity{Get,Set}RememberedLabel APIs really treat seclabels as
> an opaque data. I rather not have them understand seclabels.
Yes, that makes sense to me.
Regards,
Daniel
--
|: https
Re: [libvirt] [PATCH 1/2] qemu: Save qemuDomainGetStats error
On Thu, Dec 06, 2018 at 02:49:59PM +0100, Ján Tomko wrote:
On Tue, Nov 27, 2018 at 11:23:22AM -0500, John Ferlan wrote:
During qemuConnectGetAllDomainStats if qemuDomainGetStats causes
a failure, then when collecting more than one domain's worth of
statistics the loop in virDomainStatsRecordListFree would call
virDomainFree which would call virResetLastError effectively wiping
out the reason we failed leaving the caller with no idea why the
collection failed.
To fix this, let's save the error and restore it prior to return
so that a caller such as 'virsh domstats' doesn't get the generic
"error: An error occurred, but the cause is unknown".
Signed-off-by: John Ferlan
---
src/qemu/qemu_driver.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7d9e17e72c..2fb8eef609 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -21092,6 +21092,7 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
unsigned int flags)
{
virQEMUDriverPtr driver = conn->privateData;
+virErrorPtr save_err = NULL;
git grep virError src/qemu shows most files use orig_err as the variable
name
virDomainObjPtr *vms = NULL;
virDomainObjPtr vm;
size_t nvms;
@@ -21160,6 +21161,7 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
if (flags & VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING)
domflags |= QEMU_DOMAIN_STATS_BACKING;
if (qemuDomainGetStats(conn, vm, stats, &tmp, domflags) < 0) {
+save_err = virSaveLastError();
Since virDomainStatsRecordListFree is the one resetting the error,
I think we should only save it right before that call.
This would cause a memory leak if qemuDomainGetStats would fail for more
than one domain.
Ehm, disregard this sentence. (Thanks Erik for pointing that out)
But I still consider the cleanup section a beter place for this.
Jano
if (HAVE_JOB(domflags) && vm)
qemuDomainObjEndJob(driver, vm);
@@ -21184,6 +21186,10 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
cleanup:
Here. And we have a specific helper for that:
virErrorPreserveLast(&orig_err);
virDomainStatsRecordListFree(tmpstats);
virObjectListFreeCount(vms, nvms);
+if (save_err) {
+virSetError(save_err);
+virFreeError(save_err);
+}
virErrorRestore(&orig_err);
With that addressed:
Reviewed-by: Ján Tomko
Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 03/18] security: Include security_util
On 12/6/18 3:34 PM, Daniel P. Berrangé wrote:
> On Thu, Dec 06, 2018 at 03:17:47PM +0100, Michal Privoznik wrote:
>> On 12/6/18 12:38 PM, Daniel P. Berrangé wrote:
>>> On Thu, Nov 29, 2018 at 02:52:18PM +0100, Michal Privoznik wrote:
This file implements wrappers over XATTR getter/setter. It
ensures the proper XATTR namespace is used.
Signed-off-by: Michal Privoznik
---
src/security/Makefile.inc.am | 2 +
src/security/security_util.c | 226 +++
src/security/security_util.h | 32 +
3 files changed, 260 insertions(+)
create mode 100644 src/security/security_util.c
create mode 100644 src/security/security_util.h
>
+int
+virSecuritySetRememberedLabel(const char *name,
+ const char *path,
+ const char *label)
+{
+char *ref_name = NULL;
+char *attr_name = NULL;
+char *value = NULL;
+unsigned int refcount = 0;
+int ret = -1;
+
+if (!(ref_name = virSecurityGetRefCountAttrName(name)))
+goto cleanup;
+
+if (virFileGetXAtrr(path, ref_name, &value) < 0) {
+if (errno == ENOSYS || errno == ENOTSUP) {
+ret = 0;
+goto cleanup;
+} else if (errno != ENODATA) {
+virReportSystemError(errno,
+ _("Unable to get XATTR %s on %s"),
+ ref_name,
+ path);
+goto cleanup;
+}
+}
+
+if (value &&
+virStrToLong_ui(value, NULL, 10, &refcount) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("malformed refcount %s on %s"),
+ value, path);
+goto cleanup;
+}
+
+VIR_FREE(value);
+
+refcount++;
+
+if (refcount == 1) {
+if (!(attr_name = virSecurityGetAttrName(name)))
+goto cleanup;
+
+if (virFileSetXAtrr(path, attr_name, label) < 0)
+goto cleanup;
+}
>>>
>>> Do we need to have a
>>>
>>> } else {
>>> check the currently remember label matches
>>> what was passed into this method ?
>>> }
>>>
>>> if not, can you add API docs for this method explainng the
>>> intended semantics when label is already remembered.
>>
>> I don't think that's a good idea because that sets us off onto a path
>> where we'd have to determine whether a file is accessible or not. I
>> mean, if the current owner is UID_A:GID_A (and qemu has access through
>> both) and this new label passed into here is UID_B:GID_B that doesn't
>> necessarily mean that qemu loses the access (UID_A can be a member of
>> GID_B).
>
> I wasn't actually suggesting checking accessibility.
>
> IMHO if you are going to have 2 guests both accessing the same file,
> we should simply declare that they *MUST* both use the same label.
>
> Simply reject the idea that the 2nd guest can change the label, using
> a GID_B that magically still allows guest A to access it. Such scenarios
> are way more likely to be an admin screw up than an intentional decision.
>
> As an admin - the guest A might set svirt_image_t:s0:c12,c35. The guest B
> then comes along and sets 'svirt_image_t:s0' which grants access to
> all guests. This is a clearly a mistake because the first guests' label
> was an exclusive access label, while the second guests label was a shared
> access label. The lock manager should have blocked this, but I still
> think we should also block it here, as the lock manager won't block it
> until after you've already set the labels.
>
> IOW we're justified in requiring strict equality of labels for all
> guests, even if they technically might prevent something the kernel would
> otherwise allow.
Okay then, but this is not the correct place for that check then. In the
XATTRs there is stored the original owner, not the current one. If domA
is already running, then domB doesn't need to check if its seclabel ==
original owner rather than if its seclabel == current seclabel.
What I can do is to have virSecuritySetRememberedLabel() return the
updated value of the ref counter and if it is greater than 1 require the
seclabel to be the one that @path currently has (in the caller). I mean,
these virSecurity{Get,Set}RememberedLabel APIs really treat seclabels as
an opaque data. I rather not have them understand seclabels.
>
>> Sure, this doesn't prevent us from the following scenario:
>>
>> 1) set a seclabel for domA on path X
>> 2) user tries to start domB with a different seclabel which also
>> requires path X
>> 3) while starting domB, libvirt overwrites seclabel on X effectively
>> cutting of domA
>> 4) imagine domB can't be started (or is destroyed later
Re: [libvirt] [PATCH 1/2] lxc: stop incorrectly validating interface type
On Thu, Dec 06, 2018 at 09:37:09AM -0500, Laine Stump wrote:
> On 12/6/18 4:50 AM, Daniel P. Berrangé wrote:
> > On Wed, Dec 05, 2018 at 09:35:12PM -0500, Laine Stump wrote:
> >> Commit 017dfa27d changed a few switch statements in the LXC code to
> >> have all possible enum values, and in the process changed the switch
> >> statement in virLXCControllerGetNICIndexes() such that it returned
> >> error status for any interfaces that weren't implemented with a veth
> >> pair when it should have just been ignoring those interfaces.
> >>
> >> Since the interface type will have already been validated before
> >> reaching this function, we shouldn't be doing any validation at all -
> >> just add the ifindex of the parent veth if its a veth pair, and ignore
> >> it otherwise.
> >>
> >> Resolves: https://bugzilla.redhat.com/1656463
> >> Signed-off-by: Laine Stump
> >> ---
> >> src/lxc/lxc_controller.c | 17 +++--
> >> 1 file changed, 11 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> >> index e853d02d65..1c20f451af 100644
> >> --- a/src/lxc/lxc_controller.c
> >> +++ b/src/lxc/lxc_controller.c
> >> @@ -364,6 +364,16 @@ static int
> >> virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
> >> size_t i;
> >> int ret = -1;
> >>
> >> +/* Gather the ifindexes of the "parent" veths for all interfaces
> >> + * implemented with a veth pair. These will be used when calling
> >> + * virCgroupNewMachine (and eventually the dbus method
> >> + * CreateMachineWithNetwork). ifindexes for the child veths, and
> >> + * for macvlan interfaces, *should not* be in this list, as they
> >> + * will be moved into the container. Only the interfaces that will
> >> + * remain outside the container, but are used for communication
> >> + * with the container, should be added to the list.
> >> + */
> >> +
> >> VIR_DEBUG("Getting nic indexes");
> >> for (i = 0; i < ctrl->def->nnets; i++) {
> >> int nicindex = -1;
> >> @@ -394,14 +404,9 @@ static int
> >> virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
> >> case VIR_DOMAIN_NET_TYPE_INTERNAL:
> >> case VIR_DOMAIN_NET_TYPE_DIRECT:
> >> case VIR_DOMAIN_NET_TYPE_HOSTDEV:
> >> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> >> - _("Unsupported net type %s"),
> >> -
> >> virDomainNetTypeToString(ctrl->def->nets[i]->type));
> >> -goto cleanup;
> >> case VIR_DOMAIN_NET_TYPE_LAST:
> >> default:
> >> -virReportEnumRangeError(virDomainNetType,
> >> ctrl->def->nets[i]->type);
> >> -goto cleanup;
> >> + break;
> >> }
> > This is removing too much. Only ethernet, bridge, network & direct are going
> > to work with LXC. All others should always result in an error message here,
> > and absolutely the LAST/default case must always report enum error.
>
>
> I had originally done what you suggest, then went back and changed it in
> order to not conflict with my commit comment saying that validation
> shouldn't even be done in this function, since the value had already
> been validated and doing it again would just be adding bulk to the code
> for no reason.
My preference is always to be explicit with the validation in a switch()
unless something earlier *in the same function scope* has already done
validation. IOW don't rely on the caller having previously called something
else to do validation, as that's fragile when code is refactored.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] lxc: stop incorrectly validating interface type
On 12/6/18 4:50 AM, Daniel P. Berrangé wrote:
> On Wed, Dec 05, 2018 at 09:35:12PM -0500, Laine Stump wrote:
>> Commit 017dfa27d changed a few switch statements in the LXC code to
>> have all possible enum values, and in the process changed the switch
>> statement in virLXCControllerGetNICIndexes() such that it returned
>> error status for any interfaces that weren't implemented with a veth
>> pair when it should have just been ignoring those interfaces.
>>
>> Since the interface type will have already been validated before
>> reaching this function, we shouldn't be doing any validation at all -
>> just add the ifindex of the parent veth if its a veth pair, and ignore
>> it otherwise.
>>
>> Resolves: https://bugzilla.redhat.com/1656463
>> Signed-off-by: Laine Stump
>> ---
>> src/lxc/lxc_controller.c | 17 +++--
>> 1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
>> index e853d02d65..1c20f451af 100644
>> --- a/src/lxc/lxc_controller.c
>> +++ b/src/lxc/lxc_controller.c
>> @@ -364,6 +364,16 @@ static int
>> virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
>> size_t i;
>> int ret = -1;
>>
>> +/* Gather the ifindexes of the "parent" veths for all interfaces
>> + * implemented with a veth pair. These will be used when calling
>> + * virCgroupNewMachine (and eventually the dbus method
>> + * CreateMachineWithNetwork). ifindexes for the child veths, and
>> + * for macvlan interfaces, *should not* be in this list, as they
>> + * will be moved into the container. Only the interfaces that will
>> + * remain outside the container, but are used for communication
>> + * with the container, should be added to the list.
>> + */
>> +
>> VIR_DEBUG("Getting nic indexes");
>> for (i = 0; i < ctrl->def->nnets; i++) {
>> int nicindex = -1;
>> @@ -394,14 +404,9 @@ static int
>> virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
>> case VIR_DOMAIN_NET_TYPE_INTERNAL:
>> case VIR_DOMAIN_NET_TYPE_DIRECT:
>> case VIR_DOMAIN_NET_TYPE_HOSTDEV:
>> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> - _("Unsupported net type %s"),
>> -
>> virDomainNetTypeToString(ctrl->def->nets[i]->type));
>> -goto cleanup;
>> case VIR_DOMAIN_NET_TYPE_LAST:
>> default:
>> -virReportEnumRangeError(virDomainNetType,
>> ctrl->def->nets[i]->type);
>> -goto cleanup;
>> + break;
>> }
> This is removing too much. Only ethernet, bridge, network & direct are going
> to work with LXC. All others should always result in an error message here,
> and absolutely the LAST/default case must always report enum error.
I had originally done what you suggest, then went back and changed it in
order to not conflict with my commit comment saying that validation
shouldn't even be done in this function, since the value had already
been validated and doing it again would just be adding bulk to the code
for no reason.
However, now I see that my belief that the interface configs had been
validated is a bit "off". The config might have been validated by
libvirt, but lxc_controller.c code runs in a different process, so
"outside forces" could have changed things during the transition. And my
assumption of prior validation had been reinforced by seeing that the
function called directly before virLXCControllerGetNICIndexes() was
literally called virLXCControllerValidateNICs(), but when I actually
look at that function I see that the only thing it validates is that the
number of interfaces on the commandline matches the number of interfaces
in the config, which doesn't do us much good.
So okay, I'll change the patch to just make TYPE_DIRECT a NOP and leave
in the validation. (somebody with more drive an ambition might instead
expand *ValidateNICs to do a more complete job.)
pEpkey.asc
Description: application/pgp-keys
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 03/18] security: Include security_util
On Thu, Dec 06, 2018 at 03:17:47PM +0100, Michal Privoznik wrote:
> On 12/6/18 12:38 PM, Daniel P. Berrangé wrote:
> > On Thu, Nov 29, 2018 at 02:52:18PM +0100, Michal Privoznik wrote:
> >> This file implements wrappers over XATTR getter/setter. It
> >> ensures the proper XATTR namespace is used.
> >>
> >> Signed-off-by: Michal Privoznik
> >> ---
> >> src/security/Makefile.inc.am | 2 +
> >> src/security/security_util.c | 226 +++
> >> src/security/security_util.h | 32 +
> >> 3 files changed, 260 insertions(+)
> >> create mode 100644 src/security/security_util.c
> >> create mode 100644 src/security/security_util.h
> >> +int
> >> +virSecuritySetRememberedLabel(const char *name,
> >> + const char *path,
> >> + const char *label)
> >> +{
> >> +char *ref_name = NULL;
> >> +char *attr_name = NULL;
> >> +char *value = NULL;
> >> +unsigned int refcount = 0;
> >> +int ret = -1;
> >> +
> >> +if (!(ref_name = virSecurityGetRefCountAttrName(name)))
> >> +goto cleanup;
> >> +
> >> +if (virFileGetXAtrr(path, ref_name, &value) < 0) {
> >> +if (errno == ENOSYS || errno == ENOTSUP) {
> >> +ret = 0;
> >> +goto cleanup;
> >> +} else if (errno != ENODATA) {
> >> +virReportSystemError(errno,
> >> + _("Unable to get XATTR %s on %s"),
> >> + ref_name,
> >> + path);
> >> +goto cleanup;
> >> +}
> >> +}
> >> +
> >> +if (value &&
> >> +virStrToLong_ui(value, NULL, 10, &refcount) < 0) {
> >> +virReportError(VIR_ERR_INTERNAL_ERROR,
> >> + _("malformed refcount %s on %s"),
> >> + value, path);
> >> +goto cleanup;
> >> +}
> >> +
> >> +VIR_FREE(value);
> >> +
> >> +refcount++;
> >> +
> >> +if (refcount == 1) {
> >> +if (!(attr_name = virSecurityGetAttrName(name)))
> >> +goto cleanup;
> >> +
> >> +if (virFileSetXAtrr(path, attr_name, label) < 0)
> >> +goto cleanup;
> >> +}
> >
> > Do we need to have a
> >
> > } else {
> > check the currently remember label matches
> > what was passed into this method ?
> > }
> >
> > if not, can you add API docs for this method explainng the
> > intended semantics when label is already remembered.
>
> I don't think that's a good idea because that sets us off onto a path
> where we'd have to determine whether a file is accessible or not. I
> mean, if the current owner is UID_A:GID_A (and qemu has access through
> both) and this new label passed into here is UID_B:GID_B that doesn't
> necessarily mean that qemu loses the access (UID_A can be a member of
> GID_B).
I wasn't actually suggesting checking accessibility.
IMHO if you are going to have 2 guests both accessing the same file,
we should simply declare that they *MUST* both use the same label.
Simply reject the idea that the 2nd guest can change the label, using
a GID_B that magically still allows guest A to access it. Such scenarios
are way more likely to be an admin screw up than an intentional decision.
As an admin - the guest A might set svirt_image_t:s0:c12,c35. The guest B
then comes along and sets 'svirt_image_t:s0' which grants access to
all guests. This is a clearly a mistake because the first guests' label
was an exclusive access label, while the second guests label was a shared
access label. The lock manager should have blocked this, but I still
think we should also block it here, as the lock manager won't block it
until after you've already set the labels.
IOW we're justified in requiring strict equality of labels for all
guests, even if they technically might prevent something the kernel would
otherwise allow.
> Sure, this doesn't prevent us from the following scenario:
>
> 1) set a seclabel for domA on path X
> 2) user tries to start domB with a different seclabel which also
> requires path X
> 3) while starting domB, libvirt overwrites seclabel on X effectively
> cutting of domA
> 4) imagine domB can't be started (or is destroyed later or whatever),
> libvirt starts restore, but since X's refcount = 1, the original owner
> is not restored (domA is still cut off)
>
> But I don't see any easy solution to that. I mean:
> a) As I say we don't want to check in 3) if the new seclabel would cut
> off domA. That's for kernel to decide.
Simply don't try to decide that. Require identical labels for all
guests sharing the disk.
> b) If we would want to rollback in 4) and set domA seclabel on X we
> would need a very clever algorithm. We would need to store an array of
> seclabels in XATTR and match them with domains that are doing the
> restore. For instance: there are three domains domA, domB and domC and
> assume they all share path X and have their own seclabels
Re: [libvirt] [PATCH v2 03/18] security: Include security_util
On 12/6/18 12:38 PM, Daniel P. Berrangé wrote:
> On Thu, Nov 29, 2018 at 02:52:18PM +0100, Michal Privoznik wrote:
>> This file implements wrappers over XATTR getter/setter. It
>> ensures the proper XATTR namespace is used.
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> src/security/Makefile.inc.am | 2 +
>> src/security/security_util.c | 226 +++
>> src/security/security_util.h | 32 +
>> 3 files changed, 260 insertions(+)
>> create mode 100644 src/security/security_util.c
>> create mode 100644 src/security/security_util.h
>>
>
>
>> +/**
>> + * virSecurityGetRememberedLabel:
>> + * @name: security driver name
>> + * @path: file name
>> + * @label: label
>> + *
>> + * For given @path and security driver (@name) fetch remembered
>> + * @label. The caller must not restore label if an error is
>> + * indicated or if @label is NULL upon return.
>> + *
>> + * Returns: 0 on success,
>> + * -1 otherwise (with error reported)
>> + */
>> +int
>> +virSecurityGetRememberedLabel(const char *name,
>> + const char *path,
>> + char **label)
>> +{
>> +char *ref_name = NULL;
>> +char *attr_name = NULL;
>> +char *value = NULL;
>> +unsigned int refcount = 0;
>> +int ret = -1;
>> +
>> +*label = NULL;
>> +
>> +if (!(ref_name = virSecurityGetRefCountAttrName(name)))
>> +goto cleanup;
>> +
>> +if (virFileGetXAtrr(path, ref_name, &value) < 0) {
>> +if (errno == ENOSYS || errno == ENODATA || errno == ENOTSUP) {
>> +ret = 0;
>> +} else {
>> +virReportSystemError(errno,
>> + _("Unable to get XATTR %s on %s"),
>> + ref_name,
>> + path);
>> +}
>> +goto cleanup;
>> +}
>> +
>> +if (virStrToLong_ui(value, NULL, 10, &refcount) < 0) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("malformed refcount %s on %s"),
>> + value, path);
>> +goto cleanup;
>> +}
>> +
>> +VIR_FREE(value);
>> +
>> +refcount--;
>> +
>> +if (refcount > 0) {
>> +if (virAsprintf(&value, "%u", refcount) < 0)
>> +goto cleanup;
>> +
>> +if (virFileSetXAtrr(path, ref_name, value) < 0)
>> +goto cleanup;
>> +} else {
>> +if (virFileRemoveXAttr(path, ref_name) < 0)
>> +goto cleanup;
>> +
>> +if (!(attr_name = virSecurityGetAttrName(name)))
>> +goto cleanup;
>> +
>> +if (virFileGetXAtrr(path, attr_name, label) < 0)
>> +goto cleanup;
>
> I'm not understanding why we only fetch the label value in
> this half of the conditional ? Shouldn't we be unconditionally
> getting the label, regardless of the updated refcount value.
>
> If not, the method description could have an explanation of
> intended behaviour.
The idea is that the first time we set a label for a file the owner is
stored in XATTRs and refcount is set to 1. Any other attempt to chown()
will increase the counter (and do the chown() too). Then, restore
decrements the counter and only if it reaches 0 the original owner is
read from XATTRs and passed to secdriver which is a signal to it that it
needs to actually restore the owner.
Anyway, I will put it into a comment.
>
>> +
>> +if (virFileRemoveXAttr(path, attr_name) < 0)
>> +goto cleanup;
>> +}
>> +
>> +ret = 0;
>> + cleanup:
>> +VIR_FREE(value);
>> +VIR_FREE(attr_name);
>> +VIR_FREE(ref_name);
>> +return ret;
>> +}
>> +
>> +
>> +int
>> +virSecuritySetRememberedLabel(const char *name,
>> + const char *path,
>> + const char *label)
>> +{
>> +char *ref_name = NULL;
>> +char *attr_name = NULL;
>> +char *value = NULL;
>> +unsigned int refcount = 0;
>> +int ret = -1;
>> +
>> +if (!(ref_name = virSecurityGetRefCountAttrName(name)))
>> +goto cleanup;
>> +
>> +if (virFileGetXAtrr(path, ref_name, &value) < 0) {
>> +if (errno == ENOSYS || errno == ENOTSUP) {
>> +ret = 0;
>> +goto cleanup;
>> +} else if (errno != ENODATA) {
>> +virReportSystemError(errno,
>> + _("Unable to get XATTR %s on %s"),
>> + ref_name,
>> + path);
>> +goto cleanup;
>> +}
>> +}
>> +
>> +if (value &&
>> +virStrToLong_ui(value, NULL, 10, &refcount) < 0) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("malformed refcount %s on %s"),
>> + value, path);
>> +goto cleanup;
>> +}
>> +
>> +VIR_FREE(value);
>> +
>> +refcount++;
>> +
>> +if (refcount == 1) {
>> +if (!(attr_name = virSecurityGetAttrName(name)))
>> +
Re: [libvirt] [PATCH v3] openvswitch: Add new port VLAN mode "dot1q-tunnel"
发件人：Laine Stump 发送日期：2018-12-03 22:37:36 收件人：libvir-list@redhat.com 抄送人：luzhip...@uniudc.com 主题：Re: [libvirt] [PATCH v3] openvswitch: Add new port VLAN mode "dot1q-tunnel">On 12/2/18 10:18 PM, luzhip...@uniudc.com wrote: >> From: ZhiPeng Lu >> >> Signed-off-by: ZhiPeng Lu > > >Please include a commit message that has a brief description of the new >feature, including an XML example (yes, this is in addition to the >update to formatnetwork.html.in change). This makes it easier for >someone searching the git log history for a feature to find the commit >when it was introduced. ok，thanks >> --- >> v1->v2: >> 1. Fix "make syntax-check" failure >> v2->v3: >> 1. remove other_config when updating vlan >> >> >> docs/formatnetwork.html.in | 17 + > > >There is also a section about the element in formatdomain.html.in >that needs to be updated to show support for the new nativeMode setting. > > >> docs/schemas/networkcommon.rng | 1 + >> src/conf/netdev_vlan_conf.c | 2 +- >> src/util/virnetdevopenvswitch.c | 7 +++ >> src/util/virnetdevvlan.h| 1 + >> 5 files changed, 19 insertions(+), 9 deletions(-) >> >> diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in >> index 363a72b..3c1ae62 100644 >> --- a/docs/formatnetwork.html.in >> +++ b/docs/formatnetwork.html.in >> @@ -688,16 +688,17 @@ >> >> >>For network connections using Open vSwitch it is also possible >> - to configure 'native-tagged' and 'native-untagged' VLAN modes >> + to configure 'native-tagged' and 'native-untagged' and 'dot1q-tunnel' >> + VLAN modes. >>Since 1.1.0. This is done with the >> - optional nativeMode attribute on >> - thesubelement: nativeMode >> - may be set to 'tagged' or 'untagged'. The id >> - attribute of the subelement >> - containing nativeMode sets which VLAN is considered >> - to be the "native" VLAN for this interface, and >> + optional nativeMode attribute on the >> + subelement: nativeMode >> + may be set to 'tagged' or 'untagged' or 'dot1q-tunnel'. > > > >It would be preferable to use the official name (802.1Q) rather than a >term that is just a commonly used nickname. That makes it easier for >someone unfamiliar with the feature to learn what it is via an internet >search. 802.1ad better？ >Also, you should add a '(since 5.0.0)' after the >name of the new attribute so that it's clear that attribute was added >later (it may also help to remove ambiguity if you add a "since" tag >after each attribute name, otherwise someone might think the "since" tag >at the end applies to all of the attribute names). > > > >> + The id attribute of the >> + subelement containing nativeMode sets which VLAN is >> + considered to be the "native" VLAN for this interface, and >>the nativeMode attribute determines whether or not >> - traffic for that VLAN will be tagged. >> + traffic for that VLAN will be tagged or QinQ. > > >Here is another use of a nickname. Better to use the official name from >the spec. > > >> >> >> elements can also be specified in >> diff --git a/docs/schemas/networkcommon.rng b/docs/schemas/networkcommon.rng >> index 2699555..11c48ff 100644 >> --- a/docs/schemas/networkcommon.rng >> +++ b/docs/schemas/networkcommon.rng >> @@ -223,6 +223,7 @@ >> >> tagged >> untagged >> +dot1q-tunnel >> >> >> >> diff --git a/src/conf/netdev_vlan_conf.c b/src/conf/netdev_vlan_conf.c >> index dff49c6..79710d9 100644 >> --- a/src/conf/netdev_vlan_conf.c >> +++ b/src/conf/netdev_vlan_conf.c >> @@ -29,7 +29,7 @@ >> #define VIR_FROM_THIS VIR_FROM_NONE >> >> VIR_ENUM_IMPL(virNativeVlanMode, VIR_NATIVE_VLAN_MODE_LAST, >> - "default", "tagged", "untagged") >> + "default", "tagged", "untagged", "dot1q-tunnel") > >> int >> virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, >> virNetDevVlanPtr def) >> diff --git a/src/util/virnetdevopenvswitch.c >> b/src/util/virnetdevopenvswitch.c >> index 8fe06fd..9fec30b 100644 >> --- a/src/util/virnetdevopenvswitch.c >> +++ b/src/util/virnetdevopenvswitch.c >> @@ -91,6 +91,11 @@ virNetDevOpenvswitchConstructVlans(virCommandPtr cmd, >> virNetDevVlanPtr virtVlan) >> virCommandAddArg(cmd, "vlan_mode=native-untagged"); >> virCommandAddArgFormat(cmd, "tag=%d", virtVlan->nativeTag); >> break; >> +case VIR_NATIVE_VLAN_MODE_DOT1Q_TUNNEL: >> +virCommandAddArg(cmd, "vlan_mode=dot1q-tunnel"); >> +virCommandAddArg(cmd, "other_config:qinq-ethtype=802.1q"); >> +virCommandAddArgFormat(cmd, "tag=%d", virtVlan->nativeTag); >> +break; >> case VIR_NATIVE_VLAN_MODE_DEFAULT: >> default: >> break; >> @@ -504,6 +509,8 @@ int virNetDevOpenvswitchUpdateVlan(const char *ifname
Re: [libvirt] [PATCH 2/2] qemu: Don't fail stats collection due to IOThread capability
On Tue, Nov 27, 2018 at 11:23:23AM -0500, John Ferlan wrote:
Commit 212dc9286 made a generic qemuDomainGetIOThreadsMon which
would fail if the QEMU_CAPS_OBJECT_IOTHREAD didn't exist. Then
commit d1eac927 used that helper for the collection of all domain
stats. However, if the capability doesn't exist, then the entire
stats collection fails. Since the IOThread stats were meant to be
if available only, thus rather than failing if the capability
doesn't exist, let's just not collect the stats. Restore the caps
failure logic for qemuDomainGetIOThreadsLive.
Signed-off-by: John Ferlan
---
src/qemu/qemu_driver.c | 22 ++
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2fb8eef609..60e29577ad 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5490,16 +5490,9 @@ qemuDomainGetIOThreadsMon(virQEMUDriverPtr driver,
virDomainObjPtr vm,
qemuMonitorIOThreadInfoPtr **iothreads)
{
-qemuDomainObjPrivatePtr priv;
+qemuDomainObjPrivatePtr priv = vm->privateData;
int niothreads = 0;
-priv = vm->privateData;
-if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("IOThreads not supported with this binary"));
-return -1;
-}
-
qemuDomainObjEnterMonitor(driver, vm);
niothreads = qemuMonitorGetIOThreads(priv->mon, iothreads);
if (qemuDomainObjExitMonitor(driver, vm) < 0 || niothreads < 0)
@@ -5514,6 +5507,7 @@ qemuDomainGetIOThreadsLive(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainIOThreadInfoPtr **info)
{
+qemuDomainObjPrivatePtr priv;
qemuMonitorIOThreadInfoPtr *iothreads = NULL;
virDomainIOThreadInfoPtr *info_ret = NULL;
int niothreads = 0;
@@ -5529,6 +5523,13 @@ qemuDomainGetIOThreadsLive(virQEMUDriverPtr driver,
goto endjob;
}
+priv = vm->privateData;
+if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("IOThreads not supported with this binary"));
+goto endjob;
+}
+
if ((niothreads = qemuDomainGetIOThreadsMon(driver, vm, &iothreads)) < 0)
goto endjob;
@@ -20874,6 +20875,7 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver,
int *maxparams,
unsigned int privflags ATTRIBUTE_UNUSED)
{
+qemuDomainObjPrivatePtr priv;
dom is locked when this function is called, you can initialize priv here
size_t i;
qemuMonitorIOThreadInfoPtr *iothreads = NULL;
int niothreads;
@@ -20882,6 +20884,10 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr driver,
if (!virDomainObjIsActive(dom))
return 0;
+priv = dom->privateData;
+if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD))
+return 0;
+
if ((niothreads = qemuDomainGetIOThreadsMon(driver, dom, &iothreads)) < 0)
return -1;
Reviewed-by: Ján Tomko
Jano
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: Save qemuDomainGetStats error
On Tue, Nov 27, 2018 at 11:23:22AM -0500, John Ferlan wrote:
During qemuConnectGetAllDomainStats if qemuDomainGetStats causes
a failure, then when collecting more than one domain's worth of
statistics the loop in virDomainStatsRecordListFree would call
virDomainFree which would call virResetLastError effectively wiping
out the reason we failed leaving the caller with no idea why the
collection failed.
To fix this, let's save the error and restore it prior to return
so that a caller such as 'virsh domstats' doesn't get the generic
"error: An error occurred, but the cause is unknown".
Signed-off-by: John Ferlan
---
src/qemu/qemu_driver.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7d9e17e72c..2fb8eef609 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -21092,6 +21092,7 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
unsigned int flags)
{
virQEMUDriverPtr driver = conn->privateData;
+virErrorPtr save_err = NULL;
git grep virError src/qemu shows most files use orig_err as the variable
name
virDomainObjPtr *vms = NULL;
virDomainObjPtr vm;
size_t nvms;
@@ -21160,6 +21161,7 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
if (flags & VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING)
domflags |= QEMU_DOMAIN_STATS_BACKING;
if (qemuDomainGetStats(conn, vm, stats, &tmp, domflags) < 0) {
+save_err = virSaveLastError();
Since virDomainStatsRecordListFree is the one resetting the error,
I think we should only save it right before that call.
This would cause a memory leak if qemuDomainGetStats would fail for more
than one domain.
if (HAVE_JOB(domflags) && vm)
qemuDomainObjEndJob(driver, vm);
@@ -21184,6 +21186,10 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
cleanup:
Here. And we have a specific helper for that:
virErrorPreserveLast(&orig_err);
virDomainStatsRecordListFree(tmpstats);
virObjectListFreeCount(vms, nvms);
+if (save_err) {
+virSetError(save_err);
+virFreeError(save_err);
+}
virErrorRestore(&orig_err);
With that addressed:
Reviewed-by: Ján Tomko
Jano
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] Fix a couple get all domain stats issues
ping? Tks, John On 11/27/18 11:23 AM, John Ferlan wrote: > One is longer term (patch1), while the other is sourced in this > release (4.10.0) when IOThread stats were added. > > John Ferlan (2): > qemu: Save qemuDomainGetStats error > qemu: Don't fail stats collection due to IOThread capability > > src/qemu/qemu_driver.c | 28 > 1 file changed, 20 insertions(+), 8 deletions(-) > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] qemu: Add check for whether KVM nesting is enabled
ping?
Thanks -
John
On 11/28/18 8:55 PM, John Ferlan wrote:
> Support for nested KVM is handled via a kernel module configuration
> parameters values for kvm_intel, kvm_amd, kvm_hv (PPC), or kvm (s390).
> While it's possible to fetch the kmod config values via virKModConfig,
> unfortunately that is the static value and we need to get the
> current/dynamic value from the kernel file system.
>
> So this patch adds a new API virHostKVMSupportsNesting that will
> search the 3 kernel modules to get the nesting value and check if
> it is 'Y' (or 'y' just in case) or '1' to return a true/false whether
> the KVM kernel supports nesting.
>
> We need to do this in order to handle cases where adjustments to
> the value are made after libvirtd is started to force a refetch of
> the latest QEMU capabilities since the correct CPU settings need
> to be made for a guest to add the "vmx=on" to/for the guest config.
>
> Signed-off-by: John Ferlan
>
> NB to be removed before push - I got data from:
>
> (IBM Z)
> https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/virtualization_deployment_and_administration_guide/appe-kvm_on_zsystems
>
> (PPC slide 131)
> https://events.linuxfoundation.org/wp-content/uploads/2017/12/Taking-it-to-the-Nest-Level-Nested-KVM-on-the-POWER9-Processor-Suraj-Jitindar-Singh-IBM.pdf
>
> Signed-off-by: John Ferlan
> ---
>
> v2: https://www.redhat.com/archives/libvir-list/2018-November/msg00955.html
>
> Changes from code review...
> - Rename variables/API's to KVMSupportsNested
> - Movement of logic to check/set the 'nested' to inside locations that
> ensure KVM was enabled (via capability).
> - Change of logic to not use virKModConfig and instead look at the
> running kernel value for /sys/module/*/parameters/nested where *
> is kvm_intel, kvm_amd, kvm_hv, or kvm
>
> src/qemu/qemu_capabilities.c | 54
> 1 file changed, 54 insertions(+)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 20a1a0c201..bef92a679f 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -558,6 +558,7 @@ struct _virQEMUCaps {
> virObject parent;
>
> bool usedQMP;
> +bool kvmSupportsNesting;
>
> char *binary;
> time_t ctime;
> @@ -1530,6 +1531,7 @@ virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr
> qemuCaps)
> return NULL;
>
> ret->usedQMP = qemuCaps->usedQMP;
> +ret->kvmSupportsNesting = qemuCaps->kvmSupportsNesting;
>
> if (VIR_STRDUP(ret->binary, qemuCaps->binary) < 0)
> goto error;
> @@ -3589,6 +3591,9 @@ virQEMUCapsLoadCache(virArch hostArch,
> virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_KVM);
> virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_QEMU);
>
> +if (virXPathBoolean("boolean(./kvmSupportsNesting)", ctxt) > 0)
> +qemuCaps->kvmSupportsNesting = true;
> +
> ret = 0;
> cleanup:
> VIR_FREE(str);
> @@ -3808,6 +3813,9 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps)
> if (qemuCaps->sevCapabilities)
> virQEMUCapsFormatSEVInfo(qemuCaps, &buf);
>
> +if (qemuCaps->kvmSupportsNesting)
> +virBufferAddLit(&buf, "\n");
> +
> virBufferAdjustIndent(&buf, -2);
> virBufferAddLit(&buf, "\n");
>
> @@ -3848,6 +3856,41 @@ virQEMUCapsSaveFile(void *data,
> }
>
>
> +/* Check the kernel module parameters 'nested' file to determine if enabled
> + *
> + * Intel: 'kvm_intel' uses 'Y'
> + * AMD: 'kvm_amd' uses '1'
> + * PPC64: 'kvm_hv' uses 'Y'
> + * S390: 'kvm' uses '1'
> + */
> +static bool
> +virQEMUCapsKVMSupportsNesting(void)
> +{
> +static char const * const kmod[] = {"kvm_intel", "kvm_amd",
> +"kvm_hv", "kvm"};
> +VIR_AUTOFREE(char *) value = NULL;
> +int rc;
> +size_t i;
> +
> +for (i = 0; i < ARRAY_CARDINALITY(kmod); i++) {
> +VIR_FREE(value);
> +rc = virFileReadValueString(&value,
> "/sys/module/%s/parameters/nested",
> +kmod[i]);
> +if (rc == -2)
> +continue;
> +if (rc < 0) {
> +virResetLastError();
> +return false;
> +}
> +
> +if (value[0] == 'Y' || value[0] == 'y' || value[0] == '1')
> +return true;
> +}
> +
> +return false;
> +}
> +
> +
> static bool
> virQEMUCapsIsValid(void *data,
> void *privData)
> @@ -3856,6 +3899,7 @@ virQEMUCapsIsValid(void *data,
> virQEMUCapsCachePrivPtr priv = privData;
> bool kvmUsable;
> struct stat sb;
> +bool kvmSupportsNesting;
>
> if (!qemuCaps->binary)
> return true;
> @@ -3933,6 +3977,14 @@ virQEMUCapsIsValid(void *data,
>qemuCaps->kernelVersion);
> return false;
> }
> +
> +kvmSupportsNesting = virQEMUCapsKVMSupportsNesting();
> +
Re: [libvirt] [PATCH v2 18/18] qemu.conf: Allow users to enable/disable label remembering
On Thu, Nov 29, 2018 at 02:52:33PM +0100, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/libvirtd_qemu.aug | 1 +
> src/qemu/qemu.conf | 6 ++
> src/qemu/qemu_conf.c | 4
> src/qemu/test_libvirtd_qemu.aug.in | 1 +
> 4 files changed, 12 insertions(+)
>
> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
> index ddc4bbfd1d..8a5b39e568 100644
> --- a/src/qemu/libvirtd_qemu.aug
> +++ b/src/qemu/libvirtd_qemu.aug
> @@ -71,6 +71,7 @@ module Libvirtd_qemu =
> | str_entry "user"
> | str_entry "group"
> | bool_entry "dynamic_ownership"
> + | bool_entry "remember_owner"
> | str_array_entry "cgroup_controllers"
> | str_array_entry "cgroup_device_acl"
> | int_entry "seccomp_sandbox"
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index 8391332cb4..31e8d8476b 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -450,6 +450,12 @@
> # Set to 0 to disable file ownership changes.
> #dynamic_ownership = 1
>
> +# Whether libvirt should remember and restore the original
> +# ownership over files it is relabeling. Be aware that with the
> +# current implementation this requires exclusive access to the
> +# files which might hurt performance a bit in some cases.
What do you mean by performance impact here ? I think this is a bit
obscure to put as a comment, as users aren't given enough info to
decide if its a perf hit for them or not. I'd just leave out that
info.
> +# Defaults to 1, set to 0 to disable the feature.
> +#remember_owner = 1
>
> # What cgroup controllers to make use of with QEMU guests
> #
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index a946b05d5d..89491a37b7 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -147,6 +147,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool
> privileged)
> cfg->group = (gid_t)-1;
> }
> cfg->dynamicOwnership = privileged;
> +cfg->rememberOwner = true;
>
> cfg->cgroupControllers = -1; /* -1 == auto-detect */
>
> @@ -730,6 +731,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr
> cfg,
> if (virConfGetValueBool(conf, "dynamic_ownership",
> &cfg->dynamicOwnership) < 0)
> goto cleanup;
>
> +if (virConfGetValueBool(conf, "remember_owner", &cfg->rememberOwner) < 0)
> +goto cleanup;
> +
> if (virConfGetValueStringList(conf, "cgroup_controllers", false,
>&controllers) < 0)
> goto cleanup;
> diff --git a/src/qemu/test_libvirtd_qemu.aug.in
> b/src/qemu/test_libvirtd_qemu.aug.in
> index f1e8806ad2..92a8ae1192 100644
> --- a/src/qemu/test_libvirtd_qemu.aug.in
> +++ b/src/qemu/test_libvirtd_qemu.aug.in
> @@ -43,6 +43,7 @@ module Test_libvirtd_qemu =
> { "user" = "root" }
> { "group" = "root" }
> { "dynamic_ownership" = "1" }
> +{ "remember_owner" = "1" }
> { "cgroup_controllers"
> { "1" = "cpu" }
> { "2" = "devices" }
> --
> 2.18.1
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 17/18] tools: Provide a script to recover fubar'ed XATTRs setup
On Thu, Nov 29, 2018 at 02:52:32PM +0100, Michal Privoznik wrote:
> Our code is not bug free. The refcounting I introduced will
> almost certainly not work in some use cases. Provide a script
> that will remove all the XATTRs set by libvirt so that it can
> start cleanly.
On this point, it would be a nice idea to be able to write some
unit tests to exercise the security drivers, as this is something
we're significantly lacking coverage of.
With mocking of the chown/setxattr/etc methods we can easily
detect some ofthe bugs you fixed here, such as forgetting to
restore labels of certain resource types.
>
> Signed-off-by: Michal Privoznik
> ---
> tools/Makefile.am | 1 +
> tools/libvirt_recover_xattrs.sh | 89 +
> 2 files changed, 90 insertions(+)
> create mode 100755 tools/libvirt_recover_xattrs.sh
>
> diff --git a/tools/Makefile.am b/tools/Makefile.am
> index f069167acc..1dc009c4fb 100644
> --- a/tools/Makefile.am
> +++ b/tools/Makefile.am
> @@ -75,6 +75,7 @@ EXTRA_DIST = \
> virt-login-shell.conf \
> virsh-edit.c \
> bash-completion/vsh \
> + libvirt_recover_xattrs.sh \
> $(PODFILES) \
> $(MANINFILES) \
> $(NULL)
> +XATTRS=("trusted.libvirt.security.dac"
> +"trusted.libvirt.security.ref_dac"
> +"trusted.libvirt.security.selinux"
> +"trusted.libvirt.security.ref_selinux")
Needs updating to account for FreeBSD naming now
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 16/18] virSecuritySELinuxRestoreAllLabel: Restore more labels
On Thu, Nov 29, 2018 at 02:52:31PM +0100, Michal Privoznik wrote: > We are setting label on kernel, initrd, dtb and slic_table files. > But we never restored it. > > Signed-off-by: Michal Privoznik > --- > src/security/security_selinux.c | 16 > 1 file changed, 16 insertions(+) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 15/18] virSecuritySELinuxRestoreAllLabel: Reorder device relabeling
On Thu, Nov 29, 2018 at 02:52:30PM +0100, Michal Privoznik wrote: > It helps whe trying to match calls with virSecuritySELinuxSetAllLabel > if the order in which devices are set/restored is the same in > both functions. > > Signed-off-by: Michal Privoznik > --- > src/security/security_selinux.c | 14 +++--- > 1 file changed, 7 insertions(+), 7 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 14/18] virSecuritySELinuxTransactionRun: Implement rollback
On Thu, Nov 29, 2018 at 02:52:29PM +0100, Michal Privoznik wrote: > When iterating over list of paths/disk sources to relabel it may > happen that the process fails at some point. In that case, for > the sake of keeping seclabel refcount (stored in XATTRs) in sync > with reality we have to perform rollback. However, if that fails > too the only thing we can do is warn user. > > Signed-off-by: Michal Privoznik > --- > src/security/security_selinux.c | 13 - > 1 file changed, 12 insertions(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 13/18] security_selinux: Restore label on failed setfilecon() attempt
On Thu, Nov 29, 2018 at 02:52:28PM +0100, Michal Privoznik wrote: > It's important to keep XATTRs untouched (well, in the same state > they were in when entering the function). Otherwise our > refcounting would be messed up. > > Signed-off-by: Michal Privoznik > --- > src/security/security_selinux.c | 40 +++-- > 1 file changed, 28 insertions(+), 12 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 12/18] security_selinux: Remember old labels
On Thu, Nov 29, 2018 at 02:52:27PM +0100, Michal Privoznik wrote: > Signed-off-by: Michal Privoznik > --- > src/security/security_selinux.c | 161 ++-- > 1 file changed, 114 insertions(+), 47 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 11/18] security_selinux: Track if transaction is restore
On Thu, Nov 29, 2018 at 02:52:26PM +0100, Michal Privoznik wrote: > It is going to be important to know if the current transaction we > are running is a restore operation or set label operation. Might be worth saying why it is important :-) > > Signed-off-by: Michal Privoznik > --- > src/security/security_selinux.c | 36 +++-- > 1 file changed, 25 insertions(+), 11 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 10/18] virSecurityDACRestoreImageLabelInt: Restore even shared/RO disks
On Thu, Nov 29, 2018 at 02:52:25PM +0100, Michal Privoznik wrote: > Now that we have seclabel remembering we can safely restore > labels for shared and RO disks. In fact we need to do that to > keep seclabel refcount stored in XATTRs in sync with reality. > > Signed-off-by: Michal Privoznik > --- > src/security/security_dac.c | 8 > 1 file changed, 8 deletions(-) > > diff --git a/src/security/security_dac.c b/src/security/security_dac.c > index 9d31faa9d4..60adfaf526 100644 > --- a/src/security/security_dac.c > +++ b/src/security/security_dac.c > @@ -921,14 +921,6 @@ virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr > mgr, > if (!priv->dynamicOwnership) > return 0; > > -/* Don't restore labels on readoly/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 > - * the file on other nodes. Safest bet is thus to skip the restore step. > */ > -if (src->readonly || src->shared) > -return 0; > - > secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); > if (secdef && !secdef->relabel) > return 0; Reviewed-by: Daniel P. Berrangé though I'd probably squash this into the previous commit, otherwise the bisection has a point in time where it leaks attrs for shared/readonly disk Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 09/18] security_dac: Remember old labels
On Thu, Nov 29, 2018 at 02:52:24PM +0100, Michal Privoznik wrote: > Signed-off-by: Michal Privoznik > --- > src/security/security_dac.c | 48 ++--- > 1 file changed, 40 insertions(+), 8 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 08/18] security_dac: Allow callers to enable/disable label remembering/recall
On Thu, Nov 29, 2018 at 02:52:23PM +0100, Michal Privoznik wrote: > Because the implementation that will be used for label > remembering/recall is not atomic we have to give callers a chance > to enable or disable it. That is, enable it if and only if > metadata locking is enabled. Otherwise the feature MUST be turned > off. > > Signed-off-by: Michal Privoznik > --- > src/security/security_dac.c | 74 ++--- > 1 file changed, 45 insertions(+), 29 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 08/10] util: error: Use a more declarative approach in virErrorMsg
On Thu, Dec 06, 2018 at 11:48:15 +, Daniel Berrange wrote:
> On Wed, Dec 05, 2018 at 05:47:49PM +0100, Peter Krempa wrote:
> > Use a macro to declare how the strings for individual error codes. This
> > unifies the used condition and will allow simplifying the code further.
> >
> > Signed-off-by: Peter Krempa
> > ---
> > src/libvirt_private.syms | 1 +
> > src/util/virerror.c | 792 +--
> > src/util/virerrorpriv.h | 8 +
> > 3 files changed, 188 insertions(+), 613 deletions(-)
> >
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index 6184030d59..775b33e151 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -1753,6 +1753,7 @@ virDispatchError;
> > virErrorCopyNew;
> > virErrorInitialize;
> > virErrorMsg;
> > +virErrorMsgStrings;
> > virErrorPreserveLast;
> > virErrorRestore;
> > virErrorSetErrnoFromLastError;
> > diff --git a/src/util/virerror.c b/src/util/virerror.c
> > index 7444d671bb..d3cd06331f 100644
> > --- a/src/util/virerror.c
> > +++ b/src/util/virerror.c
> > @@ -903,6 +903,178 @@ void virRaiseErrorObject(const char *filename,
> > }
> >
> >
> > +const virErrorMsgTuple virErrorMsgStrings[VIR_ERR_NUMBER_LAST] = {
> > +{ VIR_ERR_OK, NULL, NULL },
> > +{ VIR_ERR_INTERNAL_ERROR, "internal error", "internal error: %s" },
>
> [snip]
>
> > +if (info)
> > +return virErrorMsgStrings[error].msginfo;
> > +else
> > +return virErrorMsgStrings[error].msg;
>
> The code only reads the .msginfo / .msg fields, so...
>
> > }
> >
> > +
> > /**
> > * virReportErrorHelper:
> > *
> > diff --git a/src/util/virerrorpriv.h b/src/util/virerrorpriv.h
> > index bc214393e6..1be40d8a51 100644
> > --- a/src/util/virerrorpriv.h
> > +++ b/src/util/virerrorpriv.h
> > @@ -21,6 +21,14 @@
> > #ifndef __VIR_ERROR_PRIV_H__
> > # define __VIR_ERROR_PRIV_H__
> >
> > +typedef struct {
> > +virErrorNumber error;
>
> ..what's the point of storing this which AFAICT just duplicates
> the array index number.
Originally they were in a somewhat random order, so I used a lookup in
the array as an intermediate step in the refactor.
Later I've decided to merge the patches, so they are unused now.
Currently it serves only the purpose to identify the message ...
>
> > +const char *msg;
> > +const char *msginfo;
> > +} virErrorMsgTuple;
> > +
> > +extern const virErrorMsgTuple virErrorMsgStrings[VIR_ERR_NUMBER_LAST];
>
> Could we change it to just use named array indexes during init eg
>
> const virErrorMsgTuple virErrorMsgStrings[VIR_ERR_NUMBER_LAST] = {
>[VIR_ERR_OK] = { NULL, NULL },
>[VIR_ERR_INTERNAL_ERROR = { "internal error", "internal error: %s" },
>...etc...
... but that does work here as well.
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 07/18] virSecurityDACRestoreAllLabel: Restore more labels
On Thu, Nov 29, 2018 at 02:52:22PM +0100, Michal Privoznik wrote: > We are setting label on kernel, initrd, dtb and slic_table files. > But we never restored it. > > Signed-off-by: Michal Privoznik > --- > src/security/security_dac.c | 16 > 1 file changed, 16 insertions(+) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 06/18] virSecurityDACRestoreAllLabel: Reorder device relabeling
On Thu, Nov 29, 2018 at 02:52:21PM +0100, Michal Privoznik wrote: > It helps whe trying to match calls with virSecurityDACSetAllLabel > if the order in which devices are set/restored is the same in > both functions. > > Signed-off-by: Michal Privoznik > --- > src/security/security_dac.c | 36 ++-- > 1 file changed, 18 insertions(+), 18 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 08/10] util: error: Use a more declarative approach in virErrorMsg
On Thu, Dec 06, 2018 at 11:42:44 +, Daniel Berrange wrote:
> On Wed, Dec 05, 2018 at 05:47:49PM +0100, Peter Krempa wrote:
> > Use a macro to declare how the strings for individual error codes. This
> > unifies the used condition and will allow simplifying the code further.
> >
> > Signed-off-by: Peter Krempa
> > ---
> > src/libvirt_private.syms | 1 +
> > src/util/virerror.c | 792 +--
> > src/util/virerrorpriv.h | 8 +
> > 3 files changed, 188 insertions(+), 613 deletions(-)
> >
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index 6184030d59..775b33e151 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -1753,6 +1753,7 @@ virDispatchError;
> > virErrorCopyNew;
> > virErrorInitialize;
> > virErrorMsg;
> > +virErrorMsgStrings;
> > virErrorPreserveLast;
> > virErrorRestore;
> > virErrorSetErrnoFromLastError;
> > diff --git a/src/util/virerror.c b/src/util/virerror.c
> > index 7444d671bb..d3cd06331f 100644
> > --- a/src/util/virerror.c
> > +++ b/src/util/virerror.c
> > @@ -903,6 +903,178 @@ void virRaiseErrorObject(const char *filename,
> > }
> >
> >
> > +const virErrorMsgTuple virErrorMsgStrings[VIR_ERR_NUMBER_LAST] = {
> > +{ VIR_ERR_OK, NULL, NULL },
> > +{ VIR_ERR_INTERNAL_ERROR, "internal error", "internal error: %s" },
> > +{ VIR_ERR_NO_MEMORY, "out of memory", "out of memory: %s" },
> > +{ VIR_ERR_NO_SUPPORT,
> > +"this function is not supported by the connection driver",
> > +"this function is not supported by the connection driver: %s" },
>
> The vast majority of messages have identical text apart from a small
> suffix. How about using a macro to kill the duplication in the source
> eg
>
> #define MSG(msg, suffix) \
> msg, msg # suffix
>
> { VIR_ERR_NO_SUPPORT,
> MSG("this function is not supported by the connection driver", ": %s") },
>
> Then, only a handful will need separate entries
I thought about this in a less, generic way (by adding a macro that
handles the ": %s" case specifically). I'll try that in the next
submission then.
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 05/18] virSecurityDACTransactionRun: Implement rollback
On Thu, Nov 29, 2018 at 02:52:20PM +0100, Michal Privoznik wrote: > When iterating over list of paths/disk sources to relabel it may > happen that the process fails at some point. In that case, for > the sake of keeping seclabel refcount (stored in XATTRs) in sync > with reality we have to perform rollback. However, if that fails > too the only thing we can do is warn user. > > Signed-off-by: Michal Privoznik > --- > src/security/security_dac.c | 14 +- > 1 file changed, 13 insertions(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 04/18] security_dac: Restore label on failed chown() attempt
On Thu, Nov 29, 2018 at 02:52:19PM +0100, Michal Privoznik wrote:
> It's important to keep XATTRs untouched (well, in the same state
> they were in when entering the function). Otherwise our
> refcounting would be messed up.
>
> Signed-off-by: Michal Privoznik
> ---
> src/security/security_dac.c | 20 +++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 6b64d2c07a..8155c6d58a 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -718,7 +718,25 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
> VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'",
> NULLSTR(src ? src->path : path), (long)uid, (long)gid);
>
> -return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid);
> +if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0) {
> +virErrorPtr origerr;
> +
> +virErrorPreserveLast(&origerr);
> +/* Try to restore the label. This is done so that XATTRs
> + * are left in the same state as when the control entered
> + * this function. However, if our attempt fails, there's
> + * not much we can do. XATTRs refcounting is fubar'ed and
> + * the only option we have is warn users. */
> +if (virSecurityDACRestoreFileLabelInternal(mgr, src, path) < 0)
> +VIR_WARN("Unable to restore label on '%s'. "
> + "XATTRs might have been left in inconsistent state.",
> + NULLSTR(src ? src->path : path));
> +
> +virErrorRestore(&origerr);
> +return -1;
> +}
> +
> +return 0;
> }
Reviewed-by: Daniel P. Berrangé
THough I feel this could probably just be squashed into the patch that
integrates the label remembering, as it doesn't really do anything useful
on its own.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 08/10] util: error: Use a more declarative approach in virErrorMsg
On Wed, Dec 05, 2018 at 05:47:49PM +0100, Peter Krempa wrote:
> Use a macro to declare how the strings for individual error codes. This
> unifies the used condition and will allow simplifying the code further.
>
> Signed-off-by: Peter Krempa
> ---
> src/libvirt_private.syms | 1 +
> src/util/virerror.c | 792 +--
> src/util/virerrorpriv.h | 8 +
> 3 files changed, 188 insertions(+), 613 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 6184030d59..775b33e151 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1753,6 +1753,7 @@ virDispatchError;
> virErrorCopyNew;
> virErrorInitialize;
> virErrorMsg;
> +virErrorMsgStrings;
> virErrorPreserveLast;
> virErrorRestore;
> virErrorSetErrnoFromLastError;
> diff --git a/src/util/virerror.c b/src/util/virerror.c
> index 7444d671bb..d3cd06331f 100644
> --- a/src/util/virerror.c
> +++ b/src/util/virerror.c
> @@ -903,6 +903,178 @@ void virRaiseErrorObject(const char *filename,
> }
>
>
> +const virErrorMsgTuple virErrorMsgStrings[VIR_ERR_NUMBER_LAST] = {
> +{ VIR_ERR_OK, NULL, NULL },
> +{ VIR_ERR_INTERNAL_ERROR, "internal error", "internal error: %s" },
[snip]
> +if (info)
> +return virErrorMsgStrings[error].msginfo;
> +else
> +return virErrorMsgStrings[error].msg;
The code only reads the .msginfo / .msg fields, so...
> }
>
> +
> /**
> * virReportErrorHelper:
> *
> diff --git a/src/util/virerrorpriv.h b/src/util/virerrorpriv.h
> index bc214393e6..1be40d8a51 100644
> --- a/src/util/virerrorpriv.h
> +++ b/src/util/virerrorpriv.h
> @@ -21,6 +21,14 @@
> #ifndef __VIR_ERROR_PRIV_H__
> # define __VIR_ERROR_PRIV_H__
>
> +typedef struct {
> +virErrorNumber error;
..what's the point of storing this which AFAICT just duplicates
the array index number.
> +const char *msg;
> +const char *msginfo;
> +} virErrorMsgTuple;
> +
> +extern const virErrorMsgTuple virErrorMsgStrings[VIR_ERR_NUMBER_LAST];
Could we change it to just use named array indexes during init eg
const virErrorMsgTuple virErrorMsgStrings[VIR_ERR_NUMBER_LAST] = {
[VIR_ERR_OK] = { NULL, NULL },
[VIR_ERR_INTERNAL_ERROR = { "internal error", "internal error: %s" },
...etc...
};
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 08/10] util: error: Use a more declarative approach in virErrorMsg
On Wed, Dec 05, 2018 at 05:47:49PM +0100, Peter Krempa wrote:
> Use a macro to declare how the strings for individual error codes. This
> unifies the used condition and will allow simplifying the code further.
>
> Signed-off-by: Peter Krempa
> ---
> src/libvirt_private.syms | 1 +
> src/util/virerror.c | 792 +--
> src/util/virerrorpriv.h | 8 +
> 3 files changed, 188 insertions(+), 613 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 6184030d59..775b33e151 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1753,6 +1753,7 @@ virDispatchError;
> virErrorCopyNew;
> virErrorInitialize;
> virErrorMsg;
> +virErrorMsgStrings;
> virErrorPreserveLast;
> virErrorRestore;
> virErrorSetErrnoFromLastError;
> diff --git a/src/util/virerror.c b/src/util/virerror.c
> index 7444d671bb..d3cd06331f 100644
> --- a/src/util/virerror.c
> +++ b/src/util/virerror.c
> @@ -903,6 +903,178 @@ void virRaiseErrorObject(const char *filename,
> }
>
>
> +const virErrorMsgTuple virErrorMsgStrings[VIR_ERR_NUMBER_LAST] = {
> +{ VIR_ERR_OK, NULL, NULL },
> +{ VIR_ERR_INTERNAL_ERROR, "internal error", "internal error: %s" },
> +{ VIR_ERR_NO_MEMORY, "out of memory", "out of memory: %s" },
> +{ VIR_ERR_NO_SUPPORT,
> +"this function is not supported by the connection driver",
> +"this function is not supported by the connection driver: %s" },
The vast majority of messages have identical text apart from a small
suffix. How about using a macro to kill the duplication in the source
eg
#define MSG(msg, suffix) \
msg, msg # suffix
{ VIR_ERR_NO_SUPPORT,
MSG("this function is not supported by the connection driver", ": %s") },
Then, only a handful will need separate entries
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/10] tests: virerror: Make sure that error messages stay in correct order
On Wed, Dec 05, 2018 at 05:47:51PM +0100, Peter Krempa wrote:
> Since we don't look up the error message according to the error code but
> they have to be in the correct order in virErrorMsgStrings, we need
> to make sure that they stay in the correct order.
>
> Signed-off-by: Peter Krempa
> ---
> tests/virerrortest.c | 20
> 1 file changed, 20 insertions(+)
>
> diff --git a/tests/virerrortest.c b/tests/virerrortest.c
> index 0d0377bfa8..6d333f02d0 100644
> --- a/tests/virerrortest.c
> +++ b/tests/virerrortest.c
> @@ -87,6 +87,24 @@ virErrorTestMsgs(const void *opaque ATTRIBUTE_UNUSED)
> }
>
>
> +static int
> +virErrorTestMsgOrder(const void *opaque ATTRIBUTE_UNUSED)
> +{
> +size_t i;
> +int ret = 0;
> +
> +for (i = 0; i < VIR_ERR_NUMBER_LAST; i++) {
> +if (i != virErrorMsgStrings[i].error) {
I hope coverity is smart enough not to think that ^^this is a comparison
between a signed and unsigned integer.
Reviewed-by: Erik Skultety
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 03/18] security: Include security_util
On Thu, Nov 29, 2018 at 02:52:18PM +0100, Michal Privoznik wrote:
> This file implements wrappers over XATTR getter/setter. It
> ensures the proper XATTR namespace is used.
>
> Signed-off-by: Michal Privoznik
> ---
> src/security/Makefile.inc.am | 2 +
> src/security/security_util.c | 226 +++
> src/security/security_util.h | 32 +
> 3 files changed, 260 insertions(+)
> create mode 100644 src/security/security_util.c
> create mode 100644 src/security/security_util.h
>
> +/**
> + * virSecurityGetRememberedLabel:
> + * @name: security driver name
> + * @path: file name
> + * @label: label
> + *
> + * For given @path and security driver (@name) fetch remembered
> + * @label. The caller must not restore label if an error is
> + * indicated or if @label is NULL upon return.
> + *
> + * Returns: 0 on success,
> + * -1 otherwise (with error reported)
> + */
> +int
> +virSecurityGetRememberedLabel(const char *name,
> + const char *path,
> + char **label)
> +{
> +char *ref_name = NULL;
> +char *attr_name = NULL;
> +char *value = NULL;
> +unsigned int refcount = 0;
> +int ret = -1;
> +
> +*label = NULL;
> +
> +if (!(ref_name = virSecurityGetRefCountAttrName(name)))
> +goto cleanup;
> +
> +if (virFileGetXAtrr(path, ref_name, &value) < 0) {
> +if (errno == ENOSYS || errno == ENODATA || errno == ENOTSUP) {
> +ret = 0;
> +} else {
> +virReportSystemError(errno,
> + _("Unable to get XATTR %s on %s"),
> + ref_name,
> + path);
> +}
> +goto cleanup;
> +}
> +
> +if (virStrToLong_ui(value, NULL, 10, &refcount) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("malformed refcount %s on %s"),
> + value, path);
> +goto cleanup;
> +}
> +
> +VIR_FREE(value);
> +
> +refcount--;
> +
> +if (refcount > 0) {
> +if (virAsprintf(&value, "%u", refcount) < 0)
> +goto cleanup;
> +
> +if (virFileSetXAtrr(path, ref_name, value) < 0)
> +goto cleanup;
> +} else {
> +if (virFileRemoveXAttr(path, ref_name) < 0)
> +goto cleanup;
> +
> +if (!(attr_name = virSecurityGetAttrName(name)))
> +goto cleanup;
> +
> +if (virFileGetXAtrr(path, attr_name, label) < 0)
> +goto cleanup;
I'm not understanding why we only fetch the label value in
this half of the conditional ? Shouldn't we be unconditionally
getting the label, regardless of the updated refcount value.
If not, the method description could have an explanation of
intended behaviour.
> +
> +if (virFileRemoveXAttr(path, attr_name) < 0)
> +goto cleanup;
> +}
> +
> +ret = 0;
> + cleanup:
> +VIR_FREE(value);
> +VIR_FREE(attr_name);
> +VIR_FREE(ref_name);
> +return ret;
> +}
> +
> +
> +int
> +virSecuritySetRememberedLabel(const char *name,
> + const char *path,
> + const char *label)
> +{
> +char *ref_name = NULL;
> +char *attr_name = NULL;
> +char *value = NULL;
> +unsigned int refcount = 0;
> +int ret = -1;
> +
> +if (!(ref_name = virSecurityGetRefCountAttrName(name)))
> +goto cleanup;
> +
> +if (virFileGetXAtrr(path, ref_name, &value) < 0) {
> +if (errno == ENOSYS || errno == ENOTSUP) {
> +ret = 0;
> +goto cleanup;
> +} else if (errno != ENODATA) {
> +virReportSystemError(errno,
> + _("Unable to get XATTR %s on %s"),
> + ref_name,
> + path);
> +goto cleanup;
> +}
> +}
> +
> +if (value &&
> +virStrToLong_ui(value, NULL, 10, &refcount) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("malformed refcount %s on %s"),
> + value, path);
> +goto cleanup;
> +}
> +
> +VIR_FREE(value);
> +
> +refcount++;
> +
> +if (refcount == 1) {
> +if (!(attr_name = virSecurityGetAttrName(name)))
> +goto cleanup;
> +
> +if (virFileSetXAtrr(path, attr_name, label) < 0)
> +goto cleanup;
> +}
Do we need to have a
} else {
check the currently remember label matches
what was passed into this method ?
}
if not, can you add API docs for this method explainng the
intended semantics when label is already remembered.
> +
> +if (virAsprintf(&value, "%u", refcount) < 0)
> +goto cleanup;
> +
> +if (virFileSetXAtrr(path, ref_name, value) < 0)
> +goto cleanup;
> +
> +ret = 0;
> + cleanup:
> +VIR_FREE(value);
> +VIR_FREE(attr_name);
> +
Re: [libvirt] [PATCH 08/10] util: error: Use a more declarative approach in virErrorMsg
On Thu, Dec 06, 2018 at 12:29:27 +0100, Erik Skultety wrote:
> On Wed, Dec 05, 2018 at 05:47:49PM +0100, Peter Krempa wrote:
> > Use a macro to declare how the strings for individual error codes. This
> > unifies the used condition and will allow simplifying the code further.
> >
> > Signed-off-by: Peter Krempa
> > ---
> > src/libvirt_private.syms | 1 +
> > src/util/virerror.c | 792 +--
> > src/util/virerrorpriv.h | 8 +
> > 3 files changed, 188 insertions(+), 613 deletions(-)
> >
[..]
> > diff --git a/src/util/virerror.c b/src/util/virerror.c
> > index 7444d671bb..d3cd06331f 100644
> > --- a/src/util/virerror.c
> > +++ b/src/util/virerror.c
> > @@ -903,6 +903,178 @@ void virRaiseErrorObject(const char *filename,
[...]
> > +{ VIR_ERR_UNKNOWN_HOST, "unknown host", "unknown host %s" },
> > +{ VIR_ERR_NO_CONNECT,
> > +"no connection driver available",
> > +"no connection driver available for %s" },
> > +{ VIR_ERR_INVALID_CONN, "invalid connection pointer in", "invalid
> > connection pointer in %s" },
> > +{ VIR_ERR_INVALID_DOMAIN, "invalid domain pointer in", "invalid domain
> > pointer in %s" },
>
> Most of the messages exceed the 80 chars limit, I think it's reasonable to
> play
> the consistency card (+ personally I find it more readable too) and say that
> every member should be on a separate line.
I was shooting for < 100 colums for these in this case. 80 is getting
ridiculous in some cases. Honestly I'd prefer them all on one line
rather than broken up.
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 02/18] util: Introduce xattr getter/setter/remover
On Thu, Nov 29, 2018 at 02:52:17PM +0100, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik
> ---
> src/libvirt_private.syms | 3 +
> src/util/virfile.c | 121 +++
> src/util/virfile.h | 11
> 3 files changed, 135 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 5018a13e9c..8e5b610ab1 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1827,6 +1827,7 @@ virFileGetACLs;
> virFileGetHugepageSize;
> virFileGetMountReverseSubtree;
> virFileGetMountSubtree;
> +virFileGetXAtrr;
s/Atrr/Attr/
> @@ -1873,6 +1875,7 @@ virFileRewriteStr;
> virFileSanitizePath;
> virFileSetACLs;
> virFileSetupDev;
> +virFileSetXAtrr;
s/Atrr/Attr/
> +#if HAVE_LIBATTR
> +/**
> + * virFileGetXAtrr;
> + * @path: a filename
> + * @name: name of xattr
> + * @value: read value
> + *
> + * Reads xattr with @name for given @path and stores it into
> + * @value. Caller is responsible for freeing @value.
> + *
> + * Returns: 0 on success,
> + * -1 otherwise (with errno set).
> + */
> +int
> +virFileGetXAtrr(const char *path,
> +const char *name,
> +char **value)
> +{
> +char *buf = NULL;
> +int ret = -1;
> +
> +/* We might be racing with somebody who sets the same attribute. */
> +do {
> +ssize_t need;
> +ssize_t got;
> +
> +/* The first call determines how many bytes we need to allocate. */
> +if ((need = getxattr(path, name, NULL, 0)) < 0)
> +goto cleanup;
> +
> +if (VIR_REALLOC_N_QUIET(buf, need + 1) < 0)
> +goto cleanup;
> +
> +if ((got = getxattr(path, name, buf, need)) < 0) {
> +if (errno == ERANGE)
> +continue;
> +goto cleanup;
> +}
> +
> +buf[got] = '\0';
> +break;
> +} while (1);
Nitpick, I'd expect the while(1) at the top. Only use this style of
loop when the checked loop condition would mistakenly prevent the first
iteration running.
> +
> +VIR_STEAL_PTR(*value, buf);
> +ret = 0;
> + cleanup:
> +VIR_FREE(buf);
> +return ret;
> +}
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 08/10] util: error: Use a more declarative approach in virErrorMsg
On Wed, Dec 05, 2018 at 05:47:49PM +0100, Peter Krempa wrote:
> Use a macro to declare how the strings for individual error codes. This
> unifies the used condition and will allow simplifying the code further.
>
> Signed-off-by: Peter Krempa
> ---
> src/libvirt_private.syms | 1 +
> src/util/virerror.c | 792 +--
> src/util/virerrorpriv.h | 8 +
> 3 files changed, 188 insertions(+), 613 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 6184030d59..775b33e151 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1753,6 +1753,7 @@ virDispatchError;
> virErrorCopyNew;
> virErrorInitialize;
> virErrorMsg;
> +virErrorMsgStrings;
> virErrorPreserveLast;
> virErrorRestore;
> virErrorSetErrnoFromLastError;
> diff --git a/src/util/virerror.c b/src/util/virerror.c
> index 7444d671bb..d3cd06331f 100644
> --- a/src/util/virerror.c
> +++ b/src/util/virerror.c
> @@ -903,6 +903,178 @@ void virRaiseErrorObject(const char *filename,
> }
>
>
> +const virErrorMsgTuple virErrorMsgStrings[VIR_ERR_NUMBER_LAST] = {
> +{ VIR_ERR_OK, NULL, NULL },
> +{ VIR_ERR_INTERNAL_ERROR, "internal error", "internal error: %s" },
> +{ VIR_ERR_NO_MEMORY, "out of memory", "out of memory: %s" },
> +{ VIR_ERR_NO_SUPPORT,
> +"this function is not supported by the connection driver",
> +"this function is not supported by the connection driver: %s" },
> +{ VIR_ERR_UNKNOWN_HOST, "unknown host", "unknown host %s" },
> +{ VIR_ERR_NO_CONNECT,
> +"no connection driver available",
> +"no connection driver available for %s" },
> +{ VIR_ERR_INVALID_CONN, "invalid connection pointer in", "invalid
> connection pointer in %s" },
> +{ VIR_ERR_INVALID_DOMAIN, "invalid domain pointer in", "invalid domain
> pointer in %s" },
Most of the messages exceed the 80 chars limit, I think it's reasonable to play
the consistency card (+ personally I find it more readable too) and say that
every member should be on a separate line.
Reviewed-by: Erik Skultety
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 01/18] security: Unify header conditionals
On Thu, Nov 29, 2018 at 02:52:16PM +0100, Michal Privoznik wrote:
> To avoid including a header file twice the following pattern is
> used:
>
> #ifndef __SOMETHING__
> # define __SOMETHING__
>
> where __SOMETHING__ should correspond to the header file name.
> However, some of our header files break that pattern.
Looking at the git tree as a whole, we're all over the place
with the naming of these cnoditionals. There's many othuer
files using a __VIR prefix which don't have 'vir' in the
filename:
$ git grep ifndef '*.h' | grep VIR | grep -v vir | wc -l
103
So I don't think this is something we should really change here.
I think it points to the need for a syntax-check rule to enforce
a given convention and then a tree-wide fixup to comply.
> Signed-off-by: Michal Privoznik
> ---
> src/security/security_apparmor.h | 6 +++---
> src/security/security_dac.h | 6 +++---
> src/security/security_driver.h | 6 +++---
> src/security/security_manager.h | 6 +++---
> src/security/security_nop.h | 6 +++---
> src/security/security_selinux.h | 6 +++---
> src/security/security_stack.h| 6 +++---
> 7 files changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/src/security/security_apparmor.h
> b/src/security/security_apparmor.h
> index 7872588f64..6b454d1b5c 100644
> --- a/src/security/security_apparmor.h
> +++ b/src/security/security_apparmor.h
> @@ -19,8 +19,8 @@
> * Jamie Strandboge
> *
> */
> -#ifndef __VIR_SECURITY_APPARMOR_H__
> -# define __VIR_SECURITY_APPARMOR_H__
> +#ifndef __SECURITY_APPARMOR_H__
> +# define __SECURITY_APPARMOR_H__
>
> # include "security_driver.h"
>
> @@ -30,4 +30,4 @@ extern virSecurityDriver virAppArmorSecurityDriver;
> # define PROFILE_NAME_SIZE 8 + VIR_UUID_STRING_BUFLEN /* AA_PREFIX + uuid */
> # define MAX_FILE_LEN (1024*1024*10) /* 10MB limit for sanity check */
>
> -#endif /* __VIR_SECURITY_APPARMOR_H__ */
> +#endif /* __SECURITY_APPARMOR_H__ */
> diff --git a/src/security/security_dac.h b/src/security/security_dac.h
> index 97681c9610..8007bde000 100644
> --- a/src/security/security_dac.h
> +++ b/src/security/security_dac.h
> @@ -20,8 +20,8 @@
>
> #include "security_driver.h"
>
> -#ifndef __VIR_SECURITY_DAC
> -# define __VIR_SECURITY_DAC
> +#ifndef __SECURITY_DAC__
> +# define __SECURITY_DAC__
>
> extern virSecurityDriver virSecurityDriverDAC;
>
> @@ -38,4 +38,4 @@ void virSecurityDACSetMountNamespace(virSecurityManagerPtr
> mgr,
> void virSecurityDACSetChownCallback(virSecurityManagerPtr mgr,
> virSecurityManagerDACChownCallback
> chownCallback);
>
> -#endif /* __VIR_SECURITY_DAC */
> +#endif /* __SECURITY_DAC__ */
> diff --git a/src/security/security_driver.h b/src/security/security_driver.h
> index cd221f1c78..25d49bb0f4 100644
> --- a/src/security/security_driver.h
> +++ b/src/security/security_driver.h
> @@ -19,8 +19,8 @@
> * James Morris
> *
> */
> -#ifndef __VIR_SECURITY_H__
> -# define __VIR_SECURITY_H__
> +#ifndef __SECURITY_DRIVER_H__
> +# define __SECURITY_DRIVER_H__
>
> # include "internal.h"
> # include "domain_conf.h"
> @@ -226,4 +226,4 @@ struct _virSecurityDriver {
> virSecurityDriverPtr virSecurityDriverLookup(const char *name,
> const char *virtDriver);
>
> -#endif /* __VIR_SECURITY_H__ */
> +#endif /* __SECURITY_DRIVER_H__ */
> diff --git a/src/security/security_manager.h b/src/security/security_manager.h
> index 7e82304689..139b70ec10 100644
> --- a/src/security/security_manager.h
> +++ b/src/security/security_manager.h
> @@ -20,8 +20,8 @@
> * Author: Daniel P. Berrange
> */
>
> -#ifndef VIR_SECURITY_MANAGER_H__
> -# define VIR_SECURITY_MANAGER_H__
> +#ifndef __SECURITY_MANAGER_H__
> +# define __SECURITY_MANAGER_H__
>
> # include "domain_conf.h"
> # include "vircommand.h"
> @@ -210,4 +210,4 @@ void
> virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr,
> virSecurityManagerMetadataLockStatePtr
> *state);
>
> -#endif /* VIR_SECURITY_MANAGER_H__ */
> +#endif /* __SECURITY_MANAGER_H__ */
> diff --git a/src/security/security_nop.h b/src/security/security_nop.h
> index 514b339467..7b2ded2292 100644
> --- a/src/security/security_nop.h
> +++ b/src/security/security_nop.h
> @@ -17,11 +17,11 @@
> *
> */
>
> -#ifndef __VIR_SECURITY_NOP_H__
> -# define __VIR_SECURITY_NOP_H__
> +#ifndef __SECURITY_NOP_H__
> +# define __SECURITY_NOP_H__
>
> # include "security_driver.h"
>
> extern virSecurityDriver virSecurityDriverNop;
>
> -#endif /* __VIR_SECURITY_NOP_H__ */
> +#endif /* __SECURITY_NOP_H__ */
> diff --git a/src/security/security_selinux.h b/src/security/security_selinux.h
> index 1700d8c661..11b62acb52 100644
> --- a/src/security/security_selinux.h
> +++ b/src/security/security_selinux.h
> @@ -19,9 +19,9 @@
> * James Morris
> *
> */
> -#ifndef __VIR_SECURITY_SELINUX_H__
> -# define __VIR_SECURITY_SELINUX_H__
> +#ifndef
Re: [libvirt] [PATCH 2/2] lxc: check actual type of interface not config type
On Wed, Dec 05, 2018 at 09:35:13PM -0500, Laine Stump wrote: > virLXCControllerGetNICIndexes() was deciding whether or not to add the > ifindex for an interface's ifname to the list of ifindexes sent to > CreateMachineWithNetwork based on the interface type stored in the > config. This would be incorrect in the case of type='network'> where the network was giving out macvlan interfaces > tied to a physical device (i.e. when the actual interface type was > "direct"). > > Instead of checking the setting of "net->type", we should be checking > the setting of virDomainNetGetActualType(net). > > I don't think this caused any actual misbehavior, it was just > technically wrong. > > Signed-off-by: Laine Stump > --- > src/lxc/lxc_controller.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] lxc: stop incorrectly validating interface type
On Wed, Dec 05, 2018 at 09:35:12PM -0500, Laine Stump wrote:
> Commit 017dfa27d changed a few switch statements in the LXC code to
> have all possible enum values, and in the process changed the switch
> statement in virLXCControllerGetNICIndexes() such that it returned
> error status for any interfaces that weren't implemented with a veth
> pair when it should have just been ignoring those interfaces.
>
> Since the interface type will have already been validated before
> reaching this function, we shouldn't be doing any validation at all -
> just add the ifindex of the parent veth if its a veth pair, and ignore
> it otherwise.
>
> Resolves: https://bugzilla.redhat.com/1656463
> Signed-off-by: Laine Stump
> ---
> src/lxc/lxc_controller.c | 17 +++--
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> index e853d02d65..1c20f451af 100644
> --- a/src/lxc/lxc_controller.c
> +++ b/src/lxc/lxc_controller.c
> @@ -364,6 +364,16 @@ static int
> virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
> size_t i;
> int ret = -1;
>
> +/* Gather the ifindexes of the "parent" veths for all interfaces
> + * implemented with a veth pair. These will be used when calling
> + * virCgroupNewMachine (and eventually the dbus method
> + * CreateMachineWithNetwork). ifindexes for the child veths, and
> + * for macvlan interfaces, *should not* be in this list, as they
> + * will be moved into the container. Only the interfaces that will
> + * remain outside the container, but are used for communication
> + * with the container, should be added to the list.
> + */
> +
> VIR_DEBUG("Getting nic indexes");
> for (i = 0; i < ctrl->def->nnets; i++) {
> int nicindex = -1;
> @@ -394,14 +404,9 @@ static int
> virLXCControllerGetNICIndexes(virLXCControllerPtr ctrl)
> case VIR_DOMAIN_NET_TYPE_INTERNAL:
> case VIR_DOMAIN_NET_TYPE_DIRECT:
> case VIR_DOMAIN_NET_TYPE_HOSTDEV:
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("Unsupported net type %s"),
> -
> virDomainNetTypeToString(ctrl->def->nets[i]->type));
> -goto cleanup;
> case VIR_DOMAIN_NET_TYPE_LAST:
> default:
> -virReportEnumRangeError(virDomainNetType,
> ctrl->def->nets[i]->type);
> -goto cleanup;
> + break;
> }
This is removing too much. Only ethernet, bridge, network & direct are going
to work with LXC. All others should always result in an error message here,
and absolutely the LAST/default case must always report enum error.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/10] util: error: Improve docs for virErrorMsg
On Wed, Dec 05, 2018 at 05:47:47PM +0100, Peter Krempa wrote: > Clarify how @info is used and how the returned values look like. s/how/what ...on a side note, there's no such thing as "how it looks *like*", only "how it looks". The difference between "what it looks like" and "how it looks" seems to be quite debatable, some people say the meaning is the same while some say there's a slight difference. Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/10] tests: Add test for virErrorMsg message constraints
On Wed, Dec 05, 2018 at 05:47:46PM +0100, Peter Krempa wrote: > Make sure that we don't add any broken error message strings any more. > > This ensures that both the version with and without additional info is > populated, the version without info does not have any formatting > modifiers and the version with info has exactly one. > > Signed-off-by: Peter Krempa Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/10] util: error: Export virErrorMsg for use in testsuite
On Wed, Dec 05, 2018 at 05:47:45PM +0100, Peter Krempa wrote: > Signed-off-by: Peter Krempa > --- > src/Makefile.am | 1 + > src/libvirt_private.syms | 1 + > src/util/Makefile.inc.am | 1 + > src/util/virerror.c | 5 - > src/util/virerrorpriv.h | 28 > 5 files changed, 35 insertions(+), 1 deletion(-) > create mode 100644 src/util/virerrorpriv.h > > diff --git a/src/Makefile.am b/src/Makefile.am > index 33ff280d78..0bb3ff8fc8 100644 > --- a/src/Makefile.am > +++ b/src/Makefile.am > @@ -951,6 +951,7 @@ libvirt_nss_la_SOURCES = \ > util/vircommand.h \ > util/virerror.c \ > util/virerror.h \ > + util/virerrorpriv.h \ Unrelated to the commit message and compiles fine without it. > util/virfile.c \ > util/virfile.h \ > util/virhash.c \ > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index fd63c9ca61..6184030d59 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -1752,6 +1752,7 @@ ebtablesRemoveForwardAllowIn; > virDispatchError; > virErrorCopyNew; > virErrorInitialize; > +virErrorMsg; > virErrorPreserveLast; > virErrorRestore; > virErrorSetErrnoFromLastError; > diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am > index cffbb357bc..4295babac3 100644 > --- a/src/util/Makefile.inc.am > +++ b/src/util/Makefile.inc.am > @@ -50,6 +50,7 @@ UTIL_SOURCES = \ > util/virendian.h \ > util/virerror.c \ > util/virerror.h \ > + util/virerrorpriv.h \ > util/virevent.c \ > util/virevent.h \ > util/vireventpoll.c \ > diff --git a/src/util/virerror.c b/src/util/virerror.c > index 64be00dc75..be23712a60 100644 > --- a/src/util/virerror.c > +++ b/src/util/virerror.c > @@ -31,6 +31,9 @@ > #include "virutil.h" > #include "virstring.h" > > +#define __VIR_ERROR_ALLOW_INCLUDE_PRIV_H__ > +#include "virerrorpriv.h" >From consistency POV, you also want: #undefine __VIR_ERROR_ALLOW_INCLUDE_PRIV_H__ With the proposed adjustments: Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/10] util: error: Add error message versions with info for some error codes
On Wed, Dec 05, 2018 at 05:47:44PM +0100, Peter Krempa wrote: > Few error codes were missing the version of the message with additional s/lacking a/missing the > info. In case of the modified messages it's not very likely they'll ever > report any additional data, but for the sake of consistency we'll > provide them. s/we'll/we should Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 02/10] util: error: Fix error message strings to play well with additional info
On Thu, Dec 06, 2018 at 08:56:59AM +0100, Erik Skultety wrote: > On Wed, Dec 05, 2018 at 05:47:43PM +0100, Peter Krempa wrote: > > Additional information for a string is always in form of a string or s/for a string/for an error message s/always/either > > empty. Fix two offenders. One used %d as format modifier and second s/as /as the s/ second/the second one > > always expected a string. Both are thankfully unused currently. Doesn't sound 100% right...how about: "Thankfully, neither of the offenders are currently in effect." > > > > Signed-off-by: Peter Krempa > > --- > Reviewed-by: Erik Skultety ^^This still stands though, it's just the commit message activated the "grammar " in me for some reason :P, sorry. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
