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

[John Ferlan]

On 12/21/2016 12:08 PM, Olga Krishtal wrote:
> 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.
> 
> 

I've probably reached the point where conceptualizing things just isn't
working. Between this and the other work I'm involved - there's not a
lot of space left in the short term memory.

>>
>>>                          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.

I prefer uuid - it is what it is.

>>
>> 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.
> 

Oh right - mixing my metaphors... Although a 'pooltype' field could be
useful as a way to describe any void * data...

> 
>>    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.

That's fine - I have it on my list to "get around to" converting all
lists to tables... Well actually it's to make common poolobj code which
is just another abstraction level.

> 
> 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.
> 

That's fine - be aware the Red Hat folks (including me) are all going to
disappear until 2017... There may be a few brave souls that want to work
during their "vacation" (ok company shutdown).


John

[...]