[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