On Fri, Jan 07, 2011 at 12:29:40PM -0700, Eric Blake wrote: > On 01/07/2011 12:15 PM, Eric Blake wrote: > > On 01/07/2011 08:39 AM, Daniel P. Berrange wrote: > >> 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) > >> > > > > Can you show the changes between v2 and v3? (although if you wait long > > enough, I can probably generate that myself, given a few minutes of git > > usage, and start replying based on that...) > > > > Just did it. > > > diff --git a/po/POTFILES.in b/po/POTFILES.in > > index 3d7bc8b..3521ba6 100644 > > --- a/po/POTFILES.in > > +++ b/po/POTFILES.in > > @@ -61,10 +61,10 @@ src/qemu/qemu_hotplug.c > > src/qemu/qemu_monitor.c > > src/qemu/qemu_monitor_json.c > > src/qemu/qemu_monitor_text.c > > -src/qemu/qemu_security_dac.c > > src/remote/remote_driver.c > > src/secret/secret_driver.c > > src/security/security_apparmor.c > > +src/security/security_dac.c > > src/security/security_driver.c > > src/security/security_selinux.c > > src/security/virt-aa-helper.c > > Yep, 'make syntax-check' would have complained about that. You still > had one 'make syntax-check' failure in v3, though: > > libvirt_unmarked_diagnostics > src/qemu/qemu_driver.c:1012: VIR_ERROR0("Failed to initialize > security drivers"); > maint.mk: found unmarked diagnostic(s) > > > > diff --git a/src/libvirt.c b/src/libvirt.c > > index 89b37c5..a4789bc 100644 > > --- a/src/libvirt.c > > +++ b/src/libvirt.c > > @@ -5642,6 +5642,8 @@ virDomainGetSecurityLabel(virDomainPtr domain, virSecurityLabelPtr seclabel) > > { > > virConnectPtr conn; > > > > + VIR_DOMAIN_DEBUG(domain, "seclabel=%p", seclabel); > > + > > if (!VIR_IS_CONNECTED_DOMAIN(domain)) { > > virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); > > virDispatchError(NULL); > > @@ -5684,6 +5686,8 @@ error: > > int > > virNodeGetSecurityModel(virConnectPtr conn, virSecurityModelPtr secmodel) > > { > > + DEBUG("conn=%p secmodel=%p", conn, secmodel); > > + > > Useful changes. > > > if (!VIR_IS_CONNECT(conn)) { > > virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__); > > virDispatchError(NULL); > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > > index 279559b..e9b8cb7 100644 > > --- a/src/libvirt_private.syms > > +++ b/src/libvirt_private.syms > > @@ -707,6 +707,7 @@ virSecurityDriverLookup; > > > > # security_manager.h > > virSecurityManagerClearSocketLabel; > > +virSecurityManagerFree; > > virSecurityManagerGenLabel; > > virSecurityManagerGetDOI; > > virSecurityManagerGetModel; > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > > index 0f84bb2..24e9162 100644 > > --- a/src/qemu/qemu_driver.c > > +++ b/src/qemu/qemu_driver.c > > @@ -984,12 +984,12 @@ qemuReconnectDomains(virConnectPtr conn, struct qemud_driver *driver) > > > > > > static int > > -qemudSecurityInit(struct qemud_driver *driver) > > +qemuSecurityInit(struct qemud_driver *driver) > > Why not, it fits with our other renames. > > > { > > virSecurityManagerPtr mgr = virSecurityManagerNew(driver->securityDriverName, > > driver->allowDiskFormatProbing); > > if (!mgr) > > - return -1; > > + goto error; > > > > if (driver->privileged) { > > virSecurityManagerPtr dac = virSecurityManagerNewDAC(driver->user, > > @@ -997,16 +997,21 @@ qemudSecurityInit(struct qemud_driver *driver) > > driver->allowDiskFormatProbing, > > driver->dynamicOwnership); > > if (!dac) > > - return -1; > > + goto error; > > > > if (!(driver->securityManager = virSecurityManagerNewStack(mgr, > > dac))) > > - return -1; > > + goto error; > > } else { > > driver->securityManager = mgr; > > } > > > > return 0; > > + > > +error: > > + VIR_ERROR0("Failed to initialize security drivers"); > > + virSecurityManagerFree(mgr); > > + return -1; > > Good, fixed that concern of mine. > > > } > > > > > > @@ -1044,16 +1049,13 @@ qemuCreateCapabilities(virCapsPtr oldcaps, > > > > doi = virSecurityManagerGetDOI(driver->securityManager); > > model = virSecurityManagerGetModel(driver->securityManager); > > - if (STREQ(model, "none")) { > > - model = ""; > > - doi = ""; > > + if (STRNEQ(model, "none")) { > > + if (!(caps->host.secModel.model = strdup(model))) > > + goto no_memory; > > + if (!(caps->host.secModel.doi = strdup(doi))) > > + goto no_memory; > > } > > > > - if (!(caps->host.secModel.model = strdup(model))) > > - goto no_memory; > > - if (!(caps->host.secModel.doi = strdup(doi))) > > - goto no_memory; > > - > > Almost; that cleaned up the chance of calling VIR_FREE on "", but... > > > VIR_DEBUG("Initialized caps for security driver \"%s\" with " > > "DOI \"%s\"", model, doi); > > ...if model == "none", then doi was not adjusted; do you need > NULLSTR(doi) here? > > > > > @@ -1321,7 +1323,7 @@ qemudStartup(int privileged) { > > } > > VIR_FREE(driverConf); > > > > - if (qemudSecurityInit(qemu_driver) < 0) > > + if (qemuSecurityInit(qemu_driver) < 0) > > goto error; > > > > if ((qemu_driver->caps = qemuCreateCapabilities(NULL, > > @@ -1543,6 +1545,8 @@ qemudShutdown(void) { > > VIR_FREE(qemu_driver->saveImageFormat); > > VIR_FREE(qemu_driver->dumpImageFormat); > > > > + virSecurityManagerFree(qemu_driver->securityManager); > > + > > ebtablesContextFree(qemu_driver->ebtables); > > > > if (qemu_driver->cgroupDeviceACL) { > > @@ -5420,6 +5424,12 @@ static int qemudNodeGetSecurityModel(virConnectPtr conn, > > int ret = 0; > > > > qemuDriverLock(driver); > > + memset(secmodel, 0, sizeof(*secmodel)); > > + > > + /* NULL indicates no driver, which we treat as > > + * success, but simply return no data in *secmodel */ > > + if (driver->caps->host.secModel.model == NULL) > > + goto cleanup; > > > > p = driver->caps->host.secModel.model; > > if (strlen(p) >= VIR_SECURITY_MODEL_BUFLEN-1) { > > diff --git a/src/security/security_apparmor.h b/src/security/security_apparmor.h > > index 90d9ddb..ffd8288 100644 > > --- a/src/security/security_apparmor.h > > +++ b/src/security/security_apparmor.h > > @@ -14,7 +14,7 @@ > > #ifndef __VIR_SECURITY_APPARMOR_H__ > > # define __VIR_SECURITY_APPARMOR_H__ > > > > -#include "security_driver.h" > > +# include "security_driver.h" > > You must have cppi installed :) (another make syntax-check fix). > > > > > extern virSecurityDriver virAppArmorSecurityDriver; > > > > diff --git a/src/security/security_dac.c b/src/security/security_dac.c > > index edecaf9..de5d011 100644 > > --- a/src/security/security_dac.c > > +++ b/src/security/security_dac.c > > @@ -1,13 +1,23 @@ > > /* > > - * Copyright (C) 2010 Red Hat, Inc. > > + * Copyright (C) 2010-2011 Red Hat, Inc. > > * > > * 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 > > + * This library is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + * Lesser General Public License for more details. > > + * > > + * You should have received a copy of the GNU Lesser General Public > > + * License along with this library; if not, write to the Free Software > > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > > + * > > + * POSIX DAC security driver > > */ > > + > > #include <config.h> > > #include <sys/types.h> > > #include <sys/stat.h> > > @@ -537,7 +547,7 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr, > > return 0; > > > > for (i = 0 ; i < vm->def->ndisks ; i++) { > > - /* XXX fixme - we need to recursively label the entriy tree :-( */ > > + /* XXX fixme - we need to recursively label the entire tree :-( */ > > if (vm->def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_DIR) > > continue; > > if (virSecurityDACSetSecurityImageLabel(mgr, > > diff --git a/src/security/security_dac.h b/src/security/security_dac.h > > index b690236..ccd9d1c 100644 > > --- a/src/security/security_dac.h > > +++ b/src/security/security_dac.h > > @@ -1,11 +1,20 @@ > > /* > > - * Copyright (C) 2010 Red Hat, Inc. > > + * Copyright (C) 2010-2011 Red Hat, Inc. > > * > > * 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. > > * > > + * This library is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + * Lesser General Public License for more details. > > + * > > + * You should have received a copy of the GNU Lesser General Public > > + * License along with this library; if not, write to the Free Software > > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > > + * > > * POSIX DAC security driver > > */ > > > > diff --git a/src/security/security_driver.c b/src/security/security_driver.c > > index 6d75dcc..4df73d7 100644 > > --- a/src/security/security_driver.c > > +++ b/src/security/security_driver.c > > @@ -14,6 +14,7 @@ > > #include <string.h> > > > > #include "virterror_internal.h" > > +#include "logging.h" > > > > #include "security_driver.h" > > #ifdef WITH_SECDRIVER_SELINUX > > @@ -41,7 +42,9 @@ virSecurityDriverPtr virSecurityDriverLookup(const char *name) > > virSecurityDriverPtr drv = NULL; > > int i; > > > > - for (i = 0; i < ARRAY_CARDINALITY(security_drivers) ; i++) { > > + VIR_DEBUG("name=%s", NULLSTR(name)); > > + > > + for (i = 0; i < ARRAY_CARDINALITY(security_drivers) && !drv ; i++) { > > virSecurityDriverPtr tmp = security_drivers[i]; > > > > if (name) { > > @@ -52,10 +55,12 @@ virSecurityDriverPtr virSecurityDriverLookup(const char *name) > > } else { > > switch (tmp->probe()) { > > case SECURITY_DRIVER_ENABLE: > > + VIR_DEBUG("Probed name=%s", tmp->name); > > drv = tmp; > > break; > > > > case SECURITY_DRIVER_DISABLE: > > + VIR_DEBUG("Not enabled name=%s", tmp->name); > > break; > > > > default: > > @@ -66,10 +71,10 @@ virSecurityDriverPtr virSecurityDriverLookup(const char *name) > > > > if (!drv) { > > virSecurityReportError(VIR_ERR_INTERNAL_ERROR, > > - "Security driver %s not found", NULLSTR(name)); > > + _("Security driver %s not found"), > > + NULLSTR(name)); > > return NULL; > > } > > > > return drv; > > } > > - > > diff --git a/src/security/security_manager.c b/src/security/security_manager.c > > index 7ab6e37..66cffb5 100644 > > --- a/src/security/security_manager.c > > +++ b/src/security/security_manager.c > > @@ -1,3 +1,24 @@ > > +/* > > + * security_manager.c: Internal security manager API > > + * > > + * Copyright (C) 2010-2011 Red Hat, Inc. > > + * > > + * 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. > > + * > > + * This library is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + * Lesser General Public License for more details. > > + * > > + * You should have received a copy of the GNU Lesser General Public > > + * License along with this library; if not, write to the Free Software > > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > > + * > > + * Author: Daniel P. Berrange <berrange@xxxxxxxxxx> > > + */ > > > > #include <config.h> > > > > @@ -45,6 +66,9 @@ virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary, > > virSecurityManagerNewDriver(&virSecurityDriverStack, > > virSecurityManagerGetAllowDiskFormatProbing(primary)); > > > > + if (!mgr) > > + return NULL; > > + > > virSecurityStackSetPrimary(mgr, primary); > > virSecurityStackSetSecondary(mgr, secondary); > > > > @@ -60,6 +84,9 @@ virSecurityManagerPtr virSecurityManagerNewDAC(uid_t user, > > virSecurityManagerNewDriver(&virSecurityDriverDAC, > > allowDiskFormatProbing); > > > > + if (!mgr) > > + return NULL; > > + > > virSecurityDACSetUser(mgr, user); > > virSecurityDACSetGroup(mgr, group); > > virSecurityDACSetDynamicOwnership(mgr, dynamicOwnership); > > @@ -80,7 +107,7 @@ virSecurityManagerPtr virSecurityManagerNew(const char *name, > > > > void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr) > > { > > - return mgr + sizeof(mgr); > > + return ((char*)mgr) + sizeof(mgr); > > Definitely a good fix :) > > > } > > > > > > @@ -288,4 +315,3 @@ int virSecurityManagerVerify(virSecurityManagerPtr mgr, > > virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); > > return -1; > > } > > - > > diff --git a/src/security/security_manager.h b/src/security/security_manager.h > > index c0ef84f..189b6b4 100644 > > --- a/src/security/security_manager.h > > +++ b/src/security/security_manager.h > > @@ -1,8 +1,29 @@ > > +/* > > + * security_manager.h: Internal security manager API > > + * > > + * Copyright (C) 2010-2011 Red Hat, Inc. > > + * > > + * 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. > > + * > > + * This library is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + * Lesser General Public License for more details. > > + * > > + * You should have received a copy of the GNU Lesser General Public > > + * License along with this library; if not, write to the Free Software > > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > > + * > > + * Author: Daniel P. Berrange <berrange@xxxxxxxxxx> > > + */ > > > > #ifndef VIR_SECURITY_MANAGER_H__ > > -#define VIR_SECURITY_MANAGER_H__ > > +# define VIR_SECURITY_MANAGER_H__ > > > > -# define virSecurityReportError(code, ...) \ > > +# define virSecurityReportError(code, ...) \ > > virReportErrorHelper(NULL, VIR_FROM_SECURITY, code, __FILE__, \ > > __FUNCTION__, __LINE__, __VA_ARGS__) > > > > diff --git a/src/security/security_nop.c b/src/security/security_nop.c > > index 947cf55..6d7cb47 100644 > > --- a/src/security/security_nop.c > > +++ b/src/security/security_nop.c > > @@ -1,4 +1,21 @@ > > - > > +/* > > + * Copyright (C) 2010-2011 Red Hat, Inc. > > + * > > + * 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. > > + * > > + * This library is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + * Lesser General Public License for more details. > > + * > > + * You should have received a copy of the GNU Lesser General Public > > + * License along with this library; if not, write to the Free Software > > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > > + * > > + */ > > > > #include <config.h> > > > > @@ -134,7 +151,7 @@ static int virSecurityDomainVerifyNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED > > > > virSecurityDriver virSecurityDriverNop = { > > 0, > > - "nop", > > + "none", > > Reasonable name change. > > > virSecurityDriverProbeNop, > > virSecurityDriverOpenNop, > > virSecurityDriverCloseNop, > > diff --git a/src/security/security_nop.h b/src/security/security_nop.h > > index 714e646..589a75d 100644 > > --- a/src/security/security_nop.h > > +++ b/src/security/security_nop.h > > @@ -1,16 +1,26 @@ > > /* > > - * Copyright (C) 2010 Red Hat, Inc. > > + * Copyright (C) 2010-2011 Red Hat, Inc. > > * > > * 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. > > * > > + * This library is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + * Lesser General Public License for more details. > > + * > > + * You should have received a copy of the GNU Lesser General Public > > + * License along with this library; if not, write to the Free Software > > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > > + * > > */ > > + > > #ifndef __VIR_SECURITY_NOP_H__ > > # define __VIR_SECURITY_NOP_H__ > > > > -#include "security_driver.h" > > +# include "security_driver.h" > > > > extern virSecurityDriver virSecurityDriverNop; > > > > diff --git a/src/security/security_stack.c b/src/security/security_stack.c > > index 9b3753a..e8bb058 100644 > > --- a/src/security/security_stack.c > > +++ b/src/security/security_stack.c > > @@ -1,12 +1,21 @@ > > /* > > - * Copyright (C) 2010 Red Hat, Inc. > > + * Copyright (C) 2010-2011 Red Hat, Inc. > > * > > * 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 stacked security driver > > + * This library is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + * Lesser General Public License for more details. > > + * > > + * You should have received a copy of the GNU Lesser General Public > > + * License along with this library; if not, write to the Free Software > > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > > + * > > + * Stacked security driver > > */ > > > > #include <config.h> > > @@ -57,14 +66,16 @@ virSecurityStackClose(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) > > return 0; > > } > > > > -static const char * virSecurityStackGetModel(virSecurityManagerPtr mgr) > > +static const char * > > +virSecurityStackGetModel(virSecurityManagerPtr mgr) > > { > > virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); > > > > return virSecurityManagerGetModel(priv->primary); > > } > > > > -static const char * virSecurityStackGetDOI(virSecurityManagerPtr mgr) > > +static const char * > > +virSecurityStackGetDOI(virSecurityManagerPtr mgr) > > { > > virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); > > > > @@ -81,10 +92,8 @@ virSecurityStackVerify(virSecurityManagerPtr mgr, > > if (virSecurityManagerVerify(priv->primary, def) < 0) > > rc = -1; > > > > -#if 0 > > if (virSecurityManagerVerify(priv->secondary, def) < 0) > > rc = -1; > > -#endif > > > > return rc; > > } > > @@ -99,7 +108,14 @@ virSecurityStackGenLabel(virSecurityManagerPtr mgr, > > > > if (virSecurityManagerGenLabel(priv->primary, vm) < 0) > > rc = -1; > > + > > #if 0 > > + /* We don't allow secondary drivers to generate labels. > > + * This may have to change in the future, but requires > > + * changes elsewhere in domain_conf.c and capabilities.c > > + * XML formats first, to allow recording of multiple > > + * labels > > + */ > > if (virSecurityManagerGenLabel(priv->secondary, vm) < 0) > > rc = -1; > > #endif > > @@ -118,6 +134,7 @@ virSecurityStackReleaseLabel(virSecurityManagerPtr mgr, > > if (virSecurityManagerReleaseLabel(priv->primary, vm) < 0) > > rc = -1; > > #if 0 > > + /* XXX See note in GenLabel */ > > if (virSecurityManagerReleaseLabel(priv->secondary, vm) < 0) > > rc = -1; > > #endif > > @@ -136,6 +153,7 @@ virSecurityStackReserveLabel(virSecurityManagerPtr mgr, > > if (virSecurityManagerReserveLabel(priv->primary, vm) < 0) > > rc = -1; > > #if 0 > > + /* XXX See note in GenLabel */ > > if (virSecurityManagerReserveLabel(priv->secondary, vm) < 0) > > rc = -1; > > Thanks - those extra comments help justify the #if 0. > > > #endif > > diff --git a/src/security/security_stack.h b/src/security/security_stack.h > > index c924842..bc83ff3 100644 > > --- a/src/security/security_stack.h > > +++ b/src/security/security_stack.h > > @@ -1,12 +1,21 @@ > > /* > > - * Copyright (C) 2010 Red Hat, Inc. > > + * Copyright (C) 2010-2011 Red Hat, Inc. > > * > > * 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 stacked security driver > > + * This library is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + * Lesser General Public License for more details. > > + * > > + * You should have received a copy of the GNU Lesser General Public > > + * License along with this library; if not, write to the Free Software > > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > > + * > > + * Stacked security driver > > */ > > > > #include "security_driver.h" > > > > And 'make check' is still failing: Urgh, I really hate that a simple 'make' no longer builds the test suite :-( Daniel -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list