On Fri, 04 Sep 2009, Daniel Veillard wrote:
On Fri, Sep 04, 2009 at 12:23:33PM -0500, Jamie Strandboge wrote:
This patch series implements the AppArmor security driver for sVirt. This implementation was developed for the Ubuntu AppArmorLibvirtProfile specification[1], but is general enough for any AppArmor deployment (such as Ubuntu, *SUSE and Mandriva).
This patch has seen quite a bit of real world testing in Ubuntu 9.10 (our development release) in our 0.7.0-1ubuntu3 package. I did make a few small changes after going through HACKING, but mostly I got the tests going and added documentation.
Could you triple check that the make syntax-check run clean with your patches applied, because a very quick glimpse shows up a number of malloc() which we don't allow for example. It may be because you didn't enter the files in the SCM but the checks should be done anyway on the new files please.
I noticed that syntax-check wouldn't scan the new files unless I did a git commit first. I did not run 'syntax-check' after adding the documention examples, and just found that examples/apparmor/libvirt-qemu had a trailing blank line (I was a bit surprised syntax-check checked these files (attached is an updated docs patch)). I can demonstrate that the files are being checked by removing the '#include <config.h>' line in security_apparmor.c and virt-aa-helper.c and syntax-check failing: $ make syntax-check .. require_config_h src/virt-aa-helper.c maint.mk: the above files do not include <config.h> make: *** [sc_require_config_h] Error 1 $ make syntax-check .. require_config_h src/security_apparmor.c maint.mk: the above files do not include <config.h> make: *** [sc_require_config_h] Error 1 I also knew the files were checked because, for example, I used str[n]cmp() in a couple places and prohibit_strcmp_and_strncmp caught them. I changed those to STR* before submission. I don't see that 'syntax-check' enforces not using *alloc, but I did read in HACKING that *alloc are deprecated, though I had already written the code before reading it. I do see other examples of *alloc in the codebase, and I do properly check the return code of all calls to *alloc. If you insist on those calls being replaced, I can do that, but I didn't before submitting because I knew the current calls were correct and didn't want to introduce bugs into the code. Here is what I do after a 'git pull': $ cd libvirt $ git checkout <apply patches> (using the attached patch_5_docs.patch) $ chmod 755 ./tests/virt-aa-helper-test $ git add . $ git commit ... [master 4b08401] add apparmor security driver 18 files changed, 1966 insertions(+), 3 deletions(-) create mode 100644 examples/apparmor/TEMPLATE create mode 100644 examples/apparmor/libvirt-qemu create mode 100644 examples/apparmor/usr.bin.virt-aa-helper create mode 100644 examples/apparmor/usr.sbin.libvirtd create mode 100644 src/security_apparmor.c create mode 100644 src/security_apparmor.h create mode 100644 src/virt-aa-helper.c create mode 100644 tests/secaatest.c create mode 100755 tests/virt-aa-helper-test $ ./autogen.sh $ ./configure --enable-compile-warnings=error $ make check .. CC security_apparmor.lo .. CC virt_aa_helper-virt-aa-helper.o CCLD virt-aa-helper .. CC secaatest.o CCLD secaatest .. PASS: virt-aa-helper-test .. PASS: secaatest .. (no errors, and I see my two added tests passed) $ make syntax-check void_if_before_free cast_of_argument_to_free cast_of_x_alloc_return_value prohibit_strcmp error_message_warn_fatal error_message_period prohibit_have_config_h require_config_h require_config_h_first prohibit_HAVE_MBRTOWC prohibit_assert_without_use prohibit_getopt_without_use prohibit_long_options_without_use prohibit_inttostr_without_use prohibit_error_without_use prohibit_safe_read_without_use prohibit_argmatch_without_use prohibit_root_dev_ino_without_use prohibit_c_ctype_without_use prohibit_signal_without_use changelog the_the trailing_blank unmarked_diagnostics prohibit_cvs_keyword proper_name_utf8_requires_ICONV redundant_const const_long_option makefile_TAB_only_indentation m4_quote_check po_check copyright_check avoid_write prohibit_strcmp_and_strncmp prohibit_asprintf prohibit_VIR_ERR_NO_MEMORY prohibit_nonreentrant prohibit_ctype_h TAB_in_indentation avoid_ctype_macros prohibit_virBufferAdd_with_string_literal prohibit_gethostby libvirt_unmarked_diagnostics prohibit_trailing_blank_lines Thanks! Jamie -- Jamie Strandboge | http://www.canonical.com