On 1/6/22 16:04, Ani Sinha wrote:

virConnectOpenInternal() does not report error in all failure scenarios, except in some specific cases. This inconsistent behavior forces the caller of this function report a generic error for all failure modes which then hides specific error scenarios. This change makes virConnectOpenInternal() report failure in all cases so that it can generate specific errors based on the type of failure encountered. The reporiting of the errors can be made more fine grained in subsequent changes.

Signed-off-by: Ani Sinha <ani@anisinha.ca> --- src/libvirt.c | 24 ++++++++++++++++-------- src/libxl/libxl_migration.c | 3 --- src/qemu/qemu_migration.c | 3 --- 3 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/src/libvirt.c b/src/libvirt.c index 45315f484c..53ceee1359 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -893,8 +893,12 @@ virConnectOpenInternal(const char *name, bool embed = false;

ret = virGetConnect(); - if (ret == NULL) + if (ret == NULL) { + virReportError(VIR_ERR_INVALID_CONN, + _("Failed to create connection object for URI %s"), + NULLSTR(name)); return NULL; + }

This overwrites an error reported by virGetConnect().

if (virConfLoadConfig(&conf, "libvirt.conf") < 0) goto failed; @@ -974,7 +978,7 @@ virConnectOpenInternal(const char *name, virReportError(VIR_ERR_NO_CONNECT, _("URI '%s' does not include a driver name"), name); - goto failed; + goto failed_no_report;

I'm not a big fan of this label. But, there's an easy trick to avoid it. So what's happening under 'failed' label is: virReportError(VIR_ERR_OPERATION_FAILED, _("Failed to connect to remote libvirt URI %s"), NULLSTR(uristr)); VIR_FREE(uristr); virObjectUnref(ret); and we want to avoid virReportError() but do execute VIR_FREE() and virObjectUnref(). Well, both these can be replaced with g_auto* and thus 'failed_no_report' would effectively be just plain 'return NULL'.

}

if (virConnectCheckURIMissingSlash(uristr, @@ -992,7 +996,7 @@ virConnectOpenInternal(const char *name, virReportError(VIR_ERR_NO_CONNECT, _("URI scheme '%s' for embedded driver is not valid"), ret->uri->scheme); - goto failed; + goto failed_no_report; }

root = virURIGetParam(ret->uri, "root"); @@ -1002,7 +1006,7 @@ virConnectOpenInternal(const char *name, if (!g_path_is_absolute(root)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("root path must be absolute")); - goto failed; + goto failed_no_report; }

if (virEventRequireImpl() < 0) @@ -1062,7 +1066,7 @@ virConnectOpenInternal(const char *name, __FILE__, __FUNCTION__, __LINE__, _("libvirt was built without the '%s' driver"), ret->uri->scheme); - goto failed; + goto failed_no_report; }

VIR_DEBUG("trying driver %zu (%s) ...", @@ -1112,13 +1116,13 @@ virConnectOpenInternal(const char *name, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Driver %s cannot be used in embedded mode"), virConnectDriverTab[i]->hypervisorDriver->name); - goto failed; + goto failed_no_report; } /* before starting the new connection, check if the driver only works * with a server, and so return an error if the server is missing */ if (virConnectDriverTab[i]->remoteOnly && ret->uri && !ret->uri->server) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("URI is missing the server part")); - goto failed; + goto failed_no_report; }

ret->driver = virConnectDriverTab[i]->hypervisorDriver; @@ -1155,7 +1159,7 @@ virConnectOpenInternal(const char *name, if (!ret->driver) { /* If we reach here, then all drivers declined the connection. */ virReportError(VIR_ERR_NO_CONNECT, "%s", NULLSTR(name)); - goto failed; + goto failed_no_report; }

VIR_FREE(uristr); @@ -1163,6 +1167,10 @@ virConnectOpenInternal(const char *name, return ret;

failed: + virReportError(VIR_ERR_OPERATION_FAILED, + _("Failed to connect to remote libvirt URI %s"), + NULLSTR(uristr)); + failed_no_report: VIR_FREE(uristr); virObjectUnref(ret);

diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index 6d0ab4ee28..bc2b5401da 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -1134,9 +1134,6 @@ libxlDomainMigrationSrcPerformP2P(libxlDriverPrivate *driver, virObjectLock(vm);

if (dconn == NULL) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("Failed to connect to remote libvirt URI %s: %s"), - dconnuri, virGetLastErrorMessage());

This change and the other hunk below should be a separate commit, IMO.

return ret; }

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index b9d7d582f5..2635ef1162 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5145,9 +5145,6 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriver *driver, goto cleanup;

if (dconn == NULL) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("Failed to connect to remote libvirt URI %s: %s"), - dconnuri, virGetLastErrorMessage()); return -1; }

Michal