6:32 p.m.
On 11/22/2010 11:09 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 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)
The first check for 'driver->securityDriver' being NULL is removed
by introducing a 'no op' security driver that will always be present
if no real driver is enabled. This guarentees driver->securityDriver
s/guarentees/guarantees/
!= NULL.
The second check for
'driver->securityDriver->domainSetSecurityHostdevLabel'
being non-NULL is hidden in a new abstraction called virSecurityManager.
This separates the driver callbacks, from main internal API. The addition
of a virSecurityManager object, that is separate from the virSecurityDriver
struct also allows for security drivers to carry state / configuration
information directly. Thus the DAC/Stack drivers from src/qemu which
used to pull config from 'struct qemud_driver' can now be moved into
the 'src/security' directory and store their config directly.
* src/qemu/qemu_conf.h, src/qemu/qemu_driver.c: Update to
use new virSecurityManager APIs
* src/qemu/qemu_security_dac.c, src/qemu/qemu_security_dac.h
src/qemu/qemu_security_stacked.c, src/qemu/qemu_security_stacked.h:
Move into src/security directory
* src/security/security_stack.c, src/security/security_stack.h,
src/security/security_dac.c, src/security/security_dac.h: Generic
versions of previous QEMU specific drivers
* src/security/security_apparmor.c, src/security/security_apparmor.h,
src/security/security_driver.c, src/security/security_driver.h,
src/security/security_selinux.c, src/security/security_selinux.h:
Update to take virSecurityManagerPtr object as the first param
in all callbacks
* src/security/security_nop.c, src/security/security_nop.h: Stub
implementation of all security driver APIs.
* src/security/security_manager.h, src/security/security_manager.c:
New internal API for invoking security drivers
Sounds okay, I don't know whether this would have been possible to break
up into smaller incremental patches.
src/security/security_stack.h | 24 ++
22 files changed, 2079 insertions(+), 1482 deletions(-)
One huge patch. And using 'git diff -C' didn't make it much smaller
(only security_nop.h was detected as a 51% rename; I was hoping that it
would have recognized more of the files as a rename to see just the diff
caused by the rename). Oh well, I'll start my review anyway. At least
it passes the smoke test of applying it followed by 'make'. It fails
four tests of 'make syntax-check':
libvirt_unmarked_diagnostics
src/security/security_driver.c-69-
"Security driver %s not found", NULLSTR(name));
maint.mk: found unmarked diagnostic(s)
+++ po/POTFILES.in
@@ -56,11 +56,10 @@
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_driver.c
+src/security/security_dac.c
preprocessor_indentation
cppi: src/security/security_apparmor.h: line 17: not properly indented
cppi: src/security/security_manager.h: line 3: not properly indented
cppi: src/security/security_nop.h: line 13: not properly indented
maint.mk: incorrect preprocessor indentation
prohibit_empty_lines_at_EOF
src/security/security_driver.c
src/security/security_manager.c
maint.mk: the above files end with empty line(s)
It also fails 'make check' (the testsuite needs updating to use the new
headers):
seclabeltest.c: In function 'main':
seclabeltest.c:18:5: error: implicit declaration of function
'virSecurityDriverStartup' [-Wimplicit-function-declaration]
seclabeltest.c:18:5: error: nested extern declaration of
'virSecurityDriverStartup' [-Wnested-externs]
seclabeltest.c:28:13: error: expected expression before
'virSecurityDriverGetModel'
seclabeltest.c:36:11: error: expected expression before
'virSecurityDriverGetDOI'
+++ b/src/qemu/qemu_driver.c
deleted file mode 100644
index 55dc0c6..0000000
--- a/src/qemu/qemu_security_dac.c
I checked this file and its new counterpart security/security_dac.c.
Looks like a faithful rename...
-qemuSecurityDACSetSecurityAllLabel(virSecurityDriverPtr drv,
- virDomainObjPtr vm,
- const char *stdin_path ATTRIBUTE_UNUSED)
-{
- int i;
-
- if (!driver->privileged || !driver->dynamicOwnership)
- return 0;
-
- for (i = 0 ; i < vm->def->ndisks ; i++) {
- /* XXX fixme - we need to recursively label the entriy tree :-( */
...including the typo (s/entriy/entire/ if you'd like).
diff --git a/src/qemu/qemu_security_stacked.c
b/src/qemu/qemu_security_stacked.c
And my review stops here for now (I'm out of time today).
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org