[PATCH] Make virConnectOpenInternal() report error in all cases

[Michal Prívozník]

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 at 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