login1: add support for session management #242

siavashs · 2018-01-02T17:09:26Z

This PR adds support for more logind methods, specially for session management.
Closes #238

squeed · 2018-01-04T21:20:56Z

Looks good. Any idea on how to add tests that aren't fragile?

siavashs · 2018-01-06T20:22:04Z

I think we can run loginctl and compare the result for some of the methods.

lucab · 2018-01-08T13:59:21Z

login1/dbus.go

+}
+
+func sessionFromInterfaces(session []interface{}) (*Session, error) {
+	id := session[0].(string)


Please check that session has at least 5 entries before accessing them.

lucab · 2018-01-08T14:04:21Z

login1/dbus.go

+	uid := session[1].(uint32)
+	user := session[2].(string)
+	seat := session[3].(string)
+	path := session[4].(dbus.ObjectPath)


Please perform checked type assertions here instead. That is, something like:

path, ok := session[4].(dbus.ObjectPath) if !ok { return nil, fmt.Errorf("failed to typecast session field 4 to ObjectPath") }

lucab · 2018-01-08T14:05:01Z

login1/dbus.go

+func userFromInterfaces(user []interface{}) (*User, error) {
+	uid := user[0].(uint32)
+	name := user[1].(string)
+	path := user[2].(dbus.ObjectPath)


Same as above. Please add a length check and checked type assertions.

lucab · 2018-01-08T14:08:27Z

login1/dbus.go

+		return "", err
+	}
+
+	return out.(dbus.ObjectPath), nil


Please add a checked type assertion for out.

lucab · 2018-01-08T14:11:47Z

login1/dbus.go

+	}
+
+	ret := []User{}
+


Spurious newline.

lucab · 2018-01-08T14:12:27Z

login1/dbus.go

+	}
+
+	ret := []Session{}
+


Spurious newline.

lucab · 2018-01-08T14:28:59Z

It would be nice to have at least some basic smoke tests calling those new methods and ensuring they return something meaningful without panicking. I think the docker setup on travis already has a working logind to query.

siavashs · 2018-01-10T15:42:23Z

I'll look into tests.

lucab · 2018-01-25T09:19:54Z

login1/dbus.go

+	uid := session[1].(uint32)
+	user := session[2].(string)
+	seat := session[3].(string)
+	path, ok := session[4].(dbus.ObjectPath)


Sorry, I wasn't clear enough in my previous review. Please make sure that all type assertions (i.e. also fields 0 to 3) are checked, like you did for the last field.

lucab · 2018-01-25T09:20:46Z

login1/dbus.go

