4:28 a.m.
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(a)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(a)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