Re: [libvirt] [PATCH 3/3] common-impl: make virDomainObjGetPersistentDef return only persistent config

On 19.02.2016 02:19, John Ferlan wrote: Yes, it quite mangled place. I think we'd better stick with next definitions as this is how we seem to use them in other places. 1. transient config - config for a running domain, lost after it is destroyed. 2. persistent config - config on disk for a domain. Looking at the code I've found next correnspondence of def and newDef to this definitions: 1. active persistent domain def - transient newDef - persistent 2. inactive persistent domain def - persistent 3. transient domain (active) def - transient Thus newDef has only one purpose - persistent config of active persistent domain. def could be transient or persistent depends on situation. Now one can understand the reason for the function virDomainObjSetDefTransient naming. On persistent domain start we move from 'persistent' to 'transient' for 'def' meaining. 'live' flags helps us do it before actual start is happend. I think such a function would be hard to use as usually we need to distinguish transient/persistent and have no notion of 'active'. Hmm ... if someone knows it is transient domain or inactive persistent domain it can refer to config without specifying it further as transient or persistent. Well in this case he can just use plain 'def' instead of this function. Another common usage of def is the case we need 'transient' config. It is always in 'def'. This is why we don't have virDomainObjGetTransientDef. Yes that check is there because there is no need for newDef for a transient domain. But I think the reason virDomainObjGetPersistentDef calls virDomainObjSetDefTransient is different. 'Set' function is kind of lazy initialization tool. We initialize newDef only when someone wants change persistent config for a running persistent domain for the first time. This is the reason this check is there: if(domain->newDef) return 0; However I doubt we need this lazy initialization there anymore. After cb4c2694f166beca8f3e1fb37dedeec1ff3fbdf6 commit newDef is initialized early in the process of starting of domain for lxc and qemu drivers at least. It will take time to investigate it... Just get here. virDomainObjGetDefs has the exact description of correspondence I wrote above in one place. Here it is 'live' config instead of 'transient'. Yes, if someone needs 'def' for transient domain - it needs 'transient' config and can use this function. Hmm, one asks why bother to mention 'transient' on config for transient domain as there is no other one. Well looks like we have to as we have one type for all three states - active persistent, active transient, inactive persistent. As to reporting error upon return NULL from the function. I think it is not right as if someone use the return value in this circumstances - it is a case of incorrect code and moreover it will crash instanly. After all this investigation I would like to see one day all usages of def and newDef would be replaced by getTransient and getPersistent calls )) with all the mechanics incapsulated. Nikolay.