On 21.05.2012 15:39, Marcelo Cerri wrote: > > Hi, > > This set of patches updates the libvirt's security driver mechanism to > support per-guest configurable user and group for QEMU processes running > together with other security drivers, such as SELinux and AppArmor. > > This patches implement the changes discussed in the following thread: > > https://www.redhat.com/archives/libvir-list/2012-February/msg00990.html > > Regards, > Marcelo Cerri
The feature looks reasonable but I think the implementation needs a cleanup. I've pointed out some things. Overall, there are some rules which are good to follow when adding new API: http://libvirt.org/api_extension.html It's always good to prepare environment for new API - like you do in the first patch. However, the order of other patches should be slightly different. More important, we would like to compile after each patch. IOW, I'd like to see some patches split and some merged following the guide I am linking above. It's okay to add an API without driver implementation -> splitting code into {add API, remote driver impl and let libvirt throw 'unsupported' error} and {implement new API for one driver} {implement new API for second driver} etc. On the other hand, API that requires XML change shouldn't be introduced before the change itself (it's okay if they are in the same patch). I am not an security expert so I haven't looked closely on security drivers though. Okay, I did, but just for code cleanliness or obvious mistakes. I think others may give their review too. Looking forward to v2. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
