{"action":"closed","issue":{"url":"https://api.github.com/repos/systemd/systemd/issues/9704","repository_url":"https://api.github.com/repos/systemd/systemd","labels_url":"https://api.github.com/repos/systemd/systemd/issues/9704/labels{/name}","comments_url":"https://api.github.com/repos/systemd/systemd/issues/9704/comments","events_url":"https://api.github.com/repos/systemd/systemd/issues/9704/events","html_url":"https://github.com/systemd/systemd/issues/9704","id":343746182,"node_id":"MDU6SXNzdWUzNDM3NDYxODI=","number":9704,"title":"Confusing handling of return value of start_unit(), since it returns EXIT_* which is >= 0","user":{"login":"filbranden","id":106037,"node_id":"MDQ6VXNlcjEwNjAzNw==","avatar_url":"https://avatars0.githubusercontent.com/u/106037?v=4","gravatar_id":"","url":"https://api.github.com/users/filbranden","html_url":"https://github.com/filbranden","followers_url":"https://api.github.com/users/filbranden/followers","following_url":"https://api.github.com/users/filbranden/following{/other_user}","gists_url":"https://api.github.com/users/filbranden/gists{/gist_id}","starred_url":"https://api.github.com/users/filbranden/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/filbranden/subscriptions","organizations_url":"https://api.github.com/users/filbranden/orgs","repos_url":"https://api.github.com/users/filbranden/repos","events_url":"https://api.github.com/users/filbranden/events{/privacy}","received_events_url":"https://api.github.com/users/filbranden/received_events","type":"User","site_admin":false},"labels":[{"id":268297924,"node_id":"MDU6TGFiZWwyNjgyOTc5MjQ=","url":"https://api.github.com/repos/systemd/systemd/labels/has-pr%20%E2%9C%A8","name":"has-pr ✨","color":"009800","default":false},{"id":221950896,"node_id":"MDU6TGFiZWwyMjE5NTA4OTY=","url":"https://api.github.com/repos/systemd/systemd/labels/systemctl","name":"systemctl","color":"c7def8","default":false}],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":{"url":"https://api.github.com/repos/systemd/systemd/milestones/17","html_url":"https://github.com/systemd/systemd/milestone/17","labels_url":"https://api.github.com/repos/systemd/systemd/milestones/17/labels","id":3416475,"node_id":"MDk6TWlsZXN0b25lMzQxNjQ3NQ==","number":17,"title":"v240","description":"","creator":{"login":"poettering","id":2130732,"node_id":"MDQ6VXNlcjIxMzA3MzI=","avatar_url":"https://avatars1.githubusercontent.com/u/2130732?v=4","gravatar_id":"","url":"https://api.github.com/users/poettering","html_url":"https://github.com/poettering","followers_url":"https://api.github.com/users/poettering/followers","following_url":"https://api.github.com/users/poettering/following{/other_user}","gists_url":"https://api.github.com/users/poettering/gists{/gist_id}","starred_url":"https://api.github.com/users/poettering/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/poettering/subscriptions","organizations_url":"https://api.github.com/users/poettering/orgs","repos_url":"https://api.github.com/users/poettering/repos","events_url":"https://api.github.com/users/poettering/events{/privacy}","received_events_url":"https://api.github.com/users/poettering/received_events","type":"User","site_admin":false},"open_issues":28,"closed_issues":47,"state":"open","created_at":"2018-06-11T17:19:42Z","updated_at":"2018-10-16T06:42:51Z","due_on":null,"closed_at":null},"comments":2,"created_at":"2018-07-23T18:33:15Z","updated_at":"2018-10-16T06:42:51Z","closed_at":"2018-10-16T06:42:51Z","author_association":"MEMBER","body":"I found some confusing handling of return value of start_unit() while working on #9675.\r\n\r\nIn particular, [here](https://github.com/systemd/systemd/blob/8b3169b9a171b7b5255fa7c1b6f84cd324cf6397/src/systemctl/systemctl.c#L3121):\r\n\r\n```c\r\n                q = start_unit_one(bus, method, *name, mode, &error, w, arg_wait ? &wait_context : NULL);\r\n                if (r >= 0 && q < 0)\r\n                        r = translate_bus_error_to_exit_status(q, &error);\r\n```\r\n\r\nAnd [here](https://github.com/systemd/systemd/blob/8b3169b9a171b7b5255fa7c1b6f84cd324cf6397/src/systemctl/systemctl.c#L3152):\r\n\r\n```c\r\n        if (r >= 0 && arg_wait) {\r\n```\r\n\r\nThe problem with that code is that, before it, `r` is only set [here](https://github.com/systemd/systemd/blob/8b3169b9a171b7b5255fa7c1b6f84cd324cf6397/src/systemctl/systemctl.c#L3122):\r\n\r\n```c\r\n                        r = translate_bus_error_to_exit_status(q, &error);\r\n```\r\n\r\nWhich will set `r` to one of the `EXIT_*` values, which means 0 for success and >0 for one of the error exists... So the check `r >= 0` is really meaningless and I guess what should be tested there is `r == 0` for \"no error was set, yet.\"\r\n\r\nThe first occurrence was introduced in commit e3e0314b56012f7febc279d268f2cadc1fcc0f25, which has in fact changed it from `if (r == 0 && q < 0)` to `if (r >= 0 && q < 0)`, which makes me wonder if this was somehow intentional (?!)\r\n\r\nThe second one was introduced in 93a0884126146361ca078ec627da2cf766205a1c (which implements `--wait` option), but I wonder if that one was due to a copy & paste of the code above it.\r\n\r\n(Another possibility is that `translate_bus_error_to_exit_status()` may return <0 if the error is unknown... Is that really what we're checking here?)\r\n\r\nAnyways, just wanted to raise an issue, to double check that this is indeed all intended or not. cc @keszybz who originally authored the code in e3e0314b56012f7febc279d268f2cadc1fcc0f25 and also the `if (r == 0 && q < 0)` which preceded it (commit b65205466248531d52f9d6dba16195864b023d4a).\r\n\r\nCheers,\r\nFilipe\r\n"}}
{"actor":{"display_login":"keszybz"}}