2:23 a.m.
When hotplugging a /dev device that already exists in the process namespace we
reset all the permissions, labels etc. to the same state as the parent
namespace. Then, when we are trying to label the device for qemu we find out
there are label-remembering attributes with a positive reference count and
because we are trying to set a different label (the device now has the label of
the original device from the parent namespace) we refuse to reset it. Not only
the hotplug fails (purposefully not questioning the usability of hotplugging the
same device for a second time) but it also resets the labels to something we do
not want. Now the quiestion is: What does this patchset do? You can have a guess.
Martin Kletzander (3):
qemu_namespace: Rename variable
qemu_namespace: Properly report new files
qemu_namespace: Only replicate labels on created files
src/qemu/qemu_namespace.c | 64 +++++++++++++++++++++------------------
1 file changed, 34 insertions(+), 30 deletions(-)
--
2.47.0
2:23 a.m.
New subject: [PATCH 1/3] qemu_namespace: Rename variable
The boolean actually tells whether the file existed when the function
was called and using it in more places later on makes them
confusing (e.g. do something with a file if it does not exist). To
better reflect the above and prepare for next patch rename this
variable.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_namespace.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
index bbe3d5a1f78a..71e29b4ba4f6 100644
--- a/src/qemu/qemu_namespace.c
+++ b/src/qemu/qemu_namespace.c
@@ -1002,10 +1002,10 @@ qemuNamespaceMknodOne(qemuNamespaceMknodItem *data)
bool isDev = S_ISCHR(data->sb.st_mode) || S_ISBLK(data->sb.st_mode);
bool isReg = S_ISREG(data->sb.st_mode) || S_ISFIFO(data->sb.st_mode) ||
S_ISSOCK(data->sb.st_mode);
bool isDir = S_ISDIR(data->sb.st_mode);
- bool exists = false;
+ bool existed = false;
if (virFileExists(data->file))
- exists = true;
+ existed = true;
if (virFileMakeParentPath(data->file) < 0) {
virReportSystemError(errno,
@@ -1131,7 +1131,7 @@ qemuNamespaceMknodOne(qemuNamespaceMknodItem *data)
virFileMoveMount(data->target, data->file) < 0)
goto cleanup;
- ret = exists;
+ ret = existed;
cleanup:
if (ret < 0 && delDevice) {
if (isDir)
--
2.47.0
2:32 a.m.
New subject: [PATCH 1/3] qemu_namespace: Rename variable
On Wed, Oct 16, 2024 at 09:23:05 +0200, Martin Kletzander wrote:
The boolean actually tells whether the file existed when the
function
was called and using it in more places later on makes them
confusing (e.g. do something with a file if it does not exist). To
better reflect the above and prepare for next patch rename this
variable.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_namespace.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
index bbe3d5a1f78a..71e29b4ba4f6 100644
--- a/src/qemu/qemu_namespace.c
+++ b/src/qemu/qemu_namespace.c
@@ -1002,10 +1002,10 @@ qemuNamespaceMknodOne(qemuNamespaceMknodItem *data)
bool isDev = S_ISCHR(data->sb.st_mode) || S_ISBLK(data->sb.st_mode);
bool isReg = S_ISREG(data->sb.st_mode) || S_ISFIFO(data->sb.st_mode) ||
S_ISSOCK(data->sb.st_mode);
bool isDir = S_ISDIR(data->sb.st_mode);
- bool exists = false;
+ bool existed = false;
You can also add a comment explaining this further as the function
itself isn't documented to have these semantics.
Anyways ...
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
8:01 a.m.
New subject: [PATCH 1/3] qemu_namespace: Rename variable
On Wed, Oct 16, 2024 at 09:32:54AM +0200, Peter Krempa wrote:
On Wed, Oct 16, 2024 at 09:23:05 +0200, Martin Kletzander wrote:
> The boolean actually tells whether the file existed when the function
> was called and using it in more places later on makes them
> confusing (e.g. do something with a file if it does not exist). To
> better reflect the above and prepare for next patch rename this
> variable.
>
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> src/qemu/qemu_namespace.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
> index bbe3d5a1f78a..71e29b4ba4f6 100644
> --- a/src/qemu/qemu_namespace.c
> +++ b/src/qemu/qemu_namespace.c
> @@ -1002,10 +1002,10 @@ qemuNamespaceMknodOne(qemuNamespaceMknodItem *data)
> bool isDev = S_ISCHR(data->sb.st_mode) || S_ISBLK(data->sb.st_mode);
> bool isReg = S_ISREG(data->sb.st_mode) || S_ISFIFO(data->sb.st_mode) ||
S_ISSOCK(data->sb.st_mode);
> bool isDir = S_ISDIR(data->sb.st_mode);
> - bool exists = false;
> + bool existed = false;
You can also add a comment explaining this further as the function
itself isn't documented to have these semantics.
I thought of that, started documenting a function called "MknodOne" by
saying it recreates all things, not just those needing a mknod. Then I
read you said "can" and took the liberty of leaving this task to someone
who's a better wordsmith.
Anyways ...
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
2:23 a.m.
New subject: [PATCH 2/3] qemu_namespace: Properly report new files
Function qemuNamespaceMknodOne() is supposed to return 0 if the file did
not exist before this function. If, however, the file existed, but was
removed and recreated by this function the @existed flag should be reset
to its original state (false) because the function then behaves the same
way as if the file did not exist as it needed to be recreated.
So reset the @existed flag to properly reflect what happened.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_namespace.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
index 71e29b4ba4f6..33a773917373 100644
--- a/src/qemu/qemu_namespace.c
+++ b/src/qemu/qemu_namespace.c
@@ -1022,6 +1022,7 @@ qemuNamespaceMknodOne(qemuNamespaceMknodItem *data)
data->file, data->target);
} else {
VIR_DEBUG("Creating symlink %s -> %s", data->file,
data->target);
+ existed = false;
/* First, unlink the symlink target. Symlinks change and
* therefore we have no guarantees that pre-existing
@@ -1053,6 +1054,7 @@ qemuNamespaceMknodOne(qemuNamespaceMknodItem *data)
} else {
VIR_DEBUG("Creating dev %s (%d,%d)",
data->file, major(data->sb.st_rdev),
minor(data->sb.st_rdev));
+ existed = false;
unlink(data->file);
if (mknod(data->file, data->sb.st_mode, data->sb.st_rdev) < 0) {
virReportSystemError(errno,
--
2.47.0
2:34 a.m.
New subject: [PATCH 2/3] qemu_namespace: Properly report new files
On Wed, Oct 16, 2024 at 09:23:06 +0200, Martin Kletzander wrote:
Function qemuNamespaceMknodOne() is supposed to return 0 if the file
did
not exist before this function. If, however, the file existed, but was
removed and recreated by this function the @existed flag should be reset
to its original state (false) because the function then behaves the same
The *original* state, if it existed, was 'true'. You're reseting it to the
*proper* state as the code will unlink and re-create it thus that
specific file didn't exist.
way as if the file did not exist as it needed to be recreated.
So reset the @existed flag to properly reflect what happened.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_namespace.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
index 71e29b4ba4f6..33a773917373 100644
--- a/src/qemu/qemu_namespace.c
+++ b/src/qemu/qemu_namespace.c
@@ -1022,6 +1022,7 @@ qemuNamespaceMknodOne(qemuNamespaceMknodItem *data)
data->file, data->target);
} else {
VIR_DEBUG("Creating symlink %s -> %s", data->file,
data->target);
+ existed = false;
/* First, unlink the symlink target. Symlinks change and
* therefore we have no guarantees that pre-existing
@@ -1053,6 +1054,7 @@ qemuNamespaceMknodOne(qemuNamespaceMknodItem *data)
} else {
VIR_DEBUG("Creating dev %s (%d,%d)",
data->file, major(data->sb.st_rdev),
minor(data->sb.st_rdev));
+ existed = false;
unlink(data->file);
if (mknod(data->file, data->sb.st_mode, data->sb.st_rdev) < 0)
{
virReportSystemError(errno,
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
2:23 a.m.
New subject: [PATCH 3/3] qemu_namespace: Only replicate labels on created files
Function qemuNamespaceMknodOne() is trying to replicate a file from the
parent namespace as perfectly as possible, with the same permissions,
labels, ACLs, etc.
If that file already existed it means that the qemu process is probably
using it already and the current setting is probably more correct than
the ones from the parent namespace.
In order to reflect that only replicate the file metadata when it was
(re-)created in this function.
Resolves: https://issues.redhat.com/browse/RHEL-62174
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_namespace.c | 56 ++++++++++++++++++++-------------------
1 file changed, 29 insertions(+), 27 deletions(-)
diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
index 33a773917373..6594657bfa3e 100644
--- a/src/qemu/qemu_namespace.c
+++ b/src/qemu/qemu_namespace.c
@@ -1090,43 +1090,45 @@ qemuNamespaceMknodOne(qemuNamespaceMknodItem *data)
goto cleanup;
}
- if (lchown(data->file, data->sb.st_uid, data->sb.st_gid) < 0) {
- virReportSystemError(errno,
- _("Failed to chown device %1$s"),
- data->file);
- goto cleanup;
- }
+ if (!existed) {
+ if (lchown(data->file, data->sb.st_uid, data->sb.st_gid) < 0) {
+ virReportSystemError(errno,
+ _("Failed to chown device %1$s"),
+ data->file);
+ goto cleanup;
+ }
- /* Symlinks don't have mode */
- if (!isLink &&
- chmod(data->file, data->sb.st_mode) < 0) {
- virReportSystemError(errno,
- _("Failed to set permissions for device %1$s"),
- data->file);
- goto cleanup;
- }
+ /* Symlinks don't have mode */
+ if (!isLink &&
+ chmod(data->file, data->sb.st_mode) < 0) {
+ virReportSystemError(errno,
+ _("Failed to set permissions for device
%1$s"),
+ data->file);
+ goto cleanup;
+ }
- if (data->acl &&
- virFileSetACLs(data->file, data->acl) < 0 &&
- errno != ENOTSUP) {
- virReportSystemError(errno,
- _("Unable to set ACLs on %1$s"), data->file);
- goto cleanup;
- }
+ if (data->acl &&
+ virFileSetACLs(data->file, data->acl) < 0 &&
+ errno != ENOTSUP) {
+ virReportSystemError(errno,
+ _("Unable to set ACLs on %1$s"),
data->file);
+ goto cleanup;
+ }
# ifdef WITH_SELINUX
- if (data->tcon &&
- lsetfilecon_raw(data->file, (const char *)data->tcon) < 0) {
- VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR
- if (errno != EOPNOTSUPP && errno != ENOTSUP) {
- VIR_WARNINGS_RESET
+ if (data->tcon &&
+ lsetfilecon_raw(data->file, (const char *)data->tcon) < 0) {
+ VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR
+ if (errno != EOPNOTSUPP && errno != ENOTSUP) {
+ VIR_WARNINGS_RESET
virReportSystemError(errno,
_("Unable to set SELinux label on %1$s"),
data->file);
goto cleanup;
+ }
}
- }
# endif
+ }
/* Finish mount process started earlier. */
if ((isReg || isDir) &&
--
2.47.0
2:36 a.m.
New subject: [PATCH 3/3] qemu_namespace: Only replicate labels on created
files
On Wed, Oct 16, 2024 at 09:23:07 +0200, Martin Kletzander wrote:
Function qemuNamespaceMknodOne() is trying to replicate a file from
the
parent namespace as perfectly as possible, with the same permissions,
labels, ACLs, etc.
If that file already existed it means that the qemu process is probably
using it already and the current setting is probably more correct than
the ones from the parent namespace.
In order to reflect that only replicate the file metadata when it was
(re-)created in this function.
Resolves: https://issues.redhat.com/browse/RHEL-62174
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_namespace.c | 56 ++++++++++++++++++++-------------------
1 file changed, 29 insertions(+), 27 deletions(-)
diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
index 33a773917373..6594657bfa3e 100644
--- a/src/qemu/qemu_namespace.c
+++ b/src/qemu/qemu_namespace.c
@@ -1090,43 +1090,45 @@ qemuNamespaceMknodOne(qemuNamespaceMknodItem *data)
goto cleanup;
}
- if (lchown(data->file, data->sb.st_uid, data->sb.st_gid) < 0) {
- virReportSystemError(errno,
- _("Failed to chown device %1$s"),
- data->file);
- goto cleanup;
- }
+ if (!existed) {
+ if (lchown(data->file, data->sb.st_uid, data->sb.st_gid) < 0) {
+ virReportSystemError(errno,
+ _("Failed to chown device %1$s"),
+ data->file);
+ goto cleanup;
+ }
- /* Symlinks don't have mode */
- if (!isLink &&
- chmod(data->file, data->sb.st_mode) < 0) {
- virReportSystemError(errno,
- _("Failed to set permissions for device %1$s"),
- data->file);
- goto cleanup;
- }
+ /* Symlinks don't have mode */
+ if (!isLink &&
+ chmod(data->file, data->sb.st_mode) < 0) {
+ virReportSystemError(errno,
+ _("Failed to set permissions for device
%1$s"),
+ data->file);
+ goto cleanup;
+ }
- if (data->acl &&
- virFileSetACLs(data->file, data->acl) < 0 &&
- errno != ENOTSUP) {
- virReportSystemError(errno,
- _("Unable to set ACLs on %1$s"), data->file);
- goto cleanup;
- }
+ if (data->acl &&
+ virFileSetACLs(data->file, data->acl) < 0 &&
+ errno != ENOTSUP) {
+ virReportSystemError(errno,
+ _("Unable to set ACLs on %1$s"),
data->file);
+ goto cleanup;
+ }
# ifdef WITH_SELINUX
- if (data->tcon &&
- lsetfilecon_raw(data->file, (const char *)data->tcon) < 0) {
- VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR
- if (errno != EOPNOTSUPP && errno != ENOTSUP) {
- VIR_WARNINGS_RESET
+ if (data->tcon &&
+ lsetfilecon_raw(data->file, (const char *)data->tcon) < 0) {
+ VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR
+ if (errno != EOPNOTSUPP && errno != ENOTSUP) {
+ VIR_WARNINGS_RESET
virReportSystemError(errno,
_("Unable to set SELinux label on %1$s"),
data->file);
goto cleanup;
This block should be indented as well. I guess the macro without the
semicolon at the end breaks your auto-indenter.
+ }
}
- }
# endif
+ }
/* Finish mount process started earlier. */
if ((isReg || isDir) &&
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
7:30 a.m.
New subject: [PATCH 3/3] qemu_namespace: Only replicate labels on created
files
On Wed, Oct 16, 2024 at 09:36:41AM +0200, Peter Krempa wrote:
On Wed, Oct 16, 2024 at 09:23:07 +0200, Martin Kletzander wrote:
> Function qemuNamespaceMknodOne() is trying to replicate a file from the
> parent namespace as perfectly as possible, with the same permissions,
> labels, ACLs, etc.
>
> If that file already existed it means that the qemu process is probably
> using it already and the current setting is probably more correct than
> the ones from the parent namespace.
>
> In order to reflect that only replicate the file metadata when it was
> (re-)created in this function.
>
> Resolves: https://issues.redhat.com/browse/RHEL-62174
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> src/qemu/qemu_namespace.c | 56 ++++++++++++++++++++-------------------
> 1 file changed, 29 insertions(+), 27 deletions(-)
>
> diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
> index 33a773917373..6594657bfa3e 100644
> --- a/src/qemu/qemu_namespace.c
> +++ b/src/qemu/qemu_namespace.c
> @@ -1090,43 +1090,45 @@ qemuNamespaceMknodOne(qemuNamespaceMknodItem *data)
> goto cleanup;
> }
>
> - if (lchown(data->file, data->sb.st_uid, data->sb.st_gid) < 0) {
> - virReportSystemError(errno,
> - _("Failed to chown device %1$s"),
> - data->file);
> - goto cleanup;
> - }
> + if (!existed) {
> + if (lchown(data->file, data->sb.st_uid, data->sb.st_gid) < 0) {
> + virReportSystemError(errno,
> + _("Failed to chown device %1$s"),
> + data->file);
> + goto cleanup;
> + }
>
> - /* Symlinks don't have mode */
> - if (!isLink &&
> - chmod(data->file, data->sb.st_mode) < 0) {
> - virReportSystemError(errno,
> - _("Failed to set permissions for device
%1$s"),
> - data->file);
> - goto cleanup;
> - }
> + /* Symlinks don't have mode */
> + if (!isLink &&
> + chmod(data->file, data->sb.st_mode) < 0) {
> + virReportSystemError(errno,
> + _("Failed to set permissions for device
%1$s"),
> + data->file);
> + goto cleanup;
> + }
>
> - if (data->acl &&
> - virFileSetACLs(data->file, data->acl) < 0 &&
> - errno != ENOTSUP) {
> - virReportSystemError(errno,
> - _("Unable to set ACLs on %1$s"),
data->file);
> - goto cleanup;
> - }
> + if (data->acl &&
> + virFileSetACLs(data->file, data->acl) < 0 &&
> + errno != ENOTSUP) {
> + virReportSystemError(errno,
> + _("Unable to set ACLs on %1$s"),
data->file);
> + goto cleanup;
> + }
>
> # ifdef WITH_SELINUX
> - if (data->tcon &&
> - lsetfilecon_raw(data->file, (const char *)data->tcon) < 0) {
> - VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR
> - if (errno != EOPNOTSUPP && errno != ENOTSUP) {
> - VIR_WARNINGS_RESET
> + if (data->tcon &&
> + lsetfilecon_raw(data->file, (const char *)data->tcon) < 0) {
> + VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR
> + if (errno != EOPNOTSUPP && errno != ENOTSUP) {
> + VIR_WARNINGS_RESET
> virReportSystemError(errno,
> _("Unable to set SELinux label on
%1$s"),
> data->file);
> goto cleanup;
This block should be indented as well. I guess the macro without the
semicolon at the end breaks your auto-indenter.
It broke it completely differently, this is "my fixed version" that I
screwed up manually =D Thanks for catching that and reviewing this.
> + }
> }
> - }
> # endif
> + }
>
> /* Finish mount process started earlier. */
> if ((isReg || isDir) &&
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
67
days inactive
67
days old
8 comments
2 participants
participants (2)
-
Martin Kletzander -
Peter Krempa