+func userFromInterfaces(user []interface{}) (*User, error) {
+	uid := user[0].(uint32)
+	name := user[1].(string)
+	path, ok := user[2].(dbus.ObjectPath)


Same as above, and apologies that my previous review was not precise.

lucab · 2018-01-25T09:21:58Z

Bump on this. It looks like some comments in my previous review were not precise enough, apologies for that.

siavashs · 2018-01-25T09:52:04Z

No problem, I'll fix it.
I'll try to work on the tests this weekend.

lucab · 2018-02-01T13:44:51Z

Just dropping a quick note, I'm planning to tag v16 in the next few days (once #248 and #249 are in). Unless you need this quickly in a release, we can pick this post-release once done, without hurrying.

siavashs · 2018-02-03T20:45:23Z

As I checked there are no active user sessions inside a docker container so logind responds with empty lists of users and sessions.
One solution is to create a test user inside the container and do a login:

useradd test
login -f test

Let me know if you have a better solution to handle this test case.

siavashs · 2018-02-03T22:14:30Z

So I implemented the tests by creating and activating a test user and running the tests with the same user.
The results from login1 are compared to values from os/user.Current().

siavashs · 2018-02-03T22:30:19Z

It seems running sdjournal tests as the new test user fails, while it works fine for me locally as an unprivileged user.
Any idea what might cause that error on the CI?

siavashs · 2018-02-04T12:11:36Z

These latest changes on my branch work on Ubuntu but fail on Debian!
But it fails for both in the PR tests.

lucab · 2018-02-09T10:04:51Z

login1/dbus.go

+}
+
+func sessionFromInterfaces(session []interface{}) (*Session, error) {
+	id, ok := session[0].(string)


Please check for len(session) >= 5 before starting to index-access the array.

Should we return an error if the number of fields don't match?

Yes. Rationale is that we prefer to return an error earlier to the caller if the input array is too short, instead of panicing in library code.

lucab · 2018-02-09T10:05:10Z

login1/dbus.go

+}
+
+func userFromInterfaces(user []interface{}) (*User, error) {
+	uid, ok := user[0].(uint32)


Please check for len(user) >= 3 before starting to index-access the array.

Should we return an error if the number of fields don't match?

Same as above, yes.

lucab · 2018-02-09T10:12:52Z

@siavashs While I understand it would be nice to exact-match the environment in the test, I think that that is too fragile and also requires manipulating the environment first. I would suggest instead to be a bit less strict: just ensure that calls to ListUsers and ListSessions don't panic or return errors, and if there are actual users/sessions let's loop through them and ensure fields are populated.

siavashs · 2018-02-09T16:49:51Z

Maybe an unrelated question, but why do I see a higher coverage % when running tests locally?

...
ok  	github.com/coreos/go-systemd/login1	0.009s	coverage: 53.7% of statements
...

Is this related to go version?

lucab · 2018-02-14T09:02:07Z

@siavashs my guess is that given that there are no sessions/users in the CI run, some codepaths within conditional paths are not reached. For example, both *FromInterface are only called inside a range iterator thus are not triggered on empty arrays.

This PR seems complete to me now, is there anything else you planned to add here? If not, can you please squash commits together and adopt the same title as the PR one?

siavashs · 2018-02-14T11:19:14Z

@lucab it is ready 😄
Update: Fixed the commit message

lucab · 2018-02-14T12:08:54Z

LGTM 💚 . Thanks for your contribution (and patience)!

paultag mentioned this pull request Jan 6, 2018

Add in basic login session handling #238

Closed

lucab suggested changes Jan 8, 2018

View reviewed changes

lucab suggested changes Jan 25, 2018

View reviewed changes

lucab added needs/review needs/tests needs/rework and removed needs/review labels Jan 25, 2018

lucab changed the title ~~Add logind session management methods~~ login1: add support for session management Feb 2, 2018

lucab suggested changes Feb 9, 2018

View reviewed changes

siavashs force-pushed the logind branch from a5cd146 to cb96cca Compare February 14, 2018 11:18

login1: add support for session management

e1da176

siavashs force-pushed the logind branch from cb96cca to e1da176 Compare February 14, 2018 11:35

lucab removed needs/rework needs/tests labels Feb 14, 2018

lucab approved these changes Feb 14, 2018

View reviewed changes

lucab merged commit 25fe332 into coreos:master Feb 14, 2018

siavashs deleted the logind branch February 14, 2018 14:01

lucab added this to the v17 milestone May 7, 2018

lucab added the kind/new-api label May 7, 2018

login1: add support for session management #242

login1: add support for session management #242

Uh oh!

Conversation

siavashs commented Jan 2, 2018

Uh oh!

squeed commented Jan 4, 2018

Uh oh!

siavashs commented Jan 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucab commented Jan 8, 2018

Uh oh!

siavashs commented Jan 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucab commented Jan 25, 2018

Uh oh!

siavashs commented Jan 25, 2018

Uh oh!

lucab commented Feb 1, 2018

Uh oh!

siavashs commented Feb 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

siavashs commented Feb 3, 2018

Uh oh!

siavashs commented Feb 3, 2018

Uh oh!

siavashs commented Feb 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucab commented Feb 9, 2018

Uh oh!

siavashs commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lucab commented Feb 14, 2018

Uh oh!

siavashs commented Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lucab commented Feb 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

siavashs commented Feb 3, 2018 •

edited

Loading

siavashs commented Feb 9, 2018 •

edited

Loading

siavashs commented Feb 14, 2018 •

edited

Loading