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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list