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

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(a)redhat.com&gt; >> --- >> 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.