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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 05.12.2016 15:21, Daniel P. Berrange wrote:
> 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@xxxxxxxxxx>
>>>> ---
>>>>  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.

Then if a caller wants to error out on ENOTSUPP they would have to check
for both retval and errno. Moreover, since HAVE_SYS_ACL_H variant of the
function reports errors, the !HAVE_SYS_ACL_H stub has to do so as well -
in which case users would see an error in the logs every time they start
a machine if their libvirt is build without ACLs enabled. Which reminds
me, not in this patch but I need to update the spec file so that we have
BuildRequires: libacl-devel in it.

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux