/**
* 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
