Re: [libvirt] [PATCH RFC v3 01/15] storage pools: refactoring of basic structs

On 12/15/2016 12:16 PM, Olga Krishtal wrote: > Hi, John. > I needed some time to think over everything that you have written. > Thanks a lot. Yeah - I got too wordy, but this type of change caused me to think about other possible consumers too. > With all this new information we have more freedom for changes and > refactoring. Like I thought I mentioned - this set of changes seem to have gone too far in the direction of common code. Of course the original set of changes was too much cut-n-paste for my taste. There has to be a happy medium - that's what we need to find. I usually do that by figuring out what I'll need, creating generic structs/code and slowly moving things. Even if the generic structs start in the specific code - they can be moved. I would think at this point you know what you eventually need in the fspool driver - so that's where to start. > I dig through storage_conf.h file once with all ideas you have and tried > to use them. > It would be easier for me if we agree about virpool structs. The other > changes depends on it. Sure to a degree. Some of this ends up being trial and error... or prototyping some changes to see what will and won't work. Trying to do this conceptually in email is difficult too, especially with other things going on. Again, create smaller patches that have the end goal. > Some moments are left unclear to me, so I have written down virpool.h > with comments and questions. > If you agree, please write Ok or smth else. After sending I kept thinking about things "in the background". I took a pass at trying to extract/abstract just the pool concept and suffice to say it's a bit overwhelming. It's possible - it just has to be done carefully. Essentially though if you take the concept of a PoolObj to a different level of abstraction - it's a very common thing - there are domain, network, secret, storage, etc. that all use the objects code to build up a list or hash table of objects that point at 'def' data. In any case at this time I don't think that level of abstraction would be necessary for what you're trying to accomplish. And if I do come up with something - I would hope it'll be easy to merge in what you have. > /* Pool element description */ > Lets use PoolUnit instead PoolElement: Not a fan of Unit... This could just be _virPoolDef while the consumer of this would be _virStorageBlockPoolDef and _virStorageFSPoolDef.

> > typedef struct _virPoolUnitDef virPoolUnitDef; > typedef virPoolUnitDef *virPoolUnitDefPtr; > struct _virPoolUnitDef { > char *name; > char *key; > bool building; > unsigned int > in_use; > > void* UnitTypeDef /* the same as privateData for virConnectPtr. > Stores internal information about pool unit > */ > }; Why *name in both PoolUnitDef and PoolDef? I think uuid should be here.

There should be a 'voltype' field here... Changing the name will dig out code that's using it.

As I found with the 'virStorageSource' "type" field there were a couple of source.target.type field users where source.target.type is not filled in. I have some patches to address that in a branch, but I'm waiting on other reviews to be completed before posting. Not sure I like UnitTypeDef - the common usage is privateData. Oh and you'd need a "void (*privateDataFreeFunc)(void *);". Might also need a few more things, but seems to be a good start. > Eg, 3 fields from virStorageVolDef: > int type; > virStorageVolSource source; > virStorageSource target; > > typedef struct _virPoolUnitDefList > virPoolUnitDefList; > typedef virPoolUnitDefList *virPoolUnitDefListPtr; > struct _virPoolUnitDefList { > size_t count; > virPoolUnitDefPtr *obj > }; > I don't think *List is the way to go, but since that's the model today - we can stick with it until I can work up something better. Lists work well when there's 1-10 elements on the List, but as more elements are added the search times exponentially increase.

