5:23 p.m.
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
4:07 p.m.
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
4:17 p.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10:40 p.m.
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? :)
10:39 p.m.
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.
;>
5166
days inactive
5174
days old
4 comments
3 participants
participants (3)
-
Eric Blake -
Jamie Strandboge -
Justin Clift