7:03 a.m.
v2 of:
https://www.redhat.com/archives/libvir-list/2018-October/msg00162.html
diff to v1 (all of this happened in 2/11 only):
- Moved virFileIsDir() and related checks into virSecurityManagerMetadataLock
- Use VIR_APPEND_ELEMENT_COPY_INPLACE() to properly fill FD array
- Lock sockets iff open() succeeds
Michal PrÃvoznÃk (11):
security: Always spawn process for transactions
security_manager: Rework metadata locking
Revert "security_manager: Load lock plugin on init"
Revert "qemu_conf: Introduce metadata_lock_manager"
Revert "lock_manager: Allow disabling configFile for
virLockManagerPluginNew"
Revert "lock_driver: Introduce VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK"
Revert "lock_driver: Introduce
VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA"
Revert "_virLockManagerLockDaemonPrivate: Move @hasRWDisks into dom
union"
Revert "lock_driver: Introduce new
VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON"
Revert "lock_driver_lockd: Introduce
VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA flag"
Revert "virlockspace: Allow caller to specify start and length offset
in virLockSpaceAcquireResource"
cfg.mk | 4 +-
src/locking/lock_daemon_dispatch.c | 11 +-
src/locking/lock_driver.h | 12 -
src/locking/lock_driver_lockd.c | 421 ++++++++++-------------------
src/locking/lock_driver_lockd.h | 1 -
src/locking/lock_driver_sanlock.c | 44 +--
src/locking/lock_manager.c | 10 +-
src/lxc/lxc_controller.c | 3 +-
src/lxc/lxc_driver.c | 2 +-
src/qemu/qemu_conf.c | 1 -
src/qemu/qemu_conf.h | 1 -
src/qemu/qemu_driver.c | 3 -
src/security/security_dac.c | 22 +-
src/security/security_manager.c | 233 +++++++---------
src/security/security_manager.h | 19 +-
src/security/security_selinux.c | 21 +-
src/util/virlockspace.c | 15 +-
src/util/virlockspace.h | 4 -
tests/seclabeltest.c | 2 +-
tests/securityselinuxlabeltest.c | 2 +-
tests/securityselinuxtest.c | 2 +-
tests/testutilsqemu.c | 2 +-
tests/virlockspacetest.c | 29 +-
23 files changed, 305 insertions(+), 559 deletions(-)
--
2.18.0
7:03 a.m.
New subject: [libvirt] [PATCH v2 01/11] security: Always spawn process for transactions
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 <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 12 ++++++------
src/security/security_selinux.c | 12 ++++++------
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index da4a6c72fe..21db3b9684 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -562,12 +562,12 @@ virSecurityDACTransactionCommit(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
goto cleanup;
}
- if ((pid == -1 &&
- virSecurityDACTransactionRun(pid, list) < 0) ||
- (pid != -1 &&
- virProcessRunInMountNamespace(pid,
- virSecurityDACTransactionRun,
- list) < 0))
+ if (pid == -1)
+ pid = getpid();
+
+ if (virProcessRunInMountNamespace(pid,
+ virSecurityDACTransactionRun,
+ list) < 0)
goto cleanup;
ret = 0;
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 467d1e6bfe..3c847d8dcb 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1103,12 +1103,12 @@ virSecuritySELinuxTransactionCommit(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
goto cleanup;
}
- if ((pid == -1 &&
- virSecuritySELinuxTransactionRun(pid, list) < 0) ||
- (pid != -1 &&
- virProcessRunInMountNamespace(pid,
- virSecuritySELinuxTransactionRun,
- list) < 0))
+ if (pid == -1)
+ pid = getpid();
+
+ if (virProcessRunInMountNamespace(pid,
+ virSecuritySELinuxTransactionRun,
+ list) < 0)
goto cleanup;
ret = 0;
--
2.18.0
4:06 p.m.
New subject: [libvirt] [PATCH v2 01/11] security: Always spawn process for transactions
On 10/11/18 8:03 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 <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 12 ++++++------
src/security/security_selinux.c | 12 ++++++------
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index da4a6c72fe..21db3b9684 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -562,12 +562,12 @@ virSecurityDACTransactionCommit(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
^^^^
The virSecurityDACTransactionCommit description would need some
adjustment.... Well a lot of adjustment... and the caller
virSecurityManagerTransactionCommit would also require some adjustment.
goto cleanup;
}
- if ((pid == -1 &&
- virSecurityDACTransactionRun(pid, list) < 0) ||
- (pid != -1 &&
- virProcessRunInMountNamespace(pid,
- virSecurityDACTransactionRun,
- list) < 0))
+ if (pid == -1)
+ pid = getpid();
+
+ if (virProcessRunInMountNamespace(pid,
+ virSecurityDACTransactionRun,
+ list) < 0)
As described in the v1 series cover letter, namespaces would need to be
enabled for this to work. Whether they are enabled true everywhere,
who's to say. I always assumed namespaces were used for a somewhat
narrow band of things and didn't pay that close attention to them.
I will note that will a couple of well placed VIR_WARN()'s in each of
these functions, I can see DAC and SELinux transactionCommit's getting
called. Before the guest actually starts they're called with -1, then
once it starts there's a single call with the domain PID. So I guess I'm
using NS and didn't know.
However, looking at the qemu.conf namespace setting I note things like
"privileged" and "!defined(HAVE_SYS_ACL_H) || !defined(WITH_SELINUX)"
So this all then assumes MountNamespace is being used. And what if it
isn't? Does everything that's security related start failing? and the
only way to back that out is revert this?
I guess, what I don't understand is why this usage pattern cannot be
limited to meta locking. That is, if we're metalocking, then add the
pid; otherwise, business as usual. At least that limits the scope.
FWIW:
In my "limited" testing, I have a guest that failed to start with this
code running:
# virsh start f23
error: Failed to start domain f23
error: internal error: Child process (6767) unexpected fatal signal 11
...
in the code running in the debugger I get:
2018-10-11 18:38:28.491+0000: 5088: error : virProcessWait:274 :
internal error: Child process (6220) unexpected fatal signal 11
2018-10-11 18:38:28.732+0000: 6211: error : learnIPAddressThread:688 :
encountered an error on interface vnet0 index 11: Invalid argument
I narrowed the failure down to my iSCSI disks such as:
<disk type='network' device='disk'>
<driver name='qemu' type='raw'/>
<source protocol='iscsi'
name='iqn.2013-12.com.example:iscsi-chap-netpool/3'>
<host name='192.168.122.1' port='3260'/>
<auth username='redhat'>
<secret type='iscsi' usage='libvirtiscsi'/>
</auth>
</source>
<target dev='vdc' bus='virtio'/>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x0a'
function='0x0'/>
</disk>
which doesn't make sense given the error, but who really knows... I
don't have the patience to debug much further either. Even stranger, the
same guest with <hostdev>:
<hostdev mode='subsystem' type='scsi' managed='no'>
<source protocol='iscsi'
name='iqn.2013-12.com.example:iscsi-chap-netpool/2'>
<host name='192.168.122.1' port='3260'/>
<auth username='redhat'>
<secret type='iscsi' usage='libvirtiscsi'/>
</auth>
</source>
<address type='drive' controller='0' bus='0'
target='0' unit='0'/>
</hostdev>
would work.
My selinux is "Permissive".
John
goto cleanup;
ret = 0;
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 467d1e6bfe..3c847d8dcb 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1103,12 +1103,12 @@ virSecuritySELinuxTransactionCommit(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
goto cleanup;
}
- if ((pid == -1 &&
- virSecuritySELinuxTransactionRun(pid, list) < 0) ||
- (pid != -1 &&
- virProcessRunInMountNamespace(pid,
- virSecuritySELinuxTransactionRun,
- list) < 0))
+ if (pid == -1)
+ pid = getpid();
+
+ if (virProcessRunInMountNamespace(pid,
+ virSecuritySELinuxTransactionRun,
+ list) < 0)
goto cleanup;
ret = 0;
3:45 a.m.
New subject: [libvirt] [PATCH v2 01/11] security: Always spawn process for transactions
On 10/11/2018 11:06 PM, John Ferlan wrote:
On 10/11/18 8:03 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 <mprivozn(a)redhat.com>
> ---
> src/security/security_dac.c | 12 ++++++------
> src/security/security_selinux.c | 12 ++++++------
> 2 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index da4a6c72fe..21db3b9684 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -562,12 +562,12 @@ virSecurityDACTransactionCommit(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
^^^^
The virSecurityDACTransactionCommit description would need some
adjustment.... Well a lot of adjustment... and the caller
virSecurityManagerTransactionCommit would also require some adjustment.
> goto cleanup;
> }
>
> - if ((pid == -1 &&
> - virSecurityDACTransactionRun(pid, list) < 0) ||
> - (pid != -1 &&
> - virProcessRunInMountNamespace(pid,
> - virSecurityDACTransactionRun,
> - list) < 0))
> + if (pid == -1)
> + pid = getpid();
> +
> + if (virProcessRunInMountNamespace(pid,
> + virSecurityDACTransactionRun,
> + list) < 0)
As described in the v1 series cover letter, namespaces would need to be
enabled for this to work.
Not really. I mention namespaces just to show that on majority of setups
we do fork() when relabeling paths anyway. Some might be worried that
fork()-ing is very expensive and therefore might be against this
approach. I am trying to prove them otherwise. In fact, this patch is
just about that - fork() every time, even when namespaces are disabled.
Again, I have no numbers to support that it's the majority, but since
namespaces are turned on by default (and no new bugs regarding
namespaces were to be seen recently) I am assuming that is the case.
Whether they are enabled true everywhere,
who's to say. I always assumed namespaces were used for a somewhat
narrow band of things and didn't pay that close attention to them.
No. Unless you take the extra step and disable them in qemu.conf you're
using them. And the fact, that you haven't noticed feels like pat on a
shoulder. Good job :-) You can verify this by running the following:
# nsenter -m -t $(pgrep qemu) ls -l /dev
(works if there's just one qemu process, or just replace pgrep with qemu
pid)
You will see very few items in /dev compared to how 'real' /dev looks:
# nsenter -m -t $(pgrep qemu) ls -l /dev | wc -l
15
# ls -l /dev | wc -l
188
I will note that will a couple of well placed VIR_WARN()'s in each of
these functions, I can see DAC and SELinux transactionCommit's getting
called. Before the guest actually starts they're called with -1, then
once it starts there's a single call with the domain PID. So I guess I'm
using NS and didn't know.
Yes. However, during the cmd line construction process some files are
created and qemuSecurityDomainSetPathLabel() is called even if there is
no qemu running yet. Therefore, security driver sees pid == -1. For
instance, qemuDomainWriteMasterKeyFile() is such function. But this is
not a problem (from metadata locking POV).
However, looking at the qemu.conf namespace setting I note things like
"privileged" and "!defined(HAVE_SYS_ACL_H) ||
!defined(WITH_SELINUX)"
So this all then assumes MountNamespace is being used. And what if it
isn't? Does everything that's security related start failing? and the
only way to back that out is revert this?
No. Namespaces have nothing to do with this feature. Namespaces are
Linux feature and therefore when qemu driver initializes itself it has
to make decision on the defaults. And currently the default is to enable
namespaces if supported by host (= Linux && (at least one of {ACL,
SELinux} was enabled in the build) ). I'm not going to explain that
process now as it has nothing to do with this feature.
I guess, what I don't understand is why this usage pattern cannot be
limited to meta locking. That is, if we're metalocking, then add the
pid; otherwise, business as usual. At least that limits the scope.
Namespaces and metadata locking are orthogonal features.
FWIW:
In my "limited" testing, I have a guest that failed to start with this
code running:
# virsh start f23
error: Failed to start domain f23
error: internal error: Child process (6767) unexpected fatal signal 11
Ouch. After some debugging I've found the problem. Previously, NULL
paths were not passed to virSecurityManagerMetadataLock() because they
were filtered out in DAC and SELinux drivers. But in v2 I've
de-duplicated the code. And the first thing that
virSecurityManagerMetadataLock() does is sort the paths (in order to
avoid AB/BA locking issue). For that, I've used qsort() with plain
strcmp() which can't handle NULL strings. The fix is quite easy:
diff --git i/src/security/security_manager.c
w/src/security/security_manager.c
index c054c74540..1f3e43b246 100644
--- i/src/security/security_manager.c
+++ w/src/security/security_manager.c
@@ -1257,8 +1257,17 @@ struct _virSecurityManagerMetadataLockState {
static int
cmpstringp(const void *p1, const void *p2)
{
+ const char *s1 = *(char * const *) p1;
+ const char *s2 = *(char * const *) p2;
+
+ if (!s1 && !s2)
+ return 0;
+
+ if (!s1 || !s2)
+ return s2 ? -1 : 1;
+
/* from man 3 qsort */
- return strcmp(* (char * const *) p1, * (char * const *) p2);
+ return strcmp(s1, s2);
}
#define METADATA_OFFSET 1
I've squashed this into patch #2.
Michal
9:20 a.m.
New subject: [libvirt] [PATCH v2 01/11] security: Always spawn process for transactions
On 10/12/18 4:45 AM, Michal Privoznik wrote:
On 10/11/2018 11:06 PM, John Ferlan wrote:
>
>
> On 10/11/18 8:03 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 <mprivozn(a)redhat.com>
>> ---
>> src/security/security_dac.c | 12 ++++++------
>> src/security/security_selinux.c | 12 ++++++------
>> 2 files changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
>> index da4a6c72fe..21db3b9684 100644
>> --- a/src/security/security_dac.c
>> +++ b/src/security/security_dac.c
>> @@ -562,12 +562,12 @@ virSecurityDACTransactionCommit(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
> ^^^^
>
> The virSecurityDACTransactionCommit description would need some
> adjustment.... Well a lot of adjustment... and the caller
> virSecurityManagerTransactionCommit would also require some adjustment.
>
>> goto cleanup;
>> }
>>
>> - if ((pid == -1 &&
>> - virSecurityDACTransactionRun(pid, list) < 0) ||
>> - (pid != -1 &&
>> - virProcessRunInMountNamespace(pid,
>> - virSecurityDACTransactionRun,
>> - list) < 0))
>> + if (pid == -1)
>> + pid = getpid();
>> +
>> + if (virProcessRunInMountNamespace(pid,
>> + virSecurityDACTransactionRun,
>> + list) < 0)
>
> As described in the v1 series cover letter, namespaces would need to be
> enabled for this to work.
Not really. I mention namespaces just to show that on majority of setups
we do fork() when relabeling paths anyway. Some might be worried that
fork()-ing is very expensive and therefore might be against this
approach. I am trying to prove them otherwise. In fact, this patch is
just about that - fork() every time, even when namespaces are disabled.
Considering the number of fork's out there anyway, that expense wasn't
part of my consideration. It's a means to an end.
If someone disabled Namespace's for whatever reason, then how would this
path work?
virProcessRunInMountNamespace => virProcessNamespaceHelper (child)
I get that it's "just" a fork, but my point is more that the namespace
is Namespace and if that's not true, then isn't the premise of the patch
not true?
And no IDC about non Linux (where Namespace wouldn't be available) since
non Linux wouldn't have DAC and/or SELinux either.
Perhaps the light dawning over marblehead moment for me is that the
security driver will use Namespaces similar to how the QEMU driver uses
them, but instead in the context of whatever process/thread that isn't
necessarily the domain process (in this case the libvirtd process).
Since it's been proven that the code works for the QEMU domain
processing, ergo it should "just work" for the libvirtd security driver
processing. That's very different from the commit message above and
perhaps the "assumption" I've been thinking under. There isn't a
"relationship" between the qemu.conf usage of namespaces and what's
being done here except for the libvirtd process as opposed to the qemu-*
process will use fork's and Namespaces.
Something perhaps for both the commit message *and* at least the two
modules that change here to more clearly describe the @pid usage. The
virSecurityManagerTransactionCommit will need adjustment too in order to
indicate that the underlying code will run in the Namespace specified by
the caller rather than the Namespace of the caller (a really vague, but
important difference).
Again, I have no numbers to support that it's the majority, but since
namespaces are turned on by default (and no new bugs regarding
namespaces were to be seen recently) I am assuming that is the case.
> Whether they are enabled true everywhere,
> who's to say. I always assumed namespaces were used for a somewhat
> narrow band of things and didn't pay that close attention to them.
No. Unless you take the extra step and disable them in qemu.conf you're
using them.
Well disabling isn't really well described in the qemu.conf description,
but I'm fairly certain it's by uncommenting the entry and removing the
"mount" from the [ ], leaving just "namespaces = [ ]"
For some more data points using the previously failing guest that now
works with the alteration below.
Running with the "default":
# nsenter -m -t $(pgrep qemu) ls -l /dev | wc -l
16
# ls -l /dev | wc -l
269
#
By changing to "namespaces = [ ]", I get:
# nsenter -m -t $(pgrep qemu) ls -l /dev | wc -l
269
# ls -l /dev | wc -l
269
#
Which I'm not sure I expected, but maybe that's right.
Next, if I run without the changes in this series and adding a couple of
debug messages to show when virProcessNamespaceHelper is called, when
namespaces = [ ] there are no calls to to the helper; whereas, there are
two using the default (I assume one each for DAC and SELinux).
With this series applied, when namespaces = [ ] there are 8 calls to to
the helper (all from the "parent" gdb pid); whereas, for the default the
last 2 come from the domain.
I assume that's all expected (at this point).
And the fact, that you haven't noticed feels like pat on a
shoulder. Good job :-) You can verify this by running the following:
# nsenter -m -t $(pgrep qemu) ls -l /dev
(works if there's just one qemu process, or just replace pgrep with qemu
pid)
You will see very few items in /dev compared to how 'real' /dev looks:
# nsenter -m -t $(pgrep qemu) ls -l /dev | wc -l
15
# ls -l /dev | wc -l
188
>
> I will note that will a couple of well placed VIR_WARN()'s in each of
> these functions, I can see DAC and SELinux transactionCommit's getting
> called. Before the guest actually starts they're called with -1, then
> once it starts there's a single call with the domain PID. So I guess I'm
> using NS and didn't know.
Yes. However, during the cmd line construction process some files are
created and qemuSecurityDomainSetPathLabel() is called even if there is
no qemu running yet. Therefore, security driver sees pid == -1. For
instance, qemuDomainWriteMasterKeyFile() is such function. But this is
not a problem (from metadata locking POV).
>
> However, looking at the qemu.conf namespace setting I note things like
> "privileged" and "!defined(HAVE_SYS_ACL_H) ||
!defined(WITH_SELINUX)"
> So this all then assumes MountNamespace is being used. And what if it
> isn't? Does everything that's security related start failing? and the
> only way to back that out is revert this?
No. Namespaces have nothing to do with this feature. Namespaces are
Linux feature and therefore when qemu driver initializes itself it has
to make decision on the defaults. And currently the default is to enable
namespaces if supported by host (= Linux && (at least one of {ACL,
SELinux} was enabled in the build) ). I'm not going to explain that
process now as it has nothing to do with this feature.
>
> I guess, what I don't understand is why this usage pattern cannot be
> limited to meta locking. That is, if we're metalocking, then add the
> pid; otherwise, business as usual. At least that limits the scope.
Namespaces and metadata locking are orthogonal features.
As I note above, I think I've had the light dawning moment now. I think
prior to this point I've been under the impression that the QEMU domain
setting for namespaces was somehow "affecting" or "related to"
what's
being done/changed here. In reality it's making use or reusing the same
code that QEMU uses.
Still the one reservation is - what happens if setns isn't available.
For that I think you need to somehow decide that it isn't available and
just pass -1, unless you have some other grand idea.
Whether you build in some "disable" switch just in case is up to you.
You feel confident that things will just work and I can agree, but you
know there are pessimists that may think otherwise.
John
>
> FWIW:
> In my "limited" testing, I have a guest that failed to start with this
> code running:
>
> # virsh start f23
> error: Failed to start domain f23
> error: internal error: Child process (6767) unexpected fatal signal 11
Ouch. After some debugging I've found the problem. Previously, NULL
paths were not passed to virSecurityManagerMetadataLock() because they
were filtered out in DAC and SELinux drivers. But in v2 I've
de-duplicated the code. And the first thing that
virSecurityManagerMetadataLock() does is sort the paths (in order to
avoid AB/BA locking issue). For that, I've used qsort() with plain
strcmp() which can't handle NULL strings. The fix is quite easy:
diff --git i/src/security/security_manager.c
w/src/security/security_manager.c
index c054c74540..1f3e43b246 100644
--- i/src/security/security_manager.c
+++ w/src/security/security_manager.c
@@ -1257,8 +1257,17 @@ struct _virSecurityManagerMetadataLockState {
static int
cmpstringp(const void *p1, const void *p2)
{
+ const char *s1 = *(char * const *) p1;
+ const char *s2 = *(char * const *) p2;
+
+ if (!s1 && !s2)
+ return 0;
+
+ if (!s1 || !s2)
+ return s2 ? -1 : 1;
+
/* from man 3 qsort */
- return strcmp(* (char * const *) p1, * (char * const *) p2);
+ return strcmp(s1, s2);
}
#define METADATA_OFFSET 1
>
I've squashed this into patch #2.
Michal
2:40 a.m.
New subject: [libvirt] [PATCH v2 01/11] security: Always spawn process for transactions
On 10/12/2018 04:20 PM, John Ferlan wrote:
On 10/12/18 4:45 AM, Michal Privoznik wrote:
> On 10/11/2018 11:06 PM, John Ferlan wrote:
>>
>>
>> On 10/11/18 8:03 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 <mprivozn(a)redhat.com>
>>> ---
>>> src/security/security_dac.c | 12 ++++++------
>>> src/security/security_selinux.c | 12 ++++++------
>>> 2 files changed, 12 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
>>> index da4a6c72fe..21db3b9684 100644
>>> --- a/src/security/security_dac.c
>>> +++ b/src/security/security_dac.c
>>> @@ -562,12 +562,12 @@ virSecurityDACTransactionCommit(virSecurityManagerPtr
mgr ATTRIBUTE_UNUSED,
>> ^^^^
>>
>> The virSecurityDACTransactionCommit description would need some
>> adjustment.... Well a lot of adjustment... and the caller
>> virSecurityManagerTransactionCommit would also require some adjustment.
>>
>>> goto cleanup;
>>> }
>>>
>>> - if ((pid == -1 &&
>>> - virSecurityDACTransactionRun(pid, list) < 0) ||
>>> - (pid != -1 &&
>>> - virProcessRunInMountNamespace(pid,
>>> - virSecurityDACTransactionRun,
>>> - list) < 0))
>>> + if (pid == -1)
>>> + pid = getpid();
>>> +
>>> + if (virProcessRunInMountNamespace(pid,
>>> + virSecurityDACTransactionRun,
>>> + list) < 0)
>>
>> As described in the v1 series cover letter, namespaces would need to be
>> enabled for this to work.
>
> Not really. I mention namespaces just to show that on majority of setups
> we do fork() when relabeling paths anyway. Some might be worried that
> fork()-ing is very expensive and therefore might be against this
> approach. I am trying to prove them otherwise. In fact, this patch is
> just about that - fork() every time, even when namespaces are disabled.
Considering the number of fork's out there anyway, that expense wasn't
part of my consideration. It's a means to an end.
If someone disabled Namespace's for whatever reason, then how would this
path work?
virProcessRunInMountNamespace => virProcessNamespaceHelper (child)
I get that it's "just" a fork, but my point is more that the namespace
is Namespace and if that's not true, then isn't the premise of the patch
not true?
Every Linux process runs in a namespace (unless kernel was compiled
without NS support). That is, even libvirtd runs in a namespace:
# ls -l /proc/$(pgrep libvirtd)/ns
lrwxrwxrwx. 1 root root 0 Oct 16 09:06 cgroup -> 'cgroup:[4026531835]'
lrwxrwxrwx. 1 root root 0 Oct 16 09:06 ipc -> 'ipc:[4026531839]'
lrwxrwxrwx. 1 root root 0 Oct 16 09:06 mnt -> 'mnt:[4026531840]'
...
And no IDC about non Linux (where Namespace wouldn't be available) since
non Linux wouldn't have DAC and/or SELinux either.
Actually, we have to care about non Linux, e.g. BSD does have DAC. Or
even Linux where namespaces were disabled (unlikely but not impossible).
Perhaps the light dawning over marblehead moment for me is that the
security driver will use Namespaces similar to how the QEMU driver uses
them, but instead in the context of whatever process/thread that isn't
necessarily the domain process (in this case the libvirtd process).
Since it's been proven that the code works for the QEMU domain
processing, ergo it should "just work" for the libvirtd security driver
processing. That's very different from the commit message above and
perhaps the "assumption" I've been thinking under. There isn't a
"relationship" between the qemu.conf usage of namespaces and what's
being done here except for the libvirtd process as opposed to the qemu-*
process will use fork's and Namespaces.
Something perhaps for both the commit message *and* at least the two
modules that change here to more clearly describe the @pid usage. The
virSecurityManagerTransactionCommit will need adjustment too in order to
indicate that the underlying code will run in the Namespace specified by
the caller rather than the Namespace of the caller (a really vague, but
important difference).
>
> Again, I have no numbers to support that it's the majority, but since
> namespaces are turned on by default (and no new bugs regarding
> namespaces were to be seen recently) I am assuming that is the case.
>
>> Whether they are enabled true everywhere,
>> who's to say. I always assumed namespaces were used for a somewhat
>> narrow band of things and didn't pay that close attention to them.
>
> No. Unless you take the extra step and disable them in qemu.conf you're
> using them.
Well disabling isn't really well described in the qemu.conf description,
but I'm fairly certain it's by uncommenting the entry and removing the
"mount" from the [ ], leaving just "namespaces = [ ]"
For some more data points using the previously failing guest that now
works with the alteration below.
Running with the "default":
# nsenter -m -t $(pgrep qemu) ls -l /dev | wc -l
16
# ls -l /dev | wc -l
269
#
By changing to "namespaces = [ ]", I get:
# nsenter -m -t $(pgrep qemu) ls -l /dev | wc -l
269
# ls -l /dev | wc -l
269
#
Which I'm not sure I expected, but maybe that's right.
It is, because this means that for namespaces=[] case there is exactly
the same number of items in your host /dev and what qemu sees. Compared
to previous case when namespaces=["mnt"] (the default) host /dev
contained more entries and than what qemu saw.
Next, if I run without the changes in this series and adding a couple of
debug messages to show when virProcessNamespaceHelper is called, when
namespaces = [ ] there are no calls to to the helper; whereas, there are
two using the default (I assume one each for DAC and SELinux).
With this series applied, when namespaces = [ ] there are 8 calls to to
the helper (all from the "parent" gdb pid); whereas, for the default the
last 2 come from the domain.
I assume that's all expected (at this point).
> And the fact, that you haven't noticed feels like pat on a
> shoulder. Good job :-) You can verify this by running the following:
>
> # nsenter -m -t $(pgrep qemu) ls -l /dev
>
> (works if there's just one qemu process, or just replace pgrep with qemu
> pid)
>
> You will see very few items in /dev compared to how 'real' /dev looks:
>
> # nsenter -m -t $(pgrep qemu) ls -l /dev | wc -l
> 15
> # ls -l /dev | wc -l
> 188
>
>>
>> I will note that will a couple of well placed VIR_WARN()'s in each of
>> these functions, I can see DAC and SELinux transactionCommit's getting
>> called. Before the guest actually starts they're called with -1, then
>> once it starts there's a single call with the domain PID. So I guess I'm
>> using NS and didn't know.
>
> Yes. However, during the cmd line construction process some files are
> created and qemuSecurityDomainSetPathLabel() is called even if there is
> no qemu running yet. Therefore, security driver sees pid == -1. For
> instance, qemuDomainWriteMasterKeyFile() is such function. But this is
> not a problem (from metadata locking POV).
>
>>
>> However, looking at the qemu.conf namespace setting I note things like
>> "privileged" and "!defined(HAVE_SYS_ACL_H) ||
!defined(WITH_SELINUX)"
>> So this all then assumes MountNamespace is being used. And what if it
>> isn't? Does everything that's security related start failing? and the
>> only way to back that out is revert this?
>
> No. Namespaces have nothing to do with this feature. Namespaces are
> Linux feature and therefore when qemu driver initializes itself it has
> to make decision on the defaults. And currently the default is to enable
> namespaces if supported by host (= Linux && (at least one of {ACL,
> SELinux} was enabled in the build) ). I'm not going to explain that
> process now as it has nothing to do with this feature.
>
>>
>> I guess, what I don't understand is why this usage pattern cannot be
>> limited to meta locking. That is, if we're metalocking, then add the
>> pid; otherwise, business as usual. At least that limits the scope.
>
> Namespaces and metadata locking are orthogonal features.
>
As I note above, I think I've had the light dawning moment now. I think
prior to this point I've been under the impression that the QEMU domain
setting for namespaces was somehow "affecting" or "related to"
what's
being done/changed here. In reality it's making use or reusing the same
code that QEMU uses.
Still the one reservation is - what happens if setns isn't available.
For that I think you need to somehow decide that it isn't available and
just pass -1, unless you have some other grand idea.
Yeah, I need to have a patch that introduces "virProcessRunInFork" which
will just fork() and run the callback in the child. Something like
virProcessRunInMountNamespace but this will be called when namespaces
are not available.
Will send v3 in a while.
Michal
7:03 a.m.
New subject: [libvirt] [PATCH v2 02/11] security_manager: Rework metadata locking
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 <mprivozn(a)redhat.com>
---
src/security/security_dac.c | 10 +-
src/security/security_manager.c | 208 ++++++++++++++++----------------
src/security/security_manager.h | 17 ++-
src/security/security_selinux.c | 9 +-
4 files changed, 120 insertions(+), 124 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 21db3b9684..5c4af58fa8 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, &state);
if (rv < 0)
goto cleanup;
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index c6c80e6165..62700c8b17 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -21,6 +21,10 @@
*/
#include <config.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
#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,132 @@ virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr,
}
-static virLockManagerPtr
-virSecurityManagerNewLockManager(virSecurityManagerPtr mgr,
- const char * const *paths,
- size_t npaths)
+struct _virSecurityManagerMetadataLockState {
+ 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,
- .key = "name",
- .value = { .cstr = "libvirtd-sec" },
- },
- { .type = VIR_LOCK_MANAGER_PARAM_TYPE_UINT,
- .key = "pid",
- .value = { .iv = getpid() },
- },
- };
- const unsigned int flags = 0;
- size_t i;
-
- if (virGetHostUUID(params[0].value.uuid) < 0)
- return NULL;
-
- if (!(lock = virLockManagerNew(virLockManagerPluginGetDriver(mgr->lockPlugin),
- VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON,
- ARRAY_CARDINALITY(params),
- params,
- flags)))
- return NULL;
-
- for (i = 0; i < npaths; i++) {
- if (virLockManagerAddResource(lock,
- VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA,
- paths[i], 0, NULL, 0) < 0)
- goto error;
- }
-
- return lock;
- error:
- virLockManagerFree(lock);
- return NULL;
+ /* from man 3 qsort */
+ return strcmp(* (char * const *) p1, * (char * const *) p2);
}
+#define METADATA_OFFSET 1
+#define METADATA_LEN 1
-/* How many seconds should we try to acquire the lock before
- * giving up. */
-#define LOCK_ACQUIRE_TIMEOUT 60
-
-int
-virSecurityManagerMetadataLock(virSecurityManagerPtr mgr,
- const char * const *paths,
+/**
+ * virSecurityManagerMetadataLock:
+ * @mgr: security manager object
+ * @paths: paths to lock
+ * @npaths: number of items in @paths array
+ *
+ * Lock passed @paths for metadata change. The returned state
+ * should be passed to virSecurityManagerMetadataUnlock.
+ *
+ * NOTE: this function is not thread safe (because of usage of
+ * POSIX locks).
+ *
+ * Returns: state on success,
+ * NULL on failure.
+ */
+virSecurityManagerMetadataLockStatePtr
+virSecurityManagerMetadataLock(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
+ const char **paths,
size_t npaths)
{
- virLockManagerPtr lock;
- virTimeBackOffVar timebackoff;
- int fd = -1;
- int rv = -1;
- int ret = -1;
+ size_t i = 0;
+ size_t nfds = 0;
+ int *fds = NULL;
+ virSecurityManagerMetadataLockStatePtr ret = NULL;
- virMutexLock(&lockManagerMutex);
+ if (VIR_ALLOC_N(fds, npaths) < 0)
+ return NULL;
- if (!(lock = virSecurityManagerNewLockManager(mgr, paths, npaths)))
- goto cleanup;
+ /* Sort paths to lock in order to avoid deadlocks. */
+ qsort(paths, npaths, sizeof(*paths), cmpstringp);
- if (virTimeBackOffStart(&timebackoff, 1, LOCK_ACQUIRE_TIMEOUT * 1000) < 0)
- goto cleanup;
- while (virTimeBackOffWait(&timebackoff)) {
- rv = virLockManagerAcquire(lock, NULL,
- VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK,
- VIR_DOMAIN_LOCK_FAILURE_DEFAULT, &fd);
+ for (i = 0; i < npaths; i++) {
+ const char *p = paths[i];
+ struct stat sb;
+ int fd;
- if (rv >= 0)
- break;
+ if (!p || stat(p, &sb) < 0)
+ continue;
- if (virGetLastErrorCode() == VIR_ERR_RESOURCE_BUSY)
+ if (S_ISDIR(sb.st_mode)) {
+ /* Directories can't be locked */
continue;
+ }
+
+ if ((fd = open(p, O_RDWR)) < 0) {
+ if (S_ISSOCK(sb.st_mode)) {
+ /* Sockets can be opened only if there exists the
+ * other side that listens. */
+ continue;
+ }
+
+ virReportSystemError(errno,
+ _("unable to open %s"),
+ p);
+ goto cleanup;
+ }
+
+ if (virFileLock(fd, false,
+ METADATA_OFFSET, METADATA_LEN, true) < 0) {
+ virReportSystemError(errno,
+ _("unable to lock %s for metadata change"),
+ p);
+ VIR_FORCE_CLOSE(fd);
+ goto cleanup;
+ }
- goto cleanup;
+ VIR_APPEND_ELEMENT_COPY_INPLACE(fds, nfds, fd);
}
- if (rv < 0)
+ if (VIR_ALLOC(ret) < 0)
goto cleanup;
- mgr->clientfd = fd;
- fd = -1;
+ VIR_STEAL_PTR(ret->fds, fds);
+ ret->nfds = nfds;
+ nfds = 0;
- ret = 0;
cleanup:
- virLockManagerFree(lock);
- VIR_FORCE_CLOSE(fd);
- if (ret < 0)
- virMutexUnlock(&lockManagerMutex);
+ for (i = nfds; i > 0; i--)
+ VIR_FORCE_CLOSE(fds[i - 1]);
+ VIR_FREE(fds);
return ret;
}
-int
-virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr,
- const char * const *paths,
- size_t npaths)
+void
+virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
+ virSecurityManagerMetadataLockStatePtr *state)
{
- virLockManagerPtr lock;
- int fd;
- int ret = -1;
+ size_t i;
- /* lockManagerMutex acquired from previous
- * virSecurityManagerMetadataLock() call. */
+ if (!state)
+ return;
- fd = mgr->clientfd;
- mgr->clientfd = -1;
+ for (i = 0; i < (*state)->nfds; i++) {
+ char ebuf[1024];
+ int fd = (*state)->fds[i];
- if (!(lock = virSecurityManagerNewLockManager(mgr, paths, npaths)))
- goto cleanup;
+ /* Technically, unlock is not needed because it will
+ * happen on VIR_CLOSE() anyway. But let's play it nice. */
+ if (virFileUnlock(fd, METADATA_OFFSET, METADATA_LEN) < 0) {
+ VIR_WARN("Unable to unlock fd %d: %s",
+ fd, virStrerror(errno, ebuf, sizeof(ebuf)));
+ }
- if (virLockManagerRelease(lock, NULL, 0) < 0)
- goto cleanup;
+ if (VIR_CLOSE(fd) < 0) {
+ VIR_WARN("Unable to close fd %d: %s",
+ fd, virStrerror(errno, ebuf, sizeof(ebuf)));
+ }
+ }
- ret = 0;
- cleanup:
- virLockManagerFree(lock);
- VIR_FORCE_CLOSE(fd);
- virMutexUnlock(&lockManagerMutex);
- return ret;
+ VIR_FREE((*state)->fds);
+ VIR_FREE(*state);
}
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index 10ebe5cc29..fe8a1b4a24 100644
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -199,11 +199,16 @@ int virSecurityManagerSetTPMLabels(virSecurityManagerPtr mgr,
int virSecurityManagerRestoreTPMLabels(virSecurityManagerPtr mgr,
virDomainDefPtr vm);
-int virSecurityManagerMetadataLock(virSecurityManagerPtr mgr,
- const char * const *paths,
- size_t npaths);
-int virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr,
- const char * const *paths,
- size_t npaths);
+typedef struct _virSecurityManagerMetadataLockState virSecurityManagerMetadataLockState;
+typedef virSecurityManagerMetadataLockState *virSecurityManagerMetadataLockStatePtr;
+
+virSecurityManagerMetadataLockStatePtr
+virSecurityManagerMetadataLock(virSecurityManagerPtr mgr,
+ const char **paths,
+ size_t npaths);
+
+void
+virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr,
+ virSecurityManagerMetadataLockStatePtr *state);
#endif /* VIR_SECURITY_MANAGER_H__ */
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 3c847d8dcb..3573e729b7 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -214,6 +214,7 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
void *opaque)
{
virSecuritySELinuxContextListPtr list = opaque;
+ virSecurityManagerMetadataLockStatePtr state;
bool privileged = virSecurityManagerGetPrivileged(list->manager);
const char **paths = NULL;
size_t npaths = 0;
@@ -227,13 +228,10 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
for (i = 0; i < list->nItems; i++) {
const char *p = list->items[i]->path;
- if (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;
rv = 0;
@@ -250,8 +248,7 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
}
}
- if (virSecurityManagerMetadataUnlock(list->manager, paths, npaths) < 0)
- goto cleanup;
+ virSecurityManagerMetadataUnlock(list->manager, &state);
if (rv < 0)
goto cleanup;
--
2.18.0
7:03 a.m.
New subject: [libvirt] [PATCH v2 03/11] Revert "security_manager: Load lock plugin on init"
This reverts commit 3e26b476b5f322353bf0dcd8e3f037ca672b8c62.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
cfg.mk | 4 +---
src/lxc/lxc_controller.c | 3 +--
src/lxc/lxc_driver.c | 2 +-
src/qemu/qemu_driver.c | 3 ---
src/security/security_manager.c | 25 +------------------------
src/security/security_manager.h | 2 --
tests/seclabeltest.c | 2 +-
tests/securityselinuxlabeltest.c | 2 +-
tests/securityselinuxtest.c | 2 +-
tests/testutilsqemu.c | 2 +-
10 files changed, 8 insertions(+), 39 deletions(-)
diff --git a/cfg.mk b/cfg.mk
index 4790d0b7e7..eddd110ed6 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -787,10 +787,8 @@ sc_prohibit_cross_inclusion:
case $$dir in \
util/) safe="util";; \
access/ | conf/) safe="($$dir|conf|util)";; \
- cpu/| network/| node_device/| rpc/| storage/) \
+ cpu/| network/| node_device/| rpc/| security/| storage/) \
safe="($$dir|util|conf|storage)";; \
- security/) \
- safe="($$dir|util|conf|storage|locking)";; \
xenapi/ | xenconfig/ ) safe="($$dir|util|conf|xen|cpu)";; \
*) safe="($$dir|$(mid_dirs)|util)";; \
esac; \
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 62dfd09473..e853d02d65 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -2624,8 +2624,7 @@ int main(int argc, char *argv[])
ctrl->handshakeFd = handshakeFd;
if (!(ctrl->securityManager = virSecurityManagerNew(securityDriver,
- LXC_DRIVER_NAME,
- NULL, 0)))
+ LXC_DRIVER_NAME, 0)))
goto cleanup;
if (ctrl->def->seclabels) {
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index f732305649..990871d9b3 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -1531,7 +1531,7 @@ lxcSecurityInit(virLXCDriverConfigPtr cfg)
flags |= VIR_SECURITY_MANAGER_REQUIRE_CONFINED;
virSecurityManagerPtr mgr = virSecurityManagerNew(cfg->securityDriverName,
- LXC_DRIVER_NAME, NULL, flags);
+ LXC_DRIVER_NAME, flags);
if (!mgr)
goto error;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a52e2495d5..3a1185f947 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -350,7 +350,6 @@ qemuSecurityInit(virQEMUDriverPtr driver)
while (names && *names) {
if (!(mgr = qemuSecurityNew(*names,
QEMU_DRIVER_NAME,
- cfg->metadataLockManagerName,
flags)))
goto error;
if (!stack) {
@@ -366,7 +365,6 @@ qemuSecurityInit(virQEMUDriverPtr driver)
} else {
if (!(mgr = qemuSecurityNew(NULL,
QEMU_DRIVER_NAME,
- cfg->metadataLockManagerName,
flags)))
goto error;
if (!(stack = qemuSecurityNewStack(mgr)))
@@ -383,7 +381,6 @@ qemuSecurityInit(virQEMUDriverPtr driver)
cfg->user,
cfg->group,
flags,
- cfg->metadataLockManagerName,
qemuSecurityChownCallback)))
goto error;
if (!stack) {
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 62700c8b17..c054c74540 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -32,7 +32,6 @@
#include "viralloc.h"
#include "virobject.h"
#include "virlog.h"
-#include "locking/lock_manager.h"
#include "virfile.h"
#define VIR_FROM_THIS VIR_FROM_SECURITY
@@ -46,8 +45,6 @@ struct _virSecurityManager {
unsigned int flags;
const char *virtDriver;
void *privateData;
-
- virLockManagerPluginPtr lockPlugin;
};
static virClassPtr virSecurityManagerClass;
@@ -58,12 +55,8 @@ void virSecurityManagerDispose(void *obj)
{
virSecurityManagerPtr mgr = obj;
- if (mgr->drv &&
- mgr->drv->close)
+ if (mgr->drv->close)
mgr->drv->close(mgr);
-
- virObjectUnref(mgr->lockPlugin);
-
VIR_FREE(mgr->privateData);
}
@@ -83,7 +76,6 @@ VIR_ONCE_GLOBAL_INIT(virSecurityManager);
static virSecurityManagerPtr
virSecurityManagerNewDriver(virSecurityDriverPtr drv,
const char *virtDriver,
- const char *lockManagerPluginName,
unsigned int flags)
{
virSecurityManagerPtr mgr = NULL;
@@ -103,14 +95,6 @@ virSecurityManagerNewDriver(virSecurityDriverPtr drv,
if (!(mgr = virObjectLockableNew(virSecurityManagerClass)))
goto error;
- if (!lockManagerPluginName)
- lockManagerPluginName = "nop";
-
- if (!(mgr->lockPlugin = virLockManagerPluginNew(lockManagerPluginName,
- NULL, NULL, 0))) {
- goto error;
- }
-
mgr->drv = drv;
mgr->flags = flags;
mgr->virtDriver = virtDriver;
@@ -133,7 +117,6 @@ virSecurityManagerNewStack(virSecurityManagerPtr primary)
virSecurityManagerPtr mgr =
virSecurityManagerNewDriver(&virSecurityDriverStack,
virSecurityManagerGetDriver(primary),
- NULL,
primary->flags);
if (!mgr)
@@ -142,8 +125,6 @@ virSecurityManagerNewStack(virSecurityManagerPtr primary)
if (virSecurityStackAddNested(mgr, primary) < 0)
goto error;
- mgr->lockPlugin = virObjectRef(mgr->lockPlugin);
-
return mgr;
error:
virObjectUnref(mgr);
@@ -166,7 +147,6 @@ virSecurityManagerNewDAC(const char *virtDriver,
uid_t user,
gid_t group,
unsigned int flags,
- const char *lockManagerPluginName,
virSecurityManagerDACChownCallback chownCallback)
{
virSecurityManagerPtr mgr;
@@ -177,7 +157,6 @@ virSecurityManagerNewDAC(const char *virtDriver,
mgr = virSecurityManagerNewDriver(&virSecurityDriverDAC,
virtDriver,
- lockManagerPluginName,
flags & VIR_SECURITY_MANAGER_NEW_MASK);
if (!mgr)
@@ -199,7 +178,6 @@ virSecurityManagerNewDAC(const char *virtDriver,
virSecurityManagerPtr
virSecurityManagerNew(const char *name,
const char *virtDriver,
- const char *lockManagerPluginName,
unsigned int flags)
{
virSecurityDriverPtr drv = virSecurityDriverLookup(name, virtDriver);
@@ -228,7 +206,6 @@ virSecurityManagerNew(const char *name,
return virSecurityManagerNewDriver(drv,
virtDriver,
- lockManagerPluginName,
flags);
}
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index fe8a1b4a24..c4abc34b17 100644
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -45,7 +45,6 @@ typedef enum {
virSecurityManagerPtr virSecurityManagerNew(const char *name,
const char *virtDriver,
- const char *lockManagerPluginName,
unsigned int flags);
virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary);
@@ -71,7 +70,6 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver,
uid_t user,
gid_t group,
unsigned int flags,
- const char *lockManagerPluginName,
virSecurityManagerDACChownCallback
chownCallback);
int virSecurityManagerPreFork(virSecurityManagerPtr mgr);
diff --git a/tests/seclabeltest.c b/tests/seclabeltest.c
index 7cddf96e82..a0296c787e 100644
--- a/tests/seclabeltest.c
+++ b/tests/seclabeltest.c
@@ -14,7 +14,7 @@ mymain(void)
if (virThreadInitialize() < 0)
return EXIT_FAILURE;
- mgr = virSecurityManagerNew(NULL, "QEMU", NULL,
VIR_SECURITY_MANAGER_DEFAULT_CONFINED);
+ mgr = virSecurityManagerNew(NULL, "QEMU",
VIR_SECURITY_MANAGER_DEFAULT_CONFINED);
if (mgr == NULL) {
fprintf(stderr, "Failed to start security driver");
return EXIT_FAILURE;
diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c
index aa9fae7d32..39f4eb7b6a 100644
--- a/tests/securityselinuxlabeltest.c
+++ b/tests/securityselinuxlabeltest.c
@@ -346,7 +346,7 @@ mymain(void)
if (!rc)
return EXIT_AM_SKIP;
- if (!(mgr = virSecurityManagerNew("selinux", "QEMU", NULL,
+ if (!(mgr = virSecurityManagerNew("selinux", "QEMU",
VIR_SECURITY_MANAGER_DEFAULT_CONFINED |
VIR_SECURITY_MANAGER_PRIVILEGED))) {
VIR_TEST_VERBOSE("Unable to initialize security driver: %s\n",
diff --git a/tests/securityselinuxtest.c b/tests/securityselinuxtest.c
index f1ea51b1ac..a2864cf57c 100644
--- a/tests/securityselinuxtest.c
+++ b/tests/securityselinuxtest.c
@@ -272,7 +272,7 @@ mymain(void)
int ret = 0;
virSecurityManagerPtr mgr;
- if (!(mgr = virSecurityManagerNew("selinux", "QEMU", NULL,
+ if (!(mgr = virSecurityManagerNew("selinux", "QEMU",
VIR_SECURITY_MANAGER_DEFAULT_CONFINED |
VIR_SECURITY_MANAGER_PRIVILEGED))) {
fprintf(stderr, "Unable to initialize security driver: %s\n",
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
index 332885eb77..0d3e9fc7e6 100644
--- a/tests/testutilsqemu.c
+++ b/tests/testutilsqemu.c
@@ -716,7 +716,7 @@ int qemuTestDriverInit(virQEMUDriver *driver)
if (qemuTestCapsCacheInsert(driver->qemuCapsCache, NULL) < 0)
goto error;
- if (!(mgr = virSecurityManagerNew("none", "qemu", NULL,
+ if (!(mgr = virSecurityManagerNew("none", "qemu",
VIR_SECURITY_MANAGER_PRIVILEGED)))
goto error;
if (!(driver->securityManager = virSecurityManagerNewStack(mgr)))
--
2.18.0
7:03 a.m.
New subject: [libvirt] [PATCH v2 04/11] Revert "qemu_conf: Introduce metadata_lock_manager"
This reverts commit 8b8aefb3d6ae2139ea3d4ef6d7dd2c06f57f6075.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_conf.c | 1 -
src/qemu/qemu_conf.h | 1 -
2 files changed, 2 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 17b7e11e02..32da9a7351 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -426,7 +426,6 @@ static void virQEMUDriverConfigDispose(void *obj)
virStringListFree(cfg->securityDriverNames);
VIR_FREE(cfg->lockManagerName);
- VIR_FREE(cfg->metadataLockManagerName);
virFirmwareFreeList(cfg->firmwares, cfg->nfirmwares);
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index f876f9117c..49e5c50f22 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -186,7 +186,6 @@ struct _virQEMUDriverConfig {
bool autoStartBypassCache;
char *lockManagerName;
- char *metadataLockManagerName;
int keepAliveInterval;
unsigned int keepAliveCount;
--
2.18.0
7:03 a.m.
New subject: [libvirt] [PATCH v2 05/11] Revert "lock_manager: Allow disabling configFile for virLockManagerPluginNew"
This reverts commit 35b5b244da825fb41e35e4dc62e740d716214ec9.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/locking/lock_driver.h | 4 ----
src/locking/lock_driver_lockd.c | 4 +---
src/locking/lock_driver_sanlock.c | 4 +---
src/locking/lock_manager.c | 10 +++-------
4 files changed, 5 insertions(+), 17 deletions(-)
diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h
index ae30abda7d..7c8f744be3 100644
--- a/src/locking/lock_driver.h
+++ b/src/locking/lock_driver.h
@@ -124,7 +124,6 @@ struct _virLockManagerParam {
/**
* virLockDriverInit:
* @version: the libvirt requested plugin ABI version
- * @configFile: path to config file
* @flags: the libvirt requested plugin optional extras
*
* Allow the plugin to validate the libvirt requested
@@ -132,9 +131,6 @@ struct _virLockManagerParam {
* to block its use in versions of libvirtd which are
* too old to support key features.
*
- * The @configFile variable points to config file that the driver
- * should load. If NULL, no config file should be loaded.
- *
* NB: A plugin may be loaded multiple times, for different
* libvirt drivers (eg QEMU, LXC, UML)
*
diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
index 0c672b05b1..85cdcf97be 100644
--- a/src/locking/lock_driver_lockd.c
+++ b/src/locking/lock_driver_lockd.c
@@ -371,10 +371,8 @@ static int virLockManagerLockDaemonInit(unsigned int version,
driver->requireLeaseForDisks = true;
driver->autoDiskLease = true;
- if (configFile &&
- virLockManagerLockDaemonLoadConfig(configFile) < 0) {
+ if (virLockManagerLockDaemonLoadConfig(configFile) < 0)
goto error;
- }
if (driver->autoDiskLease) {
if (driver->fileLockSpaceDir &&
diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c
index 3ad0dc9bed..b10d8197c5 100644
--- a/src/locking/lock_driver_sanlock.c
+++ b/src/locking/lock_driver_sanlock.c
@@ -446,10 +446,8 @@ static int virLockManagerSanlockInit(unsigned int version,
goto error;
}
- if (configFile &&
- virLockManagerSanlockLoadConfig(driver, configFile) < 0) {
+ if (virLockManagerSanlockLoadConfig(driver, configFile) < 0)
goto error;
- }
if (driver->autoDiskLease && !driver->hostID) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
diff --git a/src/locking/lock_manager.c b/src/locking/lock_manager.c
index 9067f5a01a..d421b6acfc 100644
--- a/src/locking/lock_manager.c
+++ b/src/locking/lock_manager.c
@@ -104,8 +104,6 @@ static void virLockManagerLogParams(size_t nparams,
/**
* virLockManagerPluginNew:
* @name: the name of the plugin
- * @driverName: the hypervisor driver that loads the plugin
- * @configDir: path to dir where config files are stored
* @flag: optional plugin flags
*
* Attempt to load the plugin $(libdir)/libvirt/lock-driver/(a)name.so
@@ -131,13 +129,11 @@ virLockManagerPluginPtr virLockManagerPluginNew(const char *name,
char *configFile = NULL;
VIR_DEBUG("name=%s driverName=%s configDir=%s flags=0x%x",
- name, NULLSTR(driverName), NULLSTR(configDir), flags);
+ name, driverName, configDir, flags);
- if (driverName && configDir &&
- virAsprintf(&configFile, "%s/%s-%s.conf",
- configDir, driverName, name) < 0) {
+ if (virAsprintf(&configFile, "%s/%s-%s.conf",
+ configDir, driverName, name) < 0)
return NULL;
- }
if (STREQ(name, "nop")) {
driver = &virLockDriverNop;
--
2.18.0
7:03 a.m.
New subject: [libvirt] [PATCH v2 06/11] Revert "lock_driver: Introduce VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK"
This reverts commit 385eb8399bdb1610447c2857abfe99cee4a9fb9e.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/locking/lock_driver.h | 4 --
src/locking/lock_driver_lockd.c | 82 ++++++++++-----------------------
2 files changed, 24 insertions(+), 62 deletions(-)
diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h
index 7c8f744be3..9be0abcfba 100644
--- a/src/locking/lock_driver.h
+++ b/src/locking/lock_driver.h
@@ -67,10 +67,6 @@ typedef enum {
VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY = (1 << 0),
/* Prevent further lock/unlock calls from this process */
VIR_LOCK_MANAGER_ACQUIRE_RESTRICT = (1 << 1),
- /* Used when acquiring more resources in which one of them
- * can't be acquired, perform a rollback and release all
- * resources acquired so far. */
- VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK = (1 << 2),
} virLockManagerAcquireFlags;
typedef enum {
diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
index 85cdcf97be..d6551e125c 100644
--- a/src/locking/lock_driver_lockd.c
+++ b/src/locking/lock_driver_lockd.c
@@ -735,34 +735,6 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr
lock,
}
-static int virLockManagerLockDaemonReleaseImpl(virNetClientPtr client,
- virNetClientProgramPtr program,
- int counter,
- virLockManagerLockDaemonResourcePtr res)
-{
- virLockSpaceProtocolReleaseResourceArgs args;
-
- memset(&args, 0, sizeof(args));
-
- args.path = res->lockspace;
- args.name = res->name;
- args.flags = res->flags;
-
- args.flags &=
- ~(VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED |
- VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE |
- VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA);
-
- return virNetClientProgramCall(program,
- client,
- counter,
- VIR_LOCK_SPACE_PROTOCOL_PROC_RELEASE_RESOURCE,
- 0, NULL, NULL, NULL,
-
(xdrproc_t)xdr_virLockSpaceProtocolReleaseResourceArgs, &args,
- (xdrproc_t)xdr_void, NULL);
-}
-
-
static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
const char *state ATTRIBUTE_UNUSED,
unsigned int flags,
@@ -773,13 +745,10 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
virNetClientProgramPtr program = NULL;
int counter = 0;
int rv = -1;
- ssize_t i;
- ssize_t lastGood = -1;
virLockManagerLockDaemonPrivatePtr priv = lock->privateData;
virCheckFlags(VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY |
- VIR_LOCK_MANAGER_ACQUIRE_RESTRICT |
- VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK, -1);
+ VIR_LOCK_MANAGER_ACQUIRE_RESTRICT, -1);
if (priv->type == VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN &&
priv->nresources == 0 &&
@@ -798,6 +767,7 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
goto cleanup;
if (!(flags & VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY)) {
+ size_t i;
for (i = 0; i < priv->nresources; i++) {
virLockSpaceProtocolAcquireResourceArgs args;
@@ -815,7 +785,6 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
(xdrproc_t)xdr_virLockSpaceProtocolAcquireResourceArgs, &args,
(xdrproc_t)xdr_void, NULL) < 0)
goto cleanup;
- lastGood = i;
}
}
@@ -826,28 +795,8 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
rv = 0;
cleanup:
- if (rv < 0) {
- int saved_errno = errno;
- virErrorPtr origerr;
-
- virErrorPreserveLast(&origerr);
- if (fd)
- VIR_FORCE_CLOSE(*fd);
-
- if (flags & VIR_LOCK_MANAGER_ACQUIRE_ROLLBACK) {
- for (i = lastGood; i >= 0; i--) {
- virLockManagerLockDaemonResourcePtr res = &priv->resources[i];
-
- if (virLockManagerLockDaemonReleaseImpl(client, program,
- counter++, res) < 0)
- VIR_WARN("Unable to release resource lockspace=%s
name=%s",
- res->lockspace, res->name);
- }
- }
-
- virErrorRestore(&origerr);
- errno = saved_errno;
- }
+ if (rv != 0 && fd)
+ VIR_FORCE_CLOSE(*fd);
virNetClientClose(client);
virObjectUnref(client);
virObjectUnref(program);
@@ -875,10 +824,27 @@ static int virLockManagerLockDaemonRelease(virLockManagerPtr lock,
goto cleanup;
for (i = 0; i < priv->nresources; i++) {
- virLockManagerLockDaemonResourcePtr res = &priv->resources[i];
+ virLockSpaceProtocolReleaseResourceArgs args;
- if (virLockManagerLockDaemonReleaseImpl(client, program,
- counter++, res) < 0)
+ memset(&args, 0, sizeof(args));
+
+ if (priv->resources[i].lockspace)
+ args.path = priv->resources[i].lockspace;
+ args.name = priv->resources[i].name;
+ args.flags = priv->resources[i].flags;
+
+ args.flags &=
+ ~(VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED |
+ VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE |
+ VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA);
+
+ if (virNetClientProgramCall(program,
+ client,
+ counter++,
+ VIR_LOCK_SPACE_PROTOCOL_PROC_RELEASE_RESOURCE,
+ 0, NULL, NULL, NULL,
+
(xdrproc_t)xdr_virLockSpaceProtocolReleaseResourceArgs, &args,
+ (xdrproc_t)xdr_void, NULL) < 0)
goto cleanup;
}
--
2.18.0
7:03 a.m.
New subject: [libvirt] [PATCH v2 07/11] Revert "lock_driver: Introduce VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA"
This reverts commit 997283b54b0e1f599aed3085ceba027eb8110acb.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/locking/lock_driver.h | 2 --
src/locking/lock_driver_lockd.c | 47 +++++++++----------------------
src/locking/lock_driver_sanlock.c | 3 +-
3 files changed, 14 insertions(+), 38 deletions(-)
diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h
index 9be0abcfba..a9d2041c30 100644
--- a/src/locking/lock_driver.h
+++ b/src/locking/lock_driver.h
@@ -51,8 +51,6 @@ typedef enum {
VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK = 0,
/* A lease against an arbitrary resource */
VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE = 1,
- /* The resource to be locked is a metadata */
- VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA = 2,
} virLockManagerResourceType;
typedef enum {
diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
index d6551e125c..268676c407 100644
--- a/src/locking/lock_driver_lockd.c
+++ b/src/locking/lock_driver_lockd.c
@@ -563,7 +563,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr
lock,
virLockManagerLockDaemonPrivatePtr priv = lock->privateData;
char *newName = NULL;
char *newLockspace = NULL;
- int newFlags = 0;
+ bool autoCreate = false;
int ret = -1;
virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY |
@@ -575,7 +575,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr
lock,
switch (priv->type) {
case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN:
- switch ((virLockManagerResourceType) type) {
+ switch (type) {
case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK:
if (params || nparams) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -602,7 +602,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr
lock,
VIR_DEBUG("Got an LVM UUID %s for %s", newName, name);
if (VIR_STRDUP(newLockspace, driver->lvmLockSpaceDir) < 0)
goto cleanup;
- newFlags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE;
+ autoCreate = true;
break;
}
virResetLastError();
@@ -619,7 +619,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr
lock,
VIR_DEBUG("Got an SCSI ID %s for %s", newName, name);
if (VIR_STRDUP(newLockspace, driver->scsiLockSpaceDir) < 0)
goto cleanup;
- newFlags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE;
+ autoCreate = true;
break;
}
virResetLastError();
@@ -631,7 +631,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr
lock,
goto cleanup;
if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, name, &newName) <
0)
goto cleanup;
- newFlags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE;
+ autoCreate = true;
VIR_DEBUG("Using indirect lease %s for %s", newName, name);
} else {
if (VIR_STRDUP(newLockspace, "") < 0)
@@ -676,8 +676,6 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr
lock,
goto cleanup;
} break;
-
- case VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA:
default:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unknown lock manager object type %d for domain lock
object"),
@@ -687,29 +685,6 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr
lock,
break;
case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON:
- switch ((virLockManagerResourceType) type) {
- case VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA:
- if (params || nparams) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Unexpected parameters for metadata
resource"));
- goto cleanup;
- }
- if (VIR_STRDUP(newLockspace, "") < 0 ||
- VIR_STRDUP(newName, name) < 0)
- goto cleanup;
- newFlags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA;
- break;
-
- case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK:
- case VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE:
- default:
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unknown lock manager object type %d for daemon lock
object"),
- type);
- goto cleanup;
- }
- break;
-
default:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unknown lock manager object type %d"),
@@ -717,15 +692,19 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr
lock,
goto cleanup;
}
- if (flags & VIR_LOCK_MANAGER_RESOURCE_SHARED)
- newFlags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED;
-
if (VIR_EXPAND_N(priv->resources, priv->nresources, 1) < 0)
goto cleanup;
VIR_STEAL_PTR(priv->resources[priv->nresources-1].lockspace, newLockspace);
VIR_STEAL_PTR(priv->resources[priv->nresources-1].name, newName);
- priv->resources[priv->nresources-1].flags = newFlags;
+
+ if (flags & VIR_LOCK_MANAGER_RESOURCE_SHARED)
+ priv->resources[priv->nresources-1].flags |=
+ VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED;
+
+ if (autoCreate)
+ priv->resources[priv->nresources-1].flags |=
+ VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE;
ret = 0;
cleanup:
diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c
index b10d8197c5..86efc83b5a 100644
--- a/src/locking/lock_driver_sanlock.c
+++ b/src/locking/lock_driver_sanlock.c
@@ -811,7 +811,7 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock,
if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY)
return 0;
- switch ((virLockManagerResourceType) type) {
+ switch (type) {
case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK:
if (driver->autoDiskLease) {
if (virLockManagerSanlockAddDisk(driver, lock, name, nparams, params,
@@ -835,7 +835,6 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock,
return -1;
break;
- case VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA:
default:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unknown lock manager object type %d for domain lock
object"),
--
2.18.0
7:03 a.m.
New subject: [libvirt] [PATCH v2 08/11] Revert "_virLockManagerLockDaemonPrivate: Move @hasRWDisks into dom union"
This reverts commit aaf34cb9013d6d746f4edf9807408cb9dfbcf01d.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/locking/lock_driver_lockd.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
index 268676c407..22a5a97913 100644
--- a/src/locking/lock_driver_lockd.c
+++ b/src/locking/lock_driver_lockd.c
@@ -63,8 +63,6 @@ struct _virLockManagerLockDaemonPrivate {
char *name;
int id;
pid_t pid;
-
- bool hasRWDisks;
} dom;
struct {
@@ -76,6 +74,8 @@ struct _virLockManagerLockDaemonPrivate {
size_t nresources;
virLockManagerLockDaemonResourcePtr resources;
+
+ bool hasRWDisks;
};
@@ -585,7 +585,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr
lock,
if (!driver->autoDiskLease) {
if (!(flags & (VIR_LOCK_MANAGER_RESOURCE_SHARED |
VIR_LOCK_MANAGER_RESOURCE_READONLY)))
- priv->t.dom.hasRWDisks = true;
+ priv->hasRWDisks = true;
return 0;
}
@@ -731,7 +731,7 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
if (priv->type == VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN &&
priv->nresources == 0 &&
- priv->t.dom.hasRWDisks &&
+ priv->hasRWDisks &&
driver->requireLeaseForDisks) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Read/write, exclusive access, disks were present, but no
leases specified"));
--
2.18.0
7:03 a.m.
New subject: [libvirt] [PATCH v2 09/11] Revert "lock_driver: Introduce new VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON"
This reverts commit 22baf6e08c65d9174b24f66370724ce961ce9576.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/locking/lock_driver.h | 2 -
src/locking/lock_driver_lockd.c | 297 +++++++++++-------------------
src/locking/lock_driver_sanlock.c | 37 ++--
3 files changed, 116 insertions(+), 220 deletions(-)
diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h
index a9d2041c30..8b7cccc521 100644
--- a/src/locking/lock_driver.h
+++ b/src/locking/lock_driver.h
@@ -42,8 +42,6 @@ typedef enum {
typedef enum {
/* The managed object is a virtual guest domain */
VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN = 0,
- /* The managed object is a daemon (e.g. libvirtd) */
- VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON = 1,
} virLockManagerObjectType;
typedef enum {
diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
index 22a5a97913..ca825e6026 100644
--- a/src/locking/lock_driver_lockd.c
+++ b/src/locking/lock_driver_lockd.c
@@ -56,21 +56,10 @@ struct _virLockManagerLockDaemonResource {
};
struct _virLockManagerLockDaemonPrivate {
- virLockManagerObjectType type;
- union {
- struct {
- unsigned char uuid[VIR_UUID_BUFLEN];
- char *name;
- int id;
- pid_t pid;
- } dom;
-
- struct {
- unsigned char uuid[VIR_UUID_BUFLEN];
- char *name;
- pid_t pid;
- } daemon;
- } t;
+ unsigned char uuid[VIR_UUID_BUFLEN];
+ char *name;
+ int id;
+ pid_t pid;
size_t nresources;
virLockManagerLockDaemonResourcePtr resources;
@@ -167,30 +156,10 @@ virLockManagerLockDaemonConnectionRegister(virLockManagerPtr lock,
memset(&args, 0, sizeof(args));
args.flags = 0;
-
- switch (priv->type) {
- case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN:
- memcpy(args.owner.uuid, priv->t.dom.uuid, VIR_UUID_BUFLEN);
- args.owner.name = priv->t.dom.name;
- args.owner.id = priv->t.dom.id;
- args.owner.pid = priv->t.dom.pid;
- break;
-
- case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON:
- memcpy(args.owner.uuid, priv->t.daemon.uuid, VIR_UUID_BUFLEN);
- args.owner.name = priv->t.daemon.name;
- args.owner.pid = priv->t.daemon.pid;
- /* This one should not be needed. However, virtlockd
- * checks for ID because not every domain has a PID. */
- args.owner.id = priv->t.daemon.pid;
- break;
-
- default:
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unknown lock manager object type %d"),
- priv->type);
- return -1;
- }
+ memcpy(args.owner.uuid, priv->uuid, VIR_UUID_BUFLEN);
+ args.owner.name = priv->name;
+ args.owner.id = priv->id;
+ args.owner.pid = priv->pid;
if (virNetClientProgramCall(program,
client,
@@ -422,18 +391,7 @@
virLockManagerLockDaemonPrivateFree(virLockManagerLockDaemonPrivatePtr priv)
}
VIR_FREE(priv->resources);
- switch (priv->type) {
- case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN:
- VIR_FREE(priv->t.dom.name);
- break;
-
- case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON:
- VIR_FREE(priv->t.daemon.name);
- break;
-
- default:
- break;
- }
+ VIR_FREE(priv->name);
VIR_FREE(priv);
}
@@ -462,82 +420,46 @@ static int virLockManagerLockDaemonNew(virLockManagerPtr lock,
if (VIR_ALLOC(priv) < 0)
return -1;
- priv->type = type;
-
- switch ((virLockManagerObjectType) type) {
+ switch (type) {
case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN:
for (i = 0; i < nparams; i++) {
if (STREQ(params[i].key, "uuid")) {
- memcpy(priv->t.dom.uuid, params[i].value.uuid, VIR_UUID_BUFLEN);
+ memcpy(priv->uuid, params[i].value.uuid, VIR_UUID_BUFLEN);
} else if (STREQ(params[i].key, "name")) {
- if (VIR_STRDUP(priv->t.dom.name, params[i].value.str) < 0)
+ if (VIR_STRDUP(priv->name, params[i].value.str) < 0)
goto cleanup;
} else if (STREQ(params[i].key, "id")) {
- priv->t.dom.id = params[i].value.iv;
+ priv->id = params[i].value.iv;
} else if (STREQ(params[i].key, "pid")) {
- priv->t.dom.pid = params[i].value.iv;
+ priv->pid = params[i].value.iv;
} else if (STREQ(params[i].key, "uri")) {
/* ignored */
} else {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unexpected parameter %s for domain object"),
+ _("Unexpected parameter %s for object"),
params[i].key);
goto cleanup;
}
}
- if (priv->t.dom.id == 0) {
+ if (priv->id == 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Missing ID parameter for domain object"));
goto cleanup;
}
- if (priv->t.dom.pid == 0)
+ if (priv->pid == 0)
VIR_DEBUG("Missing PID parameter for domain object");
- if (!priv->t.dom.name) {
+ if (!priv->name) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Missing name parameter for domain object"));
goto cleanup;
}
- if (!virUUIDIsValid(priv->t.dom.uuid)) {
+ if (!virUUIDIsValid(priv->uuid)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Missing UUID parameter for domain object"));
goto cleanup;
}
break;
- case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON:
- for (i = 0; i < nparams; i++) {
- if (STREQ(params[i].key, "uuid")) {
- memcpy(priv->t.daemon.uuid, params[i].value.uuid, VIR_UUID_BUFLEN);
- } else if (STREQ(params[i].key, "name")) {
- if (VIR_STRDUP(priv->t.daemon.name, params[i].value.str) < 0)
- goto cleanup;
- } else if (STREQ(params[i].key, "pid")) {
- priv->t.daemon.pid = params[i].value.iv;
- } else {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unexpected parameter %s for daemon object"),
- params[i].key);
- goto cleanup;
- }
- }
-
- if (!virUUIDIsValid(priv->t.daemon.uuid)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Missing UUID parameter for daemon object"));
- goto cleanup;
- }
- if (!priv->t.daemon.name) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Missing name parameter for daemon object"));
- goto cleanup;
- }
- if (priv->t.daemon.pid == 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Missing PID parameter for daemon object"));
- goto cleanup;
- }
- break;
-
default:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unknown lock manager object type %d"),
@@ -572,119 +494,107 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr
lock,
if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY)
return 0;
- switch (priv->type) {
- case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN:
+ switch (type) {
+ case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK:
+ if (params || nparams) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Unexpected parameters for disk resource"));
+ goto cleanup;
+ }
+ if (!driver->autoDiskLease) {
+ if (!(flags & (VIR_LOCK_MANAGER_RESOURCE_SHARED |
+ VIR_LOCK_MANAGER_RESOURCE_READONLY)))
+ priv->hasRWDisks = true;
+ return 0;
+ }
- switch (type) {
- case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK:
- if (params || nparams) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Unexpected parameters for disk resource"));
+ /* XXX we should somehow pass in TYPE=BLOCK info
+ * from the domain_lock code, instead of assuming /dev
+ */
+ if (STRPREFIX(name, "/dev") &&
+ driver->lvmLockSpaceDir) {
+ VIR_DEBUG("Trying to find an LVM UUID for %s", name);
+ if (virStorageFileGetLVMKey(name, &newName) < 0)
goto cleanup;
- }
- if (!driver->autoDiskLease) {
- if (!(flags & (VIR_LOCK_MANAGER_RESOURCE_SHARED |
- VIR_LOCK_MANAGER_RESOURCE_READONLY)))
- priv->hasRWDisks = true;
- return 0;
- }
- /* XXX we should somehow pass in TYPE=BLOCK info
- * from the domain_lock code, instead of assuming /dev
- */
- if (STRPREFIX(name, "/dev") &&
- driver->lvmLockSpaceDir) {
- VIR_DEBUG("Trying to find an LVM UUID for %s", name);
- if (virStorageFileGetLVMKey(name, &newName) < 0)
+ if (newName) {
+ VIR_DEBUG("Got an LVM UUID %s for %s", newName, name);
+ if (VIR_STRDUP(newLockspace, driver->lvmLockSpaceDir) < 0)
goto cleanup;
-
- if (newName) {
- VIR_DEBUG("Got an LVM UUID %s for %s", newName, name);
- if (VIR_STRDUP(newLockspace, driver->lvmLockSpaceDir) < 0)
- goto cleanup;
- autoCreate = true;
- break;
- }
- virResetLastError();
- /* Fallback to generic non-block code */
+ autoCreate = true;
+ break;
}
+ virResetLastError();
+ /* Fallback to generic non-block code */
+ }
- if (STRPREFIX(name, "/dev") &&
- driver->scsiLockSpaceDir) {
- VIR_DEBUG("Trying to find an SCSI ID for %s", name);
- if (virStorageFileGetSCSIKey(name, &newName) < 0)
- goto cleanup;
-
- if (newName) {
- VIR_DEBUG("Got an SCSI ID %s for %s", newName, name);
- if (VIR_STRDUP(newLockspace, driver->scsiLockSpaceDir) < 0)
- goto cleanup;
- autoCreate = true;
- break;
- }
- virResetLastError();
- /* Fallback to generic non-block code */
- }
+ if (STRPREFIX(name, "/dev") &&
+ driver->scsiLockSpaceDir) {
+ VIR_DEBUG("Trying to find an SCSI ID for %s", name);
+ if (virStorageFileGetSCSIKey(name, &newName) < 0)
+ goto cleanup;
- if (driver->fileLockSpaceDir) {
- if (VIR_STRDUP(newLockspace, driver->fileLockSpaceDir) < 0)
- goto cleanup;
- if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, name, &newName) <
0)
+ if (newName) {
+ VIR_DEBUG("Got an SCSI ID %s for %s", newName, name);
+ if (VIR_STRDUP(newLockspace, driver->scsiLockSpaceDir) < 0)
goto cleanup;
autoCreate = true;
- VIR_DEBUG("Using indirect lease %s for %s", newName, name);
- } else {
- if (VIR_STRDUP(newLockspace, "") < 0)
- goto cleanup;
- if (VIR_STRDUP(newName, name) < 0)
- goto cleanup;
- VIR_DEBUG("Using direct lease for %s", name);
+ break;
}
+ virResetLastError();
+ /* Fallback to generic non-block code */
+ }
- break;
- case VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE: {
- size_t i;
- char *path = NULL;
- char *lockspace = NULL;
- for (i = 0; i < nparams; i++) {
- if (STREQ(params[i].key, "offset")) {
- if (params[i].value.ul != 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Offset must be zero for this lock
manager"));
- goto cleanup;
- }
- } else if (STREQ(params[i].key, "lockspace")) {
- lockspace = params[i].value.str;
- } else if (STREQ(params[i].key, "path")) {
- path = params[i].value.str;
- } else {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unexpected parameter %s for lease
resource"),
- params[i].key);
- goto cleanup;
- }
- }
- if (!path || !lockspace) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Missing path or lockspace for lease
resource"));
+ if (driver->fileLockSpaceDir) {
+ if (VIR_STRDUP(newLockspace, driver->fileLockSpaceDir) < 0)
goto cleanup;
- }
- if (virAsprintf(&newLockspace, "%s/%s",
- path, lockspace) < 0)
+ if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, name, &newName) < 0)
+ goto cleanup;
+ autoCreate = true;
+ VIR_DEBUG("Using indirect lease %s for %s", newName, name);
+ } else {
+ if (VIR_STRDUP(newLockspace, "") < 0)
goto cleanup;
if (VIR_STRDUP(newName, name) < 0)
goto cleanup;
-
- } break;
- default:
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unknown lock manager object type %d for domain lock
object"),
- type);
- goto cleanup;
+ VIR_DEBUG("Using direct lease for %s", name);
}
+
break;
+ case VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE: {
+ size_t i;
+ char *path = NULL;
+ char *lockspace = NULL;
+ for (i = 0; i < nparams; i++) {
+ if (STREQ(params[i].key, "offset")) {
+ if (params[i].value.ul != 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Offset must be zero for this lock
manager"));
+ goto cleanup;
+ }
+ } else if (STREQ(params[i].key, "lockspace")) {
+ lockspace = params[i].value.str;
+ } else if (STREQ(params[i].key, "path")) {
+ path = params[i].value.str;
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unexpected parameter %s for lease
resource"),
+ params[i].key);
+ goto cleanup;
+ }
+ }
+ if (!path || !lockspace) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Missing path or lockspace for lease resource"));
+ goto cleanup;
+ }
+ if (virAsprintf(&newLockspace, "%s/%s",
+ path, lockspace) < 0)
+ goto cleanup;
+ if (VIR_STRDUP(newName, name) < 0)
+ goto cleanup;
- case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON:
+ } break;
default:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unknown lock manager object type %d"),
@@ -729,8 +639,7 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
virCheckFlags(VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY |
VIR_LOCK_MANAGER_ACQUIRE_RESTRICT, -1);
- if (priv->type == VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN &&
- priv->nresources == 0 &&
+ if (priv->nresources == 0 &&
priv->hasRWDisks &&
driver->requireLeaseForDisks) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c
index 86efc83b5a..ff0c9be8f7 100644
--- a/src/locking/lock_driver_sanlock.c
+++ b/src/locking/lock_driver_sanlock.c
@@ -509,32 +509,21 @@ static int virLockManagerSanlockNew(virLockManagerPtr lock,
priv->flags = flags;
- switch ((virLockManagerObjectType) type) {
- case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN:
- for (i = 0; i < nparams; i++) {
- param = ¶ms[i];
+ for (i = 0; i < nparams; i++) {
+ param = ¶ms[i];
- if (STREQ(param->key, "uuid")) {
- memcpy(priv->vm_uuid, param->value.uuid, 16);
- } else if (STREQ(param->key, "name")) {
- if (VIR_STRDUP(priv->vm_name, param->value.str) < 0)
- goto error;
- } else if (STREQ(param->key, "pid")) {
- priv->vm_pid = param->value.iv;
- } else if (STREQ(param->key, "id")) {
- priv->vm_id = param->value.ui;
- } else if (STREQ(param->key, "uri")) {
- priv->vm_uri = param->value.cstr;
- }
+ if (STREQ(param->key, "uuid")) {
+ memcpy(priv->vm_uuid, param->value.uuid, 16);
+ } else if (STREQ(param->key, "name")) {
+ if (VIR_STRDUP(priv->vm_name, param->value.str) < 0)
+ goto error;
+ } else if (STREQ(param->key, "pid")) {
+ priv->vm_pid = param->value.iv;
+ } else if (STREQ(param->key, "id")) {
+ priv->vm_id = param->value.ui;
+ } else if (STREQ(param->key, "uri")) {
+ priv->vm_uri = param->value.cstr;
}
- break;
-
- case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON:
- default:
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unknown lock manager object type %d"),
- type);
- goto error;
}
/* Sanlock needs process registration, but the only way how to probe
--
2.18.0
7:03 a.m.
New subject: [libvirt] [PATCH v2 10/11] Revert "lock_driver_lockd: Introduce VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA flag"
This reverts commit 21c34b86be5233634eb38f77be64e2263bfc4e48.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/locking/lock_daemon_dispatch.c | 10 ++--------
src/locking/lock_driver_lockd.c | 3 +--
src/locking/lock_driver_lockd.h | 1 -
3 files changed, 3 insertions(+), 11 deletions(-)
diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c
index a683ad3d6b..10248ec0b5 100644
--- a/src/locking/lock_daemon_dispatch.c
+++ b/src/locking/lock_daemon_dispatch.c
@@ -37,9 +37,6 @@ VIR_LOG_INIT("locking.lock_daemon_dispatch");
#include "lock_daemon_dispatch_stubs.h"
-#define DEFAULT_OFFSET 0
-#define METADATA_OFFSET 1
-
static int
virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNUSED,
virNetServerClientPtr client,
@@ -53,14 +50,13 @@ virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server
ATTRIBUTE_UNU
virNetServerClientGetPrivateData(client);
virLockSpacePtr lockspace;
unsigned int newFlags;
- off_t start = DEFAULT_OFFSET;
+ off_t start = 0;
off_t len = 1;
virMutexLock(&priv->lock);
virCheckFlagsGoto(VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED |
- VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE |
- VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA, cleanup);
+ VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE, cleanup);
if (priv->restricted) {
virReportError(VIR_ERR_OPERATION_DENIED, "%s",
@@ -86,8 +82,6 @@ virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server
ATTRIBUTE_UNU
newFlags |= VIR_LOCK_SPACE_ACQUIRE_SHARED;
if (flags & VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE)
newFlags |= VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE;
- if (flags & VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA)
- start = METADATA_OFFSET;
if (virLockSpaceAcquireResource(lockspace,
args->name,
diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
index ca825e6026..16fce551c3 100644
--- a/src/locking/lock_driver_lockd.c
+++ b/src/locking/lock_driver_lockd.c
@@ -723,8 +723,7 @@ static int virLockManagerLockDaemonRelease(virLockManagerPtr lock,
args.flags &=
~(VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED |
- VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE |
- VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA);
+ VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE);
if (virNetClientProgramCall(program,
client,
diff --git a/src/locking/lock_driver_lockd.h b/src/locking/lock_driver_lockd.h
index bebd804365..6931fe7425 100644
--- a/src/locking/lock_driver_lockd.h
+++ b/src/locking/lock_driver_lockd.h
@@ -25,7 +25,6 @@
enum virLockSpaceProtocolAcquireResourceFlags {
VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED = (1 << 0),
VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE = (1 << 1),
- VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA = (1 << 2),
};
#endif /* __VIR_LOCK_DRIVER_LOCKD_H__ */
--
2.18.0
7:03 a.m.
New subject: [libvirt] [PATCH v2 11/11] Revert "virlockspace: Allow caller to specify start and length offset in virLockSpaceAcquireResource"
This reverts commit afd5a27575e8b6a494d2728552fe0e89c71e32b4.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/locking/lock_daemon_dispatch.c | 3 ---
src/util/virlockspace.c | 15 +++++----------
src/util/virlockspace.h | 4 ----
tests/virlockspacetest.c | 29 +++++------------------------
4 files changed, 10 insertions(+), 41 deletions(-)
diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c
index 10248ec0b5..1b479db55d 100644
--- a/src/locking/lock_daemon_dispatch.c
+++ b/src/locking/lock_daemon_dispatch.c
@@ -50,8 +50,6 @@ virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server
ATTRIBUTE_UNU
virNetServerClientGetPrivateData(client);
virLockSpacePtr lockspace;
unsigned int newFlags;
- off_t start = 0;
- off_t len = 1;
virMutexLock(&priv->lock);
@@ -86,7 +84,6 @@ virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server
ATTRIBUTE_UNU
if (virLockSpaceAcquireResource(lockspace,
args->name,
priv->ownerPid,
- start, len,
newFlags) < 0)
goto cleanup;
diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c
index 79fa48d365..0736b4b85b 100644
--- a/src/util/virlockspace.c
+++ b/src/util/virlockspace.c
@@ -115,10 +115,8 @@ static void virLockSpaceResourceFree(virLockSpaceResourcePtr res)
static virLockSpaceResourcePtr
virLockSpaceResourceNew(virLockSpacePtr lockspace,
const char *resname,
- pid_t owner,
- off_t start,
- off_t len,
- unsigned int flags)
+ unsigned int flags,
+ pid_t owner)
{
virLockSpaceResourcePtr res;
bool shared = !!(flags & VIR_LOCK_SPACE_ACQUIRE_SHARED);
@@ -159,7 +157,7 @@ virLockSpaceResourceNew(virLockSpacePtr lockspace,
goto error;
}
- if (virFileLock(res->fd, shared, start, len, false) < 0) {
+ if (virFileLock(res->fd, shared, 0, 1, false) < 0) {
if (errno == EACCES || errno == EAGAIN) {
virReportError(VIR_ERR_RESOURCE_BUSY,
_("Lockspace resource '%s' is
locked"),
@@ -206,7 +204,7 @@ virLockSpaceResourceNew(virLockSpacePtr lockspace,
goto error;
}
- if (virFileLock(res->fd, shared, start, len, false) < 0) {
+ if (virFileLock(res->fd, shared, 0, 1, false) < 0) {
if (errno == EACCES || errno == EAGAIN) {
virReportError(VIR_ERR_RESOURCE_BUSY,
_("Lockspace resource '%s' is locked"),
@@ -614,8 +612,6 @@ int virLockSpaceDeleteResource(virLockSpacePtr lockspace,
int virLockSpaceAcquireResource(virLockSpacePtr lockspace,
const char *resname,
pid_t owner,
- off_t start,
- off_t len,
unsigned int flags)
{
int ret = -1;
@@ -645,8 +641,7 @@ int virLockSpaceAcquireResource(virLockSpacePtr lockspace,
goto cleanup;
}
- if (!(res = virLockSpaceResourceNew(lockspace, resname,
- owner, start, len, flags)))
+ if (!(res = virLockSpaceResourceNew(lockspace, resname, flags, owner)))
goto cleanup;
if (virHashAddEntry(lockspace->resources, resname, res) < 0) {
diff --git a/src/util/virlockspace.h b/src/util/virlockspace.h
index 24f2c89be6..041cf20396 100644
--- a/src/util/virlockspace.h
+++ b/src/util/virlockspace.h
@@ -22,8 +22,6 @@
#ifndef __VIR_LOCK_SPACE_H__
# define __VIR_LOCK_SPACE_H__
-# include <sys/types.h>
-
# include "internal.h"
# include "virjson.h"
@@ -52,8 +50,6 @@ typedef enum {
int virLockSpaceAcquireResource(virLockSpacePtr lockspace,
const char *resname,
pid_t owner,
- off_t start,
- off_t len,
unsigned int flags);
int virLockSpaceReleaseResource(virLockSpacePtr lockspace,
diff --git a/tests/virlockspacetest.c b/tests/virlockspacetest.c
index 3c621e7eb0..93353be285 100644
--- a/tests/virlockspacetest.c
+++ b/tests/virlockspacetest.c
@@ -98,8 +98,6 @@ static int testLockSpaceResourceLockExcl(const void *args
ATTRIBUTE_UNUSED)
{
virLockSpacePtr lockspace;
int ret = -1;
- const off_t start = 0;
- const off_t len = 1;
rmdir(LOCKSPACE_DIR);
@@ -112,13 +110,13 @@ static int testLockSpaceResourceLockExcl(const void *args
ATTRIBUTE_UNUSED)
if (virLockSpaceCreateResource(lockspace, "foo") < 0)
goto cleanup;
- if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), start, len, 0)
< 0)
+ if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), 0) < 0)
goto cleanup;
if (!virFileExists(LOCKSPACE_DIR "/foo"))
goto cleanup;
- if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), start, len, 0)
== 0)
+ if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), 0) == 0)
goto cleanup;
if (virLockSpaceDeleteResource(lockspace, "foo") == 0)
@@ -146,8 +144,6 @@ static int testLockSpaceResourceLockExclAuto(const void *args
ATTRIBUTE_UNUSED)
{
virLockSpacePtr lockspace;
int ret = -1;
- const off_t start = 0;
- const off_t len = 1;
rmdir(LOCKSPACE_DIR);
@@ -161,7 +157,6 @@ static int testLockSpaceResourceLockExclAuto(const void *args
ATTRIBUTE_UNUSED)
goto cleanup;
if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(),
- start, len,
VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE) < 0)
goto cleanup;
@@ -187,8 +182,6 @@ static int testLockSpaceResourceLockShr(const void *args
ATTRIBUTE_UNUSED)
{
virLockSpacePtr lockspace;
int ret = -1;
- const off_t start = 0;
- const off_t len = 1;
rmdir(LOCKSPACE_DIR);
@@ -202,16 +195,13 @@ static int testLockSpaceResourceLockShr(const void *args
ATTRIBUTE_UNUSED)
goto cleanup;
if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(),
- start, len,
VIR_LOCK_SPACE_ACQUIRE_SHARED) < 0)
goto cleanup;
- if (virLockSpaceAcquireResource(lockspace, "foo",
- geteuid(), start, len, 0) == 0)
+ if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(), 0) == 0)
goto cleanup;
if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(),
- start, len,
VIR_LOCK_SPACE_ACQUIRE_SHARED) < 0)
goto cleanup;
@@ -246,8 +236,6 @@ static int testLockSpaceResourceLockShrAuto(const void *args
ATTRIBUTE_UNUSED)
{
virLockSpacePtr lockspace;
int ret = -1;
- const off_t start = 0;
- const off_t len = 1;
rmdir(LOCKSPACE_DIR);
@@ -261,7 +249,6 @@ static int testLockSpaceResourceLockShrAuto(const void *args
ATTRIBUTE_UNUSED)
goto cleanup;
if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(),
- start, len,
VIR_LOCK_SPACE_ACQUIRE_SHARED |
VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE) < 0)
goto cleanup;
@@ -270,7 +257,6 @@ static int testLockSpaceResourceLockShrAuto(const void *args
ATTRIBUTE_UNUSED)
goto cleanup;
if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(),
- start, len,
VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE) == 0)
goto cleanup;
@@ -278,7 +264,6 @@ static int testLockSpaceResourceLockShrAuto(const void *args
ATTRIBUTE_UNUSED)
goto cleanup;
if (virLockSpaceAcquireResource(lockspace, "foo", geteuid(),
- start, len,
VIR_LOCK_SPACE_ACQUIRE_SHARED |
VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE) < 0)
goto cleanup;
@@ -311,8 +296,6 @@ static int testLockSpaceResourceLockPath(const void *args
ATTRIBUTE_UNUSED)
{
virLockSpacePtr lockspace;
int ret = -1;
- const off_t start = 0;
- const off_t len = 1;
rmdir(LOCKSPACE_DIR);
@@ -325,15 +308,13 @@ static int testLockSpaceResourceLockPath(const void *args
ATTRIBUTE_UNUSED)
if (virLockSpaceCreateResource(lockspace, LOCKSPACE_DIR "/foo") < 0)
goto cleanup;
- if (virLockSpaceAcquireResource(lockspace, LOCKSPACE_DIR "/foo",
- geteuid(), start, len, 0) < 0)
+ if (virLockSpaceAcquireResource(lockspace, LOCKSPACE_DIR "/foo", geteuid(),
0) < 0)
goto cleanup;
if (!virFileExists(LOCKSPACE_DIR "/foo"))
goto cleanup;
- if (virLockSpaceAcquireResource(lockspace, LOCKSPACE_DIR "/foo",
- geteuid(), start, len, 0) == 0)
+ if (virLockSpaceAcquireResource(lockspace, LOCKSPACE_DIR "/foo", geteuid(),
0) == 0)
goto cleanup;
if (virLockSpaceDeleteResource(lockspace, LOCKSPACE_DIR "/foo") == 0)
--
2.18.0
2318
days inactive
2323
days old
15 comments
2 participants
participants (2)
-
John Ferlan -
Michal Privoznik