[virt-tools-list] [PATCH virt-viewer 2/2] Report more detail about oVirt connection failures
Christophe Fergeau
cfergeau at redhat.com
Thu Aug 7 16:19:54 UTC 2014
On Thu, Aug 07, 2014 at 10:20:56AM -0500, Jonathon Jongsma wrote:
> Previously, when a user tried to connect to an ovirt vm using an
> ovirt:// URI, all connection failures were reported to the user with a
> simple "Couldn't open oVirt Session" error dialog. Now we report a
> slightly more detailed error message to the user (e.g. "Unauthorized",
> "No vm specified", etc.
>
> This change also catches an error case that wasn't handled before: an
> empty string is no longer a valid vm name.
> ---
> src/remote-viewer.c | 95 +++++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 67 insertions(+), 28 deletions(-)
>
> diff --git a/src/remote-viewer.c b/src/remote-viewer.c
> index b2cf748..30f8444 100644
> --- a/src/remote-viewer.c
> +++ b/src/remote-viewer.c
> @@ -624,7 +624,7 @@ remote_viewer_window_added(VirtViewerApp *app,
>
> #ifdef HAVE_OVIRT
> static gboolean
> -parse_ovirt_uri(const gchar *uri_str, char **rest_uri, char **name, char **username)
> +parse_ovirt_uri(const gchar *uri_str, char **rest_uri, char **name, char **username, GError** error)
> {
> char *vm_name = NULL;
> char *rel_path;
> @@ -637,16 +637,29 @@ parse_ovirt_uri(const gchar *uri_str, char **rest_uri, char **name, char **usern
> g_return_val_if_fail(name != NULL, FALSE);
>
> uri = xmlParseURI(uri_str);
> - if (uri == NULL)
> + if (uri == NULL) {
> + if (error)
> + *error = g_error_new_literal(VIRT_VIEWER_ERROR,
> + VIRT_VIEWER_ERROR_FAILED,
> + _("URI is not valid"));
g_set_error_literal?
> return FALSE;
> + }
>
> if (g_strcmp0(uri->scheme, "ovirt") != 0) {
> xmlFreeURI(uri);
> + if (error)
> + *error = g_error_new_literal(VIRT_VIEWER_ERROR,
> + VIRT_VIEWER_ERROR_FAILED,
> + _("URI is not an ovirt URI"));
Upstream spelling is 'oVirt', same comment about g_set_error_literal.
> return FALSE;
> }
>
> if (uri->path == NULL) {
> xmlFreeURI(uri);
> + if (error)
> + *error = g_error_new_literal(VIRT_VIEWER_ERROR,
> + VIRT_VIEWER_ERROR_FAILED,
> + _("No virtual machine specified"));
g_set_error_literal
> return FALSE;
> }
>
> @@ -654,12 +667,19 @@ parse_ovirt_uri(const gchar *uri_str, char **rest_uri, char **name, char **usern
> path_elements = g_strsplit(uri->path, "/", -1);
>
> element_count = g_strv_length(path_elements);
> - if (element_count == 0) {
> + if (element_count > 0)
> + vm_name = path_elements[element_count - 1];
> +
> + if (vm_name == NULL || *vm_name == '\0') {
> g_strfreev(path_elements);
> xmlFreeURI(uri);
> + if (error)
> + *error = g_error_new_literal(VIRT_VIEWER_ERROR,
> + VIRT_VIEWER_ERROR_FAILED,
> + _("No virtual machine specified"));
g_set_error_literal
> return FALSE;
> }
> - vm_name = path_elements[element_count-1];
> +
> path_elements[element_count-1] = NULL;
>
> if (username && uri->user)
> @@ -712,7 +732,7 @@ authenticate_cb(RestProxy *proxy, G_GNUC_UNUSED RestProxyAuth *auth,
>
>
> static gboolean
> -create_ovirt_session(VirtViewerApp *app, const char *uri)
> +create_ovirt_session(VirtViewerApp *app, const char *uri, GError **error)
> {
> OvirtProxy *proxy = NULL;
> OvirtApi *api = NULL;
> @@ -720,7 +740,6 @@ create_ovirt_session(VirtViewerApp *app, const char *uri)
> OvirtVm *vm = NULL;
> OvirtVmDisplay *display = NULL;
> OvirtVmState state;
> - GError *error = NULL;
> char *rest_uri = NULL;
> char *vm_name = NULL;
> char *username = NULL;
> @@ -729,6 +748,7 @@ create_ovirt_session(VirtViewerApp *app, const char *uri)
> guint secure_port;
> OvirtVmDisplayType type;
> const char *session_type;
> + GError* e = NULL;
I'd keep 'error' as the name of the local error, and use "err" or
something like that as the out error parameter in order to keep the diff
shorter. If you use g_set_error_literal throughout the function, you may
not even need the local variable.
>
> gchar *gport = NULL;
> gchar *gtlsport = NULL;
> @@ -738,12 +758,10 @@ create_ovirt_session(VirtViewerApp *app, const char *uri)
>
> g_return_val_if_fail(VIRT_VIEWER_IS_APP(app), FALSE);
>
> - if (!parse_ovirt_uri(uri, &rest_uri, &vm_name, &username))
> - goto error;
> - proxy = ovirt_proxy_new(rest_uri);
> - if (proxy == NULL)
> + if (!parse_ovirt_uri(uri, &rest_uri, &vm_name, &username, &e))
> goto error;
>
> + proxy = ovirt_proxy_new(rest_uri);
> g_object_set(proxy,
> "username", username,
> NULL);
> @@ -751,32 +769,43 @@ create_ovirt_session(VirtViewerApp *app, const char *uri)
> g_signal_connect(G_OBJECT(proxy), "authenticate",
> G_CALLBACK(authenticate_cb), app);
>
> - api = ovirt_proxy_fetch_api(proxy, &error);
> - if (error != NULL) {
> - g_debug("failed to get oVirt 'api' collection: %s", error->message);
> + api = ovirt_proxy_fetch_api(proxy, &e);
> + if (e) {
> + g_debug("failed to get oVirt 'api' collection");
I'd keep the error message in the g_debug as this can be helpful when
looking at the logs from a bug report.
> goto error;
> }
> vms = ovirt_api_get_vms(api);
> - ovirt_collection_fetch(vms, proxy, &error);
> - if (error != NULL) {
> - g_debug("failed to lookup %s: %s", vm_name, error->message);
> + if (!ovirt_collection_fetch(vms, proxy, &e)) {
> + g_debug("failed to lookup %s", vm_name);
> goto error;
> }
> vm = OVIRT_VM(ovirt_collection_lookup_resource(vms, vm_name));
> - g_return_val_if_fail(vm != NULL, FALSE);
> + if (!vm) {
> + e = g_error_new(VIRT_VIEWER_ERROR,
> + VIRT_VIEWER_ERROR_FAILED,
> + "Unable to find virtual machine '%s'",
> + vm_name);
I'd add a matching g_debug message here.
> + goto error;
> + }
> g_object_get(G_OBJECT(vm), "state", &state, NULL);
> if (state != OVIRT_VM_STATE_UP) {
> - g_debug("oVirt VM %s is not running", vm_name);
And I'd keep that one.
> + e = g_error_new(VIRT_VIEWER_ERROR,
> + VIRT_VIEWER_ERROR_FAILED,
> + "oVirt VM %s is not running",
> + vm_name);
> goto error;
> }
>
> - if (!ovirt_vm_get_ticket(vm, proxy, &error)) {
> - g_debug("failed to get ticket for %s: %s", vm_name, error->message);
> + if (!ovirt_vm_get_ticket(vm, proxy, &e)) {
> + g_debug("failed to get ticket for %s", vm_name);
(I'd keep the error message in the g_debug)
> goto error;
> }
>
> g_object_get(G_OBJECT(vm), "display", &display, NULL);
> if (display == NULL) {
g_debug()
> + e = g_error_new_literal(VIRT_VIEWER_ERROR,
> + VIRT_VIEWER_ERROR_FAILED,
> + _("Unable to get the display for the requested virtual machine"));
> goto error;
> }
>
> @@ -796,15 +825,22 @@ create_ovirt_session(VirtViewerApp *app, const char *uri)
> } else if (type == OVIRT_VM_DISPLAY_VNC) {
> session_type = "vnc";
> } else {
> - g_debug("Unknown display type: %d", type);
Keep that.
> + e = g_error_new(VIRT_VIEWER_ERROR,
> + VIRT_VIEWER_ERROR_FAILED,
> + "Unknown display type: %d",
> + type);
> goto error;
> }
>
> virt_viewer_app_set_connect_info(app, NULL, ghost, gport, gtlsport,
> session_type, NULL, NULL, 0, NULL);
>
> - if (virt_viewer_app_create_session(app, session_type) < 0)
> + if (virt_viewer_app_create_session(app, session_type) < 0) {
> + e = g_error_new_literal(VIRT_VIEWER_ERROR,
> + VIRT_VIEWER_ERROR_FAILED,
> + _("Unable to create session"));
> goto error;
> + }
>
> #ifdef HAVE_SPICE_GTK
> if (type == OVIRT_VM_DISPLAY_SPICE) {
> @@ -826,8 +862,6 @@ create_ovirt_session(VirtViewerApp *app, const char *uri)
> }
> #endif
>
> - success = TRUE;
> -
> error:
> g_free(username);
> g_free(rest_uri);
> @@ -838,8 +872,6 @@ error:
> g_free(ghost);
> g_free(host_subject);
>
> - if (error != NULL)
> - g_error_free(error);
> if (display != NULL)
> g_object_unref(display);
> if (vm != NULL)
> @@ -849,6 +881,12 @@ error:
> if (proxy != NULL)
> g_object_unref(proxy);
>
> + success = (e == NULL);
> + if (error)
> + *error = e;
> + else
> + g_clear_error(&e);
> +
> return success;
> }
>
Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20140807/cf28f4b8/attachment.sig>
More information about the virt-tools-list
mailing list