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@xxxxxxxxxxx>
> ---
> 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
