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 <jferlan@xxxxxxxxxx>
> >> ---
> >> 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 <phrdina@xxxxxxxxxx>
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
