[libvirt] [PATCH v2 10/12] migration: refactor: prepare to reuse flag vs feature checks
John Ferlan
jferlan at redhat.com
Thu Sep 17 14:56:12 UTC 2015
On 09/10/2015 09:20 AM, Nikolay Shirokovskiy wrote:
> All toURI functions have same checks for flags and features compatibility for
> direct and p2p case. Let's factor this checks out before we reuse common code
s/this/the
> in toURI functions family. As a side affect we have a more clear code
> representation of uri passing conventions for p2p and direct cases.
>
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
> ---
> src/libvirt-domain.c | 126 +++++++++++++++++++------------------------------
> 1 files changed, 49 insertions(+), 77 deletions(-)
>
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index f7d0777..483537a 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -4187,6 +4187,9 @@ virDomainMigrateToURI(virDomainPtr domain,
> const char *dname,
> unsigned long bandwidth)
> {
> + const char *dconnuri = NULL;
> + const char *miguri = NULL;
> +
> VIR_DOMAIN_DEBUG(domain, "duri=%p, flags=%lx, dname=%s, bandwidth=%lu",
> NULLSTR(duri), flags, NULLSTR(dname), bandwidth);
>
> @@ -4211,34 +4214,25 @@ virDomainMigrateToURI(virDomainPtr domain,
> goto error;
> }
>
> - if (flags & VIR_MIGRATE_PEER2PEER) {
> - if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> - VIR_DRV_FEATURE_MIGRATION_P2P)) {
> - VIR_DEBUG("Using peer2peer migration");
> - if (virDomainMigrateUnmanaged(domain, NULL, flags,
> - dname, duri, NULL, bandwidth) < 0)
> - goto error;
> - } else {
> - /* No peer to peer migration supported */
> - virReportUnsupportedError();
> - goto error;
> - }
> - } else {
> - if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> - VIR_DRV_FEATURE_MIGRATION_DIRECT)) {
> - VIR_DEBUG("Using direct migration");
> - if (virDomainMigrateUnmanaged(domain, NULL, flags,
> - dname, NULL, duri, bandwidth) < 0)
> - goto error;
> - } else {
> - /* Cannot do a migration with only the perform step */
> - virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> - _("direct migration is not supported by the"
> - " connection driver"));
> - goto error;
> - }
> + if (((flags & VIR_MIGRATE_PEER2PEER) &&
> + !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> + VIR_DRV_FEATURE_MIGRATION_P2P)) ||
^^^^
Not well aligned and shall I say a very "difficult" to read construct -
multiple ! and || with a couple of &&'s for good measure!
Can there be a
bool flg_p2p = (flags & VIR_MIGRATE_PEER2PEER);
bool drv_p2p = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver,
domain->conn,
VIR_DRV_FEATURE_MIGRATION_P2P);
bool dvr_direct = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver,
domain->conn,
VIR_DRV_FEATURE_MIGRATION_DIRECT);
const char *dconnuri = flg_p2p ? duri : NULL;
const char *miguri = !flg_p2p ? duri : NULL;
Then:
if ((flg_p2p && !drv_p2p) || (!flg_p2p && !drv_direct)) {
virReportUnsupportedError()
> + (!(flags & VIR_MIGRATE_PEER2PEER) &&
> + !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> + VIR_DRV_FEATURE_MIGRATION_DIRECT))) {
> + virReportUnsupportedError();
> + goto error;
> }
>
> + if (flags & VIR_MIGRATE_PEER2PEER)
> + dconnuri = duri;
> + else
> + miguri = duri;
> +
> + if (virDomainMigrateUnmanaged(domain, NULL, flags,
> + dname, dconnuri, miguri, bandwidth) < 0)
^^
Need to move the args over...
> + goto error;
> +
> return 0;
>
> error:
> @@ -4357,34 +4351,23 @@ :q
(virDomainPtr domain,
> VIR_MIGRATE_NON_SHARED_INC,
> error);
>
> - if (flags & VIR_MIGRATE_PEER2PEER) {
> - if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> - VIR_DRV_FEATURE_MIGRATION_P2P)) {
> - VIR_DEBUG("Using peer2peer migration");
> - if (virDomainMigrateUnmanaged(domain, dxml, flags,
> - dname, dconnuri, miguri, bandwidth) < 0)
> - goto error;
> - } else {
> - /* No peer to peer migration supported */
> - virReportUnsupportedError();
> - goto error;
> - }
> - } else {
> - if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> - VIR_DRV_FEATURE_MIGRATION_DIRECT)) {
> - VIR_DEBUG("Using direct migration");
> - if (virDomainMigrateUnmanaged(domain, dxml, flags,
> - dname, NULL, miguri, bandwidth) < 0)
> - goto error;
> - } else {
> - /* Cannot do a migration with only the perform step */
> - virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> - _("direct migration is not supported by the"
> - " connection driver"));
> - goto error;
> - }
> + if (((flags & VIR_MIGRATE_PEER2PEER) &&
> + !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> + VIR_DRV_FEATURE_MIGRATION_P2P)) ||
> + (!(flags & VIR_MIGRATE_PEER2PEER) &&
> + !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> + VIR_DRV_FEATURE_MIGRATION_DIRECT))) {
> + virReportUnsupportedError();
> + goto error;
> }
Similar difficult to read construct here...
>
> + if (!(flags & VIR_MIGRATE_PEER2PEER))
> + dconnuri = NULL;
> +
> + if (virDomainMigrateUnmanaged(domain, NULL, flags,
> + dname, dconnuri, miguri, bandwidth) < 0)
^^^
Spacing here...
This is where Coverity complains because 'dconnuri' being NULL
eventually triggers a rule violation because a call to virURIParse()
expects a NONNULL parameter. This is called via
virDomainMigrateCheckNotLocal
I'm still not sure of the best way to handle this, but may using using
local will suffice... That is after the flags, there's a
const char *duri = flg_p2p ? dconnuri : NULL;
and make the make the Unmanaged call with duri instead of dconnuri
Ugh... I just peeked ahead.... seems to mess up future changes... <sigh>
John
> + goto error;
> +
> return 0;
>
> error:
> @@ -4451,33 +4434,22 @@ virDomainMigrateToURI3(virDomainPtr domain,
> VIR_MIGRATE_NON_SHARED_INC,
> error);
>
> - if (flags & VIR_MIGRATE_PEER2PEER) {
> - if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> - VIR_DRV_FEATURE_MIGRATION_P2P)) {
> - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> - _("Peer-to-peer migration is not supported by "
> - "the connection driver"));
> - goto error;
> - }
> + if (((flags & VIR_MIGRATE_PEER2PEER) &&
> + !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> + VIR_DRV_FEATURE_MIGRATION_P2P)) ||
> + (!(flags & VIR_MIGRATE_PEER2PEER) &&
> + !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> + VIR_DRV_FEATURE_MIGRATION_DIRECT))) {
> + virReportUnsupportedError();
> + goto error;
> + }
>
> - VIR_DEBUG("Using peer2peer migration");
> - if (virDomainMigrateUnmanagedParams(domain, dconnuri, params, nparams, flags) < 0)
> - goto error;
> - } else {
> - if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
> - VIR_DRV_FEATURE_MIGRATION_DIRECT)) {
> - /* Cannot do a migration with only the perform step */
> - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> - _("Direct migration is not supported by the"
> - " connection driver"));
> - goto error;
> - }
> + if (!(flags & VIR_MIGRATE_PEER2PEER))
> + dconnuri = NULL;
>
> - VIR_DEBUG("Using direct migration");
> - if (virDomainMigrateUnmanagedParams(domain, NULL, params,
> - nparams, flags) < 0)
> - goto error;
> - }
> + if (virDomainMigrateUnmanagedParams(domain, dconnuri,
> + params, nparams, flags) < 0)
> + goto error;
>
> return 0;
>
>
More information about the libvir-list
mailing list