[libvirt] Refactoring of storage pool
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. Before sending any patch with such huge refactoring I want to agree about pools Here is an example of pool structures and prototypes of some management functions that were discussed before (thanks a lot to John Ferlan for good advice). I tried to tie everything together and here it is: /* * 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. * 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; }; /* Used to be PoolObjList * Separate struct - in case we will need additional info * upon the hash table. */ typedef struct _virPoolStore virPoolStore; typedef virPoolStore *virPoolStorePtr; struct _virPoolStore { size_t count; virHashAtomicPtr *pools; //Used to be objects obj }; /* * Pool hash and pool elements managment operations */ void virPoolElementDefFree (virPoolElementDefPtr def); void virPoolClearElements(virPoolInfoPtr poolinfo); void virPoolAuthDefFree(virPoolAuthDefPtr authdef); virPoolSourcePtr virPoolSourceNew (virPoolPrivDataFree privDataFree); void virPoolSourceClear(virPoolSourcePtr source); void virPoolDefFree(virPoolDefPtr def); void virPoolInfoFree(virPoolInfoPtr pool); virPoolStorePtr virPoolStoreNew(); * * Functions to manage pool as an object. * */ // It is reference implementation. #include "virpool.h" VIR_ENUM_IMPL(virPoolElementType, VIR_POOL_ELEMNT_LAST, "volume", "filesystem"); void virPoolElementDefFree(virPoolElementDefPtr def) { if (!def) return; VIR_FREE(def->name); VIR_FREE(def->uuid); if (def->PoolElementDefFree) def->PoolElementDefFree(def->ElementTypeDef); } void virPoolClearElements(virPoolInfoPtr poolinfo) { size_t i; for (i = 0; i < poolinfo->elementsdef.count; i++) virPoolElementDefFree(poolinfo->elementsdef.elements[i]); VIR_FREE(poolinfo->elementdef.elements); poolindo->elementsdef.count = 0; } /* General functions to work with pool */ VIR_ENUM_IMPL(virPoolSourcePathType, VIR_POOL_SOURCE_LAST, "dir", "device", "name"); void virPoolAuthDefFree(virPoolAuthDefPtr authdef) { if (!authdef) return; VIR_FREE(authdef->username); VIR_FREE(authdef->secrettype); VIR_FREE(authdef); } virPoolSourcePtr virPoolSourceNew(virPoolPrivDataFree privDataFree) { virPoolSourcePtr source; if (VIR_ALLOC(source) < 0) return -1; source->privDataFree = privDataFree; return source; } void virPoolSourceClear(virPoolSourcePtr source) { size_t i; if (!source) return; /* Free hosts */ for (i = 0; i < source->nhosts; i++) VIR_FREE(source->hosts[i].name); VIR_FREE(source->hosts); /* Free auth information * empty at the moment*/ virPoolAuthDefFree(source->auth); /* Free dev path*/ for (i = 0; i < source->nsrcpath; i++) VIR_FREE(source->srcpath[i].name); VIR_FREE(source->srcpath); VIR_FREE(source->name); VIR_FREE(source->vendor); VIR_FREE(source->product); if (source->privDataFree) source->privDataFree(source->privatePoolData); } void virPoolDefFree(virPoolDefPtr def) { if (!def) return; VIR_FREE(def->name); /*Free other fields*/ virPoolSourceClear(def->source); VIR_FREE(def->target.path); VIR_FREE(def->target.perms.lable); VIR_FREE(def); } void virPoolInfoFree(virPoolInfoPtr pool) { if (!pool) return; VIR_FREE(pool->configFile); VIR_FREE(pool->autostartLink); virPoolDefFree(pool->def); virPoolDefFree(pool->newDef); virPoolClearElements(pool); } virPoolStorePtr virPoolStoreNew() { virPoolStorePtr poolstore; if (VIR_ALLOC(poolstore) < 0) return NULL; poolstore->count = 0; poolstore->pools = virHashAtomicNew(0, virPoolInfoFree); return poolstore; } I will be glad to here any ideas and advises. -- Best regards, Olga
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 -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
On 26/01/17 17:49, Daniel P. Berrange wrote:
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.
I completely understand you, because it is difficult to predict and describe features for vGPU pools without any example. So yes, it is mostly storage/filesystem pools specific. There still left private structs for more freedom.
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.
I like this version too, (it should be slightly changed and rebased). However, this refactoring example is the result of discussions with John Ferlan. (He actually got pretty good ideas changes in storage pools). We need John's opinion so we all know that we are on the same page.
Regards, Daniel
-- Best regards, Olga
On 01/26/2017 11:16 AM, Olga Krishtal wrote:
On 26/01/17 17:49, Daniel P. Berrange wrote:
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.
I completely understand you, because it is difficult to predict and describe features for vGPU pools without any example. So yes, it is mostly storage/filesystem pools specific. There still left private structs for more freedom.
Sorry the GPU stuff was just "in my head" at the time. One of the ideas being floated was (ahem) borrowing the StoragePool code in order to make a pool of GPU's, but I haven't followed it closely since then. So, yeah let's not worry about it. Actually some other work I started may help that effort more.
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.
Still there are some concepts that are duplicated and a bug fix for one would be beneficial to have in the other rather than having to make the same change in two places because there's just piles of cut-n-paste code or not making in one place and having the same bug in the other place. The task is to figure out what those could be.
I like this version too, (it should be slightly changed and rebased). However, this refactoring example is the result of discussions with John Ferlan. (He actually got pretty good ideas changes in storage pools). We need John's opinion so we all know that we are on the same page.
Even after reading a couple of times, I cannot recall the entire frame of reference from my responses to the patch stream. What I do recall is seeing so much cut-n-paste code that it concerned me. Now for external API's there's probably no way around that. However, for the internal inner workings I would think if you find yourself essentially copying the *whole* module or *whole* data structure to be used by the FSPool, then that's where a data structures and code could be made common. For data structures that includes (at least and off the top of my head) things such as: virStorageVolDef virStorageVolObj virStoragePoolDef virStoragePoolObj virStorageSourcePtr virStoragePoolTargetPtr The bugger is *how* best to do that. I don't have all the answers here, but I would prefer to see a logical approach that will allow you to continue to make progress while also leaving me enough time to work through stuff I'm on the hook for ;-). One hard part of this for me is the "FS" nomenclature being used - I keep thinking about the WITH_STORAGE_FS type code. Still with all the changes for the vstorage changes "in" now - I hope you can understand what type and a methodology to "share" data structures/code. FWIW: Beyond your changes - there's been even more changes made to the code since you posted in December and other changes I have floating around in a couple git branches which were started because of thinking about what you were trying to accomplish. One set of changes is to try to "objectify" the guts (e.g. virStorage*Obj[List]Ptr) in order to make a more consistent API to access driver objects. I have that code compiling, passing the local test infrastructure, but it's not complete yet. I know that will be useful for the FSPool - the bulk of those virFSPoolObj would get common code to use. My plan is to post an RFC soon, but I want the vHBA test and node_device changes I posted yesterday to get pushed since my branch touches the same area... The second set of changes is to make accessors for all virStorageSource data rather than having so many places "think" it knows what to do (I've already found/fixed a couple of places where the wrong type field was being used). I need to update those changes to merge in Peter's storage_util changes. But given your patch 1 - I think those would be a good starting point. Not sure I answered the base question. You have in your mind where you need to get to with the FSPool and what's been essentially copied. With more time now looking at things - what changes do you believe could make use of common code. John
participants (3)
-
Daniel P. Berrange -
John Ferlan -
Olga Krishtal