src/libvirt.c | 24 ++++++++++++++++-------- src/libxl/libxl_migration.c | 3 --- src/qemu/qemu_migration.c | 3 --- 3 files changed, 16 insertions(+), 14 deletions(-)
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;
+ }
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;
}
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());
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;
}
--
2.25.1
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
On Thu, Jan 06, 2022 at 20:34:26 +0530, 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));
'virGetConnect' is reporting errors in all cases when NULL is returned.
> return NULL;
> + }
>
> if (virConfLoadConfig(&conf, "libvirt.conf") < 0)
This seems to be reporting them too.
> 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;
> }
>
> if (virConnectCheckURIMissingSlash(uristr,
e.g this function is also reporting errors and better than the one you
are replacing it with.
Based on at least the two counts above I think you'll need to be more
specific in your claims in the commit message and re-asses all the code
paths.
On Thu, Jan 06, 2022 at 08:34:26PM +0530, Ani Sinha wrote: > virConnectOpenInternal() does not report error in all failure scenarios, except > in some specific cases. I don't believe this is correct. My reading of the code is that it is already reporting errors in all exit paths that can return NULL. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, 6 Jan 2022, Daniel P. Berrangé wrote: > On Thu, Jan 06, 2022 at 08:34:26PM +0530, Ani Sinha wrote: > > virConnectOpenInternal() does not report error in all failure scenarios, except > > in some specific cases. > > I don't believe this is correct. My reading of the code is that it > is already reporting errors in all exit paths that can return NULL. Yes you are likely right. I should have gone one level deeper in the call chain. I have not checked all the code paths exhaistively but for the one I checked, the lowest level functions are indeed reporting the more finer grained errors to their callers. I have sent an updated patch that just has the last two hunks. I have kept virConnectOpenInternal() unchanged.
© 2016 - 2024 Red Hat, Inc.