On Thu, Jul 13, 2017 at 03:40:26PM -0400, John Ferlan wrote: > > > On 07/11/2017 04:27 AM, Pavel Hrdina wrote: > > On Tue, May 09, 2017 at 11:30:09AM -0400, John Ferlan wrote: > >> A virStoragePoolObjPtr will be an 'obj'. > >> > >> A virStoragePoolPtr will be a 'pool'. > >> > >> Signed-off-by: John Ferlan <jfer...@redhat.com> > >> --- > >> src/test/test_driver.c | 443 > >> ++++++++++++++++++++++++------------------------- > >> 1 file changed, 219 insertions(+), 224 deletions(-) > >> > >> diff --git a/src/test/test_driver.c b/src/test/test_driver.c > >> index 548f318..c0e46af 100644 > >> --- a/src/test/test_driver.c > >> +++ b/src/test/test_driver.c > > > > [...] > > > >> @@ -4057,18 +4056,18 @@ testStoragePoolLookupByUUID(virConnectPtr conn, > >> const unsigned char *uuid) > >> { > >> testDriverPtr privconn = conn->privateData; > >> - virStoragePoolObjPtr pool; > >> + virStoragePoolObjPtr obj; > >> virStoragePoolPtr ret = NULL; > > > > There you didn't changed the "ret" to "pool". > > > >> - if (!(pool = testStoragePoolObjFindByUUID(privconn, uuid))) > >> + if (!(obj = testStoragePoolObjFindByUUID(privconn, uuid))) > >> goto cleanup; > >> > >> - ret = virGetStoragePool(conn, pool->def->name, pool->def->uuid, > >> + ret = virGetStoragePool(conn, obj->def->name, obj->def->uuid, > >> NULL, NULL); > >> > >> cleanup: > >> - if (pool) > >> - virStoragePoolObjUnlock(pool); > >> + if (obj) > >> + virStoragePoolObjUnlock(obj); > >> return ret; > >> } > >> > >> @@ -4078,18 +4077,18 @@ testStoragePoolLookupByName(virConnectPtr conn, > >> const char *name) > >> { > >> testDriverPtr privconn = conn->privateData; > >> - virStoragePoolObjPtr pool; > >> + virStoragePoolObjPtr obj; > >> virStoragePoolPtr ret = NULL; > > > > Same here. > > > >> - if (!(pool = testStoragePoolObjFindByName(privconn, name))) > >> + if (!(obj = testStoragePoolObjFindByName(privconn, name))) > >> goto cleanup; > >> > >> - ret = virGetStoragePool(conn, pool->def->name, pool->def->uuid, > >> + ret = virGetStoragePool(conn, obj->def->name, obj->def->uuid, > >> NULL, NULL); > >> > >> cleanup: > >> - if (pool) > >> - virStoragePoolObjUnlock(pool); > >> + if (obj) > >> + virStoragePoolObjUnlock(obj); > >> return ret; > >> } > >> > > > > [...] > > > >> @@ -4345,7 +4344,7 @@ testStoragePoolCreateXML(virConnectPtr conn, > >> { > >> testDriverPtr privconn = conn->privateData; > >> virStoragePoolDefPtr def; > >> - virStoragePoolObjPtr pool = NULL; > >> + virStoragePoolObjPtr obj = NULL; > >> virStoragePoolPtr ret = NULL; > > > > And here. > > > >> virObjectEventPtr event = NULL; > >> > > > > [...] > > > >> @@ -4419,7 +4418,7 @@ testStoragePoolDefineXML(virConnectPtr conn, > >> { > >> testDriverPtr privconn = conn->privateData; > >> virStoragePoolDefPtr def; > >> - virStoragePoolObjPtr pool = NULL; > >> + virStoragePoolObjPtr obj = NULL; > >> virStoragePoolPtr ret = NULL; > > > > And here > > > > I can adjust those... I was more focused on the "existing" @pool and > @obj variables and didn't consider the existing @ret variables. > > >> virObjectEventPtr event = NULL; > >> > > > > [...] > > > > I don't like these type of patches. The value to noise ration is poor. > > I'm hesitating to give this patch an ACK even though I probably done > > that in the past. > > > > Pavel > > > > So while I understand the concern, my argument becomes is it technically > incorrect to change the names? Secondary to that if you're considering > multiple driver/vir*obj changes at one time, it is *far easier* to > consider an "@obj" to be that thing in the list vir*obj.c list rather > than what is either passed or gets returned from/to the consumer - a > @pool. And yes, technically a @pool is an object - I know. > > The second thing that gets fixed by these changes is inconsistent usage. > For instance, prior to these changes look at testParseStorage and > testOpenVolumesForPool. The former uses @obj for a virStoragePoolObjPtr > while the latter uses @pool for a virStoragePoolObjPtr. It becomes > difficult to "follow" code with inconsistencies. So if someone takes the > effort to make things consistent - I think that's better in the long > run. Makes the code more maintainable for a reader, but yes a hassle for > someone back porting some subsequent change because of the merge conflict. > > The make code more consistent wins the argument my left and right brain > have over this. > > If there's not an ACK, then so be it - I can remove it, but it probably > affects subsequent patches too.
It's my personal feeling that there is no much value in this patch, however the code will be at least consistent. Reviewed-by: Pavel Hrdina <phrd...@redhat.com>
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list