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

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.