Bug 1543871
| Summary: | [NMCI][abrt] [faf] NetworkManager: unknown function(): /usr/sbin/NetworkManager killed by 5 | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | Red Hat Enterprise Linux 7 | Reporter: | Vladimir Benes <vbenes> | ||||||
| Component: | NetworkManager | Assignee: | Beniamino Galvani <bgalvani> | ||||||
| Status: | CLOSED ERRATA | QA Contact: | Desktop QE <desktop-qa-list> | ||||||
| Severity: | unspecified | Docs Contact: | |||||||
| Priority: | unspecified | ||||||||
| Version: | 7.5 | CC: | atragler, bgalvani, fgiudici, lrintel, rkhan, sukulkar, thaller, thudziec | ||||||
| Target Milestone: | rc | ||||||||
| Target Release: | --- | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| URL: | http://faf.lab.eng.brq.redhat.com/faf/reports/bthash/f99d93593152655702dcd8f15e003ae6b24f1f08/ | ||||||||
| Whiteboard: | |||||||||
| Fixed In Version: | NetworkManager-1.12.0-0.1.el7 | Doc Type: | If docs needed, set a value | ||||||
| Doc Text: | Story Points: | --- | |||||||
| Clone Of: | Environment: | ||||||||
| Last Closed: | 2018-10-30 11:11:28 UTC | Type: | Bug | ||||||
| Regression: | --- | Mount Type: | --- | ||||||
| Documentation: | --- | CRM: | |||||||
| Verified Versions: | Category: | --- | |||||||
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||
| Cloudforms Team: | --- | Target Upstream Version: | |||||||
| Embargoed: | |||||||||
| Attachments: |
|
||||||||
|
Description
Vladimir Benes
2018-02-09 12:56:06 UTC
Created attachment 1396900 [details]
[PATCH] ovs: add error code for callbacks to indicate NM is quitting
> g_set_error_literal (&error, G_IO_ERROR, G_IO_ERROR_CLOSED, "Quitting");
let's not re-use GIO error codes for internal purposes. G_IO_ERROR_CANCELLED has a very particular meaning, let's not extend it but define our own error codes.
We seldomly care about error codes, but when we do, we should define our own.
See for example:
NM_UTILS_ERROR_CANCELLED_DISPOSING
nm_utils_error_set_cancelled()
nm_utils_error_is_cancelled()
and possibly re-use/extend that one.
(note that nm_utils_error_set_cancelled() may indeed re-use G_IO_ERROR_CANCELLED error code, but that is correct because G_IO_ERROR_CANCELLED has exactly the meaning "cancelled by user".
On the other hand, G_IO_ERROR_CLOSED has some meaning that sounds like related to a socket. But here you re-use it for for a different meaning, which is wrong).
Created attachment 1398172 [details] [PATCH v2] ovs: add error code for callbacks to indicate NM is quitting (In reply to Thomas Haller from comment #3) > We seldomly care about error codes, but when we do, we should define our own. > > See for example: > NM_UTILS_ERROR_CANCELLED_DISPOSING > nm_utils_error_set_cancelled() > nm_utils_error_is_cancelled() > and possibly re-use/extend that one. Okay, how about v2? (In reply to Beniamino Galvani from comment #4) > Created attachment 1398172 [details] > [PATCH v2] ovs: add error code for callbacks to indicate NM is quitting > > (In reply to Thomas Haller from comment #3) > > We seldomly care about error codes, but when we do, we should define our own. > > > > See for example: > > NM_UTILS_ERROR_CANCELLED_DISPOSING > > nm_utils_error_set_cancelled() > > nm_utils_error_is_cancelled() > > and possibly re-use/extend that one. > > Okay, how about v2? I like v2. It is quite ugly that calling "callback" consumes the @error (takes ownership). And IIUC, _transact_cb() leaks error. Could you do a follow-up commit to fix that by not passing ownership on? That way, you can also re-use the error variable in ovsdb_disconnect() (In reply to Thomas Haller from comment #5) > It is quite ugly that calling "callback" consumes the @error (takes > ownership). And IIUC, _transact_cb() leaks error. Could you do a follow-up > commit to fix that by not passing ownership on? That way, you can also > re-use the error variable in ovsdb_disconnect() Added commit to bg/rh1543871. (In reply to Beniamino Galvani from comment #6) > (In reply to Thomas Haller from comment #5) > > It is quite ugly that calling "callback" consumes the @error (takes > > ownership). And IIUC, _transact_cb() leaks error. Could you do a follow-up > > commit to fix that by not passing ownership on? That way, you can also > > re-use the error variable in ovsdb_disconnect() > > Added commit to bg/rh1543871. lgtm, but could you add a "Fixes" comment, because this fixes a memory leak (doesn't it)? Applied to master: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=cf79615169b4c2df8c5d99aa43e370e90e2d82ef https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=878a3a4125e07b3f82d7ed7b418021ae1398aaae and nm-1-10. *** Bug 1589724 has been marked as a duplicate of this bug. *** *** Bug 1592251 has been marked as a duplicate of this bug. *** According to FAF server, we haven't seen this crash with 1.12 anymore. Since the problem described in this bug report should be resolved in a recent advisory, it has been closed with a resolution of ERRATA. For information on the advisory, and where to find the updated files, follow the link below. If the solution does not work for you, open a new bug report. https://access.redhat.com/errata/RHBA-2018:3207 |