Re: [PATCH 3/3] qemu_namespace: Only replicate labels on created files

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxx>

Attachment: signature.asc
Description: PGP signature