Re: [libvirt] [PATCH 12/12] Insert access control checks for virDomainObjPtr into QEMU driver

On 05/02/2012 05:44 AM, Daniel P. Berrange wrote: 600 lines qualifies as minimal - wow, we've got a lot of new code to add for the other checks. I'm wondering if you can factor this for a smaller addition: The pattern of getting a VM, followed by an access check, is pretty common. What if we introduce a helper function: vm = qemuDomainFindByUUIDAccess(driver, dom->uuid, VIR_ACCESS_PERM_DOMAIN_READ); which does both the virDOmainFindByUUID(&driver->domains, dom->uuid) and the virAccessManagerCheckDomain(driver->accessManager, vm->def, access_perm). Hmm, code like this, where you check two separate permissions, makes me wonder if we should allow virAccessManagerCheckDomain to take a bitmap argument for all checks to run, as well as a simple way of generating a bitmap for one or more access bits in one go. Or even some glue magic to allow checking an unbounded list of permissions in what appears to be one call: virAccessManagerCheckDomain(driver->accessManager, vm->def, VIR_ACCESS_PERM_DOMAIN_STOP, VIR_ACCESS_PERM_DOMAIN_HIBERNATE) #define virAccessManagerCheck(...) \ virAccessManagerCheckAll(__VA_ARGS__, 0) virAccessManagerCheckAll(manager, def, ...) { va_list list; int perm; va_start(list, def); while ((perm = va_arg(list, int)) virAccessManagerCheckDomainOne(manager, def, perm); va_end(list); } The bulk of the patch looks mechanical, and I didn't closely check it for inaccuracies. But the overall idea seems worthwhile. I'm afraid that this series won't make it into 0.9.12, though, as it is rather invasive at this point when we already have rc1 out. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org