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