On 01/06/2011 05:35 AM, Daniel P. Berrange wrote: > The current security driver usage requires horrible code like > > if (driver->securityDriver && > driver->securityDriver->domainSetSecurityHostdevLabel && > driver->securityDriver->domainSetSecurityHostdevLabel(driver->securityDriver, > vm, hostdev) < 0) This is a v2 of the earlier https://www.redhat.com/archives/libvir-list/2010-November/msg00984.html, but omits the rest of the 8-patch series that v1 was included with. That's okay, but I'm a bit curious on the progress of the rest of that series (in part because it involved adding virCommand handshaking, where I'm not sure if I need to lend a hand at getting that in) :) > > This pair of checks for NULL clutters up the code, making the driver > calls 2 lines longer than they really need to be. The goal of the > patchset is to change the calling convention to simply > > if (virSecurityManagerSetHostdevLabel(driver->securityDriver, > vm, hostdev) < 0) > -qemudSecurityInit(struct qemud_driver *qemud_drv) > +qemudSecurityInit(struct qemud_driver *driver) > { > + virSecurityManagerPtr mgr = virSecurityManagerNew(driver->securityDriverName, > + driver->allowDiskFormatProbing); > + if (!mgr) > return -1; > - } > > - /* No primary security driver wanted to be enabled: just setup > - * the DAC driver on its own */ > - if (ret == -2) { > - qemud_drv->securityDriver = &qemuDACSecurityDriver; > - VIR_INFO0(_("No security driver available")); > + if (driver->privileged) { > + virSecurityManagerPtr dac = virSecurityManagerNewDAC(driver->user, > + driver->group, > + driver->allowDiskFormatProbing, > + driver->dynamicOwnership); > + if (!dac) > + return -1; Does this leak mgr? > + > + if (!(driver->securityManager = virSecurityManagerNewStack(mgr, > + dac))) > + return -1; Likewise. > @@ -1555,7 +1540,6 @@ qemudShutdown(void) { > VIR_FREE(qemu_driver->spicePassword); > VIR_FREE(qemu_driver->hugetlbfs_mount); > VIR_FREE(qemu_driver->hugepage_path); > - VIR_FREE(qemu_driver->securityDriverName); > VIR_FREE(qemu_driver->saveImageFormat); > VIR_FREE(qemu_driver->dumpImageFormat); Is there any state in a virSecurityManagerPtr that might necessitate a cleanup function (and even if there isn't right now, what happens if we extend virSecurityManager in the future), such that you are missing a call here to virSecurityManagerFree(qemu_driver->securityManager)? > diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c > index 468d0a3..d82ba73 100644 > --- a/src/security/security_apparmor.c > +++ b/src/security/security_apparmor.c > @@ -1,4 +1,3 @@ > - > /* > * AppArmor security driver for libvirt > * Copyright (C) 2009-2010 Canonical Ltd. Add 2011 and/or Red Hat copyright annotations? > diff --git a/src/security/security_dac.c b/src/security/security_dac.c > new file mode 100644 > index 0000000..edecaf9 > --- /dev/null > +++ b/src/security/security_dac.c > @@ -0,0 +1,703 @@ > +/* > + * Copyright (C) 2010 Red Hat, Inc. 2010-2011 > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * QEMU POSIX DAC security driver Do we still need the term QEMU here, or should it be dropped now that it's generic? > +static int > +virSecurityDACSetOwnership(const char *path, int uid, int gid) > +{ > + VIR_INFO("Setting DAC user and group on '%s' to '%d:%d'", path, uid, gid); %d and uid/gid are not compatible on cygwin; in util/util.c, we use %u, (unsigned int)uid for a portable alternative. (I'm not sure if this file would end up being compiled on cygwin, even though the old qemu version has been skipped to date). Multiple instances, but worth a separate cleanup patch, similar to commit c685993d7, so that this (already large) patch remains as a straight code motion with no hidden cleanup. > + > +static int > +virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, > + const char *path, > + size_t depth ATTRIBUTE_UNUSED, > + void *opaque) > +{ > + virSecurityManagerPtr mgr = opaque; > + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); Rather than passing mgr as opaque, and recomputing priv each time through the loop, ... > + > + return virSecurityDACSetOwnership(path, priv->user, priv->group); > +} > + > + > +static int > +virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr, > + virDomainObjPtr vm ATTRIBUTE_UNUSED, > + virDomainDiskDefPtr disk) > + > +{ > + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); > + > + if (!priv->dynamicOwnership) > + return 0; > + > + return virDomainDiskDefForeachPath(disk, > + virSecurityManagerGetAllowDiskFormatProbing(mgr), > + false, > + virSecurityDACSetSecurityFileLabel, > + mgr); why not just pass priv as opaque in the first place? > +static int > +virSecurityDACSetSecurityPCILabel(pciDevice *dev ATTRIBUTE_UNUSED, > + const char *file, > + void *opaque) > +{ > + virSecurityManagerPtr mgr = opaque; > + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); > +static int > +virSecurityDACSetSecurityUSBLabel(usbDevice *dev ATTRIBUTE_UNUSED, > + const char *file, > + void *opaque) > +{ > + virSecurityManagerPtr mgr = opaque; > + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); > +static int > +virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr, > + virDomainObjPtr vm ATTRIBUTE_UNUSED, > + virDomainHostdevDefPtr dev) > +{ > + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); > + ret = usbDeviceFileIterate(usb, virSecurityDACSetSecurityUSBLabel, mgr); > + ret = pciDeviceFileIterate(pci, virSecurityDACSetSecurityPCILabel, mgr); Likewise. > + > +static int > +virSecurityDACRestoreSecurityPCILabel(pciDevice *dev ATTRIBUTE_UNUSED, > + const char *file, > + void *opaque ATTRIBUTE_UNUSED) > +{ > + return virSecurityDACRestoreSecurityFileLabel(file); > +static int > +virSecurityDACRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr, > + virDomainObjPtr vm ATTRIBUTE_UNUSED, > + virDomainHostdevDefPtr dev) > + > +{ > + ret = usbDeviceFileIterate(usb, virSecurityDACRestoreSecurityUSBLabel, mgr); Why pass mgr, when it is unused, and when the pre-code-motion version passed NULL? > + /* XXX fixme - we need to recursively label the entriy tree :-( */ Faithful copy of the typo (s/entriy/entire/) > diff --git a/src/security/security_dac.h b/src/security/security_dac.h > new file mode 100644 > index 0000000..b690236 > --- /dev/null > +++ b/src/security/security_dac.h > @@ -0,0 +1,27 @@ > +/* > + * Copyright (C) 2010 Red Hat, Inc. 2010-2011 > + > +void virSecurityDACSetUser(virSecurityManagerPtr mgr, > + uid_t user); > +void virSecurityDACSetGroup(virSecurityManagerPtr mgr, > + gid_t group); > + > +void virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr, > + bool dynamic); Setters, but no getters. I guess that's okay for now. > diff --git a/src/security/security_manager.c b/src/security/security_manager.c > new file mode 100644 > index 0000000..7ab6e37 > --- /dev/null > +++ b/src/security/security_manager.c > @@ -0,0 +1,291 @@ No copyright header? > +virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary, > + virSecurityManagerPtr secondary) > +{ > + virSecurityManagerPtr mgr = > + virSecurityManagerNewDriver(&virSecurityDriverStack, > + virSecurityManagerGetAllowDiskFormatProbing(primary)); Should this be virSecurityManagerGetAllowDiskFormatProbing(primary) || virSecurityManagerGetAllowDiskFormatProbing(secondary) or is the intent that creating a stack allows you to override probing permitted in the secondary with a primary that disables probing? > + > + virSecurityStackSetPrimary(mgr, primary); > + virSecurityStackSetSecondary(mgr, secondary); You need an early exit if mgr is NULL, since virSecurityStackSetPrimary isn't too happy with a NULL mgr. > +virSecurityManagerPtr virSecurityManagerNewDAC(uid_t user, > + gid_t group, > + bool allowDiskFormatProbing, > + bool dynamicOwnership) > +{ > + virSecurityManagerPtr mgr = > + virSecurityManagerNewDriver(&virSecurityDriverDAC, > + allowDiskFormatProbing); > + > + virSecurityDACSetUser(mgr, user); Likewise about needing an early exit if mgr is NULL. > +void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr) > +{ > + return mgr + sizeof(mgr); Won't work. You meant to do: return (char *) mgr + sizeof(mgr); otherwise, you are doing pointer arithmetic on pointers that are sizeof(mgr) large, and accessing (WAAAY) beyond array bounds. (I'm surprised it didn't dump core on you during testing.) And yes, I meant to cast to (char*), even though the return type is (void*), since pointer arithmetic on (void*) is a gcc extension not required by C99. > +void virSecurityManagerFree(virSecurityManagerPtr mgr) This function was not exported. (Hmm, it answers my earlier question about qemu cleanup failing to call this function). > diff --git a/src/security/security_manager.h b/src/security/security_manager.h > new file mode 100644 > index 0000000..c0ef84f > --- /dev/null > +++ b/src/security/security_manager.h > @@ -0,0 +1,74 @@ > + > +#ifndef VIR_SECURITY_MANAGER_H__ No copyright header? > +#define VIR_SECURITY_MANAGER_H__ > + > +# define virSecurityReportError(code, ...) \ > + virReportErrorHelper(NULL, VIR_FROM_SECURITY, code, __FILE__, \ > + __FUNCTION__, __LINE__, __VA_ARGS__) Line up those \. > diff --git a/src/security/security_nop.c b/src/security/security_nop.c > new file mode 100644 > index 0000000..947cf55 > --- /dev/null > +++ b/src/security/security_nop.c > @@ -0,0 +1,168 @@ > + > + > +#include <config.h> No copyright header? > diff --git a/src/security/security_nop.h b/src/security/security_nop.h > new file mode 100644 > index 0000000..714e646 > --- /dev/null > +++ b/src/security/security_nop.h > @@ -0,0 +1,17 @@ > +/* > + * Copyright (C) 2010 Red Hat, Inc. s/2010/2011/ (unlike other headers where you are doing code motion and preserving the old copyright years, this file is completely new) > diff --git a/src/security/security_stack.c b/src/security/security_stack.c > new file mode 100644 > index 0000000..9b3753a > --- /dev/null > +++ b/src/security/security_stack.c > @@ -0,0 +1,383 @@ > +/* > + * Copyright (C) 2010 Red Hat, Inc. 2010-2011 > + > +static const char * virSecurityStackGetModel(virSecurityManagerPtr mgr) Unusual formatting; I'd expect either: static const char *virSecurityStackGetModel(virSecurityManagerPtr mgr) or static const char * virSecurityStackGetModel(virSecurityManagerPtr mgr) > +static int > +virSecurityStackVerify(virSecurityManagerPtr mgr, > + virDomainDefPtr def) > +{ > + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); > + int rc = 0; > + > + if (virSecurityManagerVerify(priv->primary, def) < 0) > + rc = -1; > + > +#if 0 > + if (virSecurityManagerVerify(priv->secondary, def) < 0) > + rc = -1; > +#endif What happened here? The original code verified the secondary driver first, not second, and here, you aren't even verifying the secondary driver. Are you really fixing a bug in the original code? If so, can we separate the bug fix from the code motion into two commits? > +static int > +virSecurityStackGenLabel(virSecurityManagerPtr mgr, > + virDomainObjPtr vm) > +{ > + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); > + int rc = 0; > + > + if (virSecurityManagerGenLabel(priv->primary, vm) < 0) > + rc = -1; > +#if 0 > + if (virSecurityManagerGenLabel(priv->secondary, vm) < 0) > + rc = -1; > +#endif Likewise. Here, it makes a bit more sense that you only need to generate one label to be shared among both stacks, rather than two. But what if the primary doesn't care about labels while the secondary does - shouldn't you have a fallback in that case? The next couple of functions have similar #ifdef 0 questions. > + > + > +static int > +virSecurityStackSetSecurityImageLabel(virSecurityManagerPtr mgr, > + virDomainObjPtr vm, > + virDomainDiskDefPtr disk) > +{ > + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); > + int rc = 0; > + > + if (virSecurityManagerSetImageLabel(priv->secondary, vm, disk) < 0) > + rc = -1; > + if (virSecurityManagerSetImageLabel(priv->primary, vm, disk) < 0) > + rc = -1; The rest of this file better matches my expectations of just code motion. > diff --git a/src/security/security_stack.h b/src/security/security_stack.h > new file mode 100644 > index 0000000..c924842 > --- /dev/null > +++ b/src/security/security_stack.h > @@ -0,0 +1,24 @@ > +/* > + * Copyright (C) 2010 Red Hat, Inc. 2010-2011 > + > +void virSecurityStackSetPrimary(virSecurityManagerPtr mgr, > + virSecurityManagerPtr primary); > +void virSecurityStackSetSecondary(virSecurityManagerPtr mgr, > + virSecurityManagerPtr secondary); Again, setters with no getters, but I'm okay with that for now. I had enough comments (including real bugs) that it's probably worth a v3. -- Eric Blake eblake@xxxxxxxxxx +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list