Sundry small cleanups by keszybz · Pull Request #8199 · systemd/systemd

keszybz · 2018-02-16T09:49:27Z

No description provided.

This usually is very annoying to users who then cannot log in, so make sure we always warn if that happens (selinux, or whatever other reason). This reverts a790812.

sourcejedi · 2018-02-16T10:41:21Z

src/shared/conf-parser.h

-                     void *data,                                        \
-                     void *userdata) {                                  \
-                                                                        \
+        int function(GENERIC_PARSER_ARGS) {                             \


Hmm, should we err on the side of keeping the argument list visible in this function definition, like the .c files? GENERIC_PARSER_ARGS works nicely in the big list of function declarations, but maybe we shouldn't use it outside of that. We could even #undef it immediately after the declarations.

The macro is defined just a few lines above, so I think it is OK to also use the macro here.

ehh. I'm not used to reading function definitions that way.

Just having #undef GENERIC_PARSER_ARGS somewhere would make me happier. It communicates the point made in the commit message, that we don't expect people to remember it. If you think the #undef would look ok placed after these two function definitions, then I defer to submitters judgement.

BTW apologies for clicking the wrong button, I was trying to do a full review from the start.

It'd need to be undefined after DEFINE_CONFIG_PARSE_ENUM is used, so there doesn't seem to be a nice way to do this.

ohh. sorry.

sourcejedi · 2018-02-16T11:19:56Z

src/login/logind.c

-                        unlinkat(dirfd(d), de->d_name, 0);
+                        if (unlinkat(dirfd(d), de->d_name, 0) < 0)
+                                log_warning("Failed to remove /run/systemd/seats/%s: %m",
+                                            de->d_name);


everybody's favourite task: adding error handling.

We've got a different approach for each of the three calls in this commit... and each call looks like it's filling the exact same role.

The journald one sounds a bit worrying, if it could later restore a stale stream with the wrong fd? Should the journald one be raised to LOG_WARNING as well?

I'm not really worried about the udev one being ignored.

I made them both log_warning now.

poettering · 2018-02-16T11:44:07Z

src/test/test-utf8.c

+        assert_se(utf8_n_codepoints("zażółcić gęślą jaźń") == 19);
+        assert_se(utf8_n_codepoints("串") == 1);
+        assert_se(utf8_n_codepoints("") == 0);
+        assert_se(utf8_n_codepoints("\xF1") == (size_t) -1);


hey, come on! you totally missed out on the chance to count the codepoints in an emoji infested string!

I mean, emojis! EMOJIS!

poettering · 2018-02-16T11:49:25Z

lgtm (except of course the massive missed opportunity of testing emoji strings in the utf8_n_codepoints() tests.)

sourcejedi

Afraid I bounced off "do not initialize join_controllers by default", but I was able to review everything else.

Thanks for pulling in my suggested comment fix, and this looks nice generally. I have two sundry comments.

sourcejedi · 2018-02-16T11:31:51Z

src/core/main.c

-        assert(lvalue);
-        assert(rvalue);
-
-        arg_join_controllers = strv_free_free(arg_join_controllers);


Equally, having to use arg_join_controllers directly instead of through data was a sign we were being a bit weird. Nice cleanup :).

sourcejedi · 2018-02-16T11:40:04Z

src/shared/conf-parser.h

-                     void *data,                                        \
-                     void *userdata) {                                  \
-                                                                        \
+        int function(GENERIC_PARSER_ARGS) {                             \


ehh. I'm not used to reading function definitions that way.

Just having #undef GENERIC_PARSER_ARGS somewhere would make me happier. It communicates the point made in the commit message, that we don't expect people to remember it. If you think the #undef would look ok placed after these two function definitions, then I defer to submitters judgement.

BTW apologies for clicking the wrong button, I was trying to do a full review from the start.

yuwata · 2018-02-19T07:06:04Z

The CIs fail to start systemd...

[    3.437497] traps: systemd[1] general protection ip:7fbe50a5158a sp:7fff55dbeaa8 error:0 in libc-2.23.so[7fbe509b2000+1c0000]
[    3.441074] systemd[1]: Caught <SEGV>, dumped core as pid 326.
[    3.442023] systemd[1]: Freezing execution.

Coverity now started warning about this ("Calling unlinkat without checking return value (as is done elsewhere 12 out of 15 times).", and it is right: most of the time we should at list print a log message so people can figure out something is wrong when this happens. v2: - use warning level in journald too (this is unlikely to happen ever, so it should be safe to something that is visible by default).

The arguments have to be indentical everywhere, so let's use a macro to make things more readable. But only in the headers, in the .c files let's keep them verbose so that it's easy to see the argument list.

config_parse_join_controllers would free the destination argument on failure, which is contrary to our normal style, where failed parsing has no effect. Moving it to shared also allows a test to be added.

We're moving towards unified cgroup hierarchy where this is not necessary. This makes main.c a bit simpler.

systemd#8125 (review)

Follow up for review of systemd#8184.

keszybz · 2018-02-19T15:12:53Z

I fixed the important issue (lack of fancy shmancy unicode) and the other one (missing array terminator leading to a crash).

login,user-sessions: always warn when we fail to remove nologin file

af229d7

This usually is very annoying to users who then cannot log in, so make sure we always warn if that happens (selinux, or whatever other reason). This reverts a790812.

keszybz mentioned this pull request Feb 16, 2018

let's make "systemd-run --scope --user" work again on cgroupsv2 #8125

Merged

sourcejedi reviewed Feb 16, 2018

View reviewed changes

poettering reviewed Feb 16, 2018

View reviewed changes

poettering added util-lib good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels Feb 16, 2018

sourcejedi reviewed Feb 16, 2018

View reviewed changes

yuwata added ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels Feb 19, 2018

keszybz added 8 commits February 19, 2018 15:00

test-process-util: one more (void)

6fb05b0

shared/conf-parser: define a macro for the repeating argument set

b48382e

The arguments have to be indentical everywhere, so let's use a macro to make things more readable. But only in the headers, in the .c files let's keep them verbose so that it's easy to see the argument list.

Move config_parse_join_controllers to shared, add test

9ecdba8

config_parse_join_controllers would free the destination argument on failure, which is contrary to our normal style, where failed parsing has no effect. Moving it to shared also allows a test to be added.

pid1: do not initialize join_controllers by default

56c8d74

We're moving towards unified cgroup hierarchy where this is not necessary. This makes main.c a bit simpler.

core: drop obsolete comment

f4aa0bd

systemd#8125 (review)

Some comment adjustments

f95dbcc

Follow up for review of systemd#8184.

tests: add a simple test for utf8_n_codepoints()

e2cbc80

keszybz force-pushed the small-things branch from 15a31cf to e2cbc80 Compare February 19, 2018 14:23

keszybz removed the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label Feb 19, 2018

poettering merged commit 30663b6 into systemd:master Feb 19, 2018

keszybz deleted the small-things branch February 19, 2018 19:12

Uh oh!

Conversation

keszybz commented Feb 16, 2018

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poettering commented Feb 16, 2018

Uh oh!

sourcejedi left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuwata commented Feb 19, 2018

Uh oh!

keszybz commented Feb 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

sourcejedi Feb 16, 2018 •

edited

Loading

sourcejedi Feb 16, 2018 •

edited

Loading