Re: [libvirt] [PATCHv3 2/5] lib: Add few flags for the bulk stats APIs

2014-08-28 Thread Peter Krempa 
On 08/27/14 22:50, Eric Blake wrote:
 On 08/27/2014 12:25 PM, Peter Krempa wrote:
 Add domain list filtering functions and a flag to enforce checking
 whether the remote daemon supports the requested stats groups.
 ---
  include/libvirt/libvirt.h.in | 15 +++
  src/libvirt.c| 29 -
  2 files changed, 43 insertions(+), 1 deletion(-)

 diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
 index e79c9ad..9358314 100644
 --- a/include/libvirt/libvirt.h.in
 +++ b/include/libvirt/libvirt.h.in
 @@ -2513,6 +2513,21 @@ typedef enum {
  VIR_DOMAIN_STATS_STATE = (1  0), /* return domain state */
  } virDomainStatsTypes;

 +typedef enum {
 +VIR_CONNECT_GET_ALL_DOMAINS_STATS_ACTIVE = 
 VIR_CONNECT_LIST_DOMAINS_ACTIVE,
 +VIR_CONNECT_GET_ALL_DOMAINS_STATS_INACTIVE = 
 VIR_CONNECT_LIST_DOMAINS_INACTIVE,
 +
 +VIR_CONNECT_GET_ALL_DOMAINS_STATS_PERSISTENT = 
 VIR_CONNECT_LIST_DOMAINS_PERSISTENT,
 +VIR_CONNECT_GET_ALL_DOMAINS_STATS_TRANSIENT = 
 VIR_CONNECT_LIST_DOMAINS_TRANSIENT,
 +
 +VIR_CONNECT_GET_ALL_DOMAINS_STATS_RUNNING = 
 VIR_CONNECT_LIST_DOMAINS_RUNNING,
 +VIR_CONNECT_GET_ALL_DOMAINS_STATS_PAUSED = 
 VIR_CONNECT_LIST_DOMAINS_PAUSED,
 +VIR_CONNECT_GET_ALL_DOMAINS_STATS_SHUTOFF = 
 VIR_CONNECT_LIST_DOMAINS_SHUTOFF,
 +VIR_CONNECT_GET_ALL_DOMAINS_STATS_OTHER = 
 VIR_CONNECT_LIST_DOMAINS_OTHER,
 
 Is this going to confuse the python bindings?  (I know in other places
 where we have set up one enum to match another that it didn't seem to
 work).  But fixing that would be a separate patch; it may be that
 docs/apibuild.py needs to learn how to do symbolic name dereferencing
 before generating the xml that feeds the python bindings (see also
 commit 9b291bb).
 
 
 + *
 + * Similarly to virConnectListAllDomains @flags can contain various flags to
 
 s/ListAllDomains/ListAllDomains,/
 
 @@ -21590,7 +21610,7 @@ virConnectGetAllDomainStats(virConnectPtr conn,
   * @doms: NULL terminated array of domains
   * @stats: stats to return, binary-OR of virDomainStatsTypes
   * @retStats: Pointer that will be filled with the array of returned stats.
 - * @flags: extra flags; not used yet, so callers should always pass 0
 + * @flags: extra flags; binary-OR of virConnectGetAllDomainStatsFlags;
 
 Why the trailing semicolon?
 
 + *
 + * Note that specifying any of the domain list filtering flags in @flags
 + * doesn't have effect on this function.
 
 Fair enough, I suppose.  But I'd rather we error out if filtering flags
 were requested, instead of silently ignoring them, so that if we later
 change our mind, we can do in-place filtering.  I'll probably have more
 to say on this on a later patch.

I changed this sentence to:

 * Note that any of the domain list filtering flags in @flags will be rejected
 * by this function.
 *

 
 Looks reasonable, so ACK with the grammar fixes
 

And all the other tweaks you've suggested. 

Peter





signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv3 2/5] lib: Add few flags for the bulk stats APIs

2014-08-27 Thread Eric Blake 
On 08/27/2014 12:25 PM, Peter Krempa wrote:
 Add domain list filtering functions and a flag to enforce checking
 whether the remote daemon supports the requested stats groups.
 ---
  include/libvirt/libvirt.h.in | 15 +++
  src/libvirt.c| 29 -
  2 files changed, 43 insertions(+), 1 deletion(-)
 
 diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
 index e79c9ad..9358314 100644
 --- a/include/libvirt/libvirt.h.in
 +++ b/include/libvirt/libvirt.h.in
 @@ -2513,6 +2513,21 @@ typedef enum {
  VIR_DOMAIN_STATS_STATE = (1  0), /* return domain state */
  } virDomainStatsTypes;
 
 +typedef enum {
 +VIR_CONNECT_GET_ALL_DOMAINS_STATS_ACTIVE = 
 VIR_CONNECT_LIST_DOMAINS_ACTIVE,
 +VIR_CONNECT_GET_ALL_DOMAINS_STATS_INACTIVE = 
 VIR_CONNECT_LIST_DOMAINS_INACTIVE,
 +
 +VIR_CONNECT_GET_ALL_DOMAINS_STATS_PERSISTENT = 
 VIR_CONNECT_LIST_DOMAINS_PERSISTENT,
 +VIR_CONNECT_GET_ALL_DOMAINS_STATS_TRANSIENT = 
 VIR_CONNECT_LIST_DOMAINS_TRANSIENT,
 +
 +VIR_CONNECT_GET_ALL_DOMAINS_STATS_RUNNING = 
 VIR_CONNECT_LIST_DOMAINS_RUNNING,
 +VIR_CONNECT_GET_ALL_DOMAINS_STATS_PAUSED = 
 VIR_CONNECT_LIST_DOMAINS_PAUSED,
 +VIR_CONNECT_GET_ALL_DOMAINS_STATS_SHUTOFF = 
 VIR_CONNECT_LIST_DOMAINS_SHUTOFF,
 +VIR_CONNECT_GET_ALL_DOMAINS_STATS_OTHER = VIR_CONNECT_LIST_DOMAINS_OTHER,

Is this going to confuse the python bindings?  (I know in other places
where we have set up one enum to match another that it didn't seem to
work).  But fixing that would be a separate patch; it may be that
docs/apibuild.py needs to learn how to do symbolic name dereferencing
before generating the xml that feeds the python bindings (see also
commit 9b291bb).


 + *
 + * Similarly to virConnectListAllDomains @flags can contain various flags to

s/ListAllDomains/ListAllDomains,/

 @@ -21590,7 +21610,7 @@ virConnectGetAllDomainStats(virConnectPtr conn,
   * @doms: NULL terminated array of domains
   * @stats: stats to return, binary-OR of virDomainStatsTypes
   * @retStats: Pointer that will be filled with the array of returned stats.
 - * @flags: extra flags; not used yet, so callers should always pass 0
 + * @flags: extra flags; binary-OR of virConnectGetAllDomainStatsFlags;

Why the trailing semicolon?

 + *
 + * Note that specifying any of the domain list filtering flags in @flags
 + * doesn't have effect on this function.

Fair enough, I suppose.  But I'd rather we error out if filtering flags
were requested, instead of silently ignoring them, so that if we later
change our mind, we can do in-place filtering.  I'll probably have more
to say on this on a later patch.

Looks reasonable, so ACK with the grammar fixes

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



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list