[libvirt] [PATCH v1 05/21] virfile: Introduce ACL helpers

Daniel P. Berrange berrange at redhat.com
Mon Dec 5 14:21:03 UTC 2016


On Mon, Dec 05, 2016 at 03:18:29PM +0100, Michal Privoznik wrote:
> On 05.12.2016 15:00, Daniel P. Berrange wrote:
> > On Thu, Nov 24, 2016 at 03:47:54PM +0100, Michal Privoznik wrote:
> >> Namely, virFileGetACLs, virFileSetACLs, virFileFreeACLs and
> >> virFileCopyACLs. These functions are going to be required when we
> >> are creating /dev for qemu. We have copy anything that's in
> >> host's /dev exactly as is. Including ACLs.
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> >> ---
> >>  configure.ac             | 10 +++++-
> >>  src/Makefile.am          |  4 +--
> >>  src/libvirt_private.syms |  4 +++
> >>  src/util/virfile.c       | 81 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>  src/util/virfile.h       | 11 +++++++
> >>  5 files changed, 107 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/configure.ac b/configure.ac
> >> index 5661752..972b399 100644
> >> --- a/configure.ac
> >> +++ b/configure.ac
> >> @@ -332,11 +332,19 @@ dnl Availability of various common headers (non-fatal if missing).
> >>  AC_CHECK_HEADERS([pwd.h regex.h sys/un.h \
> >>    sys/poll.h syslog.h mntent.h net/ethernet.h linux/magic.h \
> >>    sys/un.h sys/syscall.h sys/sysctl.h netinet/tcp.h ifaddrs.h \
> >> -  libtasn1.h sys/ucred.h sys/mount.h])
> >> +  libtasn1.h sys/ucred.h sys/mount.h sys/acl.h])
> >>  dnl Check whether endian provides handy macros.
> >>  AC_CHECK_DECLS([htole64], [], [], [[#include <endian.h>]])
> >>  AC_CHECK_FUNCS([stat stat64 __xstat __xstat64 lstat lstat64 __lxstat __lxstat64])
> >>  
> >> +ACL_CFLAGS=""
> >> +ACL_LIBS=""
> >> +if test "x$ac_cv_header_sys_acl_h" = "xyes" ; then
> >> +    ACL_LIBS="-lacl"
> >> +fi
> >> +AC_SUBST([ACL_CFLAGS])
> >> +AC_SUBST([ACL_LIBS])
> >> +
> >>  dnl We need to decide at configure time if libvirt will use real atomic
> >>  dnl operations ("lock free") or emulated ones with a mutex.
> >>  
> >> diff --git a/src/Makefile.am b/src/Makefile.am
> >> index b358e0e..c08a350 100644
> >> --- a/src/Makefile.am
> >> +++ b/src/Makefile.am
> >> @@ -1125,12 +1125,12 @@ libvirt_util_la_SOURCES =					\
> >>  libvirt_util_la_CFLAGS = $(CAPNG_CFLAGS) $(YAJL_CFLAGS) $(LIBNL_CFLAGS) \
> >>  		$(AM_CFLAGS) $(AUDIT_CFLAGS) $(DEVMAPPER_CFLAGS) \
> >>  		$(DBUS_CFLAGS) $(LDEXP_LIBM) $(NUMACTL_CFLAGS)	\
> >> -		$(POLKIT_CFLAGS) $(GNUTLS_CFLAGS) \
> >> +		$(POLKIT_CFLAGS) $(GNUTLS_CFLAGS) $(ACL_CFLAGS) \
> >>  		-I$(srcdir)/conf
> >>  libvirt_util_la_LIBADD = $(CAPNG_LIBS) $(YAJL_LIBS) $(LIBNL_LIBS) \
> >>  		$(THREAD_LIBS) $(AUDIT_LIBS) $(DEVMAPPER_LIBS) \
> >>  		$(LIB_CLOCK_GETTIME) $(DBUS_LIBS) $(MSCOM_LIBS) $(LIBXML_LIBS) \
> >> -		$(SECDRIVER_LIBS) $(NUMACTL_LIBS) \
> >> +		$(SECDRIVER_LIBS) $(NUMACTL_LIBS) $(ACL_LIBS) \
> >>  		$(POLKIT_LIBS)
> >>  
> >>  
> >> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> >> index ae563eb..5f560bf 100644
> >> --- a/src/libvirt_private.syms
> >> +++ b/src/libvirt_private.syms
> >> @@ -1559,6 +1559,7 @@ virFileActivateDirOverride;
> >>  virFileBindMountDevice;
> >>  virFileBuildPath;
> >>  virFileClose;
> >> +virFileCopyACLs;
> >>  virFileDeleteTree;
> >>  virFileDirectFdFlag;
> >>  virFileExists;
> >> @@ -1568,6 +1569,8 @@ virFileFindHugeTLBFS;
> >>  virFileFindMountPoint;
> >>  virFileFindResource;
> >>  virFileFindResourceFull;
> >> +virFileFreeACLs;
> >> +virFileGetACLs;
> >>  virFileGetHugepageSize;
> >>  virFileGetMountReverseSubtree;
> >>  virFileGetMountSubtree;
> >> @@ -1602,6 +1605,7 @@ virFileResolveAllLinks;
> >>  virFileResolveLink;
> >>  virFileRewrite;
> >>  virFileSanitizePath;
> >> +virFileSetACLs;
> >>  virFileSetupDev;
> >>  virFileSkipRoot;
> >>  virFileStripSuffix;
> >> diff --git a/src/util/virfile.c b/src/util/virfile.c
> >> index 6200004..c777025 100644
> >> --- a/src/util/virfile.c
> >> +++ b/src/util/virfile.c
> >> @@ -48,6 +48,9 @@
> >>  #if HAVE_SYS_SYSCALL_H
> >>  # include <sys/syscall.h>
> >>  #endif
> >> +#if HAVE_SYS_ACL_H
> >> +# include <sys/acl.h>
> >> +#endif
> >>  
> >>  #ifdef __linux__
> >>  # if HAVE_LINUX_MAGIC_H
> >> @@ -3573,3 +3576,81 @@ virFileBindMountDevice(const char *src ATTRIBUTE_UNUSED,
> >>      return -1;
> >>  }
> >>  #endif /* !defined(HAVE_SYS_MOUNT_H) */
> >> +
> >> +
> >> +#if defined(HAVE_SYS_ACL_H) && !defined(LIBVIRT_NSS) && !defined(LIBVIRT_SETUID_RPC_CLIENT)
> > 
> > You can simplify this by just adding #undef HAVE_SYS_ACL_H to config-post.h
> > for both the NSS + SETUID blocks
> 
> Ah, will do.
> 
> > 
> >> +int
> >> +virFileGetACLs(const char *file,
> >> +               void **acl)
> >> +{
> >> +    if (!(*acl = acl_get_file(file, ACL_TYPE_ACCESS)))
> >> +        return -1;
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +
> >> +int
> >> +virFileSetACLs(const char *file,
> >> +               void *acl)
> >> +{
> >> +    if (acl_set_file(file, ACL_TYPE_ACCESS, acl) < 0)
> >> +        return -1;
> >> +
> >> +    return 0;
> >> +}
> > 
> > Should we consider doing virReportSystemError in these methods
> > rather than letting the caller report the error, or is there
> > a need for the caller to ignore errors in some cases ?
> 
> I'd rather not. If a filesystem is mounted with ACLs disabled, we can
> avoid spurious errors in the logs ("Hey, I couldn't copy ACLs on
> /dev/blaah") even if the error is properly ignored later in the code.
> And then if GetACLs() doesn't report the error, no other *ACL() function
> should.

I was thinking that if you get ENOTSUPP with GetACLs() we'd just return
success, and a NULL acl. The SetACLs method would obviously have to
treat a NULL acl as a no-op. IOW, it could "just work" even when ACLs
are disabled.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|




More information about the libvir-list mailing list