[libvirt] [PATCH RFC v3 01/15] storage pools: refactoring of basic structs
Olga Krishtal
okrishtal at virtuozzo.com
Wed Dec 21 17:08:50 UTC 2016
On 20/12/16 20:18, John Ferlan wrote:
>
> 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.
No - this is not a pool definition, but the definition of the pool's part.
>
>>
>> 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.
In here it is char *key; field.
>
> There should be a 'voltype' field here... Changing the name will dig out
> code that's using it.
Why voltype there is no volume. I do not see how we can set voltype
without knowledge what
kind of object we store in pool.
> 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.
Ok. Let's implement lists and and all changes that is necessary for
storage pools
to work with the new pool object. But I will think about it and may be
later we
change it . But it easier to start with it.
So in virpool.h we will have some bricks to describe common parts, and
Storage/FS structs will look like:
struct virStorageBlockPoolDef {
virPoolDefPtr poolDef;
some structs to describe storage
}
The same is for virStorageFSPool.
In this case we do not need to describe precisely pool element.
Moreover, we will have
API to manage common pool parts, and I think the changes won't be that huge.
I think I will send rfc of virpool.h/c separately rather keeping
discussion over here,
but will leave links to this conversation.
>
> 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 at virtuozzo.com>
>>>> ---
>>>> 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
>>
--
Best regards,
Olga
More information about the libvir-list
mailing list