Re: [libvirt] [PATCH v2 12/12] migration: refactor: one return in forURI family functions

On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote: > May be a matter of a taste but this version with one return point in every > function looks simplier to understand and to exetend too. Anyway after such s/exetend/extend > a heavy refactoring a little cleanup will not hurt. > > Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; > --- > src/libvirt-domain.c | 61 +++++++++++++++++++++++--------------------------- > 1 files changed, 28 insertions(+), 33 deletions(-) > Although I know Daniel has ACK'd already - I figured I'd complete it too... BTW: I can confirm the following will work in ToURI3: const char *duri = (flags & VIR_MIGRATE_PEER2PEER) ? dconnuri : NULL; ... Remove the dconnuri = NULL, replace 'dconnuri' with 'duri' in the virDomainMigrateUnmanagedParams call and Coverity stops complaining. Curiously, I see the *ToURI2 function has a similar construct without a complaint, but it's calling virDomainMigrateUnmanaged and I'll assume Coverity gets sufficiently boggled with the call path that it doesn't matter. Your call if you want to change the *ToURI2 function to use a local static... Beyond that these changes seem perfectly reasonable to me as well.

John > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c > index 2d98997..aad2849 100644 > --- a/src/libvirt-domain.c > +++ b/src/libvirt-domain.c > @@ -4216,6 +4216,7 @@ virDomainMigrateToURI(virDomainPtr domain, > const char *dname, > unsigned long bandwidth) > { > + int ret = -1; > const char *dconnuri = NULL; > const char *miguri = NULL; > > @@ -4225,26 +4226,24 @@ virDomainMigrateToURI(virDomainPtr domain, > virResetLastError(); > > virCheckDomainReturn(domain, -1); > - virCheckReadOnlyGoto(domain->conn->flags, error); > - virCheckNonNullArgGoto(duri, error); > + virCheckReadOnlyGoto(domain->conn->flags, cleanup); > + virCheckNonNullArgGoto(duri, cleanup); > > if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0) > - goto error; > + goto cleanup; > > if (flags & VIR_MIGRATE_PEER2PEER) > dconnuri = duri; > else > miguri = duri; > > - if (virDomainMigrateUnmanaged(domain, NULL, flags, > - dname, dconnuri, miguri, bandwidth) < 0) > - goto error; > - > - return 0; > + ret = virDomainMigrateUnmanaged(domain, NULL, flags, > + dname, dconnuri, miguri, bandwidth); > + cleanup: > + if (ret < 0) > + virDispatchError(domain->conn); > > - error: > - virDispatchError(domain->conn); > - return -1; > + return ret; > } > > > @@ -4343,6 +4342,7 @@ virDomainMigrateToURI2(virDomainPtr domain, > const char *dname, > unsigned long bandwidth) > { > + int ret = -1; > VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, miguri=%s, dxml=%s, " > "flags=%lx, dname=%s, bandwidth=%lu", > NULLSTR(dconnuri), NULLSTR(miguri), NULLSTR(dxml), > @@ -4351,23 +4351,20 @@ virDomainMigrateToURI2(virDomainPtr domain, > virResetLastError(); > > virCheckDomainReturn(domain, -1); > - virCheckReadOnlyGoto(domain->conn->flags, error); > + virCheckReadOnlyGoto(domain->conn->flags, cleanup); > > if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0) > - goto error; > + goto cleanup; > > if (!(flags & VIR_MIGRATE_PEER2PEER)) > dconnuri = NULL; > > - if (virDomainMigrateUnmanaged(domain, NULL, flags, > - dname, dconnuri, miguri, bandwidth) < 0) > - goto error; > - > - return 0; > - > - error: > - virDispatchError(domain->conn); > - return -1; > + ret = virDomainMigrateUnmanaged(domain, NULL, flags, > + dname, dconnuri, miguri, bandwidth); > + cleanup: > + if (ret < 0) > + virDispatchError(domain->conn); > + return ret; > } > > > @@ -4415,6 +4412,7 @@ virDomainMigrateToURI3(virDomainPtr domain, > unsigned int nparams, > unsigned int flags) > { > + int ret = -1; > VIR_DOMAIN_DEBUG(domain, "dconnuri=%s, params=%p, nparms=%u flags=%x", > NULLSTR(dconnuri), params, nparams, flags); > VIR_TYPED_PARAMS_DEBUG(params, nparams); > @@ -4422,23 +4420,20 @@ virDomainMigrateToURI3(virDomainPtr domain, > virResetLastError(); > > virCheckDomainReturn(domain, -1); > - virCheckReadOnlyGoto(domain->conn->flags, error); > + virCheckReadOnlyGoto(domain->conn->flags, cleanup); > > if (virDomainMigrateUnmanagedCheckCompat(domain, flags) < 0) > - goto error; > + goto cleanup; > > if (!(flags & VIR_MIGRATE_PEER2PEER)) > dconnuri = NULL; > > - if (virDomainMigrateUnmanagedParams(domain, dconnuri, > - params, nparams, flags) < 0) > - goto error; > - > - return 0; > - > - error: > - virDispatchError(domain->conn); > - return -1; > + ret = virDomainMigrateUnmanagedParams(domain, dconnuri, > + params, nparams, flags); > + cleanup: > + if (ret < 0) > + virDispatchError(domain->conn); > + return ret; > } > > >