[libvirt] [PATCH 1/2] util: Fix VIR_ERR_ACCESS_DENIED formatting
In commit ccc72d5cb I thought I had the brilliant idea that I
could alter an error message in virErrorMsg to take two arguments
"drivername" and NULL. Then passing the failed drivername for
VIR_ERR_ACCESS_DENIED users would magically allow any "real"
error message to be included. Of course it worked for the
majority of errors, except for 1 teeny, tiny, problem - there
was a caller using VIR_ERR_ACCESS_DENIED that actually passed
"real" error message - virAccessDriverPolkitGetCaller, but
didn't pass a driver name. So when this code went to call
virErrorMsg it resulted in garbage being printed in that
second argument because that's not how things are written.
So in order to avoid that issue, reset the error back to the
usual if info == NULL, the print just some text; otherwise,
print a formatted output. For a majority of error messages
this will print the driver name after "access deined from:".
For the one case in question, the correct error message will
be displayed and no garbage chars.
Signed-off-by: John Ferlan
---
src/util/virerror.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virerror.c b/src/util/virerror.c
index 10f1b55c5f..df239bf371 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c
@@ -1442,9 +1442,9 @@ virErrorMsg(virErrorNumber error, const char *info)
break;
case VIR_ERR_ACCESS_DENIED:
if (info == NULL)
-errmsg = _("access denied from '%s'");
+errmsg = _("access denied");
else
-errmsg = _("access denied from '%s': %s");
+errmsg = _("access denied from: %s");
break;
case VIR_ERR_DBUS_SERVICE:
if (info == NULL)
--
2.17.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] Fix some errors around VIR_ACCESS_DENIED
Patch 1 fixes my own error made in this release fortunately, but only seen because I was invesigating Patch 2 Patch 2 is perhaps a longer term issue, but perhaps coming more to light now that the nwfilter bindings have been separated and use a virConnectOpen for nwfilter:///system during reconnection processing; whereas, previously the filter bindings would have been "hidden" within various nwfilter, domain, and network driver callbacks and throughs. John Ferlan (2): util: Fix VIR_ERR_ACCESS_DENIED formatting qemu: Set identity for the reconnect all thread src/qemu/qemu_process.c | 5 + src/util/virerror.c | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) -- 2.17.2 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] qemu: Set identity for the reconnect all thread
https://bugzilla.redhat.com/show_bug.cgi?id=1631622
If polkit authentication is enabled, an attempt to open
the connection failed during virAccessDriverPolkitGetCaller
when the call to virIdentityGetCurrent returned NULL resulting
in the errors:
virAccessDriverPolkitGetCaller:87 : access denied from:
Policy kit denied action org.libvirt.api.connect.getattr from
virAccessManagerSanitizeError:204 : access denied from: nwfilter
Because qemuProcessReconnect runs in a thread during
daemonRunStateInit processing it doesn't have the thread
local identity. Thus when the virGetConnectNWFilter is
called as part of the qemuProcessFiltersInstantiate when
virDomainConfNWFilterInstantiate is run the attempt to get
the idenity fails and results in the anonymous error above.
To fix this, let's grab/use the virIdenityPtr of the process
that will be creating the thread, e.g. what daemonRunStateInit
has set and use that for our thread. That way any other similar
processing that uses/requires an identity for any other call
that would have previously been successfully run won't fail in
a similar manner.
Signed-off-by: John Ferlan
---
src/qemu/qemu_process.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 06a65b44e4..93f6a2279a 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -81,6 +81,7 @@
#include "netdev_bandwidth_conf.h"
#include "virresctrl.h"
#include "virvsock.h"
+#include "viridentity.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -7716,6 +7717,7 @@ qemuProcessRefreshCPU(virQEMUDriverPtr driver,
struct qemuProcessReconnectData {
virQEMUDriverPtr driver;
virDomainObjPtr obj;
+virIdentityPtr identity;
};
/*
* Open an existing VM's monitor, re-detect VCPU threads
@@ -7753,6 +7755,7 @@ qemuProcessReconnect(void *opaque)
bool retry = true;
bool tryMonReconn = false;
+virIdentitySetCurrent(data->identity);
VIR_FREE(data);
qemuDomainObjRestoreJob(obj, );
@@ -7979,6 +7982,7 @@ qemuProcessReconnect(void *opaque)
virObjectUnref(cfg);
virObjectUnref(caps);
virNWFilterUnlockFilterUpdates();
+virIdentitySetCurrent(NULL);
return;
error:
@@ -8022,6 +8026,7 @@ qemuProcessReconnectHelper(virDomainObjPtr obj,
memcpy(data, src, sizeof(*data));
data->obj = obj;
+data->identity = virIdentityGetCurrent();
virNWFilterReadLockFilterUpdates();
--
2.17.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/5] snapshot: Implement reverting for external disk snapshots
Hi Peter,
Many thanks for the reviews. I replied inline below.
On 07/11/2018 17:40, Peter Krempa wrote:
> On Sun, Oct 21, 2018 at 19:38:48 +0300, Povilas Kanapickas wrote:
> [...]
>> + int* out_mapping)
>> +{
>> +size_t i, j;
>> +int found_idx;
>> +virDomainSnapshotDiskDefPtr snap_disk;
>> +
>> +for (i = 0; i < snap_def->ndisks; ++i) {
>> +snap_disk = &(snap_def->disks[i]);
>> +if (snap_disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
>> +continue;
>> +
>> +found_idx = -1;
>> +for (j = 0; j < dom_def->ndisks; ++j) {
>> +if (STREQ(dom_def->disks[j]->dst, snap_disk->name)) {
>> +found_idx = j;
>> +break;
>> +}
>> +}
>> +out_mapping[i] = found_idx;
>> +}
>> +}
>> +
>> +/* This function reverts an external snapshot disk state. With external
>> disks
>> + we can't just revert to the disks listed in the domain stored within the
>> + snapshot, because it's read-only and might be used by other VMs in
>> different
>> + backing chains. Since the contents of the current disks will be discarded
>> + in favor of data in the snapshot, we reuse them by resetting them and
>> + pointing the backing image link to the disks listed within the snapshot.
>> +
>> + The domain is expected to be inactive.
>> +
>> + new_def is the new domain definition that will be stored to vm sometime
>> in
>> + the future.
>> + */
>> +static int
>> +qemuDomainSnapshotRevertInactiveExternal(virQEMUDriverPtr driver,
>> + virDomainObjPtr vm,
>> + virDomainSnapshotObjPtr snap,
>> + virDomainDefPtr new_def)
>> +{
>> +/* We have the following disks recorded in the snapshot and the current
>> + domain definition:
>> +
>> +- the current disk state before the revert (vm->def->disks). We'll
>> + discard and reuse them.
>
> So this has multiple problems:
>
> 1) You can have a chain of snapshots and revert in middle of that:
> - this will destroy any snapshots which depend on the one you are
> reverting to and that is not acceptable
The intention was to drop just the most current disk, the one that does
not have a snapshot yet. Then we can create another file and point its
backing image to the disk storing the snapshot that we want to revert
to. As far as I understand, this does not affect any later snapshots in
the chain.
> 2) The snapshot can have a different mapping of disks. When reverting
> the old topology should be kept as it existed at the time when the
> snapshot was created.
Agreed. But what would be the best outcome in situations when a snapshot
does not include a certain disk? My thinking is that if user does not
include a disk into a snapshot then he doesn't care about the change of
its contents since the last snapshot. Thus we could iterate the parent
snapshots until we find one that includes that disk and then revert the
state to that disk. If neither the snapshot or its parents include that
disk then we could probably leave the current disk state unchanged.
> 3) you can't return to the state that you are leaving (might be
> desirable but needs to be opt-in)
>
> This basically means that we need a new reversion API which will allow
> to take an XML and will basically fork the snapshot into an alternate
> history and also will mark the state you are leaving so that we can
> return to that state.
>
> Reverting to that "new" state will result into just swithcing the
> definitions and continuing to use the images and make changes. (Which
> should be done for any leaf snapshot in the snapshot tree).
>
> Deleting the state as you do is IMO not acceptable.
I think we could simplify that a lot if we accepted that revert is by
definition a lossy operation and any changes since last snapshot would
be lost.
Otherwise storing the previous state would just complicate things:
consider the situation where we are repeatedly reverting back and forth
between two snapshots. Either we will need to save all states between
the reverts and implement ability to store potentially more than one
previous state as a child of any snapshot. Or we will need to accept
that at some point data loss is acceptable and, say, limit the number of
state children of each snapshot to one.
I suggest that if we accepted snapshot operation as lossy, then your use
case could be implemented by a pair of and operations. Bonus would be that such snapshots
pointing to previous states could be manipulated by existing APIs, so
tools won't need to be updated and so on.
The main problem I see currently is that user needs to somehow specify
new paths for all disks in the snapshot he wants to revert to. All paths
listed in the snapshot currently may be part of other snapshot chains.
I've written a bit about this issue and
[libvirt] [PATCH v3] lxc: Include support to lxc version 3.0 or higher.
This patch introduce the new settings for LXC 3.0 or higher. The older
versions keep the compatibility to deprecated settings for LXC, but
after release 3.0, the compatibility was removed. This commit adds the
support to the refactored settings.
v1-v2: Michal's suggestions to handle differences between versions.
v2-v3: Adding suggestions from Pino and John too.
Signed-off-by: Julio Faracco
---
src/lxc/lxc_native.c | 45 +++-
1 file changed, 32 insertions(+), 13 deletions(-)
diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
index cbdec723dd..d3ba12bb0e 100644
--- a/src/lxc/lxc_native.c
+++ b/src/lxc/lxc_native.c
@@ -200,8 +200,13 @@ lxcSetRootfs(virDomainDefPtr def,
int type = VIR_DOMAIN_FS_TYPE_MOUNT;
VIR_AUTOFREE(char *) value = NULL;
-if (virConfGetValueString(properties, "lxc.rootfs", ) <= 0)
-return -1;
+if (virConfGetValueString(properties, "lxc.rootfs.path", ) <= 0) {
+virResetLastError();
+
+/* Check for pre LXC 3.0 legacy key */
+if (virConfGetValueString(properties, "lxc.rootfs", ) <= 0)
+return -1;
+}
if (STRPREFIX(value, "/dev/"))
type = VIR_DOMAIN_FS_TYPE_BLOCK;
@@ -684,8 +689,13 @@ lxcCreateConsoles(virDomainDefPtr def, virConfPtr
properties)
virDomainChrDefPtr console;
size_t i;
-if (virConfGetValueString(properties, "lxc.tty", ) <= 0)
-return 0;
+if (virConfGetValueString(properties, "lxc.tty.max", ) <= 0) {
+virResetLastError();
+
+/* Check for pre LXC 3.0 legacy key */
+if (virConfGetValueString(properties, "lxc.tty", ) <= 0)
+return 0;
+}
if (virStrToLong_i(value, NULL, 10, ) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse int: '%s'"),
@@ -724,12 +734,13 @@ lxcIdmapWalkCallback(const char *name, virConfValuePtr
value, void *data)
char type;
unsigned long start, target, count;
-if (STRNEQ(name, "lxc.id_map") || !value->str)
+/* LXC 3.0 uses "lxc.idmap", while legacy used "lxc.id_map" */
+if (STRNEQ(name, "lxc.idmap") || STRNEQ(name, "lxc.id_map") || !value->str)
return 0;
if (sscanf(value->str, "%c %lu %lu %lu", ,
, , ) != 4) {
-virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid lxc.id_map: '%s'"),
+virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid: '%s'"),
value->str);
return -1;
}
@@ -1041,19 +1052,27 @@ lxcParseConfigString(const char *config,
if (VIR_STRDUP(vmdef->os.init, "/sbin/init") < 0)
goto error;
-if (virConfGetValueString(properties, "lxc.utsname", ) <= 0 ||
-VIR_STRDUP(vmdef->name, value) < 0)
-goto error;
+if (virConfGetValueString(properties, "lxc.uts.name", ) <= 0) {
+virResetLastError();
+
+/* Check for pre LXC 3.0 legacy key */
+if (virConfGetValueString(properties, "lxc.utsname", ) <= 0)
+goto error;
+}
+
if (!vmdef->name && (VIR_STRDUP(vmdef->name, "unnamed") < 0))
goto error;
if (lxcSetRootfs(vmdef, properties) < 0)
goto error;
-/* Look for fstab: we shouldn't have it */
-if (virConfGetValue(properties, "lxc.mount")) {
+/* LXC 3.0 uses "lxc.mount.fstab", while legacy used just "lxc.mount".
+ * In either case, generate the error to use "lxc.mount.entry" instead */
+if (virConfGetValue(properties, "lxc.mount.fstab") ||
+virConfGetValue(properties, "lxc.mount")) {
virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("lxc.mount found, use lxc.mount.entry lines
instead"));
+ _("lxc.mount.fstab or lxc.mount found, use "
+ "lxc.mount.entry lines instead"));
goto error;
}
@@ -1069,7 +1088,7 @@ lxcParseConfigString(const char *config,
if (lxcCreateConsoles(vmdef, properties) < 0)
goto error;
-/* lxc.id_map */
+/* lxc.idmap or legacy lxc.id_map */
if (virConfWalk(properties, lxcIdmapWalkCallback, vmdef) < 0)
goto error;
--
2.19.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 00/37] BaselineHypervisorCPU using QEMU QMP exchanges
Hi Chris, On 11/9/18 11:27 AM, Chris Venteicher wrote: > Quoting Chris Venteicher (2018-11-02 22:13:01) >> Some architectures (S390) depend on QEMU to compute baseline CPU model and >> expand a models feature set. >> >> Interacting with QEMU requires starting the QEMU process and completing one >> or >> more query-cpu-model-baseline QMP exchanges with QEMU in addition to a >> query-cpu-model-expansion QMP exchange to expand all features in the model. >> >> See "s390x CPU models: exposing features" patch set on Qemu-devel for >> discussion >> of QEMU aspects. >> >> This is part of resolution of: >> https://bugzilla.redhat.com/show_bug.cgi?id=1511999 >> >> Version 4 attempts to address all issues from V1,2,3 including making the >> QEMU process activation for QMP Queries generic (not specific to >> capabilities) >> and moving that code from qemu_capabilities to qemu_process. >> >> Version 4 greatly expands the number of patches to make the set easier >> to review. >> >> The patches to do hypervisor baseline using QEMU are primarily in >> qemu_driver.c >> > Patches 1-2 create the cpumodel to/from Json conversion code. > Patches 3-4 modify the cpu expansion interface. > Patches 28-36 are the actual baseline changes. > >> The patches to make the process code generic (not capabilities specific) >> are mostly in qemu_process.c > > Patches 5 -> 27 move the process code from qemu_capabilities to > qemu_process. > > A lot of these are code move or rename patches so the patches with real > implementation changes are easy to identify. > > I might have ended up with too many patches though. > Want to mention... the whole "process" block could be moved to it's own > series if that would be easier to review. I've been meaning to provide an in-depth review of these patches, but other things have been holding me up -- my apologies. At a quick glance, a lot of your patches involve a few short patches that don't necessarily provide much context on their own. A lot of patches in this series can definitely be merged. I think moving the "qemu process" code into its own series would be helpful. I would include something in the cover that they will benefit the hypervisor CPU baseline and comparison patches. If you feel confident with your qemu process patches, then you could also send your baseline patch series at the same time, and state that your baseline patches rely on your QEMU process patches (make sure to include a mailing list archive link in that header so reviewers can easily reference the other patch series). Also, make sure you CC the authors and contributors of the respective files that you touch. Most of them are listed at the top of the file. (I think Jiri might be interested in this series as well.) So let's start there. Repost your qemu_process code as it's own series, and I'd recommend tagging it with "RFC" (Request For Comments) instead of giving it a version number. This will prompt reviewers that you're mainly looking for areas of improvement and some guidance. > >> Many of the patches are cut / paste of existing code. The patches that >> change functionality are as modular and minimal as possible to make >> reviewing easier. >> >> I attempted to make the new qemu_process functions >> consistent with the existing domain activation qemu_process functions. >> >> A thread safe library function creates a unique directory under libDir for >> each QEMU >> process (for QMP messaging) to decouple processes in terms of sockets and >> file system footprint. >> >> The last patch is based on past discussion of QEMU cpu >> expansion only returning migratable features except for one x86 case where >> non-migratable features are explicitly requested. The patch records that >> features >> are migratable based on QEMU only returning migratable features. The main >> result >> is the CPU xml for S390 CPU's contains the same migratability info the X86 >> currently >> does. The testcases were updated to reflect the change. >> >> Every patch should compile independently if applied in sequence. >> >> Thanks, >> Chris >> >> >> Chris Venteicher (37): >> qemu_monitor: Introduce qemuMonitorCPUModelInfoNew >> qemu_monitor: Introduce qemuMonitorCPUModelInfo / JSON conversion >> qemu_capabilities: Introduce virQEMuCapsMigratablePropsDiff >> qemu_monitor: qemuMonitorGetCPUModelExpansion inputs and outputs >> CPUModelInfo >> qemu_process: Move process code from qemu_capabilities to qemu_process >> qemu_process: Use qemuProcess prefix >> qemu_process: Limit qemuProcessNew to const input strings >> qemu_process: Refer to proc not cmd in process code >> qemu_process: Use consistent name for stop process function >> qemu_capabilities: Stop QEMU process before freeing >> qemu_process: Use qemuProcess struct for a single process >> qemu_process: Persist stderr in qemuProcess struct >> qemu_capabilities: Detect caps probe failure by checking monitor ptr >> qemu_process:
Re: [libvirt] [PATCH v4 00/37] BaselineHypervisorCPU using QEMU QMP exchanges
Quoting Chris Venteicher (2018-11-02 22:13:01) > Some architectures (S390) depend on QEMU to compute baseline CPU model and > expand a models feature set. > > Interacting with QEMU requires starting the QEMU process and completing one or > more query-cpu-model-baseline QMP exchanges with QEMU in addition to a > query-cpu-model-expansion QMP exchange to expand all features in the model. > > See "s390x CPU models: exposing features" patch set on Qemu-devel for > discussion > of QEMU aspects. > > This is part of resolution of: > https://bugzilla.redhat.com/show_bug.cgi?id=1511999 > > Version 4 attempts to address all issues from V1,2,3 including making the > QEMU process activation for QMP Queries generic (not specific to capabilities) > and moving that code from qemu_capabilities to qemu_process. > > Version 4 greatly expands the number of patches to make the set easier > to review. > > The patches to do hypervisor baseline using QEMU are primarily in > qemu_driver.c > Patches 1-2 create the cpumodel to/from Json conversion code. Patches 3-4 modify the cpu expansion interface. Patches 28-36 are the actual baseline changes. > The patches to make the process code generic (not capabilities specific) > are mostly in qemu_process.c Patches 5 -> 27 move the process code from qemu_capabilities to qemu_process. A lot of these are code move or rename patches so the patches with real implementation changes are easy to identify. I might have ended up with too many patches though. Want to mention... the whole "process" block could be moved to it's own series if that would be easier to review. > Many of the patches are cut / paste of existing code. The patches that > change functionality are as modular and minimal as possible to make > reviewing easier. > > I attempted to make the new qemu_process functions > consistent with the existing domain activation qemu_process functions. > > A thread safe library function creates a unique directory under libDir for > each QEMU > process (for QMP messaging) to decouple processes in terms of sockets and > file system footprint. > > The last patch is based on past discussion of QEMU cpu > expansion only returning migratable features except for one x86 case where > non-migratable features are explicitly requested. The patch records that > features > are migratable based on QEMU only returning migratable features. The main > result > is the CPU xml for S390 CPU's contains the same migratability info the X86 > currently > does. The testcases were updated to reflect the change. > > Every patch should compile independently if applied in sequence. > > Thanks, > Chris > > > Chris Venteicher (37): > qemu_monitor: Introduce qemuMonitorCPUModelInfoNew > qemu_monitor: Introduce qemuMonitorCPUModelInfo / JSON conversion > qemu_capabilities: Introduce virQEMuCapsMigratablePropsDiff > qemu_monitor: qemuMonitorGetCPUModelExpansion inputs and outputs > CPUModelInfo > qemu_process: Move process code from qemu_capabilities to qemu_process > qemu_process: Use qemuProcess prefix > qemu_process: Limit qemuProcessNew to const input strings > qemu_process: Refer to proc not cmd in process code > qemu_process: Use consistent name for stop process function > qemu_capabilities: Stop QEMU process before freeing > qemu_process: Use qemuProcess struct for a single process > qemu_process: Persist stderr in qemuProcess struct > qemu_capabilities: Detect caps probe failure by checking monitor ptr > qemu_process: Introduce qemuProcessStartQmp > qemu_process: Add debug message in qemuProcessLaunchQmp > qemu_process: Collect monitor code in single function > qemu_process: Store libDir in qemuProcess struct > qemu_process: Setup paths within qemuProcessInitQmp > qemu_process: Stop retaining Qemu Monitor config in qemuProcess > qemu_process: Don't open monitor if process failed > qemu_process: Cleanup qemuProcess alloc function > qemu_process: Cleanup qemuProcessStopQmp function > qemu_process: Catch process free before process stop > qemu_monitor: Make monitor callbacks optional > qemu_process: Enter QMP command mode when starting QEMU Process > qemu_process: Use unique directories for QMP processes > qemu_process: Stop locking QMP process monitor immediately > qemu_capabilities: Introduce CPUModelInfo to virCPUDef function > qemu_capabilities: Introduce virCPUDef to CPUModelInfo function > qemu_monitor: Support query-cpu-model-baseline QMP command > qemu_driver: Consolidate code to baseline using libvirt > qemu_driver: Decouple code for baseline using libvirt > qemu_driver: Identify using libvirt as a distinct way to compute > baseline > qemu_driver: Support baseline calculation using QEMU > qemu_driver: Support feature expansion via QEMU when baselining cpu > qemu_driver: Remove unsupported props in expanded hypervisor baseline > output > qemu_monitor: Default props to migratable when expanding cpu model
Re: [libvirt] [PATCH 0/2] qemu: Drop priv->gotShutdown
On 11/7/18 9:37 AM, Jiri Denemark wrote: > Jiri Denemark (2): > qemu: Drop unreachable code from qemuProcessHandleStop > qemu: Drop priv->gotShutdown > > src/qemu/qemu_domain.h | 1 - > src/qemu/qemu_driver.c | 3 ++- > src/qemu/qemu_process.c | 13 + > 3 files changed, 3 insertions(+), 14 deletions(-) > Reviewed-by: John Ferlan (series) John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 04/13] security_manager: Rework metadata locking
On 11/9/18 7:42 AM, Michal Privoznik wrote:
> On 11/09/2018 03:59 AM, John Ferlan wrote:
>>
>>
>> On 10/17/18 5:06 AM, Michal Privoznik wrote:
>>> Trying to use virlockd to lock metadata turns out to be too big
>>> gun. Since we will always spawn a separate process for relabeling
>>> we are safe to use thread unsafe POSIX locks and take out
>>> virtlockd completely out of the picture.
>>>
>>> Signed-off-by: Michal Privoznik
>>> ---
>>> src/security/security_dac.c | 10 +-
>>> src/security/security_manager.c | 225 +---
>>> src/security/security_manager.h | 17 ++-
>>> src/security/security_selinux.c | 9 +-
>>> 4 files changed, 139 insertions(+), 122 deletions(-)
>>>
>>> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
>>> index 580d800bb1..ad2561a241 100644
>>> --- a/src/security/security_dac.c
>>> +++ b/src/security/security_dac.c
>>> @@ -204,6 +204,7 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
>>> void *opaque)
>>> {
>>> virSecurityDACChownListPtr list = opaque;
>>> +virSecurityManagerMetadataLockStatePtr state;
>>> const char **paths = NULL;
>>> size_t npaths = 0;
>>> size_t i;
>>> @@ -216,14 +217,10 @@ virSecurityDACTransactionRun(pid_t pid
>>> ATTRIBUTE_UNUSED,
>>> for (i = 0; i < list->nItems; i++) {
>>> const char *p = list->items[i]->path;
>>>
>>> -if (!p ||
>>> -virFileIsDir(p))
>>> -continue;
>>> -
>>> VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p);
>>> }
>>>
>>> -if (virSecurityManagerMetadataLock(list->manager, paths, npaths) < 0)
>>> +if (!(state = virSecurityManagerMetadataLock(list->manager, paths,
>>> npaths)))
>>> goto cleanup;
>>>
>>> for (i = 0; i < list->nItems; i++) {
>>> @@ -246,8 +243,7 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
>>> break;
>>> }
>>>
>>> -if (virSecurityManagerMetadataUnlock(list->manager, paths, npaths) < 0)
>>> -goto cleanup;
>>> +virSecurityManagerMetadataUnlock(list->manager, );
>>>
>>> if (rv < 0)
>>> goto cleanup;
>>> diff --git a/src/security/security_manager.c
>>> b/src/security/security_manager.c
>>> index c6c80e6165..b675f8ab46 100644
>>> --- a/src/security/security_manager.c
>>> +++ b/src/security/security_manager.c
>>> @@ -21,6 +21,10 @@
>>> */
>>> #include
>>>
>>> +#include
>>> +#include
>>> +#include
>>> +
>>> #include "security_driver.h"
>>> #include "security_stack.h"
>>> #include "security_dac.h"
>>> @@ -30,14 +34,11 @@
>>> #include "virlog.h"
>>> #include "locking/lock_manager.h"
>>> #include "virfile.h"
>>> -#include "virtime.h"
>>>
>>> #define VIR_FROM_THIS VIR_FROM_SECURITY
>>>
>>> VIR_LOG_INIT("security.security_manager");
>>>
>>> -virMutex lockManagerMutex = VIR_MUTEX_INITIALIZER;
>>> -
>>> struct _virSecurityManager {
>>> virObjectLockable parent;
>>>
>>> @@ -47,10 +48,6 @@ struct _virSecurityManager {
>>> void *privateData;
>>>
>>> virLockManagerPluginPtr lockPlugin;
>>> -/* This is a FD that represents a connection to virtlockd so
>>> - * that connection is kept open in between MetdataLock() and
>>> - * MetadataUnlock() calls. */
>>> -int clientfd;
>>> };
>>>
>>> static virClassPtr virSecurityManagerClass;
>>> @@ -66,7 +63,6 @@ void virSecurityManagerDispose(void *obj)
>>> mgr->drv->close(mgr);
>>>
>>> virObjectUnref(mgr->lockPlugin);
>>> -VIR_FORCE_CLOSE(mgr->clientfd);
>>>
>>> VIR_FREE(mgr->privateData);
>>> }
>>> @@ -119,7 +115,6 @@ virSecurityManagerNewDriver(virSecurityDriverPtr drv,
>>> mgr->flags = flags;
>>> mgr->virtDriver = virtDriver;
>>> VIR_STEAL_PTR(mgr->privateData, privateData);
>>> -mgr->clientfd = -1;
>>>
>>> if (drv->open(mgr) < 0)
>>> goto error;
>>> @@ -1276,129 +1271,153 @@
>>> virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr,
>>> }
>>>
>>>
>>> -static virLockManagerPtr
>>> -virSecurityManagerNewLockManager(virSecurityManagerPtr mgr,
>>> - const char * const *paths,
>>> - size_t npaths)
>>> +struct _virSecurityManagerMetadataLockState {
>>
>> LockPrivate?
>>
>> When I first saw State I had a different thought.
>>
>> I like this better than the previous model... Keeping track of fds is
>> like other models used. How do these locks handle restarts? Are the
>> locks free'd if the daemon dies?
>
> Yes they are. Since this runs in a child (also the child body is very
> short) then when the parent (libvirtd) is killed the children are killed
> too. The POSIX locks I'm using here are tied to [PID,inode] pair.
> Therefore, if PID dies the lock is released.
>
Great thanks... The whole separation between libvirtd and virtlockd with
having the previous incarnation floating around in the back of my head
makes it "difficult" to
Re: [libvirt] [PATCH v3 03/13] security: Always spawn process for transactions
On 11/9/18 7:42 AM, Michal Privoznik wrote:
> On 11/08/2018 11:45 PM, John Ferlan wrote:
>>
>>
>> On 10/17/18 5:06 AM, Michal Privoznik wrote:
>>> In the next commit the virSecurityManagerMetadataLock() is going
>>> to be turned thread unsafe. Therefore, we have to spawn a
>>> separate process for it. Always.
>>>
>>> Signed-off-by: Michal Privoznik
>>> ---
>>> src/security/security_dac.c | 2 +-
>>> src/security/security_selinux.c | 2 +-
>>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>
>> Based slightly upon the KVM Forum presentation detailing some
>> performance issues related to the usage of virFork for
>> virQEMUCapsIsValid and calls to virFileAccessibleAs:
>>
>> https://www.redhat.com/archives/libvir-list/2018-October/msg01333.html
>>
>> Given that this patch would thus virFork "always", what kind of impact
>> could that have? Have you been able to do any performance profiling?
>> What would cause a single round of SELinux & DAC settings?
>
> This is what I was discussing with Daniel. Although I can't recall
> where. Anyway, fork() is expensive sure, but my argumentation in
> previous versions was that we are doing it already anyway. Namespaces
> are enabled by default and unless users turned them off this adds no new
> fork(). Only if namespaces are turned off then there is new fork(). At
> any rate, this is one fork per one secdriver call. It's not one fork per
> path (which would be horrible).
>
I too recall seeing the convo w/ Daniel, but couldn't find it in the
recent patches... I wonder if it was on internal IRC.
I did do a small amount of profiling before I asked, but my config
doesn't have some of the qemu_command qemuSecuritySet* calls for sockets
and tapfd's that are outside the batched qemuSecuritySetAllLabel. As an
aside, qemuSecurityDomainSetPathLabel strays from the normal
qemuSecurity{Set|Restore}* nomenclature (/me being grumpy about searches
for call patterns and would gladly R-by a qemuSecuritySetDomain* patch
as well as one that describes the functionality including the well
there's no need to run the Restore because these are domain transient
things that get removed anyway ;-)).
Anyway, currently it's just a "handful" of calls, but some in areas that
we seem to get flack on for. If we can get ahead of that flack because
we know what we're about to do w/r/t virFork that'd be good.
>>
>> I know in an earlier patch I wasn't including performance of virFork
>> because I believed that the only time it would be used would be for
>> metadata locking when (re)labeling would be batched and for that case
>> the "one off" virFork would seem reasonable.
>
> This is still true. There is no extra fork() if you have namespaces
> enabled. Unfortunately, POSIX file locking is fubared and doing fork is
> the least painful option.
>
>>
>> Since it is possible to turn off the NS code and thus go through the
>> direct call, I think there's "room for it" here too for those singular
>> cases we could use "pid == -1" to indicate direct calls without virFork
>> and "pid == 0" to batch together calls using virProcessRunInFork.
>>
>> That way when you *know* you are batching together more than singular
>> changes, then the caller can choose to run in a fork knowing the
>> possible performance penalty, but with the benefits. For those that are
>> batched we're stuck, but my memory on all the metadata locking paths is
>> fuzzy already.
>>
>> What's here does work, but after that KVM Forum preso I guess we
>> definitely need to pay attention to areas that can be perf bottlenecks
>> for paths that may be really common.
>>
>> Having a way to disable or avoid virFork is something we may just need
>> to have. I'm willing to be convinced otherwise - I'm just "wary" now.
>
> The metadata locking needs to be there so that the setting seclabels is
> atomic. I mean, so far chown() and setfcon() are atomic. However, there
> will be some xattr related calls around those. Therefore to make the
> whole critical section atomic there has to be a lock:
>
> lock(file);
> xattr(file);
> chown(file);
> xattr(file);
> unlock(file);
>
> The xattr() calls set/recall the original owner of the file. I can make
> this configurable, but if there is no locking the only thing libvirt can
> do is chown(), not the xattr() because that wouldn't be atomic. (I'm
> saying only chown(), but it is the same story for setfcon().) Therefore,
> if users disable metadata locking the original owner of the file can't
> be preserved. On the other hand, we can enable it by default and have an
> opt out for cases where it doesn't work - just like we have for
> namespaces. And I believe people did disable them in their early days
> (even though I don't understand why, they were bugfree :-P)
>
I understand (generically) why we need the lock. I'm OK with it being
enabled by default. That's not the question/ask. Building in a way to
allow disabling usage of virFork and/or metadata lock knowing the
"penalty" or downside to
Re: [libvirt] [PATCH v3 03/13] security: Always spawn process for transactions
On 11/08/2018 11:45 PM, John Ferlan wrote:
>
>
> On 10/17/18 5:06 AM, Michal Privoznik wrote:
>> In the next commit the virSecurityManagerMetadataLock() is going
>> to be turned thread unsafe. Therefore, we have to spawn a
>> separate process for it. Always.
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> src/security/security_dac.c | 2 +-
>> src/security/security_selinux.c | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>
> Based slightly upon the KVM Forum presentation detailing some
> performance issues related to the usage of virFork for
> virQEMUCapsIsValid and calls to virFileAccessibleAs:
>
> https://www.redhat.com/archives/libvir-list/2018-October/msg01333.html
>
> Given that this patch would thus virFork "always", what kind of impact
> could that have? Have you been able to do any performance profiling?
> What would cause a single round of SELinux & DAC settings?
This is what I was discussing with Daniel. Although I can't recall
where. Anyway, fork() is expensive sure, but my argumentation in
previous versions was that we are doing it already anyway. Namespaces
are enabled by default and unless users turned them off this adds no new
fork(). Only if namespaces are turned off then there is new fork(). At
any rate, this is one fork per one secdriver call. It's not one fork per
path (which would be horrible).
>
> I know in an earlier patch I wasn't including performance of virFork
> because I believed that the only time it would be used would be for
> metadata locking when (re)labeling would be batched and for that case
> the "one off" virFork would seem reasonable.
This is still true. There is no extra fork() if you have namespaces
enabled. Unfortunately, POSIX file locking is fubared and doing fork is
the least painful option.
>
> Since it is possible to turn off the NS code and thus go through the
> direct call, I think there's "room for it" here too for those singular
> cases we could use "pid == -1" to indicate direct calls without virFork
> and "pid == 0" to batch together calls using virProcessRunInFork.
>
> That way when you *know* you are batching together more than singular
> changes, then the caller can choose to run in a fork knowing the
> possible performance penalty, but with the benefits. For those that are
> batched we're stuck, but my memory on all the metadata locking paths is
> fuzzy already.
>
> What's here does work, but after that KVM Forum preso I guess we
> definitely need to pay attention to areas that can be perf bottlenecks
> for paths that may be really common.
>
> Having a way to disable or avoid virFork is something we may just need
> to have. I'm willing to be convinced otherwise - I'm just "wary" now.
The metadata locking needs to be there so that the setting seclabels is
atomic. I mean, so far chown() and setfcon() are atomic. However, there
will be some xattr related calls around those. Therefore to make the
whole critical section atomic there has to be a lock:
lock(file);
xattr(file);
chown(file);
xattr(file);
unlock(file);
The xattr() calls set/recall the original owner of the file. I can make
this configurable, but if there is no locking the only thing libvirt can
do is chown(), not the xattr() because that wouldn't be atomic. (I'm
saying only chown(), but it is the same story for setfcon().) Therefore,
if users disable metadata locking the original owner of the file can't
be preserved. On the other hand, we can enable it by default and have an
opt out for cases where it doesn't work - just like we have for
namespaces. And I believe people did disable them in their early days
(even though I don't understand why, they were bugfree :-P)
>
>
>> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
>> index da4a6c72fe..580d800bb1 100644
>> --- a/src/security/security_dac.c
>> +++ b/src/security/security_dac.c
>
> The function description would need an update way to describe this @pid
> usage which differs from the current description.
>
>> @@ -563,7 +563,7 @@ virSecurityDACTransactionCommit(virSecurityManagerPtr
>> mgr ATTRIBUTE_UNUSED,
>> }
>>
>> if ((pid == -1 &&
>> - virSecurityDACTransactionRun(pid, list) < 0) ||
>> + virProcessRunInFork(virSecurityDACTransactionRun, list) < 0) ||
>> (pid != -1 &&
>> virProcessRunInMountNamespace(pid,
>> virSecurityDACTransactionRun,
>
> I think I've previously disclosed my dislike of the format, why not:
>
> if (pid > 0) {
> rc = virProcessRunInMountNamespace(pid, ...);
> } else {
> if (pid == -1)
> rc = virSecurityDACTransactionRun(-pid, list);
> else
> rc = virProcessRunInFork(virSecurityDACTransactionRun, list));
> }
> if (rc < 0)
> goto cleanup;
>
> to me that's a lot cleaner and doesn't involve trying to look at
> multiple if statements with ||'s.
Okay, I'll change this.
>
>> diff --git
Re: [libvirt] [PATCH v3 04/13] security_manager: Rework metadata locking
On 11/09/2018 03:59 AM, John Ferlan wrote:
>
>
> On 10/17/18 5:06 AM, Michal Privoznik wrote:
>> Trying to use virlockd to lock metadata turns out to be too big
>> gun. Since we will always spawn a separate process for relabeling
>> we are safe to use thread unsafe POSIX locks and take out
>> virtlockd completely out of the picture.
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> src/security/security_dac.c | 10 +-
>> src/security/security_manager.c | 225 +---
>> src/security/security_manager.h | 17 ++-
>> src/security/security_selinux.c | 9 +-
>> 4 files changed, 139 insertions(+), 122 deletions(-)
>>
>> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
>> index 580d800bb1..ad2561a241 100644
>> --- a/src/security/security_dac.c
>> +++ b/src/security/security_dac.c
>> @@ -204,6 +204,7 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
>> void *opaque)
>> {
>> virSecurityDACChownListPtr list = opaque;
>> +virSecurityManagerMetadataLockStatePtr state;
>> const char **paths = NULL;
>> size_t npaths = 0;
>> size_t i;
>> @@ -216,14 +217,10 @@ virSecurityDACTransactionRun(pid_t pid
>> ATTRIBUTE_UNUSED,
>> for (i = 0; i < list->nItems; i++) {
>> const char *p = list->items[i]->path;
>>
>> -if (!p ||
>> -virFileIsDir(p))
>> -continue;
>> -
>> VIR_APPEND_ELEMENT_COPY_INPLACE(paths, npaths, p);
>> }
>>
>> -if (virSecurityManagerMetadataLock(list->manager, paths, npaths) < 0)
>> +if (!(state = virSecurityManagerMetadataLock(list->manager, paths,
>> npaths)))
>> goto cleanup;
>>
>> for (i = 0; i < list->nItems; i++) {
>> @@ -246,8 +243,7 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
>> break;
>> }
>>
>> -if (virSecurityManagerMetadataUnlock(list->manager, paths, npaths) < 0)
>> -goto cleanup;
>> +virSecurityManagerMetadataUnlock(list->manager, );
>>
>> if (rv < 0)
>> goto cleanup;
>> diff --git a/src/security/security_manager.c
>> b/src/security/security_manager.c
>> index c6c80e6165..b675f8ab46 100644
>> --- a/src/security/security_manager.c
>> +++ b/src/security/security_manager.c
>> @@ -21,6 +21,10 @@
>> */
>> #include
>>
>> +#include
>> +#include
>> +#include
>> +
>> #include "security_driver.h"
>> #include "security_stack.h"
>> #include "security_dac.h"
>> @@ -30,14 +34,11 @@
>> #include "virlog.h"
>> #include "locking/lock_manager.h"
>> #include "virfile.h"
>> -#include "virtime.h"
>>
>> #define VIR_FROM_THIS VIR_FROM_SECURITY
>>
>> VIR_LOG_INIT("security.security_manager");
>>
>> -virMutex lockManagerMutex = VIR_MUTEX_INITIALIZER;
>> -
>> struct _virSecurityManager {
>> virObjectLockable parent;
>>
>> @@ -47,10 +48,6 @@ struct _virSecurityManager {
>> void *privateData;
>>
>> virLockManagerPluginPtr lockPlugin;
>> -/* This is a FD that represents a connection to virtlockd so
>> - * that connection is kept open in between MetdataLock() and
>> - * MetadataUnlock() calls. */
>> -int clientfd;
>> };
>>
>> static virClassPtr virSecurityManagerClass;
>> @@ -66,7 +63,6 @@ void virSecurityManagerDispose(void *obj)
>> mgr->drv->close(mgr);
>>
>> virObjectUnref(mgr->lockPlugin);
>> -VIR_FORCE_CLOSE(mgr->clientfd);
>>
>> VIR_FREE(mgr->privateData);
>> }
>> @@ -119,7 +115,6 @@ virSecurityManagerNewDriver(virSecurityDriverPtr drv,
>> mgr->flags = flags;
>> mgr->virtDriver = virtDriver;
>> VIR_STEAL_PTR(mgr->privateData, privateData);
>> -mgr->clientfd = -1;
>>
>> if (drv->open(mgr) < 0)
>> goto error;
>> @@ -1276,129 +1271,153 @@
>> virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr,
>> }
>>
>>
>> -static virLockManagerPtr
>> -virSecurityManagerNewLockManager(virSecurityManagerPtr mgr,
>> - const char * const *paths,
>> - size_t npaths)
>> +struct _virSecurityManagerMetadataLockState {
>
> LockPrivate?
>
> When I first saw State I had a different thought.
>
> I like this better than the previous model... Keeping track of fds is
> like other models used. How do these locks handle restarts? Are the
> locks free'd if the daemon dies?
Yes they are. Since this runs in a child (also the child body is very
short) then when the parent (libvirtd) is killed the children are killed
too. The POSIX locks I'm using here are tied to [PID,inode] pair.
Therefore, if PID dies the lock is released.
>
>> +size_t nfds;
>> +int *fds;
>> +};
>> +
>> +
>> +static int
>> +cmpstringp(const void *p1, const void *p2)
>> {
>> -virLockManagerPtr lock;
>> -virLockManagerParam params[] = {
>> -{ .type = VIR_LOCK_MANAGER_PARAM_TYPE_UUID,
>> -.key = "uuid",
>> -},
>> -{ .type = VIR_LOCK_MANAGER_PARAM_TYPE_STRING,
>>
Re: [libvirt] [PATCH] qemu:address:Fix segfault in qemuDomainPrimeVirtioDeviceAddresses
On Fri, 2018-11-09 at 14:41 +0800, Wang Yechao wrote: > On aarch64, lauch vm with the follow configuration: > > > > > function="0x2"/> > > > > libvirtd will crash when access the net->model. > > Signed-off-by: Wang Yechao > --- > src/qemu/qemu_domain_address.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Reviewed-by: Andrea Bolognani and pushed after slightly tweaking the commit message. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] qemu: disable external snapshot of readonly disk
Disable external snapshot of readonly disk for inactive domains
as this operation is not very useful. As to active domains
such snapshot was not possible before already but error message was
not helpful so now it will be better.
Signed-off-by: Nikolay Shirokovskiy
---
Diff from v1 [1]
- move check to qemuDomainSnapshotPrepareDiskExternal
- disable such snapshot for inactive domain as well
[1] [PATCH] qemu: snapshot: better error for active external readonly disk
https://www.redhat.com/archives/libvir-list/2018-October/msg01322.html
continues in
https://www.redhat.com/archives/libvir-list/2018-November/msg00265.html
src/qemu/qemu_driver.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a52e249..e747a5b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14514,6 +14514,13 @@
qemuDomainSnapshotPrepareDiskExternal(virDomainDiskDefPtr disk,
int ret = -1;
struct stat st;
+if (disk->src->readonly) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("external snapshot for readonly disk %s "
+ "is not supported"), disk->dst);
+return -1;
+}
+
if (qemuTranslateSnapshotDiskSourcePool(snapdisk) < 0)
return -1;
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
