Skip to content

snapd-env-generator: fix when PATH is empty or unset#5808

Merged
mvo5 merged 2 commits intocanonical:masterfrom
mvo5:fix-systemd-env-generator
Sep 12, 2018
Merged

snapd-env-generator: fix when PATH is empty or unset#5808
mvo5 merged 2 commits intocanonical:masterfrom
mvo5:fix-systemd-env-generator

Conversation

@mvo5
Copy link
Copy Markdown
Contributor

@mvo5 mvo5 commented Sep 10, 2018

When the PATH is empty or unset the generator will generate the
wrong output (PATH=:/snap/bin). This causes havoc. This PR fixes
it by outputing only a PATH=/snap/bin segment that systemd will
correctly piece together.

When the PATH is empty or unset the generator will generate the
wrong output (PATH=:/snap/bin). This causes havoc. This PR fixes
it by outputing only a PATH=/snap/bin segment that systemd will
correctly piece together.
@mvo5 mvo5 added the ⚠ Critical High-priority stuff (e.g. to fix master) label Sep 10, 2018
@mvo5 mvo5 added this to the 2.35 milestone Sep 10, 2018
Copy link
Copy Markdown
Contributor

@stolowski stolowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

char *path = getenv("PATH");
if (path == NULL)
path = "";
if (path == NULL || sc_streq(path, "")) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, shall I add sc_strempty for cases like this?

Copy link
Copy Markdown

@smoser smoser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change does not fix the problem.

environment-generators inherit environment that /sbin/init
started with, but no other systemd defaults. Therefore, there is no
easy way to append to an environment variable that is not set as snapd
was hoping to do. The only solution currently is to copy the expected
"default path" into the snapd environment-generator.

There is a more long winded response at https://bugs.launchpad.net/cloud-images/+bug/1791691

When the PATH is not set when the generator runs, simply do nothing
for now. If we print PATH=/snap/bin thats the wrong path and
systemd needs to be smarter to collect paths first. If we add a
default PATH the problem is that this is very inconcistent, see
LP:#1792004.

See also https://bugs.launchpad.net/cloud-images/+bug/1791691
@mvo5
Copy link
Copy Markdown
Contributor Author

mvo5 commented Sep 11, 2018

@smoser Thanks Scott. I just talked on irc with @xnox and he plans to fix this issue in systemd. Until then his advice is that if the PATH is unset our generator should do nothing. Picking a default PATH seems to be problematic - there are just too many too chose from: https://bugs.launchpad.net/ubuntu/+source/dpkg/+bug/1792004

@powersj
Copy link
Copy Markdown

powersj commented Sep 11, 2018

@mvo5 I want to confirm that you were going to land and upload your latest change of doing nothing hopefully today/tomorrow?

@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #5808 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5808      +/-   ##
==========================================
- Coverage   79.13%   79.13%   -0.01%     
==========================================
  Files         531      531              
  Lines       40720    40735      +15     
==========================================
+ Hits        32223    32234      +11     
- Misses       5884     5885       +1     
- Partials     2613     2616       +3
Impacted Files Coverage Δ
overlord/ifacestate/udevmonitor/udevmon.go 74.68% <0%> (-6.02%) ⬇️
overlord/ifacestate/handlers.go 63.75% <0%> (ø) ⬆️
overlord/hookstate/hookmgr.go 74.51% <0%> (+0.96%) ⬆️
interfaces/hotplug/udevadm.go 76.59% <0%> (+2.52%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b814491...71707f0. Read the comment docs.

@mvo5 mvo5 merged commit fda3415 into canonical:master Sep 12, 2018
@mvo5
Copy link
Copy Markdown
Contributor Author

mvo5 commented Sep 12, 2018

@powersj Yes, this is scheduled to be uploaded ASAP (hopefully today). We would have done it yesterday already but github was having too much trouble.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

⚠ Critical High-priority stuff (e.g. to fix master)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants