On 07/11/2018 03:51 AM, Shichangkuo wrote: > > >> -----Original Message----- >> From: Michal Privoznik [mailto:mprivozn@xxxxxxxxxx] >> Sent: Tuesday, July 10, 2018 9:27 PM >> To: shichangkuo (Cloud); 'libvir-list@xxxxxxxxxx'; 'jferlan@xxxxxxxxxx' >> Subject: Re: [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@xxxxxxx> >>> --- >>> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
