Re: [libvirt] Refactoring of storage pool

On Wed, Jan 25, 2017 at 08:02:30PM +0300, Olga Krishtal wrote: > Hi everyone! > Half a year ago we started discussion about filesystem pools: > https://www.redhat.com/archives/libvir-list/2016-April/msg01941.html > https://www.redhat.com/archives/libvir-list/2016-May/msg00208.html > https://www.redhat.com/archives/libvir-list/2016-September/msg00463.html > > At the end we have decided not to use copy/paste and try to merge code: > https://www.redhat.com/archives/libvir-list/2016-December/msg00051.html > > However, after first try it had became obvious (the discussion is in the link above) > that pools can be describe as a separate object, that later will be used in storage pool, > filesystem pool, vgpu pool, etc. This latter is an extension of previous discussion; > I just want to separate refactoring of storage pools and the implementation of > filesystem pools a bit. I'm not really in favour of trying to merge the code for storage pools and filesystem pools at the XML parsing level. While they do share a lot of common concepts, I expect that they'll need to grow apart over time. Having one parsing two to deal with what is going to be two similar, but none the less, distinct formats is going to cause as many problems as it solves IMHO. > /* > * virpool.h: pool utility to manage structures commonly used for pools. > * > */ > > /* The dscription of pool element > * For storage pool it used to be virStorageVolDef > * instead of key we use uuid, because key is specific > * for storage pools, and for other pool types (filesystem pools) > * or gpu pool we do nor have key. This kind of difference is just one example of why I think it is counter- productive to merge these XML parses into one. > * type field is also specific - that is why we put it into > * void* ElementTypeDef. > * ElementTypeDef - stores internal information about pool element(volume, item, etc) > * (the same as privateData for virConnectPtr) > * For example, for storage pool voluems it consists from: > * int type; > * virStorageVolSource source; > * virStorageSource target; > */ > > #include "virhash.h" > > typedef enum { > VIR_POOL_ELEMENT_VOLUME, > VIR_POOL_ELEMENT_FILESYSTEM, > > VIR_POOL_ELEMENT_LAST, > } virPoolElemenType; > VIR_ENUM_DECL(virPoolElementType) > > typedef void (*virPoolElementTypeDefFree) (void *PoolElementDef); > > typedef struct _virPoolElementDef virPoolElementDef; > typedef virPoolElementDef *virPoolElementDefPtr; > struct _virPoolElementDef { > char *name; > char *uuid; > bool building; > unsigned int in_use; > int type; > void* ElementTypeDef; > virPoolElementTypeDefFree PoolElementDefFree; > }; > > typedef struct _virPoolElementDefList virPoolElementDefList; > typedef virPoolElementDefList *virPoolElementDefListPtr; > struct _virPoolElementDefList { > size_t count; > virPoolElementDefPtr *elements; > }; > > /* General information about pool source*/ > typedef struct _virPoolSourceHost virPoolSourceHost; > typedef virPoolSourceHost *virPoolSourceHostPtr; > struct _virPoolSourceHost { > char *name; > int port; > }; > > typedef struct _virPoolAuthDef virPoolAuthDef; > typedef virPoolAuthDef *virPoolAuthDefPtr; > struct _virPoolAuthDef { > char *username; > char *secrettype; /* <secret type='%s' for disk source */ > int authType; /* virStorageAuthType */ > virSecretLookupTypeDef seclookupdef; > }; > > typedef enum { > VIR_POOL_SOURCE_DIR, > VIR_POOL_SOURCE_DEVICE, > VIR_POOL_SOURCE_NAME, > > VIR_POOL_SOURCE_LAST, > } virPoolSourcePathType; > VIR_ENUM_DECL(virPoolSourcePathType) > > typedef struct _virPoolSourcePath virPoolSourcePath; > typedef virPoolSourcePath *virPoolSourcePathPtr; > struct _virPoolSourcePath { > virPoolSourcePathType sourcetype; > char *path; > }; > > typedef void(*virPoolPrivDataFree) (void *privePoolData); > > typedef struct _virPoolSource virPoolSource; > typedef virPoolSource *virPoolSourcePtr; > struct _virPoolSource{ > /* One or more host definitions */ > size_t nhost; > virPoolSourceHostPtr hosts; > > /* Authentication information */ > virPoolAuthDefPtr auth; > > /* One or more devices */ > size_t nsrcpath; > virPoolSourcePathPtr srcpaths; > > /* Name of the source */ > char *name; > /* Vendor of the source */ > char *vendor; > > /* Product name of the source */ > char *product; > /*Some private data */ > void *privatePoolData; > virPoolPrivDataFree privDataFree; > }; > > typedef struct _virPoolPathPerms virPoolPathPerms; > typedef virPoolPathPerms *virPoolPathPermsPtr; > struct _virPoolPathPerms { > mode_t mode; > uid_t uid; > gid_t gid; > char *label; > } > > > typedef struct _virPoolTarget virPoolTarget; > typedef virPoolTarget *virPoolTargetPtr; > struct _virPoolTarget{ > char *path; /* Optional path to target */ > virPoolPathPerms perms; /* Default permissions for path */ > }; > typedef struct _virPoolDef virPoolDef; > typedef virPoolDef *virPoolDefPtr; > struct _virPoolDef { > char *name; > unsigned char uuid[VIR_UUID_BUFLEN]; > > virPoolSource source; > virPoolTarget target; > }; > > typedef struct _virPoolInfo virPoolInfo; // used to be virPoolObj > typedef virPoolInfo *virPoolInfoPtr; > struct _virPoolInfo { > > char *configFile; > char *autostartLink; > bool active; > int autostart; > unsigned int asyncjobs; > > virPoolDefPtr def; > virPoolDefPtr newDef; > > virPoolElementDefList elementsdef; > }; This is all still pretty specific to storage pools / filesystme pools. I don't think it is going to nicely extend to cover other things like GPU pools.

My preference is still to stick with the patches you previously proposed at https://www.redhat.com/archives/libvir-list/2016-December/msg00051.html so that future changes made to help storage pools don't inadvetantly affect filesystem code or vica-verca.

Regards, Daniel