On Tue, Aug 26, 2014 at 04:49:02PM -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.
> ---
>
> I could have sworn that I had already sent this revised patch to the list, but
> I can't find any evidence of having sent it. Fixed issues from Christophe's
> earlier review.
>
> src/remote-viewer.c | 82 ++++++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 62 insertions(+), 20 deletions(-)
>
> diff --git a/src/remote-viewer.c b/src/remote-viewer.c
> index cb43365..03998bf 100644
> --- a/src/remote-viewer.c
> +++ b/src/remote-viewer.c
> @@ -670,7 +670,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;
> @@ -683,16 +683,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(VIRT_VIEWER_ERROR,
> + VIRT_VIEWER_ERROR_FAILED,
> + _("URI is not valid"));
I mentioned using g_set_error_litteral in my earlier review. Were there
some issues with it?
> return FALSE;
> + }
>
> if (g_strcmp0(uri->scheme, "ovirt") != 0) {
> xmlFreeURI(uri);
> + if (error)
> + *error = g_error_new(VIRT_VIEWER_ERROR,
> + VIRT_VIEWER_ERROR_FAILED,
> + _("URI is not an oVirt URI"));
> return FALSE;
> }
>
> if (uri->path == NULL) {
> xmlFreeURI(uri);
> + if (error)
> + *error = g_error_new(VIRT_VIEWER_ERROR,
> + VIRT_VIEWER_ERROR_FAILED,
> + _("No virtual machine specified"));
> return FALSE;
> }
> g_return_val_if_fail(*uri->path == '/', FALSE);
> @@ -701,12 +714,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(VIRT_VIEWER_ERROR,
> + VIRT_VIEWER_ERROR_FAILED,
> + _("No virtual machine specified"));
> return FALSE;
> }
> - vm_name = path_elements[element_count-1];
> +
> path_elements[element_count-1] = NULL;
>
> if (username && uri->user)
> @@ -789,7 +809,7 @@ virt_viewer_app_set_ovirt_foreign_menu(VirtViewerApp *app,
>
>
> static gboolean
> -create_ovirt_session(VirtViewerApp *app, const char *uri)
> +create_ovirt_session(VirtViewerApp *app, const char *uri, GError **err)
> {
> OvirtProxy *proxy = NULL;
> OvirtApi *api = NULL;
> @@ -815,12 +835,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, &error))
> goto error;
>
> + proxy = ovirt_proxy_new(rest_uri);
> g_object_set(proxy,
> "username", username,
> NULL);
> @@ -830,19 +848,29 @@ create_ovirt_session(VirtViewerApp *app, const char *uri)
>
> api = ovirt_proxy_fetch_api(proxy, &error);
> if (error != NULL) {
> - g_debug("failed to get oVirt 'api' collection: %s", error->message);
> + g_debug("Failed to get oVirt 'api' collection: %s", error->message);
> goto error;
> }
> vms = ovirt_api_get_vms(api);
> - ovirt_collection_fetch(vms, proxy, &error);
> - if (error != NULL) {
> + if (!ovirt_collection_fetch(vms, proxy, &error)) {
> g_debug("failed to lookup %s: %s", vm_name, error->message);
> goto error;
> }
> vm = OVIRT_VM(ovirt_collection_lookup_resource(vms, vm_name));
> - g_return_val_if_fail(vm != NULL, FALSE);
> + if (!vm) {
> + error = g_error_new(VIRT_VIEWER_ERROR,
> + VIRT_VIEWER_ERROR_FAILED,
> + "Unable to find virtual machine '%s'",
> + vm_name);
> + g_debug("Unable to find virtual machine '%s'", vm_name);
> + goto error;
> + }
> g_object_get(G_OBJECT(vm), "state", &state, NULL);
> if (state != OVIRT_VM_STATE_UP) {
> + error = g_error_new(VIRT_VIEWER_ERROR,
> + VIRT_VIEWER_ERROR_FAILED,
> + "oVirt VM %s is not running",
> + vm_name);
> g_debug("oVirt VM %s is not running", vm_name);
> goto error;
> }
> @@ -854,6 +882,9 @@ create_ovirt_session(VirtViewerApp *app, const char *uri)
>
> g_object_get(G_OBJECT(vm), "display", &display, NULL);
> if (display == NULL) {
> + error = g_error_new(VIRT_VIEWER_ERROR,
> + VIRT_VIEWER_ERROR_FAILED,
> + _("Unable to get the display for the requested virtual machine"));
> goto error;
> }
>
> @@ -873,6 +904,10 @@ create_ovirt_session(VirtViewerApp *app, const char *uri)
> } else if (type == OVIRT_VM_DISPLAY_VNC) {
> session_type = "vnc";
> } else {
> + error = g_error_new(VIRT_VIEWER_ERROR,
> + VIRT_VIEWER_ERROR_FAILED,
> + "Unknown display type: %d",
> + type);
> g_debug("Unknown display type: %d", type);
> goto error;
> }
> @@ -886,8 +921,12 @@ create_ovirt_session(VirtViewerApp *app, const char *uri)
> 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) {
> + error = g_error_new(VIRT_VIEWER_ERROR,
> + VIRT_VIEWER_ERROR_FAILED,
> + _("Unable to create session"));
> goto error;
> + }
>
> #ifdef HAVE_SPICE_GTK
> if (type == OVIRT_VM_DISPLAY_SPICE) {
> @@ -909,8 +948,6 @@ create_ovirt_session(VirtViewerApp *app, const char *uri)
> }
> #endif
>
> - success = TRUE;
> -
> error:
> g_free(username);
> g_free(rest_uri);
> @@ -921,8 +958,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)
> @@ -932,6 +967,12 @@ error:
> if (proxy != NULL)
> g_object_unref(proxy);
>
> + success = (error == NULL);
> + if (err)
> + *err = error;
> + else
> + g_clear_error(&error);
same comment about using g_set_error which should remove the need for
this bit of code.
> +
> return success;
> }
>
> @@ -1158,8 +1199,9 @@ retry_dialog:
> }
> #ifdef HAVE_OVIRT
> if (g_strcmp0(type, "ovirt") == 0) {
> - if (!create_ovirt_session(app, guri)) {
> - virt_viewer_app_simple_message_dialog(app, _("Couldn't open oVirt session"));
> + if (!create_ovirt_session(app, guri, &error)) {
> + virt_viewer_app_simple_message_dialog(app, _("Couldn't open oVirt session: %s"), error->message);
> + g_clear_error(&error);
> goto cleanup;
> }
> } else
> --
> 1.9.3
>
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/virt-tools-list
Attachment:
pgpK7qum0etZF.pgp
Description: PGP signature
_______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
