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

Re: [libvirt] [PATCHv2 2/5] remote: Implement bulk domain stats APIs in the remote driver



On 08/26/2014 12:22 PM, Eric Blake wrote:
> On 08/26/2014 08:14 AM, Peter Krempa wrote:
>> Implement the remote driver support for shuffling the domain stats
>> around.
>> ---
>>  daemon/remote.c              | 91 ++++++++++++++++++++++++++++++++++++++++++++
>>  src/remote/remote_driver.c   | 88 ++++++++++++++++++++++++++++++++++++++++++
>>  src/remote/remote_protocol.x | 26 ++++++++++++-
>>  src/remote_protocol-structs  | 23 +++++++++++
>>  4 files changed, 227 insertions(+), 1 deletion(-)
> 
> Rearranging things in my review (man, I wish git format-patch -Ofilename
> could be offloaded into git config by more than just an alias - it's
> nice to have a filename that lists .x and .h to show before .c).
> 

>> +
>> +    /**
>> +     * @generate: none
>> +     * @acl: domain:read
>> +     */
>> +    REMOTE_PROC_DOMAIN_LIST_GET_STATS = 343
> 
> Merge conflicts; trivial to resolve :)

Oops, missed a review comment.  This is the wrong ACL.  I think you want
to have:

@acl: connect:search_domains
@aclfilter: domain:read

which says that you can only call this API if you are also allowed to
call virConnectListAllDomains (connect:search_domains), and that within
this API, you will be filtering any domain that the user cannot normally
obtain stats on via other means (for example, virDomainGetState and
virDomainGetCPUStats both require domain:read - but they are called on a
single domain, whereas this function is called on every domain and
filtering out the domains that cannot be called individually).  I didn't
audit if ALL stats that you plan on querying all use domain:read, or if
there are even more limitations to place on this API.

Which means patch 3/5 is probably busted for not using a filter function.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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