After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 689569 - Code cleanup
Code cleanup
Status: RESOLVED FIXED
Product: gdm
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: GDM maintainers
GDM maintainers
Depends on:
Blocks:
 
 
Reported: 2012-12-03 21:07 UTC by Colin Walters
Modified: 2012-12-03 22:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
configure: Require GLib 2.35.0 (777 bytes, patch)
2012-12-03 21:07 UTC, Colin Walters
committed Details | Review
build: Don't automatically use _FORTIFY_SOURCE (1.09 KB, patch)
2012-12-03 21:07 UTC, Colin Walters
committed Details | Review
Remove handling of SIGSEGV/SIGFPE/SIGILL etc. (8.01 KB, patch)
2012-12-03 21:07 UTC, Colin Walters
none Details | Review
gdm-crash-logger: Delete (5.72 KB, patch)
2012-12-03 21:07 UTC, Colin Walters
none Details | Review
Port to g_unix_signal_add(), drop GdmSignalHandler (43.32 KB, patch)
2012-12-03 21:07 UTC, Colin Walters
committed Details | Review
Drop calls to g_type_init()/g_thread_init() (8.51 KB, patch)
2012-12-03 21:07 UTC, Colin Walters
committed Details | Review
gdm-md5: Delete (18.37 KB, patch)
2012-12-03 21:07 UTC, Colin Walters
committed Details | Review
gdm-common: Drop some dead code (4.29 KB, patch)
2012-12-03 21:08 UTC, Colin Walters
committed Details | Review
smartcard: Port to g_thread_new() (1.35 KB, patch)
2012-12-03 21:08 UTC, Colin Walters
committed Details | Review
gui/simple-greeter: Fix minor compiler warnings (1.91 KB, patch)
2012-12-03 21:08 UTC, Colin Walters
committed Details | Review

Description Colin Walters 2012-12-03 21:07:41 UTC
I was going to fix something else, then I noticed some old code, and went
on kind of a deletion/cleanup spree...

We're not warning-free at compile time yet - that is leftover for removing
the fallback greeter.
Comment 1 Colin Walters 2012-12-03 21:07:43 UTC
Created attachment 230580 [details] [review]
configure: Require GLib 2.35.0

For future changes.
Comment 2 Colin Walters 2012-12-03 21:07:46 UTC
Created attachment 230581 [details] [review]
build: Don't automatically use _FORTIFY_SOURCE

OS builders can add this flag externally if desired.  The problem with
us doing this unconditionally, it causes the cpp to spam us with
warnings if we're building without optimization (i.e. CFLAGS='-ggdb
-O0') which I often do so I can use gdb.
Comment 3 Colin Walters 2012-12-03 21:07:48 UTC
Created attachment 230582 [details] [review]
Remove handling of SIGSEGV/SIGFPE/SIGILL etc.

Modern operating systems have "crash catching" functionality; for
example, systemd comes with "systemd-coredump" which collects cores
automatically.

Attempting to handle these kinds of fatal signals internally is now
much worse, because the original source of the problem will be masked.
systemd won't collect a core file that would include a backtrace, for
example.

Also, with these removed, we can move forward porting to
g_unix_signal_add().
Comment 4 Colin Walters 2012-12-03 21:07:51 UTC
Created attachment 230583 [details] [review]
gdm-crash-logger: Delete

Nothing is using this currently, and modern operating systems come
with this kind of thing built in (e.g. systemd-coredump).
Comment 5 Colin Walters 2012-12-03 21:07:54 UTC
Created attachment 230584 [details] [review]
Port to g_unix_signal_add(), drop GdmSignalHandler

The level of copy/paste going on here before is rather astonishing.
For example, in some cases, I dropped spurious handling of SIGHUP,
when the code didn't have any settings to reread.

Anyways, the code is now clearer, and we get to drop all the bits of
gdm-signal-handler.[ch] for the integrated GLib handling.
Comment 6 Colin Walters 2012-12-03 21:07:57 UTC
Created attachment 230585 [details] [review]
Drop calls to g_type_init()/g_thread_init()

Both are deprecated and no longer necessary.
Comment 7 Colin Walters 2012-12-03 21:07:59 UTC
Created attachment 230586 [details] [review]
gdm-md5: Delete

Nothing was using it...
Comment 8 Colin Walters 2012-12-03 21:08:02 UTC
Created attachment 230587 [details] [review]
gdm-common: Drop some dead code

