>> bluetooth: the code cannot be reached

-         } else if (s_cdma) {
+         } else {
               fallback_prefix = _("CDMA connection");
               if (!nm_setting_cdma_get_number (s_cdma))
                    g_object_set (G_OBJECT (s_cdma), NM_SETTING_GSM_NUMBER,
          }


>> utils: no need to use MIN()

The commit message contains unrelated coverity warnings.

Also, ULONG can be 32 bit. How about the fixup!?



Rest LGTM

Looks good to me with thaller's fixups.

Looks good to me with thaller's fixups.

Fixed and committed to master:
9e2203c merge: fix number of defects found by Coverity scan (bgo #741122)
49bbafb utils: fix converting milliseconds to microseconds
448b073 bluetooth: the code cannot be reached
b11416d libnm: check pspec before accessing it in handle_property_changed()
2859933 callout: ignore waitpid() return value
43b4c8f platform: ignore nm_platform_ip4_route_add/delete return value
7744bd0 utils: initialize timespec structure
d80bb52 tui: width and height parameters was swapped, but they are ignored anyway
f931281 tui: set GError so that it is not NULL later
0da3b96 libnm-core: do not access array if it is NULL
e52f352 util: fix _log_connection_sort_names_fcn()
d637b3e libnm: missing break on get_property

Reopened the bug for additional fixes on coverity scan on latest master.
See branch jk/coverity.

I think this bug should merge with bug 728320, and the remaining bits of th/bg728320_coverity branch should be merged or dropped.

>> libnm-core: mute coverity for RESOURCE_LEAK (CWE-772) in g_r

-    g_return_val_if_fail (item != NULL, FALSE);
+    if (!item) {
+         g_warn_if_reached ();
+         return FALSE;
+    }


Why does this change fix the warning. Does it?

Also, I would rather see:

  if (!item)
      g_return_val_if_reached (FALSE);



>> tests: mute coverity for CHECKED_RETURN (CWE-252) in tests

How about adding
  nmtst_assert_setting_verifies()
to include/nm-test-utils.h
-- analog to nmtst_assert_connection_verifies_without_normalization()



>> device: mute coverity CHECKED_RETURN (CWE-252)

While at it, also fix missing whitespace after declaration of @tmp_str:
(and move @success there too?)

     for (i = 0; i < num; i++) {
          gs_free char *tmp_str = NULL;
+         gboolean success;
+
          addr = nm_setting_ip_config_get_address (s_ip4, i);


>> ifcfg-rh: coverity complained about not checking stat() return value
    
I think if stat() fails, @file_stat should not be used (and some form of error handling is necessary).


>> tests: mute coverity INFINITE_LOOP error

How about marking @result and @notified as volatile? Would that avoid the warning?


-    guint result = 0;
+    volatile guint result = 0;

-                      &result);
+                      (gpointer) &result);

(In reply to comment #7)
> >> tests: mute coverity INFINITE_LOOP error
> 
> How about marking @result and @notified as volatile? Would that avoid the
> warning?

they're not volatile though, coverity is just wrong, so it's better to just use coverity markup.

Likewise for the RESOURCE_LEAK ones; the patched version is the sort of thing that if I saw it, I would "fix" it to use g_return_val_if_fail() again. So it would be better to just block the coverity warning

(In reply to comment #7)
> >> libnm-core: mute coverity for RESOURCE_LEAK (CWE-772) in g_r
> 
> -    g_return_val_if_fail (item != NULL, FALSE);
> +    if (!item) {
> +         g_warn_if_reached ();
> +         return FALSE;
> +    }
> 
> 
> Why does this change fix the warning. Does it?
> 
It does.

> Also, I would rather see:
> 
>   if (!item)
>       g_return_val_if_reached (FALSE);
> 
Ok, I changed it to this.
 
> 
> >> tests: mute coverity for CHECKED_RETURN (CWE-252) in tests
> 
> How about adding
>   nmtst_assert_setting_verifies()
> to include/nm-test-utils.h
> -- analog to nmtst_assert_connection_verifies_without_normalization()
> 
Done. 

> 
> >> device: mute coverity CHECKED_RETURN (CWE-252)
> 
> While at it, also fix missing whitespace after declaration of @tmp_str:
> (and move @success there too?)
> 
>      for (i = 0; i < num; i++) {
>           gs_free char *tmp_str = NULL;
> +         gboolean success;
> +
>           addr = nm_setting_ip_config_get_address (s_ip4, i);
>
Done.

> 
> >> ifcfg-rh: coverity complained about not checking stat() return value
> 
> I think if stat() fails, @file_stat should not be used (and some form of error
> handling is necessary).
> 
> 
> >> tests: mute coverity INFINITE_LOOP error
> 
> How about marking @result and @notified as volatile? Would that avoid the
> warning?
> 
> 
> -    guint result = 0;
> +    volatile guint result = 0;
> 
> -                      &result);
> +                      (gpointer) &result);

I agree with Dan that the suppress comment is better here. (Anyway, I tried with volatile and that caused another coverity issue for using volatile in later.)

Also fixed two remaining NEGATIVE_RETURNS defects.
So now there are no problems (except DEADCODE in gtkdoc-scangobj generated helper code, e.g. docs/libnm/libnm-scan.c)

Pushed to jk/coverity2.

why not:

     mode_t st_mode = 0;

     if (stat (HOSTNAME_FILE, &file_stat) == 0)
         st_mode = file_stat.st_mode
     matchpathcon (HOSTNAME_FILE, st_mode, &se_ctx);


The rest LGTM.


If this fixes all coverity warnings, can we close bug 728320?

(In reply to comment #11)
> 
> why not:
> 
>      mode_t st_mode = 0;
> 
>      if (stat (HOSTNAME_FILE, &file_stat) == 0)
>          st_mode = file_stat.st_mode
>      matchpathcon (HOSTNAME_FILE, st_mode, &se_ctx);
> 
Ok, changed to this.

> 
> If this fixes all coverity warnings, can we close bug 728320?
Closed.

Commits in master:
fa94aaf merge: fix all defects found by coverity (bgo #741122)
5252287 tests: fix NEGATIVE_RETURNS (CWE-394) in tests
ce6323d tests: mute coverity INFINITE_LOOP error
405d198 ifcfg-rh: coverity complained about not checking stat() return value
4da19b8 device: mute coverity CHECKED_RETURN (CWE-252)
6603e7f tests: mute coverity for CHECKED_RETURN (CWE-252) in tests
afb0e2c libnm-core: mute coverity for RESOURCE_LEAK (CWE-772) in g_return_val_if_fail()
bcd5b2c cli: fix DEADCODE (CWE-561) found by coverity
ed088b0 cli: mute coverity for Error: DEADCODE (CWE-561)

just re-reviewed and everything looks fine for nm-1-0 imho

(In reply to comment #12)
> Commits in master:
> fa94aaf merge: fix all defects found by coverity (bgo #741122)
> 5252287 tests: fix NEGATIVE_RETURNS (CWE-394) in tests
> ce6323d tests: mute coverity INFINITE_LOOP error
> 405d198 ifcfg-rh: coverity complained about not checking stat() return value
> 4da19b8 device: mute coverity CHECKED_RETURN (CWE-252)
> 6603e7f tests: mute coverity for CHECKED_RETURN (CWE-252) in tests
> afb0e2c libnm-core: mute coverity for RESOURCE_LEAK (CWE-772) in
> g_return_val_if_fail()
> bcd5b2c cli: fix DEADCODE (CWE-561) found by coverity
> ed088b0 cli: mute coverity for Error: DEADCODE (CWE-561)

backported to nm-1-0 branch:
http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=8dd6a3b60075700ce2c0976670bd1d55fd5ae135