[libvirt] [PATCH] storage: prefer using newDef to save configfile

Michal Privoznik mprivozn at redhat.com
Wed Jul 11 09:30:29 UTC 2018


On 07/11/2018 03:51 AM, Shichangkuo wrote:
> 
> 
>> -----Original Message-----
>> From: Michal Privoznik [mailto:mprivozn at redhat.com]
>> Sent: Tuesday, July 10, 2018 9:27 PM
>> To: shichangkuo (Cloud); 'libvir-list at redhat.com'; 'jferlan at redhat.com'
>> Subject: Re: [libvirt] [PATCH] storage: prefer using newDef to save configfile
>>
>> On 07/10/2018 09:15 AM, Shichangkuo wrote:
>>> When re-defining an active storage pool, the configfile has not
>>> changed. This issue was introduced by bfcd8fc9,
>>> storage: Use virStoragePoolObjGetDef accessor for driver. So we prefer using
>> newDef to save configfile.
>>>
>>> Signed-off-by: Changkuo Shi <shi.changkuo at h3c.com>
>>> ---
>>>  src/storage/storage_driver.c |    6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> The commit message seems to have long lines. Also, next time please send patch
>> rebased to the latest HEAD. I had to go all the way down to v4.4.0 only to apply this
>> patch cleanly.
>>
> Hi, Michal
>     Thanks for you reply. I will change the commit message format and use the latest HEAD in patch V1.
> 
>>>
>>> diff --git a/src/storage/storage_driver.c
>>> b/src/storage/storage_driver.c index b0de96d..fab3c5b 100644
>>> --- a/src/storage/storage_driver.c
>>> +++ b/src/storage/storage_driver.c
>>> @@ -844,13 +844,14 @@ storagePoolDefineXML(virConnectPtr conn,
>>>
>>>      if (!(obj = virStoragePoolObjAssignDef(driver->pools, newDef)))
>>>          goto cleanup;
>>> -    newDef = NULL;
>>> +    newDef = virStoragePoolObjGetNewDef(obj);
>>>      def = virStoragePoolObjGetDef(obj);
>>>
>>> -    if (virStoragePoolObjSaveDef(driver, obj, def) < 0) {
>>> +    if (virStoragePoolObjSaveDef(driver, obj, newDef ? newDef : def)
>>> + < 0) {
>>
>> Why wouldn't newDef be set at this point? It is always going to be a non-NULL
>> pointer.
>>
> I think the purpose of bfcd8fc9 'storage: Use virStoragePoolObjGetDef accessor for driver' is avoiding to use newDef directly.
> so I use virStoragePoolObjGetNewDef to set newDef again, and it will be NULL when we define a storage pool at the first time or the pool is inactive already.


Agrh. Good point. Apparently I was too distracted yesterday. Your patch
is correct after all.

Tweaked the commit message a bit, ACKed and pushed.

Congratulations on your first libvirt contribution!

Michal




More information about the libvir-list mailing list