The only thing that the @optional argument does is that it makes
the function return 1 instead of 0 if setting SELinux context
failed in a non-critical fashion. Drop the argument then and
return 1 in that case. This enables caller to learn if SELinux
context was set or not.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/security/security_selinux.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 0523613d4a..35385f4a23 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1261,19 +1261,23 @@ virSecuritySELinuxGetProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
  * virSecuritySELinuxSetFileconImpl:
  * @path: path to the file to set context on
  * @tcon: target context to set
- * @optional: whether to treat errors as fatal
  * @privileged: whether running as privileged user
  *
  * Set @tcon SELinux context on @path. If unable to do so, check SELinux
  * configuration and produce sensible error message suggesting solution.
+ * It may happen that setting context fails but hypervisor will be able to
+ * open the @path successfully. This is because some file systems don't
+ * support SELinux, are RO, or the @path had the correct context from the
+ * start. If that is the case, a positive one is returned.
  *
  * Returns: -1 if failed to set context and SELinux is in enforcing mode
- *           1 if failed to set context and @optional is true
- *           0 otherwise.
+ *           1 if failed to set context,
+ *           0 if context was set successfully.
  */
 static int
-virSecuritySELinuxSetFileconImpl(const char *path, const char *tcon,
-                                 bool optional, bool privileged)
+virSecuritySELinuxSetFileconImpl(const char *path,
+                                 const char *tcon,
+                                 bool privileged)
 {
     security_context_t econ;
 
@@ -1289,7 +1293,7 @@ virSecuritySELinuxSetFileconImpl(const char *path, const char *tcon,
             if (STREQ(tcon, econ)) {
                 freecon(econ);
                 /* It's alright, there's nothing to change anyway. */
-                return optional ? 1 : 0;
+                return 1;
             }
             freecon(econ);
         }
@@ -1326,9 +1330,9 @@ virSecuritySELinuxSetFileconImpl(const char *path, const char *tcon,
                 VIR_INFO("Setting security context '%s' on '%s' not supported",
                          tcon, path);
             }
-            if (optional)
-                return 1;
         }
+
+        return 1;
     }
     return 0;
 }
@@ -1388,7 +1392,7 @@ virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr,
         }
     }
 
-    if (virSecuritySELinuxSetFileconImpl(path, tcon, optional, privileged) < 0)
+    if (virSecuritySELinuxSetFileconImpl(path, tcon, privileged) < 0)
         goto cleanup;
 
     ret = 0;
@@ -1553,7 +1557,7 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr,
         }
     }
 
-    if (virSecuritySELinuxSetFileconImpl(newpath, fcon, false, privileged) < 0)
+    if (virSecuritySELinuxSetFileconImpl(newpath, fcon, privileged) < 0)
         goto cleanup;
 
     ret = 0;
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Aug 22, 2019 at 05:19:05PM +0200, Michal Privoznik wrote:
>The only thing that the @optional argument does is that it makes
>the function return 1 instead of 0 if setting SELinux context
>failed in a non-critical fashion. Drop the argument then and
>return 1 in that case. This enables caller to learn if SELinux
>context was set or not.
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/security/security_selinux.c | 24 ++++++++++++++----------
> 1 file changed, 14 insertions(+), 10 deletions(-)
>
>diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
>index 0523613d4a..35385f4a23 100644
>--- a/src/security/security_selinux.c
>+++ b/src/security/security_selinux.c
>@@ -1261,19 +1261,23 @@ virSecuritySELinuxGetProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
>  * virSecuritySELinuxSetFileconImpl:
>  * @path: path to the file to set context on
>  * @tcon: target context to set
>- * @optional: whether to treat errors as fatal
>  * @privileged: whether running as privileged user
>  *
>  * Set @tcon SELinux context on @path. If unable to do so, check SELinux
>  * configuration and produce sensible error message suggesting solution.
>+ * It may happen that setting context fails but hypervisor will be able to
>+ * open the @path successfully. This is because some file systems don't
>+ * support SELinux, are RO, or the @path had the correct context from the
>+ * start. If that is the case, a positive one is returned.
>  *
>  * Returns: -1 if failed to set context and SELinux is in enforcing mode
>- *           1 if failed to set context and @optional is true
>- *           0 otherwise.
>+ *           1 if failed to set context,
>+ *           0 if context was set successfully.

This is where this does not agree with following patches.  The code works, but
the comment is still a bit unclear, even though it is at least a tad less
confusing (well, the code doesn't really help with constructing a proper
documentation).

I would go with:

     * Returns:  0 if context was set successfully
     *           1 if setting the context failed in a non-critical fashion
     *           -1 in case of error

or something along the lines of the above, critical change being the description
of return value 1.  Feel free to split the change between PATCH 1 and this one
in a way that makes sense or just squash them together, that's fine as well.

If you go with my suggestion for the comment, then:

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list