-unknown-origin.[ch] according to ChangeLog was about copyright
status...but the file clearly has a copyright header now, so
presumably this is just long dead history.

Nothing was using gdm_safe_fopen_w().  After deleting that, we're left
with VE_IGNORE_EINTR(), which is now just in gdm-common.h
Comment 9 Colin Walters 2012-12-03 21:08:04 UTC
Created attachment 230588 [details] [review]
smartcard: Port to g_thread_new()

g_thread_create() is deprecated.
Comment 10 Colin Walters 2012-12-03 21:08:07 UTC
Created attachment 230589 [details] [review]
gui/simple-greeter: Fix minor compiler warnings

Unused variables.
Comment 11 Ray Strode [halfline] 2012-12-03 21:20:55 UTC
Review of attachment 230580 [details] [review]:

sure
Comment 12 Ray Strode [halfline] 2012-12-03 21:21:22 UTC
Review of attachment 230581 [details] [review]:

ok
Comment 13 Ray Strode [halfline] 2012-12-03 21:24:49 UTC
Review of attachment 230582 [details] [review]:

I'm okay with this, but It would be good if we could solve bug 676181 first since then we'll have a common story for where to look when things go wrong.
Comment 14 Ray Strode [halfline] 2012-12-03 21:31:04 UTC
Review of attachment 230584 [details] [review]:

nice cleanup
Comment 15 Ray Strode [halfline] 2012-12-03 21:31:17 UTC
Review of attachment 230585 [details] [review]:

sure
Comment 16 Ray Strode [halfline] 2012-12-03 21:31:30 UTC
Review of attachment 230586 [details] [review]:

YES PLEASE
Comment 17 Ray Strode [halfline] 2012-12-03 21:34:52 UTC
Review of attachment 230587 [details] [review]:

well, i'd normally be hesitant about moving stuff that was specifically seggregated out, but what's left is so small and obvious, I don't think it counts as really copywritable.  I think it would be good to remove VE_IGNORE_EINTR from the code entirely at some point though.
Comment 18 Ray Strode [halfline] 2012-12-03 21:35:19 UTC
Review of attachment 230588 [details] [review]:

::: gui/simple-greeter/extensions/smartcard/gdm-smartcard-manager.c
@@ +1374,3 @@
                                                    module);
 
+        worker->thread = g_thread_new ("smartcrad",

smartcard
Comment 19 Ray Strode [halfline] 2012-12-03 21:36:01 UTC
Review of attachment 230589 [details] [review]:

ok
Comment 20 Colin Walters 2012-12-03 21:43:39 UTC
(In reply to comment #13)
> Review of attachment 230582 [details] [review]:
> 
> I'm okay with this, but It would be good if we could solve bug 676181 first
> since then we'll have a common story for where to look when things go wrong.

It's true that potentially you will no longer see "gdm-session-worker exited with code 1" or whatever in the logs.  But on the other hand, you will now have a full core file that you can inspect with "systemd-coredumpctl gdb", send to a retrace server for analysis, etc. 

When gdm starts logging to the journal, you will also see crashes there - they'll be in the same place.
Comment 21 Ray Strode [halfline] 2012-12-03 21:58:44 UTC
it's more than "gdm-session-worker exited with code 1" it's a full stack trace on crash right?

Right now when a component crashes we get a stack trace in the GDM log. you're proposing moving the crash to the journal. I'm saying if you're going to do that, then we should move all logging to the journal at the same time.

make sense? or am i missing something?
Comment 22 Colin Walters 2012-12-03 22:25:21 UTC
(In reply to comment #21)
> it's more than "gdm-session-worker exited with code 1" it's a full stack trace
> on crash right?
> 
> Right now when a component crashes we get a stack trace in the GDM log. you're
> proposing moving the crash to the journal. I'm saying if you're going to do
> that, then we should move all logging to the journal at the same time.
> 
> make sense? or am i missing something?

I understand what you're saying now yes, but I disagree with the rationale for tying them together; regardless I've rebased the patch series so those two aren't committed yet.

To give you a bit of understanding where I'm coming from, what I want is that the automated testing boots up each GNOME build in qemu, and asserts that there are no crashes in "systemd-coredumpctl" and that "systemctl --failed" is empty.

Getting to the point where we have a totally clean ~/.cache/gdm/session.log or equivalent is a *lot* more work.
Comment 23 Colin Walters 2012-12-03 22:32:13 UTC
I moved the patches to 676181 then.