Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zed: fix sending emails #12292

Merged
merged 1 commit into from
Jun 29, 2021
Merged

zed: fix sending emails #12292

merged 1 commit into from
Jun 29, 2021

Conversation

lnicola
Copy link
Contributor

@lnicola lnicola commented Jun 28, 2021

Motivation and Context

Fixes #12291

Commit 6fc3099 broke the quoting when invoking the mail program, revert part of that change

Description

I reverted that line to the old version. There might be a way to do the same thing without eval, though.

How Has This Been Tested?

I tested that notifications work again on my NAS.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@lnicola
Copy link
Contributor Author

lnicola commented Jun 28, 2021

r? @nabijaczleweli

@nabijaczleweli
Copy link
Contributor

nabijaczleweli commented Jun 28, 2021

What do you have ZED_EMAIL_* set to that you're seeing a difference? I would expect these to be by-definition-identical (not that I'm doubting that this is the fix, but it's certainly rather surprising).

@lnicola
Copy link
Contributor Author

lnicola commented Jun 28, 2021

ZED_EMAIL_ADDR="me@domain.tld"
#ZED_EMAIL_PROG="mail"
#ZED_EMAIL_OPTS="-s '@SUBJECT@' @ADDRESS@"

There's a snippet in the linked issue showing what happens:

[pid 3221429] execve("/usr/bin/mail", ["mail", "-s", "'ZFS", "scrub_finish", "event", "for", "tank", "on", "nas.home'", "me@domain"], 0x55a8699eb2a0 /* 28 vars */) = 0

The argv there is bogus.

@lnicola
Copy link
Contributor Author

lnicola commented Jun 28, 2021

Another way to show the same effect of eval (for some reason it doesn't work with command echo instead of cat):

$ strace -fe trace=execve sh -c "eval cat foo 'bar baz'" 2>&1 | rg execve
execve("/usr/bin/sh", ["sh", "-c", "eval cat foo 'bar baz'"], 0x7ffd9b787fc0 /* 62 vars */) = 0
[pid 56964] execve("/usr/bin/cat", ["cat", "foo", "bar", "baz"], 0x562353f80ed0 /* 62 vars */) = 0

$ strace -fe trace=execve sh -c "cat foo 'bar baz'" 2>&1 | rg "execve"
execve("/usr/bin/sh", ["sh", "-c", "cat foo 'bar baz'"], 0x7ffedbe6d8c0 /* 62 vars */) = 0
execve("/usr/bin/cat", ["cat", "foo", "bar baz"], 0x56338bec4bd0 /* 62 vars */) = 0

@nabijaczleweli
Copy link
Contributor

nabijaczleweli commented Jun 28, 2021

Oh, right, of course, this PR is correct, yeah.

I'd extend the quote all the way to eval "${ZED_EMAIL_PROG} ${ZED_EMAIL_OPTS}", or remove it entirely, personally, since the eval "${ZED_EMAIL_PROG}" ${ZED_EMAIL_OPTS} phrasing suggests that ZED_EMAIL_PROG will be executed as argv[0], as in a normal eval-less usage, but it won't

Commit 6fc3099 broke the quoting when invoking the mail program, revert
that change.

Signed-off-by: Laurențiu Nicola <lnicola@dend.ro>
@lnicola
Copy link
Contributor Author

lnicola commented Jun 28, 2021

Ah, good point about the quotes, I removed them.

@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Jun 28, 2021
@lnicola
Copy link
Contributor Author

lnicola commented Jun 28, 2021

Should I worry about the failing CI? That domain (build.zfsonlinux.org) doesn't resolve for me, but the check doesn't fail in other PRs.

@behlendorf
Copy link
Contributor

@lnicola the CI failure here was unrelated, so you don't need to do anything but thanks for asking. This should be good to go.

@tonyhutter tonyhutter merged commit 3482c2b into openzfs:master Jun 29, 2021
@lnicola lnicola deleted the patch-1 branch June 29, 2021 19:53
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 29, 2021
Commit 6fc3099 broke the quoting when invoking the mail program, revert
that change.

Signed-off-by: Laurențiu Nicola <lnicola@dend.ro>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

zed email notifications don't work
4 participants