I tend to agree. I don’t see how any legitimate GError * (from g_error_new or the like) would have a NULL message. So this is really defensive coding. Alternate solution would be to assert, but I don’t like killing the viewer if this ever happens. Christophe > On 9 Feb 2017, at 17:44, Jonathon Jongsma <jjongsma@xxxxxxxxxx> wrote: > > ...now I see that this was discussed in the review thread for your > other patch. I still think this should not be necessary. If error is > non-NULL, it should be guaranteed to have a non-NULL message. > > Jonathon > > > On Thu, 2017-02-09 at 10:40 -0600, Jonathon Jongsma wrote: >> Out of curiosity, what was the situation where you encountered a non- >> NULL error with a NULL error->message? It doesn't seem like this >> should >> be necessary. >> >> Jonathon >> >> >> >> On Thu, 2017-02-09 at 13:02 +0100, Christophe de Dinechin wrote: >>> --- >>> src/ovirt-foreign-menu.c | 14 +++++++------- >>> src/remote-viewer.c | 22 ++++++++++++---------- >>> src/virt-viewer-app.c | 14 +++++++------- >>> src/virt-viewer-file-transfer-dialog.c | 2 +- >>> src/virt-viewer-file.c | 6 +++--- >>> src/virt-viewer-session-spice.c | 8 ++++---- >>> src/virt-viewer-util.c | 4 ++++ >>> src/virt-viewer-util.h | 3 +++ >>> src/virt-viewer.c | 10 +++++----- >>> 9 files changed, 46 insertions(+), 37 deletions(-) >>> >>> diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c >>> index a51f2c9..8a2507f 100644 >>> --- a/src/ovirt-foreign-menu.c >>> +++ b/src/ovirt-foreign-menu.c >>> @@ -367,7 +367,7 @@ static void updated_cdrom_cb(GObject >>> *source_object, >>> const char *current_file = foreign_menu->priv- >>>> current_iso_name; >>> >>> >>> if (error != NULL) { >>> - g_warning("failed to update cdrom resource: %s", >>> error- >>>> message); >>> >>> + g_warning("failed to update cdrom resource: %s", >>> VV_MSG(error->message)); >>> g_clear_error(&error); >>> } >>> g_debug("setting OvirtCdrom:file back to '%s'", >>> @@ -504,7 +504,7 @@ static void cdrom_file_refreshed_cb(GObject >>> *source_object, >>> >>> ovirt_resource_refresh_finish(cdrom, result, &error); >>> if (error != NULL) { >>> - g_warning("failed to refresh cdrom content: %s", error- >>>> message); >>> >>> + g_warning("failed to refresh cdrom content: %s", >>> VV_MSG(error->message)); >>> g_clear_error(&error); >>> return; >>> } >>> @@ -548,7 +548,7 @@ static void cdroms_fetched_cb(GObject >>> *source_object, >>> >>> ovirt_collection_fetch_finish(cdrom_collection, result, >>> &error); >>> if (error != NULL) { >>> - g_warning("failed to fetch cdrom collection: %s", error- >>>> message); >>> >>> + g_warning("failed to fetch cdrom collection: %s", >>> VV_MSG(error->message)); >>> g_clear_error(&error); >>> return; >>> } >>> @@ -600,7 +600,7 @@ static void storage_domains_fetched_cb(GObject >>> *source_object, >>> >>> ovirt_collection_fetch_finish(collection, result, &error); >>> if (error != NULL) { >>> - g_warning("failed to fetch storage domains: %s", error- >>>> message); >>> >>> + g_warning("failed to fetch storage domains: %s", >>> VV_MSG(error->message)); >>> g_clear_error(&error); >>> return; >>> } >>> @@ -663,7 +663,7 @@ static void vms_fetched_cb(GObject >>> *source_object, >>> collection = OVIRT_COLLECTION(source_object); >>> ovirt_collection_fetch_finish(collection, result, &error); >>> if (error != NULL) { >>> - g_debug("failed to fetch VM list: %s", error->message); >>> + g_debug("failed to fetch VM list: %s", VV_MSG(error- >>>> message)); >>> >>> g_clear_error(&error); >>> return; >>> } >>> @@ -713,7 +713,7 @@ static void api_fetched_cb(GObject >>> *source_object, >>> proxy = OVIRT_PROXY(source_object); >>> menu->priv->api = ovirt_proxy_fetch_api_finish(proxy, result, >>> &error); >>> if (error != NULL) { >>> - g_debug("failed to fetch toplevel API object: %s", error- >>>> message); >>> >>> + g_debug("failed to fetch toplevel API object: %s", >>> VV_MSG(error->message)); >>> g_clear_error(&error); >>> return; >>> } >>> @@ -746,7 +746,7 @@ static void iso_list_fetched_cb(GObject >>> *source_object, >>> ovirt_collection_fetch_finish(collection, result, &error); >>> if (error != NULL) { >>> g_warning("failed to fetch files for ISO storage domain: >>> %s", >>> - error->message); >>> + VV_MSG(error->message)); >>> g_clear_error(&error); >>> return; >>> } >>> diff --git a/src/remote-viewer.c b/src/remote-viewer.c >>> index e4b0909..e4470ae 100644 >>> --- a/src/remote-viewer.c >>> +++ b/src/remote-viewer.c >>> @@ -278,7 +278,7 @@ spice_ctrl_do_connect(SpiceCtrlController *ctrl >>> G_GNUC_UNUSED, >>> GError *error = NULL; >>> >>> if (!virt_viewer_app_initial_connect(self, &error)) { >>> - const gchar *msg = error ? error->message : >>> + const gchar *msg = error ? VV_MSG(error->message) : >>> _("Failed to initiate connection"); >>> virt_viewer_app_simple_message_dialog(self, msg); >>> g_clear_error(&error); >>> @@ -591,7 +591,7 @@ spice_ctrl_listen_async_cb(GObject *object, >>> if (error != NULL) { >>> virt_viewer_app_simple_message_dialog(app, >>> _("Controller >>> connection failed: %s"), >>> - error->message); >>> + VV_MSG(error- >>>> message)); >>> >>> g_clear_error(&error); >>> exit(EXIT_FAILURE); /* TODO: make start async? */ >>> } >>> @@ -859,7 +859,7 @@ create_ovirt_session(VirtViewerApp *app, const >>> char *uri, GError **err) >>> >>> 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", >>> VV_MSG(error->message)); >>> #ifdef HAVE_OVIRT_CANCEL >>> if (g_error_matches(error, OVIRT_REST_CALL_ERROR, >>> OVIRT_REST_CALL_ERROR_CANCELLED)) { >>> g_clear_error(&error); >>> @@ -873,7 +873,7 @@ create_ovirt_session(VirtViewerApp *app, const >>> char *uri, GError **err) >>> vms = ovirt_api_get_vms(api); >>> ovirt_collection_fetch(vms, proxy, &error); >>> if (error != NULL) { >>> - g_debug("failed to fetch oVirt 'vms' collection: %s", >>> error- >>>> message); >>> >>> + g_debug("failed to fetch oVirt 'vms' collection: %s", >>> VV_MSG(error->message)); >>> goto error; >>> } >>> if (vm_name == NULL || >>> @@ -891,13 +891,13 @@ create_ovirt_session(VirtViewerApp *app, >>> const >>> char *uri, GError **err) >>> if (state != OVIRT_VM_STATE_UP) { >>> g_set_error(&error, VIRT_VIEWER_ERROR, >>> VIRT_VIEWER_ERROR_FAILED, >>> _("oVirt VM %s is not running"), vm_name); >>> - g_debug("%s", error->message); >>> + g_debug("%s", VV_MSG(error->message)); >>> goto error; >>> } >>> g_object_set(app, "guest-name", vm_name, NULL); >>> >>> if (!ovirt_vm_get_ticket(vm, proxy, &error)) { >>> - g_debug("failed to get ticket for %s: %s", vm_name, error- >>>> message); >>> >>> + g_debug("failed to get ticket for %s: %s", vm_name, >>> VV_MSG(error->message)); >>> goto error; >>> } >>> >>> @@ -931,7 +931,7 @@ create_ovirt_session(VirtViewerApp *app, const >>> char *uri, GError **err) >>> if (ghost == NULL) { >>> g_set_error(&error, VIRT_VIEWER_ERROR, >>> VIRT_VIEWER_ERROR_FAILED, >>> _("oVirt VM %s has no host information"), >>> vm_name); >>> - g_debug("%s", error->message); >>> + g_debug("%s", VV_MSG(error->message)); >>> goto error; >>> } >>> >>> @@ -942,7 +942,7 @@ create_ovirt_session(VirtViewerApp *app, const >>> char *uri, GError **err) >>> } else { >>> g_set_error(&error, VIRT_VIEWER_ERROR, >>> VIRT_VIEWER_ERROR_FAILED, >>> _("oVirt VM %s has unknown display type: %d"), >>> vm_name, type); >>> - g_debug("%s", error->message); >>> + g_debug("%s", VV_MSG(error->message)); >>> goto error; >>> } >>> >>> @@ -1136,7 +1136,7 @@ retry_dialog: >>> g_free(path); >>> if (error) { >>> g_prefix_error(&error, _("Invalid file %s: "), >>> guri); >>> - g_warning("%s", error->message); >>> + g_warning("%s", VV_MSG(error->message)); >>> goto cleanup; >>> } >>> g_object_get(G_OBJECT(vvfile), "type", &type, NULL); >>> @@ -1197,7 +1197,9 @@ cleanup: >>> >>> if (!ret && priv->open_recent_dialog) { >>> if (error != NULL) { >>> - virt_viewer_app_simple_message_dialog(app, _("Unable >>> to >>> connect: %s"), error->message); >>> + virt_viewer_app_simple_message_dialog(app, >>> + _("Unable to >>> connect: %s"), >>> + VV_MSG(error- >>>> message)); >>> >>> } >>> g_clear_error(&error); >>> goto retry_dialog; >>> diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c >>> index 2e7e193..34e60b8 100644 >>> --- a/src/virt-viewer-app.c >>> +++ b/src/virt-viewer-app.c >>> @@ -256,7 +256,7 @@ virt_viewer_app_save_config(VirtViewerApp >>> *self) >>> // with the vm name so user can make sense of it later. >>> gchar *comment = g_key_file_get_comment(priv->config, >>> priv- >>>> uuid, NULL, &error); >>> >>> if (error) { >>> - g_debug("Unable to get comment from key file: %s", >>> error->message); >>> + g_debug("Unable to get comment from key file: %s", >>> VV_MSG(error->message)); >>> g_clear_error(&error); >>> } else { >>> if (!comment || *comment == '\0') >>> @@ -267,7 +267,7 @@ virt_viewer_app_save_config(VirtViewerApp >>> *self) >>> >>> if ((data = g_key_file_to_data(priv->config, NULL, &error)) == >>> NULL || >>> !g_file_set_contents(priv->config_file, data, -1, &error)) >>> { >>> - g_warning("Couldn't save configuration: %s", error- >>>> message); >>> >>> + g_warning("Couldn't save configuration: %s", VV_MSG(error- >>>> message)); >>> >>> g_clear_error(&error); >>> } >>> g_free(data); >>> @@ -376,7 +376,7 @@ >>> virt_viewer_app_get_monitor_mapping_for_section(VirtViewerApp >>> *self, >>> const gchar >>> if (error) { >>> if (error->code != G_KEY_FILE_ERROR_GROUP_NOT_FOUND >>> && error->code != G_KEY_FILE_ERROR_KEY_NOT_FOUND) >>> - g_warning("Error reading monitor assignments for %s: >>> %s", section, error->message); >>> + g_warning("Error reading monitor assignments for %s: >>> %s", section, VV_MSG(error->message)); >>> g_clear_error(&error); >>> } else { >>> mapping = virt_viewer_parse_monitor_mappings(mappings, >>> nmappings, get_n_client_monitors()); >>> @@ -1240,7 +1240,7 @@ virt_viewer_app_activate(VirtViewerApp *self, >>> GError **error) >>> >>> if (ret == FALSE) { >>> if(error != NULL && *error != NULL) >>> - virt_viewer_app_show_status(self, (*error)->message); >>> + virt_viewer_app_show_status(self, VV_MSG((*error)- >>>> message)); >>> >>> priv->connected = FALSE; >>> } else { >>> virt_viewer_app_show_status(self, _("Connecting to graphic >>> server")); >>> @@ -1726,7 +1726,7 @@ virt_viewer_app_init(VirtViewerApp *self) >>> if (g_error_matches(error, G_FILE_ERROR, G_FILE_ERROR_NOENT)) >>> g_debug("No configuration file %s", self->priv- >>>> config_file); >>> >>> else if (error) >>> - g_warning("Couldn't load configuration: %s", error- >>>> message); >>> >>> + g_warning("Couldn't load configuration: %s", VV_MSG(error- >>>> message)); >>> >>> >>> g_clear_error(&error); >>> >>> @@ -1829,7 +1829,7 @@ >>> virt_viewer_app_on_application_startup(GApplication *app) >>> >>> if (!virt_viewer_app_start(self, &error)) { >>> if (error && !g_error_matches(error, VIRT_VIEWER_ERROR, >>> VIRT_VIEWER_ERROR_CANCELLED)) >>> - virt_viewer_app_simple_message_dialog(self, error- >>>> message); >>> >>> + virt_viewer_app_simple_message_dialog(self, >>> VV_MSG(error->message)); >>> >>> g_clear_error(&error); >>> g_application_quit(app); >>> @@ -1865,7 +1865,7 @@ virt_viewer_app_local_command_line >>> (GApplication *gapp, >>> >>> if (!g_option_context_parse(context, &argc, args, &error)) { >>> if (error != NULL) { >>> - g_printerr(_("%s\n"), error->message); >>> + g_printerr(_("%s\n"), VV_MSG(error->message)); >>> g_error_free(error); >>> } >>> >>> diff --git a/src/virt-viewer-file-transfer-dialog.c b/src/virt- >>> viewer-file-transfer-dialog.c >>> index 07d25a7..80930eb 100644 >>> --- a/src/virt-viewer-file-transfer-dialog.c >>> +++ b/src/virt-viewer-file-transfer-dialog.c >>> @@ -218,7 +218,7 @@ static void task_finished(SpiceFileTransferTask >>> *task, >>> >>> if (error && !g_error_matches(error, G_IO_ERROR, >>> G_IO_ERROR_CANCELLED)) { >>> self->priv->failed = g_slist_prepend(self->priv->failed, >>> g_object_ref(task)); >>> - g_warning("File transfer task %p failed: %s", task, error- >>>> message); >>> >>> + g_warning("File transfer task %p failed: %s", task, >>> VV_MSG(error->message)); >>> } >>> >>> self->priv->file_transfers = g_slist_remove(self->priv- >>>> file_transfers, task); >>> >>> diff --git a/src/virt-viewer-file.c b/src/virt-viewer-file.c >>> index 9ff2a05..4a45af4 100644 >>> --- a/src/virt-viewer-file.c >>> +++ b/src/virt-viewer-file.c >>> @@ -217,7 +217,7 @@ virt_viewer_file_get_string(VirtViewerFile* >>> self, >>> >>> result = g_key_file_get_string(self->priv->keyfile, group, >>> key, >>> &inner_error); >>> if (inner_error && inner_error->domain != G_KEY_FILE_ERROR) >>> - g_critical("%s", inner_error->message); >>> + g_critical("%s", VV_MSG(inner_error->message)); >>> g_clear_error(&inner_error); >>> >>> return result; >>> @@ -246,7 +246,7 @@ >>> virt_viewer_file_get_string_list(VirtViewerFile* >>> self, const char *group, >>> >>> result = g_key_file_get_string_list(self->priv->keyfile, >>> group, >>> key, length, &inner_error); >>> if (inner_error && inner_error->domain != G_KEY_FILE_ERROR) >>> - g_critical("%s", inner_error->message); >>> + g_critical("%s", VV_MSG(inner_error->message)); >>> g_clear_error(&inner_error); >>> >>> return result; >>> @@ -275,7 +275,7 @@ virt_viewer_file_get_int(VirtViewerFile* self, >>> >>> result = g_key_file_get_integer(self->priv->keyfile, group, >>> key, >>> &inner_error); >>> if (inner_error && inner_error->domain != G_KEY_FILE_ERROR) >>> - g_critical("%s", inner_error->message); >>> + g_critical("%s", VV_MSG(inner_error->message)); >>> g_clear_error(&inner_error); >>> >>> return result; >>> diff --git a/src/virt-viewer-session-spice.c b/src/virt-viewer- >>> session-spice.c >>> index c3fce48..e4d0e32 100644 >>> --- a/src/virt-viewer-session-spice.c >>> +++ b/src/virt-viewer-session-spice.c >>> @@ -297,7 +297,7 @@ usb_connect_failed(GObject *object >>> G_GNUC_UNUSED, >>> if (g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) >>> return; >>> >>> - g_signal_emit_by_name(self, "session-usb-failed", error- >>>> message); >>> >>> + g_signal_emit_by_name(self, "session-usb-failed", >>> VV_MSG(error- >>>> message)); >>> >>> } >>> >>> static void >>> virt_viewer_session_spice_set_has_sw_reader(VirtViewerSessionSpice >>> *session, >>> @@ -702,7 +702,7 @@ >>> virt_viewer_session_spice_main_channel_event(SpiceChannel *channel, >>> >>> if (self->priv->pass_try > 0) >>> g_signal_emit_by_name(session, "session-auth-refused", >>> - error != NULL ? error->message : >>> _("Invalid password")); >>> + error != NULL ? VV_MSG(error- >>>> message) : _("Invalid password")); >>> >>> self->priv->pass_try++; >>> >>> /* The username is firstly pre-filled with the username of >>> the current >>> @@ -742,7 +742,7 @@ >>> virt_viewer_session_spice_main_channel_event(SpiceChannel *channel, >>> { >>> const GError *error = spice_channel_get_error(channel); >>> >>> - g_debug("main channel: failed to connect %s", error ? >>> error- >>>> message : ""); >>> >>> + g_debug("main channel: failed to connect %s", error ? >>> VV_MSG(error->message) : ""); >>> >>> if (g_error_matches(error, G_IO_ERROR, >>> G_IO_ERROR_PROXY_NEED_AUTH) || >>> g_error_matches(error, G_IO_ERROR, >>> G_IO_ERROR_PROXY_AUTH_FAILED)) { >>> @@ -1128,7 +1128,7 @@ >>> virt_viewer_session_spice_channel_destroy(G_GNUC_UNUSED >>> SpiceSession >>> *s, >>> >>> self->priv->channel_count--; >>> if (self->priv->channel_count == 0) >>> - g_signal_emit_by_name(self, "session-disconnected", error >>> ? >>> error->message : NULL); >>> + g_signal_emit_by_name(self, "session-disconnected", error >>> ? >>> VV_MSG(error->message) : NULL); >>> } >>> >>> VirtViewerSession * >>> diff --git a/src/virt-viewer-util.c b/src/virt-viewer-util.c >>> index 0491f73..e49b25f 100644 >>> --- a/src/virt-viewer-util.c >>> +++ b/src/virt-viewer-util.c >>> @@ -41,6 +41,8 @@ >>> >>> #include "virt-viewer-util.h" >>> >>> +const char *virt_viewer_default_error = "Unknown virt-viewer >>> error"; >>> // Localized during init >>> + >>> GQuark >>> virt_viewer_error_quark(void) >>> { >>> @@ -265,6 +267,8 @@ static BOOL is_handle_valid(HANDLE h) >>> >>> void virt_viewer_util_init(const char *appname) >>> { >>> + virt_viewer_default_error = _(virt_viewer_default_error); >>> + >>> #ifdef G_OS_WIN32 >>> /* >>> * This named mutex will be kept around by Windows until the >>> diff --git a/src/virt-viewer-util.h b/src/virt-viewer-util.h >>> index 5515f66..6abfd1c 100644 >>> --- a/src/virt-viewer-util.h >>> +++ b/src/virt-viewer-util.h >>> @@ -36,6 +36,9 @@ enum { >>> #define VIRT_VIEWER_ERROR virt_viewer_error_quark () >>> #define VIRT_VIEWER_RESOURCE_PREFIX "/org/virt-manager/virt- >>> viewer" >>> >>> +extern const char *virt_viewer_default_error; >>> +#define VV_MSG(msg) ((msg) ? (msg) : virt_viewer_default_error) >>> + >>> GQuark virt_viewer_error_quark(void); >>> >>> void virt_viewer_util_init(const char *appname); >>> diff --git a/src/virt-viewer.c b/src/virt-viewer.c >>> index 1121146..320f803 100644 >>> --- a/src/virt-viewer.c >>> +++ b/src/virt-viewer.c >>> @@ -630,7 +630,7 @@ virt_viewer_open_connection(VirtViewerApp *self >>> G_GNUC_UNUSED, int *fd) >>> >>> err = virGetLastError(); >>> if (err && err->code != VIR_ERR_NO_SUPPORT) { >>> - g_debug("Error %s", err->message ? err->message : >>> "Unknown"); >>> + g_debug("Error %s", VV_MSG(err->message) ? err->message : >>> "Unknown"); >>> return TRUE; >>> } >>> #endif >>> @@ -642,7 +642,7 @@ virt_viewer_open_connection(VirtViewerApp *self >>> G_GNUC_UNUSED, int *fd) >>> if (virDomainOpenGraphics(priv->dom, 0, pair[0], >>> VIR_DOMAIN_OPEN_GRAPHICS_SKIPAUTH) < >>> 0) { >>> err = virGetLastError(); >>> - g_debug("Error %s", err && err->message ? err->message : >>> "Unknown"); >>> + g_debug("Error %s", err && VV_MSG(err->message) ? err- >>>> message : "Unknown"); >>> >>> close(pair[0]); >>> close(pair[1]); >>> return TRUE; >>> @@ -680,7 +680,7 @@ virt_viewer_domain_event(virConnectPtr conn >>> G_GNUC_UNUSED, >>> case VIR_DOMAIN_EVENT_STARTED: >>> virt_viewer_update_display(self, dom, &error); >>> if (error) { >>> - virt_viewer_app_simple_message_dialog(app, error- >>>> message); >>> >>> + virt_viewer_app_simple_message_dialog(app, >>> VV_MSG(error- >>>> message)); >>> >>> g_clear_error(&error); >>> } >>> >>> @@ -688,7 +688,7 @@ virt_viewer_domain_event(virConnectPtr conn >>> G_GNUC_UNUSED, >>> if (error) { >>> /* we may want to consolidate error reporting in >>> app_activate() instead */ >>> - g_warning("%s", error->message); >>> + g_warning("%s", VV_MSG(error->message)); >>> g_clear_error(&error); >>> } >>> break; >>> @@ -778,7 +778,7 @@ choose_vm(GtkWindow *main_window, >>> virErrorPtr err = virGetLastError(); >>> g_set_error_literal(error, >>> VIRT_VIEWER_ERROR, >>> VIRT_VIEWER_ERROR_FAILED, >>> - err && err->message ? err->message : >>> "unknown libvirt error"); >>> + err && err->message ? err->message : >>> _("Unknown libvirt error")); >>> } else if (virDomainGetState(dom, &i, NULL, 0) < 0 || i != >>> VIR_DOMAIN_RUNNING) { >>> g_set_error(error, >>> VIRT_VIEWER_ERROR, VIRT_VIEWER_ERROR_FAILED, >> >> _______________________________________________ >> virt-tools-list mailing list >> virt-tools-list@xxxxxxxxxx >> https://www.redhat.com/mailman/listinfo/virt-tools-list > > _______________________________________________ > virt-tools-list mailing list > virt-tools-list@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/virt-tools-list _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list