Re: [libvirt] [PATCH 02/50] list: Expose pool type via virStoragePoolGetInfo

On 2012年07月20日 21:01, Daniel P. Berrange wrote: > On Fri, Jul 20, 2012 at 09:00:21PM +0800, Osier Yang wrote: >> On 2012年07月20日 20:44, Daniel P. Berrange wrote: >>> On Fri, Jul 20, 2012 at 02:28:26PM +0200, Jiri Denemark wrote: >>>> On Fri, Jul 20, 2012 at 00:40:33 +0800, Osier Yang wrote: >>>>> Mainly for later patches' use, to filter the pools by pool type. >>>>> >>>>> include/libvirt/libvirt.h.in: Add enum virStoragePoolType; Add >>>>> pool type to virStoragePoolInfo. >>>>> >>>>> src/conf/storage_conf.h: Remove enum virStoragePoolType. >>>>> >>>>> src/storage/storage_driver.c: Expose pool type via >>>>> storagePoolGetInfo. >>>>> --- >>>>> include/libvirt/libvirt.h.in | 17 +++++++++++++++++ >>>>> src/conf/storage_conf.h | 19 ------------------- >>>>> src/storage/storage_driver.c | 2 ++ >>>>> 3 files changed, 19 insertions(+), 19 deletions(-) >>>>> >>>>> diff --git a/include/libvirt/libvirt.h.in >>>>> b/include/libvirt/libvirt.h.in >>>>> index e34438c..2820b2a 100644 >>>>> --- a/include/libvirt/libvirt.h.in >>>>> +++ b/include/libvirt/libvirt.h.in >>>>> @@ -2335,6 +2335,22 @@ typedef struct _virStoragePool virStoragePool; >>>>> */ >>>>> typedef virStoragePool *virStoragePoolPtr; >>>>> >>>>> +typedef enum { >>>>> + VIR_STORAGE_POOL_DIR, /* Local directory */ >>>>> + VIR_STORAGE_POOL_FS, /* Local filesystem */ >>>>> + VIR_STORAGE_POOL_NETFS, /* Networked filesystem - eg NFS, GFS, >>>>> etc */ >>>>> + VIR_STORAGE_POOL_LOGICAL, /* Logical volume groups / volumes */ >>>>> + VIR_STORAGE_POOL_DISK, /* Disk partitions */ >>>>> + VIR_STORAGE_POOL_ISCSI, /* iSCSI targets */ >>>>> + VIR_STORAGE_POOL_SCSI, /* SCSI HBA */ >>>>> + VIR_STORAGE_POOL_MPATH, /* Multipath devices */ >>>>> + VIR_STORAGE_POOL_RBD, /* RADOS Block Device */ >>>>> + VIR_STORAGE_POOL_SHEEPDOG, /* Sheepdog device */ >>>>> + >>>>> +#ifdef VIR_ENUM_SENTINELS >>>>> + VIR_STORAGE_POOL_LAST, >>>>> +#endif >>>>> +} virStoragePoolType; >>>>> >>>>> typedef enum { >>>>> VIR_STORAGE_POOL_INACTIVE = 0, /* Not running */ >>>>> @@ -2365,6 +2381,7 @@ typedef enum { >>>>> typedef struct _virStoragePoolInfo virStoragePoolInfo; >>>>> >>>>> struct _virStoragePoolInfo { >>>>> + int type; /* virStoragePoolType */ >>>>> int state; /* virStoragePoolState flags */ >>>>> unsigned long long capacity; /* Logical size bytes */ >>>>> unsigned long long allocation; /* Current allocation bytes */ >>>> >>>> Oops, you can't change public structs. Any combination of an app >>>> and libvirt >>>> library that would not have the same definition of this struct >>>> would fail. >> >> Oh, my bad. How about a new API like: >> >> int virStoragePoolGetInfoFlags (virStoragePoolPtr pool, >> virTypedParameterPtr params, >> int *nparams, >> unsigned int flags); >> >> With the param fields like: >> >> # define VIR_STORAGE_POOL_GET_INFO_STATE "state" >> # define VIR_STORAGE_POOL_GET_INFO_TYPE "type" >> # define VIR_STORAGE_POOL_GET_INFO_CAPACITY "capacity" >> # define VIR_STORAGE_POOL_GET_INFO_ALLOCATION "allocation" >> >> Assuming one wants to get more info about a pool in future, we would >> need a new API like this, with no suffering from not able to change >> to public struct. >> >>> >>> Fortunately no other part of this patch series appears to rely on this >>> extra field. Just remove this addition& the place in storage_driver.c >>> which sets it. The rest of this patch series can still be reviewed >>> as is >> >> The 'type' is used to filter the returned pool objects, so patches >> 1/50 to 14/50 should be skipped, though there is several patches >> in the range not related with storage pool specificly. > > Filtering is done inside the storage driver, so I don't see why > this needs to be exposed in the public API. Patch 12/50 will explain it: if the server side is old enough without the new API listAllStoragePools, and virsh is new enough to have the new introduced option "--type". I.e. new virsh talks to old libvirt, It will need to get the pool type to filter the results in virsh layer.