[libvirt] [PATCH] 1/1: implement usb and pci hot attach in AppArmor driver

[Eric Blake]

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