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@redhat.com> + */
#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@redhat.com> + */
#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