Re: [libvirt] [PATCH 04/19] test: Add helpers to fetch active/inactive storage pool by name

On Tue, May 09, 2017 at 11:30:11AM -0400, John Ferlan wrote: > Rather than have repetitive code - create/use a couple of helpers: > > testStoragePoolObjFindActiveByName and testStoragePoolObjFindInactiveByName I would wrap this line, it's too long for no reason.

> This will also allow for the reduction of some cleanup path logic. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > src/test/test_driver.c | 256 +++++++++++++++++-------------------------------- > 1 file changed, 86 insertions(+), 170 deletions(-) > > diff --git a/src/test/test_driver.c b/src/test/test_driver.c > index efa54ff..9918df6 100644 > --- a/src/test/test_driver.c > +++ b/src/test/test_driver.c > @@ -4030,6 +4030,46 @@ testStoragePoolObjFindByName(testDriverPtr privconn, > > > static virStoragePoolObjPtr > +testStoragePoolObjFindActiveByName(testDriverPtr privconn, > + const char *name) > +{ > + virStoragePoolObjPtr obj; > + > + if (!(obj = testStoragePoolObjFindByName(privconn, name))) > + return NULL; > + > + if (!virStoragePoolObjIsActive(obj)) { > + virReportError(VIR_ERR_OPERATION_INVALID, > + _("storage pool '%s' is not active"), name); > + virStoragePoolObjUnlock(obj); > + return NULL; > + } > + > + return obj; > +} > + > + > +static virStoragePoolObjPtr > +testStoragePoolObjFindInactiveByName(testDriverPtr privconn, > + const char *name) > +{ > + virStoragePoolObjPtr obj; > + > + if (!(obj = testStoragePoolObjFindByName(privconn, name))) > + return NULL; > + > + if (virStoragePoolObjIsActive(obj)) { > + virReportError(VIR_ERR_OPERATION_INVALID, > + _("storage pool '%s' is already active"), name); I would remove the "already" for the error message. Since this is only test driver I'll leave it up to you. The reason is that for some APIs like "Undefine" the error message doesn't make sense. Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com&gt;