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

> -----Original Message----- > From: Michal Privoznik [mailto:mprivozn@redhat.com] > Sent: Tuesday, July 10, 2018 9:27 PM > To: shichangkuo (Cloud); &#39;libvir-list(a)redhat.com&#39;; &#39;jferlan(a)redhat.com&#39; > 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(a)h3c.com&gt; >> --- >> 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.