On Thu, Jan 06, 2011 at 11:21:45AM -0700, Eric Blake wrote: > 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 security driver patch is proving a total PITA to rebase with people changes, so I want to get it merged ASAP independant of the rest of the locking changes. > > - /* 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. Yep, both fixed. > > @@ -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)? Three was a missing call to virSecurityManagerFree > > + * > > + * 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? That's dropped. > > > +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. %d does work here, because uid/gid are both declared as ints, not uid_t/gid_t. > > +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, ... There's no serious overhead there, so I prefer to pass the full object around. > > + > > +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. There's no compelling need for any getters in this code. > > 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? All drivers should have the same probe setting so it only needs to check the primary driver. > > + > > + 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. Yep, fixed. > > > +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); Strange how it actually worked, but that must have been luck, or perhaps I mistakenly didn't have the DAC driver active when i tested it first. > 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? I enabled the second verify even though none of the secondary drivers implement it. > > +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? Our architecture only allows for a single label, so we can't allow secondary drivers to generate labels. We do actually want to enable this in the future though. So I've added a comment about it. All the things you mention should be fixed in v3, along with a few other minor bugs I discovered after more testing Daniel -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list