Merge lp:~charlesk/indicator-messages/fix-937438 into lp:indicator-messages/0.3

Proposed by Charles Kerr
Status: Merged
Approved by: Ted Gould
Approved revision: 254
Merged at revision: 250
Proposed branch: lp:~charlesk/indicator-messages/fix-937438
Merge into: lp:indicator-messages/0.3
Diff against target: 127 lines (+61/-57)
1 file modified
src/status-items.c (+61/-57)
To merge this branch: bzr merge lp:~charlesk/indicator-messages/fix-937438
Reviewer Review Type Date Requested Status
Lars Karlitski (community) Approve
Ted Gould (community) Approve
Review via email: mp+94017@code.launchpad.net

Description of the change

The code seems to be fine. It looks like Coverity is throwing this warning because it's confused by the overlapping goto's.

I've resolved this by not using goto, but maybe there's a simpler way of placating Coverity here?

To post a comment you must log in.
Revision history for this message
Ted Gould (ted) wrote :

On Tue, 2012-02-21 at 16:34 +0000, charles wrote:
> The code seems to be fine. It looks like Coverity is throwing this
> warning because it's confused by the overlapping goto's.

Yeah, I agree, but in general we should probably get rid of goto's. I
was probably just being lazy when I wrote this code anyway, I like the
updated code better :-)

  review approve
  status approved

review: Approve
Revision history for this message
Lars Karlitski (larsu) wrote :

Extra points for getting rid of the gpointer -> gchar * cast ;)

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/status-items.c'
2--- src/status-items.c 2011-07-13 20:17:58 +0000
3+++ src/status-items.c 2012-02-21 16:33:19 +0000
4@@ -253,62 +253,66 @@
5 static gboolean
6 load_status_provider (gpointer dir)
7 {
8- gchar * provider = (gchar *)dir;
9-
10- if (!g_file_test(provider, G_FILE_TEST_EXISTS)) {
11- goto exit_final;
12- }
13-
14- g_debug("Loading status provider: %s", provider);
15-
16- GModule * module;
17-
18- module = g_module_open(provider, G_MODULE_BIND_LAZY | G_MODULE_BIND_LOCAL);
19- if (module == NULL) {
20- g_warning("Unable to module for: %s", provider);
21- goto exit_module_fail;
22- }
23-
24- /* Got it */
25- GType (*type_func) (void);
26- if (!g_module_symbol(module, STATUS_PROVIDER_EXPORT_S, (gpointer *)&type_func)) {
27- g_warning("Unable to find type symbol in: %s", provider);
28- goto exit_module_fail;
29- }
30-
31- GType provider_type = type_func();
32- if (provider_type == 0) {
33- g_warning("Unable to create type from: %s", provider);
34- goto exit_module_fail;
35- }
36-
37- StatusProvider * sprovider = STATUS_PROVIDER(g_object_new(provider_type, NULL));
38- if (sprovider == NULL) {
39- g_warning("Unable to build provider from: %s", provider);
40- goto exit_module_fail;
41- }
42-
43- /* On update let's talk to all of them and create the aggregate
44- value to export */
45- g_signal_connect(G_OBJECT(sprovider), STATUS_PROVIDER_SIGNAL_STATUS_CHANGED, G_CALLBACK(update_status), NULL);
46-
47- /* Attach the module object to the status provider so
48- that when the status provider is free'd the module
49- is close automatically. */
50- g_object_set_data_full(G_OBJECT(sprovider), "status-provider-module", module, module_destroy_in_idle);
51-
52- status_providers = g_list_prepend(status_providers, sprovider);
53-
54- /* Force and update every time just so we know we're
55- in a consistent state*/
56- update_status();
57-
58- goto exit_final;
59-
60-exit_module_fail:
61- g_module_close(module);
62-
63-exit_final:
64+ gchar * provider = dir;
65+
66+ /* load the module */
67+ GModule * module = NULL;
68+ if (g_file_test(provider, G_FILE_TEST_EXISTS)) {
69+ g_debug("Loading status provider: %s", provider);
70+ module = g_module_open(provider, G_MODULE_BIND_LAZY | G_MODULE_BIND_LOCAL);
71+ if (module == NULL) {
72+ g_warning("Unable to open module: %s", provider);
73+ }
74+ }
75+
76+ /* find the status provider's GType */
77+ GType provider_type = 0;
78+ if (module != NULL) {
79+ GType (*type_func) (void);
80+ if (!g_module_symbol(module, STATUS_PROVIDER_EXPORT_S, (gpointer *)&type_func)) {
81+ g_warning("Unable to find type symbol in: %s", provider);
82+ } else {
83+ provider_type = type_func();
84+ if (provider_type == 0) {
85+ g_warning("Unable to create type from: %s", provider);
86+ }
87+ }
88+ }
89+
90+ /* instantiate the status provider */
91+ StatusProvider * sprovider = NULL;
92+ if (provider_type != 0) {
93+ sprovider = STATUS_PROVIDER(g_object_new(provider_type, NULL));
94+ if (sprovider == NULL) {
95+ g_warning("Unable to build provider from: %s", provider);
96+ }
97+ }
98+
99+ /* use the provider */
100+ if (sprovider != NULL) {
101+ /* On update let's talk to all of them and create the aggregate
102+ value to export */
103+ g_signal_connect(G_OBJECT(sprovider),
104+ STATUS_PROVIDER_SIGNAL_STATUS_CHANGED,
105+ G_CALLBACK(update_status), NULL);
106+
107+ /* Attach the module object to the status provider so
108+ that when the status provider is free'd the module
109+ is closed automatically. */
110+ g_object_set_data_full(G_OBJECT(sprovider),
111+ "status-provider-module",
112+ module, module_destroy_in_idle);
113+ module = NULL; /* don't close module in this func */
114+
115+ status_providers = g_list_prepend(status_providers, sprovider);
116+
117+ /* Force an update to ensure a consistent state */
118+ update_status();
119+ }
120+
121+ /* cleanup */
122+ if (module != NULL)
123+ g_module_close(module);
124 g_free(provider);
125- return FALSE;
126+ return FALSE; /* only call this idle func once */
127 }

Subscribers

People subscribed via source and target branches