[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]