Re: [libvirt] [PATCH] 1/1: implement usb and pci hot attach in AppArmor driver
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org