[libvirt] [PATCH] 1/1: implement usb and pci hot attach in AppArmor driver
The AppArmor security driver has partial support for hostdev devices in that if they already exist in the XML, virt-aa-helper can find them and add them to the profile. Hot attach does not work[1] because AppArmorSetSecurityHostdevLabel and AppArmorRestoreSecurityHostdevLabel are not currently implemented. From the patch description: Implement AppArmorSetSecurityHostdevLabel() and AppArmorRestoreSecurityHostdevLabel() for hostdev and pcidev attach. virt-aa-helper also has to be adjusted because *FileIterate() is used for pci and usb devices and the corresponding XML for hot attached hostdev and pcidev is not in the XML passed to virt-aa-helper. The new '-F filename' option is added to append a rule to the profile as opposed to the existing '-f filename', which rewrites the libvirt-<uuid>.files file anew. This new '-F' option will append a rule to an existing libvirt-<uuid>.files if it exists, otherwise it acts the same as '-f'. load_profile() and reload_profile() have been adjusted to add an 'append' argument, which when true will use '-F' instead of '-f' when executing virt-aa-helper. All existing calls to load_profile() and reload_profile() have been adjusted to use the old behavior (ie append==false) except AppArmorSetSavedStateLabel() where it made sense to use the new behavior. This patch also adds tests for '-F' The tests still use the old convention of cat with sed that Eric Blake mentioned should be improved-- I will be submitting another patch for this. This patch compiles fine with --enable-compile-warnings=error, passes the parts of 'make check' that this patch touches (ie, the daemon-conf fails here, but it always fails for me) and passes 'syntax-check'. Jamie [1]https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/640993 -- Jamie Strandboge | http://www.canonical.com
On 09/23/2010 04:23 PM, Jamie Strandboge wrote:
The AppArmor security driver has partial support for hostdev devices in that if they already exist in the XML, virt-aa-helper can find them and add them to the profile. Hot attach does not work[1] because AppArmorSetSecurityHostdevLabel and AppArmorRestoreSecurityHostdevLabel are not currently implemented. From the patch description:
The tests still use the old convention of cat with sed that Eric Blake mentioned should be improved-- I will be submitting another patch for this. This patch compiles fine with --enable-compile-warnings=error, passes the parts of 'make check' that this patch touches (ie, the daemon-conf fails here, but it always fails for me) and passes 'syntax-check'.
See this thread [1] for dealing with daemon-conf failures. In the meantime, it would be nice if someone wanted to contribute a patch to SKIP instead of FAIL if the prereq of sasl support is not present. (Unfortunately, I don't know the Ubuntu counterpart to Fedora's cyrus-sasl-devel.) [1] https://www.redhat.com/archives/libvir-list/2010-March/msg00395.html Now, on to the review:
+/* Data structure to pass to *FileIterate so we have everything we need */ +struct SDPDOP {
Interesting name; when I see all-caps, I usually think I'm dealing with a macro rather than a struct...
+ virSecurityDriverPtr drv; + virDomainObjPtr vm; +};
...but I figured out how it maps to the contents, and since it is not exposed to other files, there's no harm in that choice of name.
+done: + ptr->drv = NULL; + ptr->vm = NULL;
Not strictly necessary, since...
+ VIR_FREE(ptr);
...they are just about to be discarded, and VIR_FREE doesn't dereference any nested stale pointers.
+ return ret; } @@ -284,6 +291,8 @@ update_include_file(const char *include_
clean: VIR_FREE(pcontent); + clean2: + VIR_FREE(existing);
No need to add a clean2 label; jumping to clean is adequate even where you added jumps to clean2, because pcontent is already NULL.
+ if (arg == 'F') + ctl->append = true; + else + ctl->append = false;
Maybe it's me, but stylistically, I like ?: for these uses: ctl->append = arg == 'F'; ACK. I cleaned up those nits, and pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 09/30/2010 03:07 PM, Eric Blake wrote:
+ if (arg == 'F') + ctl->append = true; + else + ctl->append = false;
Maybe it's me, but stylistically, I like ?: for these uses: ctl->append = arg == 'F';
Hmm - I mentioned liking ?:, then used something even shorter :) To clarify: when I see 'if (simple_cond) a=b; else a=c;', I generally turn it into 'a=(simple_cond)?b:c', but I also find 'bool_cond ? true : false' to be overkill when 'bool_cond' does just as well. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 10/01/2010 07:17 AM, Eric Blake wrote:
On 09/30/2010 03:07 PM, Eric Blake wrote:
+ if (arg == 'F') + ctl->append = true; + else + ctl->append = false;
Maybe it's me, but stylistically, I like ?: for these uses: ctl->append = arg == 'F';
Hmm - I mentioned liking ?:, then used something even shorter :)
To clarify: when I see 'if (simple_cond) a=b; else a=c;', I generally turn it into 'a=(simple_cond)?b:c', but I also find 'bool_cond ? true : false' to be overkill when 'bool_cond' does just as well.
You write perl too don't you? :)
On 10/01/2010 07:07 AM, Eric Blake wrote: <snip>
Maybe it's me, but stylistically, I like ?: for these uses: ctl->append = arg == 'F';
They make it a real PITA to follow when debugging (ie in Eclipse) though. When the if has the value assignment on separate lines, there's no doubt at all about which way the decision went. Using ?, the person doing the debugging either has to do extra debugging work (adding further mental distraction), or make an assumption (potential gotcha). I kind of hate the ? short hand in code I have to debug, due to this. ;>
participants (3)
-
Eric Blake -
Jamie Strandboge -
Justin Clift