[PATCH] Don't require secdrivers to implement .domainMoveImageMetadata

[Erik Skultety]

On Mon, May 18, 2020 at 09:26:20AM +0200, Michal Privoznik wrote:
> On 5/18/20 8:16 AM, Erik Skultety wrote:
> > On Fri, May 15, 2020 at 05:44:38PM +0200, Michal Privoznik wrote:
> > > The AppArmor secdriver does not use labels to grant access to
> > > resources. Therefore, it doesn't use XATTRs and hence it lacks
> > > implementation of .domainMoveImageMetadata callback. This leads
> > > to a harmless but needless error message appearing in the logs:
> > > 
> > >    virSecurityManagerMoveImageMetadata:476 : this function is not
> > >    supported by the connection driver: virSecurityManagerMoveImageMetadata
> > > 
> > > Closes: https://gitlab.com/libvirt/libvirt/-/issues/25
> > > 
> > > Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> > > ---
> > >   src/security/security_manager.c |  3 +--
> > >   src/security/security_nop.c     | 10 ----------
> > >   2 files changed, 1 insertion(+), 12 deletions(-)
> > > 
> > > diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> > > index 2dea294784..b1237d63b6 100644
> > > --- a/src/security/security_manager.c
> > > +++ b/src/security/security_manager.c
> > > @@ -473,8 +473,7 @@ virSecurityManagerMoveImageMetadata(virSecurityManagerPtr mgr,
> > >           return ret;
> > >       }
> > > -    virReportUnsupportedError();
> > > -    return -1;
> > > +    return 0;
> > >   }
> > 
> > To ^this hunk:
> > Reviewed-by: Erik Skultety <eskultet at redhat.com>
> > 
> > > diff --git a/src/security/security_nop.c b/src/security/security_nop.c
> > > index c1856eb421..d5f715b916 100644
> > > --- a/src/security/security_nop.c
> > > +++ b/src/security/security_nop.c
> > > @@ -225,15 +225,6 @@ virSecurityDomainSetImageLabelNop(virSecurityManagerPtr mgr G_GNUC_UNUSED,
> > >       return 0;
> > >   }
> > > -static int
> > > -virSecurityDomainMoveImageMetadataNop(virSecurityManagerPtr mgr G_GNUC_UNUSED,
> > > -                                      pid_t pid G_GNUC_UNUSED,
> > > -                                      virStorageSourcePtr src G_GNUC_UNUSED,
> > > -                                      virStorageSourcePtr dst G_GNUC_UNUSED)
> > > -{
> > > -    return 0;
> > > -}
> > > -
> > 
> > ^This is an unrelated change and I also think that the Nop driver should
> > implement (ideally) all the functions, so I don't see a reason in removing
> > this one, otherwise we should remove more than just this very function.
> > 
> 
> It's not unrelated. The NOP driver is always present (see security_drivers[]
> in src/security/security_driver.c). Therefore, this dummy implementation was
> needed because if no other driver was loaded then NOP implementation saved
> us from reporting unsupported error.

Yes, I know, what I meant by "unrelated" here was just the fact that in order
to fix Apparmor, you only need the first hunk, I guess I'll be more explicit
next time :). It's true that with the first hunk the second becomes redundant,
but I still feel like the NOP driver should cover the full spectrum of
operations we support, but maybe I'm trying to be overly cautious here.

-- 
Erik Skultety