I think HashTable's are the better option (Domains, Secrets, Networks, and DomainSnapshot use them). See the virDomainObjList for a model. In any case, why are there voldefs in a list? Is this some sort of backing store listing for the same volume object? Or was this meant to be the volume object list? > /* General information about pool source */ > > Why do we need this information in general part for pool of any kind? This structure manages how a domain views the storage. It is quite confusing and you have to pay close attention to the usage. > As I understand this structure is used when storage is remote or I am > missing smth? Yes, this is essentially for remote/network storage > typedef struct _virPoolSourceHost > virPoolSourceHost; > typedef virPoolSourceHost > *virPoolSourceHostPtr; > struct _virPoolSourceHost { > char *name; > int port; > > }; For purposes of what you're trying to accomplish - it can be block storage specific unless you feel fspools would/could use NFS or would have their own need for a network structure. > I think it should be somewhere in private part, because you have > mentioned some other type of > pool - vGPU, are you sure that authorization will be the same? A vGPU would be something local and shouldn't need auth. A vGPU is something new dealing with graphics where there will be a need to have a pool of vGPU's to choose from much the same way there is storage. > Otherwise, what is virPoolAuth for general pool description? > > typedef struct _virPoolAuthDef virPoolAuthDef; > typedef virPoolAuthDef > *virPoolAuthDefPtr; > struct _virPoolAuthDef { > char *username; > char *secrettype; /* <secret type='%s' for disk source > */ > int authType; /* virStorageAuthType > */ > virSecretLookupTypeDef seclookupdef; > }; secrets are usually associated with remote storage; however, they're also something for local only as evidenced by the LUKS encryption secret. We could start as a private thing, but it could be moved as well. Again, depends on the implementation. Of course I've got the vzstorage pool concepts floating in and out of consciousness - tough to focus on one over the other! > > typedef enum > { > > VIR_POOL_SOURCE_DIR, > VIR_POOL_SOURCE_DEVICE, > > VIR_POOL_SOURCE_NAME, > > > > VIR_POOL_LAST, > } > > typedef struct _virPoolSourcePath virPoolSourcePath; > typedef virPoolSourcePath *virPoolSourcePathPtr; > struct _virPoolSourcePath { > virPoolSourcePathType sourcetype; > char *path; > }; > > typedef struct _virPoolSource virPoolSource; > typedef virPoolSource *virPoolSourcePtr; > struct _virPoolSource{ > /* One or more host definitions */ > size_t nhost; > virPoolSourceHostPtr hosts; > > /* Authentication information */ > virPoolAuthDefPtr auth; /*Not sure about it. We need authorization */ > /* when we have to authorize in storage, > but in Pool? */ > /* One or more devices */ > size_t nsrcpath; > virPoolSourcePathPtr srcpaths; > > /* Name of the source */ > May be we can use it for device name like in virPoolSourcePath > char *name; > > /* Vendor of the source */ > char *vendor; > > /* Product name of the source */ > char *product; > }; I have come to realize that mucking in virStorageSource I think is going to require a the structure to become private with a bunch of accessors to fields. Too much code today knows the structure and makes use of it. I started working through doing something like that as a result of other changes I've been posting related to storage source data. The biggest offender in peeking at the StorageSource is the path to the storage. > 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{ > What will be general description for path field? > char *path; /* Optional path to target */ > virPoolPathPerms perms; /* Default permissions for path */ > }; http://libvirt.org/formatstorage.html Target elements, path. There are virStorageSource fields that aren't used for 'target' storage... But there's virStorageSourcePtr used for backingStore... You have to follow code paths to understand usage. > typedef struct _virPoolDef virPoolDef; > typedef virPoolDef *virPoolDefPtr; > struct _virPoolDef { > char *name; > unsigned char uuid[VIR_UUID_BUFLEN]; > > virPoolSource source; > virPoolTarget target; > }; > > typedef struct _virPoolObj virPoolObj; > typedef virPoolObj *virPoolObjPtr; > struct _virPoolObj { > virMutex lock; ^^ Should use the virObjectLockable object... Yes requires some 'other' adjustments... > char *configFile; > char *autostartLink; > bool active; > int autostart; > unsigned int asyncjobs; > > virPoolDefPtr def; > virPoolDefPtr newDef; > > virPoolUnitDefList elements; you'd have to call this 'units' instead of 'elements', which is why I said "Elements" at first. > }; > > typedef struct _virPoolObjList virPoolObjList; > typedef virPoolObjList *virPoolObjListPtr; > struct _virPoolObjList { > size_t count; > virPoolObjPtr *objs; > }; Again, a hash table will be better... I think there's perhaps : _virPoolDef -> Common pool definition with private data pointers _virStorageBlockPoolDef -> Consumer of _virPoolDef as private data _virStorageFSPoolDef -> Consumer of _virPoolDef as private data _virPoolObj -> Common pool object with private data pointer _virStorageBlockPoolObj -> Consumer of _virPoolObj private data _virStorageFSPoolObj -> Consumer of _virPoolObj _virPoolObjList-> Common pool object list manipulation and search API I see no need (yet) for a StorageBlock or StoragePool object list. The ObjList is a table of objects with common reference/lookup functions. The tricky part there is the filtering API's currently used for various lookup APIs in order to pull out specific "types" of objects. The PoolObj some common stuff (def, newdef, some flags, locking) as well as a privateData for specific stuff. It would also have a 'def' that could describe the common lookup of uuid/name. The PoolDef is mostly private just to allow for easier lookups - that would copy the 'def' uuid/name fields > And some of functions prototype: > void virPoolSourceClear(virPoolSourcePtr source); > void virPoolSourceFree(virPoolSourcePtr source); > void virPoolDefFree(virPoolDefPtr def); > void virPoolObjFree(virPoolObjPtr pool); > void virPoolObjListFree(virPoolObjListPtr pools); > void virPoolObjRemove(virPoolObjListPtr pools, > virPoolObjPtr pool); > void virPoolObjLock(virPoolObjPtr obj); > void virPoolObjUnlock(virPoolObjPtr obj); FWIW: At the higher abstraction level the driver (StoragePool) is responsible to managing some sort of list or table of objects. Using cscope's egrep search capabilities - "^struct vir.*ObjList" returns 8 such instances. Of those 4 use a counted list (Interface, NodeDevice, NWFilter, StoragePool) and 4 use a hash table (Network, DomainSnapshot, Domain, Secret). Each ObjList would manage the _vir*Obj element corresponding to it (cscope egrep "^struct vir.*Obj {". Each of those Objects has commonality (parent/lock, def, newDef, state bits, privateData, privateDataFreeFunc). For example, search on consumers of "*ObjListNew()" type calls or anything that could be found by a cscope egrep of "^struct vir.*ObjList". There's quite a bit of cut-n-paste there. In any case, the previous 3 paragraphs are something on my radar, but I figured I'd share... > On 08/12/16 02:43, John Ferlan wrote: >> On 12/02/2016 10:38 AM, Olga Krishtal wrote: >>> This is the first patch in fspool patchest. >>> FSPool and storage pools has a lot in common, however we >>> want to have separate drivers for its managment. >>> >>> We want to use almost all storage pool descriptional structures >>> for filesystem pool. All common structs is moved to >>> virpoolcommon.h >>> >> More simply stated - for what's going to be I think a bit more complex >> than originally thought... The changes would be extracting out common >> pool mgmt code into their own API's so that they can be used by future >> patches (eg. the File System Storage Driver). >> >> >>> Signed-off-by: Olga Krishtal <okrishtal(a)virtuozzo.com&gt; >>> --- >>> include/libvirt/libvirt-storage.h | 5 +- >>> src/Makefile.am | 5 +- >>> src/conf/storage_conf.h | 126 +++-------------------------- >>> src/datatypes.h | 4 +- >>> src/util/virpoolcommon.h | 166 ++++++++++++++++++++++++++++++++++++++ >>> 5 files changed, 184 insertions(+), 122 deletions(-) >>> create mode 100644 src/util/virpoolcommon.h >>> >> Patch fails "make syntax-check" >> >> preprocessor_indentation >> cppi: src/util/virpoolcommon.h: line 166: not properly indented >> maint.mk: incorrect preprocessor indentation >> cfg.mk:684: recipe for target 'sc_preprocessor_indentation' failed >> make: *** [sc_preprocessor_indentation] Error 1 >> >> Which is the last "# endif" in the file. > Now I see, it is cppi that fails. I guess I did not have this test > installed. > That is the problem that I always have! May be all others failures with > syntax-check > is because of it. Yeah - there's stuff I've found along the way like this too... Usually when something like this is discovered we try to make sure there's some sort of requirement added into the build system so that everyone would see the issue and not just those that have already tripped across it. I think that's where those Requires/BuildRequires come from - been a while since I thought about it though. >> ... >> >> First and most important, thanks for taking this on - it's going to be >> bumpy, but I think if we take it piece by piece that will be better in >> the long run. Also on the horizon is a need for a common pool structure >> for vGPU's - I think Laine will find having a common pool structure >> quite helpful for that work. >> >> While I understand why you're providing all the patches, I'm going to >> only focus on the first three for now. I'd prefer to agree on the >> infrastructure first, then add the fspool code. Hopefully that makes the >> mean time between patch series shorter, too. >> >> I see the first few patches as a means to create a common pool >> infrastructure. I think this is the right path, although I think you >> swung too far in that direction with what you've done. >> >> The new nomenclature doesn't need the "common" in the name/structures >> either. It's just "virpool.h" and "virpool.c" and it manages structures >> commonly used for pools. Currently it's just the block storage driver >> that needs it, but you're adding fspool which will be a file system >> storage driver. >> >> Just the first couple of patches have spawned off a few more ideas and a >> few "pre" patches... I really think it would be better to create the >> new structures and API's in one patch. Just keep track of wh >> >> This patch definitely can be further split... I think if you copy/paste >> code from one place to another and rename functions, fix comments, etc. >> and essentially set up the common pool code, then subsequent patches >> would just remove code. You can always note in the commit message where >> code was sourced from... >> >> Also remember, you're trying to hit a moving target. The storage >> pool/volume code is constantly changing - if you try to do everything at >> once there's bound to be conflicts. I already of some with what's here >> and in the subsequent patches. I also have other commitments so larger >> series will languish ;-) - reviews can take considerable time. >> >> >> >>> diff --git a/include/libvirt/libvirt-storage.h b/include/libvirt/libvirt-storage.h >>> index 0974f6e..8572685 100644 >>> --- a/include/libvirt/libvirt-storage.h >>> +++ b/include/libvirt/libvirt-storage.h >> You need to be very careful with changes here. The problem being >> existing code that may rely on _virStoragePool or _virStorageVol. We can >> always add, but it's the change that causes issues. That's why stuff got >> moved into datatypes.h because that's not the external API. >> >> In any case, there is an example - search on virBlkioParameter >> >>> @@ -34,7 +34,7 @@ >>> * >>> * a virStoragePool is a private structure representing a storage pool >>> */ >>> -typedef struct _virStoragePool virStoragePool; >>> +typedef struct _virPoolCommon virStoragePool; >> I think it would be just _virPool, but, what you have won't work. I >> believe you'd need something like: >> >> /* As of 3.x.x the _virPool was introduced to generalize a block >> * storage pool. This allows for backwards compatibility. */ >> # define _virStoragePool _virPool >> typedef struct _virPool virStoragePool >> >> That way anything that has _virStoragePool will/should do the right thing... > I see. I was not quite sure that everyone will agree to accept such > change. So I decided > to use virStoragePool as a base and to have a union in case fspools will > need smth specific. > But if it is OK. That I am totally in. > The point I was making is changing from _virStoragePool to _virPoolCommon won't necessary fly without the ability to have some sort of definition. The usage of "_virPool" instead of "_virCommonPool" is my nomenclature. The example I referenced is how BlkioParameter did the changeover. I think it will work fine, but the details are something that are only known once someone jumps into the pool and starts swimming or sinking (a metaphor for the heap of trouble you get yourself into once you start actually coding things). John >>> >>> /** >>> * virStoragePoolPtr: >>> @@ -44,7 +44,6 @@ typedef struct _virStoragePool virStoragePool; >>> */ >>> typedef virStoragePool *virStoragePoolPtr; >>> >>> - >> Unnecessary whitespace adjustment. >> >>> typedef enum { >>> VIR_STORAGE_POOL_INACTIVE = 0, /* Not running */ >>> VIR_STORAGE_POOL_BUILDING = 1, /* Initializing pool, not available */ >>> @@ -104,7 +103,7 @@ typedef virStoragePoolInfo *virStoragePoolInfoPtr; >>> * >>> * a virStorageVol is a private structure representing a storage volume >>> */ >>> -typedef struct _virStorageVol virStorageVol; >>> +typedef struct _virItemCommon virStorageVol; >> Similar issue here - that virItemCommon should be changed to something >> like "virPoolElement" >>> >>> /** >>> * virStorageVolPtr: >>> diff --git a/src/Makefile.am b/src/Makefile.am >>> index 8ee5567..f8d4a5b 100644 >>> --- a/src/Makefile.am >>> +++ b/src/Makefile.am >>> @@ -170,7 +170,7 @@ UTIL_SOURCES = \ >>> util/virstats.c util/virstats.h \ >>> util/virstorageencryption.c util/virstorageencryption.h \ >>> util/virstoragefile.c util/virstoragefile.h \ >>> - util/virstring.h util/virstring.c \ >>> + util/virstring.h util/virstring.c \ >> ?? Loss of whitespace? >> >>> util/virsysinfo.c util/virsysinfo.h \ >>> util/virsystemd.c util/virsystemd.h \ >>> util/virthread.c util/virthread.h \ >>> @@ -185,7 +185,8 @@ UTIL_SOURCES = \ >>> util/viruuid.c util/viruuid.h \ >>> util/virxdrdefs.h \ >>> util/virxml.c util/virxml.h \ >>> - $(NULL) >>> + util/virpoolcommon.h \ >> I think this should be util/virpool.h >>> + $(NULL) >> The $(NULL) should line up properly >> >> caveat emptor: I'm not a Makefile expert - not even close, but I would >> also think there would need to be rules to ensure proper rebuilds >> when/if virpool.h changes as well. Whatever depends upon it, such as >> STORAGE_CONF_SOURCES. >> >>> >>> EXTRA_DIST += $(srcdir)/util/keymaps.csv $(srcdir)/util/virkeycode-mapgen.py >>> >>> diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h >>> index 185ae5e..8a9a789 100644 >>> --- a/src/conf/storage_conf.h >>> +++ b/src/conf/storage_conf.h >> NB: Changes in here would be done after we settle on the common pool >> structures... >> >>> @@ -27,6 +27,7 @@ >>> # include "internal.h" >>> # include "virstorageencryption.h" >>> # include "virstoragefile.h" >>> +# include "virpoolcommon.h" >>> # include "virbitmap.h" >>> # include "virthread.h" >>> # include "device_conf.h" >>> @@ -58,7 +59,6 @@ struct _virStorageVolSource { >>> * backend for partition type creation */ >>> }; >>> >>> - >> Loss of whitespace >> >>> typedef struct _virStorageVolDef virStorageVolDef; >>> typedef virStorageVolDef *virStorageVolDefPtr; >>> struct _virStorageVolDef { >>> @@ -73,6 +73,7 @@ struct _virStorageVolDef { >>> virStorageSource target; >>> }; >>> >>> + >> Add of whitespace - it's just a consistency thing - it has nothing to do >> with the changes and IMO should not be done. >> >>> typedef struct _virStorageVolDefList virStorageVolDefList; >>> typedef virStorageVolDefList *virStorageVolDefListPtr; >>> struct _virStorageVolDefList { >>> @@ -112,12 +113,8 @@ typedef enum { >>> /* >>> * For remote pools, info on how to reach the host >>> */ >>> -typedef struct _virStoragePoolSourceHost virStoragePoolSourceHost; >>> +typedef virPoolSourceHost virStoragePoolSourceHost; >>> typedef virStoragePoolSourceHost *virStoragePoolSourceHostPtr; >>> -struct _virStoragePoolSourceHost { >>> - char *name; >>> - int port; >>> -}; >>> >> This would be common >> >>> >>> /* >>> @@ -142,127 +139,27 @@ struct _virStoragePoolSourceDeviceExtent { >>> int type; /* virStorageFreeType */ >>> }; >>> >>> -typedef struct _virStoragePoolSourceInitiatorAttr virStoragePoolSourceInitiatorAttr; >>> -struct _virStoragePoolSourceInitiatorAttr { >>> - char *iqn; /* Initiator IQN */ >>> -}; >>> - >> I don't see this as common... It's SCSI pool specific >> >>> /* >>> * Pools can be backed by one or more devices, and some >>> * allow us to track free space on underlying devices. >>> */ >>> -typedef struct _virStoragePoolSourceDevice virStoragePoolSourceDevice; >>> +typedef virPoolSourceDevice virStoragePoolSourceDevice; >>> typedef virStoragePoolSourceDevice *virStoragePoolSourceDevicePtr; >>> -struct _virStoragePoolSourceDevice { >> Not sure totally removing works - some of this data is storage specific, >> while other parts would be more common. See my thoughts at the bottom. >> >> >>> - int nfreeExtent; >>> - virStoragePoolSourceDeviceExtentPtr freeExtents; >>> - char *path; >>> - int format; /* Pool specific source format */ >>> - int part_separator; /* enum virTristateSwitch */ >>> - >>> - /* When the source device is a physical disk, >>> - * the geometry data is needed >>> - */ >>> - struct _geometry { >>> - int cylinders; >>> - int heads; >>> - int sectors; >>> - } geometry; >>> -}; >> BTW: I think you should make a storage_conf.h specific geometry >> structure as a separate patch prior to *any* other changes and send that >> patch. >> >> eg. >> typedef struct _virStoragePoolGeometry virStoragePoolGeometry; >> typedef struct virStoragePoolGeometry *virStoragePoolGeometryPtr; >> struct _virStoragePoolGeometry { >> int cylinders; >> int heads; >> int sectors; >> }; >> >> ... Then instead of _geometry inline: >> >> virStoragePoolGeometry geometry; >> >> >>> >>> -typedef enum { >>> - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_DEFAULT = 0, >>> - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST, >>> - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST, >>> - >>> - VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_LAST, >>> -} virStoragePoolSourceAdapterType; >>> -VIR_ENUM_DECL(virStoragePoolSourceAdapter) >>> - >> This is SCSI storage pool specific >> -typedef struct _virStoragePoolSourceAdapter virStoragePoolSourceAdapter; >> +typedef virPoolSourceAdapter virStoragePoolSourceAdapter; >> typedef virStoragePoolSourceAdapter *virStoragePoolSourceAdapterPtr; >> -struct _virStoragePoolSourceAdapter { >> - int type; /* virStoragePoolSourceAdapterType */ >> - >> - union { >> - struct { >> - char *name; >> - virPCIDeviceAddress parentaddr; /* host address */ >> - int unique_id; >> - bool has_parent; >> - } scsi_host; >> - struct { >> - char *parent; >> - char *wwnn; >> - char *wwpn; >> - int managed; /* enum virTristateSwitch */ >> - } fchost; >> - } data; >> -}; >> This is SCSI storage pool specific. scsi_host and fchost are _scsi >> backend specific constructs. >> >>> >>> -typedef struct _virStoragePoolSource virStoragePoolSource; >>> +typedef virPoolSource virStoragePoolSource; >>> typedef virStoragePoolSource *virStoragePoolSourcePtr; >>> -struct _virStoragePoolSource { >> So some of this is storage specific and some could be considered common >> I suppose. >> >>> - /* An optional (maybe multiple) host(s) */ >>> - size_t nhost; >>> - virStoragePoolSourceHostPtr hosts; >> Common - a generic pool could have host elements >> >>> - >>> - /* And either one or more devices ... */ >>> - size_t ndevice; >>> - virStoragePoolSourceDevicePtr devices; >> Common - a generic pool could have some sort of device elements >> >>> - >>> - /* Or a directory */ >>> - char *dir; >> Common >> >>> - >>> - /* Or an adapter */ >>> - virStoragePoolSourceAdapter adapter; >> Specific >> >>> >>> - /* Or a name */ >>> - char *name; >> Common >> >>> - >>> - /* Initiator IQN */ >>> - virStoragePoolSourceInitiatorAttr initiator; >> Specific >> >>> - >>> - /* Authentication information */ >>> - virStorageAuthDefPtr auth; >> Common, but the name needs to change to something like >> "virPoolAuthDefPtr" (that could be a separate pre patch). >> >>> - >>> - /* Vendor of the source */ >>> - char *vendor; >> Common >> >>> - >>> - /* Product name of the source*/ >>> - char *product; >> Common >> >>> - >>> - /* Pool type specific format such as filesystem type, >>> - * or lvm version, etc. >>> - */ >>> - int format; >> This is actually specific to 4 pool types fs, netfs, disk, and logical >> For the fs/netfs it's the file system format type while for disk and >> logical it's the partition format type... I have more ideas at the end. >> >>> -}; >> And of course while reviewing this I saw something wrong in the >> formatstorage.html page... A storage pool doesn't have timestamps nor >> does it have encryption (<sigh> ... sent a patch to resolve). >> >>> - >>> -typedef struct _virStoragePoolTarget virStoragePoolTarget; >>> +typedef virPoolTarget virStoragePoolTarget; >>> typedef virStoragePoolTarget *virStoragePoolTargetPtr; >>> -struct _virStoragePoolTarget { >>> - char *path; /* Optional local filesystem mapping */ >>> - virStoragePerms perms; /* Default permissions for volumes */ >>> -}; >> Looks like it could be common, although virStoragePerms could change to >> something like virPoolPathPerms. Also instead of moving it to >> virstoragefile.c - add it to the virpool common code with the adjusted >> name. I could see a "need" for a pool element to have some sort of >> permissions/label'ing required. Anything with a path... >> >>> >>> -typedef struct _virStoragePoolDef virStoragePoolDef; >>> +typedef virPoolDef virStoragePoolDef; >>> typedef virStoragePoolDef *virStoragePoolDefPtr; >>> -struct _virStoragePoolDef { >> I think some of this is common while other parts are specific >> >>> - char *name; >>> - unsigned char uuid[VIR_UUID_BUFLEN]; >> Definitely common >> >>> - int type; /* virStoragePoolType */ >> Is specific and common... >> >>> - >>> - unsigned long long allocation; /* bytes */ >>> - unsigned long long capacity; /* bytes */ >>> - unsigned long long available; /* bytes */ >> Storage specific - could make some sort of structure to hold all 3 in >> order to share the data... >> >>> - >>> - virStoragePoolSource source; >>> - virStoragePoolTarget target; >> 'source' uses the adjusted struct above, while target should be able to >> be just common >> >>> -}; >>> >>> typedef struct _virStoragePoolObj virStoragePoolObj; >>> typedef virStoragePoolObj *virStoragePoolObjPtr; >>> - >>> struct _virStoragePoolObj { >>> virMutex lock; >>> >>> @@ -272,9 +169,8 @@ struct _virStoragePoolObj { >>> int autostart; >>> unsigned int asyncjobs; >>> >>> - virStoragePoolDefPtr def; >>> - virStoragePoolDefPtr newDef; >>> - >>> + virPoolDefPtr def; >>> + virPoolDefPtr newDef; >> After a few hours of reviewing and writing some comments, I came back to >> this change.... I think this is the magic place where things will get >> interesting... >> >> If you "consider" _virStoragePoolObj to be common, you get "_virPoolObj" >> which would need to be defined in virpool.h. It would look a bit like: >> >> struct _virPoolObj { >> virObjectLockable parent; >> virMutex lock; >> >> virPoolDefPtr def; >> virPoolDefPtr newDef; >> >> void *privateData; >> void (*privateDataFreeFunc)(void *); >> }; >> >> where the "privateData" in this case would be the "rest" of this >> virStoragePoolObj: >> >> struct _virStoragePoolPrivateObj { >> char *configFile; >> char *autostartLink; >> bool active; >> int autostart; >> unsigned int asyncjobs; >> virStorageVolDefList volumes; >> }; >> >> at least for the block storage pool. It could be different for the FS >> storage... and even more different for anything else. >> >> The key being - the virpool.c is managing the memory and calls to free >> come in the form of callbacks. It gets really confusing; however, the >> good news is I've recently done object creation in this manner in >> "virsecretobj", so I think that might be a good example for you to use. >> Although the secretobjs don't have the need (yet) for private data like >> you would for pools. You can look at the virDomainObj to see how it >> manages the privateData. >> >> If this is just overload, let me know - I can help here. You'd just be >> gated on me creating the infrastructure. I think you can look at the >> rest of this, but with my new information - I'd have to rethink how much >> applies <sigh>. >> >> >>> virStorageVolDefList volumes; >>> }; >>> >>> @@ -307,7 +203,7 @@ typedef virStoragePoolSourceList *virStoragePoolSourceListPtr; >>> struct _virStoragePoolSourceList { >>> int type; >>> unsigned int nsources; >>> - virStoragePoolSourcePtr sources; >>> + virPoolSourcePtr sources; >>> }; >>> >>> typedef bool (*virStoragePoolObjListFilter)(virConnectPtr conn, >>> diff --git a/src/datatypes.h b/src/datatypes.h >>> index 2b6adb4..1eaf4c6 100644 >>> --- a/src/datatypes.h >>> +++ b/src/datatypes.h >>> @@ -561,7 +561,7 @@ struct _virInterface { >>> * >>> * Internal structure associated to a storage pool >> s/storage// >> >> IOW: This is an internal structure to describe a pool. >> >>> */ >>> -struct _virStoragePool { >>> +struct _virPoolCommon { >> s/Common// >> >> IOW: Just "_virPool {" >> >>> virObject object; >>> virConnectPtr conn; /* pointer back to the connection */ >>> char *name; /* the storage pool external name */ >>> @@ -580,7 +580,7 @@ struct _virStoragePool { >>> * >>> * Internal structure associated to a storage volume >> s/storage volume/pool element/ >> >>> */ >>> -struct _virStorageVol { >>> +struct _virItemCommon { >> Similar comments/issues, although I really don't like the "_virItem" as >> a name. How about "_virPoolElement"? >> >>> virObject object; >>> virConnectPtr conn; /* pointer back to the connection */ >>> char *pool; /* Pool name of owner */ >> Be careful to check comments of the structures - remove {S|s}torage and >> {V|v}vol[ume] >> >>> diff --git a/src/util/virpoolcommon.h b/src/util/virpoolcommon.h >>> new file mode 100644 >>> index 0000000..d54de36 >>> --- /dev/null >>> +++ b/src/util/virpoolcommon.h >>> @@ -0,0 +1,166 @@ >>> +/* >>> + * virpoolcommon.h: utility to operate common parts in storage pools and >>> + * filesystem pools >> Even better how about just a purely "common" pool format. Would >> initially be for storage pools, but eventually for the fspool. I also >> see a use for something else on the horizon - a graphics (vgpu) pool. >> >> I also think this should just be virpool.h - the common would be a given. >> >>> + * >>> + * This library is free software; you can redistribute it and/or >>> + * modify it under the terms of the GNU Lesser General Public >>> + * License as published by the Free Software Foundation; either >>> + * version 2.1 of the License, or (at your option) any later version. >>> + * >>> + * This library is distributed in the hope that it will be useful, >>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >>> + * Lesser General Public License for more details. >>> + * >>> + * You should have received a copy of the GNU Lesser General Public >>> + * License along with this library. If not, see >>> + * <http://www.gnu.org/licenses/>;. >>> + * >>> + */ >>> + >>> +#ifndef __VIR_POOL_COMMON_H__ >>> +# define __VIR_POOL_COMMON_H__ >> Changing name to just virpool.h adjust the above macros too. >> >>> + >>> +# include "virstoragefile.h" >> But you wouldn't be including virstoragefile - that would be done >> elsewhere by code including this virpool.h that would also need >> virstoragefile.h >> >>> +# include "virthread.h" >> Why was this needed? >> >>> +# include "virpci.h" >> I think virpci.h won't be necessary either... >> >>> + >>> +/* >>> + * For remote pools, info on how to reach the host >>> + */ >>> +typedef struct _virPoolSourceHost virPoolSourceHost; >>> +typedef virPoolSourceHost *virPoolSourceHostPtr; >>> +struct _virPoolSourceHost { >>> + char *name; >>> + int port; >>> +}; >>> + >>> +/* >>> + * Available extents on the underlying storage >>> + */ >>> +typedef struct _virPoolSourceDeviceExtent virPoolSourceDeviceExtent; >>> +typedef virPoolSourceDeviceExtent *virPoolSourceDeviceExtentPtr; >>> +struct _virPoolSourceDeviceExtent { >>> + unsigned long long start; >>> + unsigned long long end; >>> + int type; /* virStorageFreeType */ >>> +}; >> NB: The above hunk is block storage specific for the disk backend and >> thus wouldn't have/need a common structure. >> >>> + >>> +/* >>> + * Pools can be backed by one or more devices, and some >>> + * allow us to track free space on underlying devices. >>> + */ >>> +typedef struct _virPoolSourceDevice virPoolSourceDevice; >>> +typedef virPoolSourceDevice *virPoolSourceDevicePtr; >>> +struct _virPoolSourceDevice { >> So part of this *isn't* common, it's specific - you could create a >> common structure and then a block storage specific that includes the >> common parts >> >>> + int nfreeExtent; >>> + virPoolSourceDeviceExtentPtr freeExtents; >> The above two are Storage specific >> >>> + char *path; >>> + int format; /* Pool specific source format */ >> These would be common >> >>> + int part_separator; /* enum virTristateSwitch */ >>> + >> Very specific to a disk pool for *a* specific case as a result of >> multipath devices being able to use "user friendly names" or not. >> >> >>> + /* When the source device is a physical disk, >>> + * the geometry data is needed >>> + */ >>> + struct _geometry { >>> + int cylinders; >>> + int heads; >>> + int sectors; >>> + } geometry; >>> +}; >> Specific to the disk pool. >> >>> + >>> +typedef enum { >>> + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_DEFAULT = 0, >>> + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST, >>> + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST, >>> + >>> + VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_LAST, >>> +} virStoragePoolSourceAdapterType; >>> +VIR_ENUM_DECL(virStoragePoolSourceAdapter) >> This would be SCSI storage pool specific and not for common file. >> >>> + >>> +typedef struct _virPoolSourceAdapter virPoolSourceAdapter; >>> +typedef virPoolSourceAdapter *virPoolSourceAdapterPtr; >>> +struct _virPoolSourceAdapter { >>> + int type; /* virStoragePoolSourceAdapterType */ >>> + >>> + union { >>> + struct { >>> + char *name; >>> + virPCIDeviceAddress parentaddr; /* host address */ >>> + int unique_id; >>> + bool has_parent; >>> + } scsi_host; >>> + struct { >>> + char *parent; >>> + char *wwnn; >>> + char *wwpn; >>> + int managed; /* enum virTristateSwitch */ >>> + } fchost; >>> + } data; >>> +}; >> The above is SCSI storage specific. >> >>> + >>> +typedef struct _virPoolSourceInitiatorAttr virPoolSourceInitiatorAttr; >>> +struct _virPoolSourceInitiatorAttr { >>> + char *iqn; /* Initiator IQN */ >>> +}; >> Same here, SCSI storage >> >>> + >>> +typedef struct _virPoolSource virPoolSource; >>> +typedef virPoolSource *virPoolSourcePtr; >>> +struct _virPoolSource { >> See my comment earlier regarding which is common and specific - this >> becomes the common stuff, but it's easier for me to just point out below >> all the structs, so see the end... >> >>> + /* An optional (maybe multiple) host(s) */ >>> + size_t nhost; >>> + virPoolSourceHostPtr hosts; >>> + >>> + /* And either one or more devices ... */ >>> + size_t ndevice; >>> + virPoolSourceDevicePtr devices; >>> + >>> + /* Or a directory */ >>> + char *dir; >>> + >>> + /* Or an adapter */ >>> + virPoolSourceAdapter adapter; >>> + >>> + /* Or a name */ >>> + char *name; >>> + >>> + /* Initiator IQN */ >>> + virPoolSourceInitiatorAttr initiator; >>> + >>> + /* Authentication information */ >>> + virStorageAuthDefPtr auth; >>> + >>> + /* Vendor of the source */ >>> + char *vendor; >>> + >>> + /* Product name of the source*/ >>> + char *product; >>> + >>> + /* Pool type specific format such as filesystem type, >>> + * or lvm version, etc. >>> + */ >>> + int format; >>> +}; >>> + >>> +typedef struct _virPoolTarget virPoolTarget; >>> +typedef virPoolTarget *virPoolTargetPtr; >>> +struct _virPoolTarget { >>> + char *path; /* Optional local filesystem mapping */ >>> + virStoragePerms perms; /* Default permissions for volumes/items */ >>> +}; >> This seems generic, although the comments need some tweaking to be more >> generic. A 'path' to the pool target... The virStoragePerms could >> become virPoolPathPerms with the virStoragePerms struct being your guide. >> >>> + >>> +typedef struct _virPoolDef virPoolDef; >>> +typedef virPoolDef *virPoolDefPtr; >>> +struct _virPoolDef { >>> + char *name; >>> + unsigned char uuid[VIR_UUID_BUFLEN]; >>> + int type; /* virStoragePoolType */ >> s/Storage// >> >> I'm conflicted over how to use type... >> and of course there would need to be a virPoolType list which would >> initially just be "BLOCK_STORAGE", but would eventually have >> "FILESYSTEM_STORAGE" (and down the road VGPU_DEVICES). >> >>> + >>> + unsigned long long allocation; /* bytes */ >>> + unsigned long long capacity; /* bytes */ >>> + unsigned long long available; /* bytes */ >>> + >> These would be "storage" related, but less so common type data. They're >> all "sizing" elements and could also be their own storage specific >> structure. >> >>> + virPoolSource source; >>> + virPoolTarget target; >>> +}; >>> +# endif /* __VIR_POOL_COMMON_H__ */ >>> >> What follows is my thoughts for various structures. I would think you >> have it fresh in your mind where the exact overlap is. >> >> These would be "virpool.h" type structures (left out some details): >> >> typedef enum { >> VIR_POOL_BLOCK_STORAGE, >> >> VIR_POOL_LAST, >> } virPoolType; >> >> struct _virPoolDef { >> int type; /* virPoolType */ >> char *name; >> unsigned char uuid[VIR_UUID_BUFLEN]; >> virPoolSource source; >> virPoolTarget target; >> }; >> >> BTW: I'm conflicted over the virPoolType since I'm not sure how it would >> be used other than to save it. We cannot use the virStoragePoolType >> enum's because those would be specific to the block storage pool... I'd >> suspect that FS >> >> struct _virPoolSource { >> /* One or more host definitions */ >> size_t nhost; >> virPoolSourceHostPtr hosts; >> >> /* Authentication information */ >> virStorageAuthDefPtr 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; >> }; >> >> typedef enum { >> VIR_POOL_SOURCE_DIR, >> VIR_POOL_SOURCE_DEVICE, >> VIR_POOL_SOURCE_NAME, >> >> VIR_POOL_LAST, >> } virPoolSourcePathType; >> >> >> struct _virPoolSourcePath { >> virPoolSourcePathType sourcetype; >> char *path; >> }; >> >> NB: Turns out 'logical' can have both 'device' and 'name', but "name" >> ends up being something we can add to a logical specific structure. Also >> 'gluster' can have both 'dir' and 'name', but it seems name is primary >> when both are used (see tests/*xmlin/*pool*gluster*.xml). >> >> Still the key is they are all paths of some sort: >> >> <dir path='$PATH'> >> <device path='$PATH'> >> >> where <device> it could be a path to a BLOCK device (/dev/) or it could >> be an iSCSI initiator string (and thus SOURCE_NAME being used...). >> Search on "<dir" or "<device" in tests/*xmlin/*pool*.xml >> >> struct _virPoolSourceHost { >> char *name; >> int port; >> }; >> >> struct _virPoolAuthDef { >> char *username; >> char *secrettype; /* <secret type='%s' for disk source */ >> int authType; /* virStorageAuthType */ >> virSecretLookupTypeDef seclookupdef; >> }; >> >> struct _virPoolTarget { >> char *path; /* Optional path to target */ >> virPoolPathPerms perms; /* Default permissions for path */ >> }; >> >> struct _virPoolPathPerms { >> mode_t mode; >> uid_t uid; >> gid_t gid; >> char *label; >> } >> >> >> Below here would be storage_conf.h type adjustments: >> >> struct _virStoragePoolDef { >> virPoolDef pool; >> virStoragePoolType storagetype; >> >> virStoragePoolSizes size; >> >> /* Based on storagetype, a subdata could be allocated to have/save >> * storagetype specific data */ >> void *subdata; /* vir*StoragePoolDefPtr */ >> >> /* In order to make life easy - format will be defined here >> * although it's only used by the some pools (fs, netfs, >> * disk, and logical via virStoragePoolFormat* enums */ >> int format; /* virStoragePoolFormat* */ >> }; >> >> struct _virStoragePoolSizes { >> unsigned long long allocation; /* bytes */ >> unsigned long long capacity; /* bytes */ >> unsigned long long available; /* bytes */ >> }; >> >> struct _virSCSIStoragePoolDef { >> virStoragePoolSourceAdapter adapter; >> virStoragePoolSourceInitiatorAttr initiator; >> }; >> >> struct _virDiskStoragePoolDef { >> int nfreeExtent; >> virDiskStoragePoolDeviceExtentPtr freeExtents; >> virStoragePoolGeometry geometry; >> int part_separator; /* enum virTristateSwitch */ >> }; >> >> struct _virDiskStoragePoolDeviceExtent { >> unsigned long long start; >> unsigned long long end; >> int type; /* virStorageFreeType */ >> }; >> >> >> NB: Gluster wouldn't need a pool specific data - it would just use the >> "name" and "dir" separately. Also, I think there's other patches that >> will want to have multiple gluster paths, so this (more or less) allows >> for that. >> >> I probably left out some structures, but I hope this makes sense... Yes, >> it's going to be painful to make the switchover. That's why I suggest >> taking it slowly and not trying to pile on all the FSPool changes along. >> >> >> John >> >> > > -- > Best regards, > Olga >