9:10 a.m.
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(a)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.
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