Comment on attachment 170999 [details] [review]
patch fixing issue.

[Setting "patch" flag and correcting mime type so this can actually be queried for.]

Created attachment 224268 [details] [review]
daemon: Extract unsafe code from GSpawnChildSetupFunc

For example, g_warning() can try to lock a mutex; if another thread
happened to be holding that mutex from before we forked, we'd
deadlock.

Created attachment 224269 [details] [review]
daemon: Delete unused group code

gdm-simple-slave is only passing user-name, so this group lookup
stuff is unnecessary complication.

I can't imagine a use case for it either - you really want the default
group for the user, not some arbitrary other group.

Created attachment 224270 [details] [review]
daemon: Do global uid-specific setup before launching dbus

We want to create the runtime dir for example before we do anything
else.

This also cleans up the code significantly by not looking up the
username twice; we just pass the passwd entry down into child
functions.

Created attachment 224271 [details] [review]
daemon: Further cleanup of error handling

Rather than an incoherent mess of "sometimes print errors, sometimes
pass them up, sometimes do both", clean up the chain of dbus-launch
running to consistently pass errors up, and print in the first public
API that doesn't throw a GError.

Created attachment 224272 [details] [review]
daemon: Create home directory for greeter user

For OSTree, I'm trying to move components to a model where they
automatically create whatever directories/files are needed in /var.
This helps with upgrade/downgrade scenarios.

In GDM's case, this is pretty easy to do because we start with full
root privileges.

Blah, these were meant to go on bug 683951

*** Bug 683951 has been marked as a duplicate of this bug. ***

(In reply to comment #0)
> Created an attachment (id=170999) [details] [review]
> patch fixing issue.
> 
> Since GDM allows the screenshot directory (GDM_SCREENSHOT_DIR) to be configured
> via --with-screenshot-dir, it isn't necessarily in the same directory as the
> GDM_XAUTH_DIR (/var/run/gdm).  So, GDM can fail if the specified directory is
> in a subdirectory that doesn't exist.  This minor patch fixes this issue.  It's
> a trivial fix and makes GDM a bit more robust, so I'm hoping this can go
> upstream.

Hi Brian - can you elaborate? In what cases would the screenshot parent directory not exist?

I'm not sure gdm should be the process creating e.g. /var/run automatically...you really want the ownership/permissions to be propagated from a process that handles that more centrally.

Review of attachment 224268 [details] [review]:

::: daemon/gdm-launch-environment.c
@@ +382,3 @@
 typedef struct {
+        struct passwd   *pwent;
+        struct group    *grent;

pwent and grent point to glibc owned memory with that could go away accidentally at any time. Even if it's okay with the current circumstances, I don't like the idea of maintaining a pointer to them in a data struct (conceptually it rubs me the wrong way).  I'd rather you copy the fields you need.

Review of attachment 224269 [details] [review]:

decruftification request graciously accepted.

Review of attachment 224270 [details] [review]:

::: daemon/gdm-launch-environment.c
@@ +439,3 @@
 static gboolean
+spawn_command_line_sync_as_user (const char      *command_line,
+                                 struct passwd   *pwent,

hmm again, i think i'd rather the fields you use get passed in directly.  I don't really like keeping this memory that can disappear at any moment hanging around in a data structure (even if it's safe as written currently)

Review of attachment 224271 [details] [review]:

sure

::: daemon/gdm-launch-environment.c
@@ +556,2 @@
  out:
+        if (match_info)

!= NULL

@@ +557,3 @@
+        if (match_info)
+                g_match_info_free (match_info);
+        if (re)

!= NULL

@@ +601,2 @@
  out:
+        if (env) {

!= NULL

@@ +767,3 @@
 
+        if (!start_dbus_daemon (launch_environment, passwd_entry, error))
+                goto out;

braces

Review of attachment 224272 [details] [review]:

::: daemon/gdm-launch-environment.c
@@ +769,3 @@
+        if (!ensure_directory_with_uid_gid (passwd_entry->pw_dir,
+                                            passwd_entry->pw_uid, passwd_entry->pw_gid,
+                                            error))

all arguments on their own line if any arguments on their own line. I was going to say add braces too, but none of the surround coding does so...

Created attachment 224289 [details] [review]
daemon: Major cleanup of greeter environment setup

Ok, this is a rollup of the other 4 - trying to address the comments I
got cascading conflicts.  Let me know if you have a non-small desire
for it to be split again.

Created attachment 224290 [details] [review]
daemon: Create home directory for greeter user

Rebased

I'd prefer if the patches were split, but i'm not going to sweat it.  So feel free to push these guys.

I'm not super happy that various debug messages are getting dropped from the child func, but I understand your deadlock concern.

Eventually we'll use sd_journal_stream_fd for debugging and can just drop to doing write() calls to get debug back in slippery areas like that I suppose.

(In reply to comment #17)
> I'd prefer if the patches were split, but i'm not going to sweat it.  So feel
> free to push these guys.

Thanks for the review!

> I'm not super happy that various debug messages are getting dropped from the
> child func, but I understand your deadlock concern.

Yeah, I agree:

> Eventually we'll use sd_journal_stream_fd for debugging and can just drop to
> doing write() calls to get debug back in slippery areas like that I suppose.

To report errors correctly from child->parent, you create a pipe dedicated to errors, and write them over that.  This is what gspawn.c does.

Maybe in the GSubprocess future we could have a helper for making the pipe and turning child errors into GErrors.

Attachment 224289 [details] pushed as aea73de - daemon: Major cleanup of greeter environment setup
Attachment 224290 [details] pushed as a5df862 - daemon: Create home directory for greeter user

well bubbling them to the parent is one idea, i was just saying log straight to the journal.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gdm/issues/37.