[libvirt] [PATCH v2 00/11] Implement alternative metadata locking
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
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@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
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@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;
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@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
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@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
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@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
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@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
This reverts commit 3e26b476b5f322353bf0dcd8e3f037ca672b8c62. Signed-off-by: Michal Privoznik <mprivozn@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
This reverts commit 8b8aefb3d6ae2139ea3d4ef6d7dd2c06f57f6075. Signed-off-by: Michal Privoznik <mprivozn@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
This reverts commit 35b5b244da825fb41e35e4dc62e740d716214ec9. Signed-off-by: Michal Privoznik <mprivozn@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/@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
This reverts commit 385eb8399bdb1610447c2857abfe99cee4a9fb9e. Signed-off-by: Michal Privoznik <mprivozn@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
This reverts commit 997283b54b0e1f599aed3085ceba027eb8110acb. Signed-off-by: Michal Privoznik <mprivozn@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
This reverts commit aaf34cb9013d6d746f4edf9807408cb9dfbcf01d. Signed-off-by: Michal Privoznik <mprivozn@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
This reverts commit 22baf6e08c65d9174b24f66370724ce961ce9576. Signed-off-by: Michal Privoznik <mprivozn@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
This reverts commit 21c34b86be5233634eb38f77be64e2263bfc4e48. Signed-off-by: Michal Privoznik <mprivozn@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
This reverts commit afd5a27575e8b6a494d2728552fe0e89c71e32b4. Signed-off-by: Michal Privoznik <mprivozn@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
participants (2)
-
John Ferlan -
Michal Privoznik